Repository: pydantic/pydantic
Pull request: fix(field): remove some side effects of `default_factory`
<!-- Thank you for your contribution! -->
<!-- Unless your change is trivial, please create an issue to discuss the change before creating a PR -->
<!-- See https://pydantic-docs.helpmanual.io/contributing/ for help on Contributing -->

## Change Summary
Fix some side effects of `default_factory`:
- call it only once when no validation is needed
- remove default value of generated schema

## Related issue number

fix #1491 and #1520

## Checklist

* [ ] Unit tests for the changes exist
* [ ] Tests pass on CI and coverage remains at 100%
* [ ] Documentation reflects the changes where applicable
* [ ] `changes/<pull request or issue id>-<github username>.md` file added describing change
  (see [changes/README.md](https://github.com/samuelcolvin/pydantic/blob/master/changes/README.md) for details)


Problem statement:
`default_factory` being run prior to object init
# Bug

```
             pydantic version: 1.5.1
```

`default_factory` gets run twice before any object instantiation. This is obvious for callables with side-effects:

```py
>>> from pydantic import BaseModel, Field
>>> class Seq():
...     def __init__(self):
...         self.v = 0 
...     def __call__(self):
...         self.v += 1
...         return self.v
...   
>>> class MyModel(BaseModel):
...     id: int = Field(default_factory=Seq())
...   
>>> MyModel()
MyModel(id=3)
```

Hints from issue discussion:
I'm not sure I would call this a bug.

Since it'll be consistent, you should be able to just start with `v = -2` or whatever - I know it looks odd but as long as you include a comment I think it should be fine.

I can't think of another safe workaround.

Commit message: Avoid some side effects of default factory

- by calling it only once if possible (fix #1491)
- by not setting the default value in the schema (fix #1520)

Patch to review:
diff --git a/changes/1491-PrettyWood.md b/changes/1491-PrettyWood.md
new file mode 100644
index 00000000000..a09bb0c87c0
--- /dev/null
+++ b/changes/1491-PrettyWood.md
@@ -0,0 +1,2 @@
+Avoid some side effects of `default_factory` by calling it only once
+if possible and by not setting a default value in the schema
diff --git a/docs/usage/models.md b/docs/usage/models.md
index 7ca9ecf43b7..3d4d459e76b 100644
--- a/docs/usage/models.md
+++ b/docs/usage/models.md
@@ -492,6 +492,10 @@ _(This script is complete, it should run "as is")_
 
 Where `Field` refers to the [field function](schema.md#field-customisation).
 
+!!! warning
+    If you don't declare the type of the field or if you want to validate default values with `validate_all`,
+    *pydantic* will need to call the `default_factory`, which could lead to side effects !
+
 ## Parsing data into a specified type
 
 Pydantic includes a standalone utility function `parse_obj_as` that can be used to apply the parsing
diff --git a/pydantic/fields.py b/pydantic/fields.py
index c49ad7f0470..6fd08358f1b 100644
--- a/pydantic/fields.py
+++ b/pydantic/fields.py
@@ -296,7 +296,7 @@ def infer(
 
         if isinstance(value, FieldInfo):
             field_info = value
-            value = field_info.default_factory() if field_info.default_factory is not None else field_info.default
+            value = None if field_info.default_factory is not None else field_info.default
         else:
             field_info = FieldInfo(value, **field_info_from_config)
         required: 'BoolUndefined' = Undefined
@@ -341,7 +341,14 @@ def prepare(self) -> None:
         Note: this method is **not** idempotent (because _type_analysis is not idempotent),
         e.g. calling it it multiple times may modify the field and configure it incorrectly.
         """
+
+        # to prevent side effects by calling the default_factory for nothing, we only call it
+        # when we want to validate the default value i.e. when `validate_all` is set to True
+        if self.default_factory is not None and self.type_ is not None and not self.model_config.validate_all:
+            return
+
         default_value = self.get_default()
+
         if default_value is not None and self.type_ is None:
             self.type_ = default_value.__class__
             self.outer_type_ = self.type_
diff --git a/tests/test_main.py b/tests/test_main.py
index 76e05b70ad1..3ff17badafa 100644
--- a/tests/test_main.py
+++ b/tests/test_main.py
@@ -1088,6 +1088,46 @@ class FunctionModel(BaseModel):
     assert m.uid is uuid4
 
 
+def test_default_factory_side_effect():
+    """It should call only once the given factory"""
+
+    class Seq:
+        def __init__(self):
+            self.v = 0
+
+        def __call__(self):
+            self.v += 1
+            return self.v
+
+    class MyModel(BaseModel):
+        id: int = Field(default_factory=Seq())
+
+    m1 = MyModel()
+    assert m1.id == 1
+    m2 = MyModel()
+    assert m2.id == 2
+    assert m1.id == 1
+
+
+def test_default_factory_side_effect_2():
+    """It should call only once the given factory"""
+
+    v = 0
+
+    def factory():
+        nonlocal v
+        v += 1
+        return v
+
+    class MyModel(BaseModel):
+        id: int = Field(default_factory=factory)
+
+    m1 = MyModel()
+    assert m1.id == 1
+    m2 = MyModel()
+    assert m2.id == 2
+
+
 @pytest.mark.skipif(sys.version_info < (3, 7), reason='field constraints are set but not enforced with python 3.6')
 def test_none_min_max_items():
     # None default


Code review comments:
diff_hunk: @@ -341,7 +341,14 @@ def prepare(self) -> None:
         Note: this method is **not** idempotent (because _type_analysis is not idempotent),
         e.g. calling it it multiple times may modify the field and configure it incorrectly.
         """
+
+        # to prevent side effects by calling the default_factory for nothing, we only call it
+        # when we want to validate the default value i.e. when `validate_all` is set to True
+        if self.default_factory is not None and self.type_ is not None and not self.model_config.validate_all:
path: pydantic/fields.py
text: @author:
I added a check of `self.type_` to avoid regressions but as it's a beta feature I don't know if we could actually just force people to set a type when using it and remove the check here. I'm not sure it makes sense to use `default_factory` without one

@user1:
I think requiring a type with `default_factory` would be a better solution to this.

@author:
Alright glad we agree on this ! Will remove the extra check then

@author:
@user1 Change done in fe533a0