diff --git a/changes/1491-PrettyWood.md b/changes/1491-PrettyWood.md new file mode 100644 index 0000000000..a09bb0c87c --- /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 ebae22793d..3365a181fc 100644 --- a/docs/usage/models.md +++ b/docs/usage/models.md @@ -512,6 +512,11 @@ _(This script is complete, it should run "as is")_ Where `Field` refers to the [field function](schema.md#field-customisation). +!!! warning + The `default_factory` expects the field type to be set. + Moreover 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 8967fcdf25..67a3c13b96 100644 --- a/pydantic/fields.py +++ b/pydantic/fields.py @@ -295,7 +295,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 @@ -340,7 +340,19 @@ 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: + if self.type_ is None: + raise errors_.ConfigError( + f'you need to set the type of field {self.name!r} when using `default_factory`' + ) + if 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_edge_cases.py b/tests/test_edge_cases.py index d7078d32fd..13e9e57d47 100644 --- a/tests/test_edge_cases.py +++ b/tests/test_edge_cases.py @@ -1531,3 +1531,23 @@ class Model(BaseModel): Model(v=None) Model() + + +def test_default_factory_side_effect(): + """It may call `default_factory` more than once when `validate_all` is set""" + + v = 0 + + def factory(): + nonlocal v + v += 1 + return v + + class MyModel(BaseModel): + id: int = Field(default_factory=factory) + + class Config: + validate_all = True + + m1 = MyModel() + assert m1.id == 2 # instead of 1 diff --git a/tests/test_main.py b/tests/test_main.py index dfc53b1371..32237d7672 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -5,7 +5,7 @@ import pytest -from pydantic import BaseModel, Extra, Field, NoneBytes, NoneStr, Required, ValidationError, constr +from pydantic import BaseModel, ConfigError, Extra, Field, NoneBytes, NoneStr, Required, ValidationError, constr def test_success(): @@ -52,26 +52,32 @@ def test_ultra_simple_repr(): assert m.to_string() == 'a=10.2 b=10' -def test_default_dict_repr(): +def test_default_factory_field(): def myfunc(): return 1 class Model(BaseModel): a: int = Field(default_factory=myfunc) - b = Field(default_factory=myfunc) m = Model() - assert str(m) == 'a=1 b=1' - assert repr(m) == 'Model(a=1, b=1)' + assert str(m) == 'a=1' assert ( repr(m.__fields__['a']) == "ModelField(name='a', type=int, required=False, default_factory='')" ) - assert ( - repr(m.__fields__['b']) == "ModelField(name='b', type=int, required=False, default_factory='')" - ) - assert dict(m) == {'a': 1, 'b': 1} - assert m.dict() == {'a': 1, 'b': 1} - assert m.json() == '{"a": 1, "b": 1}' + assert dict(m) == {'a': 1} + assert m.json() == '{"a": 1}' + + +def test_default_factory_no_type_field(): + def myfunc(): + return 1 + + with pytest.raises(ConfigError) as e: + + class Model(BaseModel): + a = Field(default_factory=myfunc) + + assert str(e.value) == "you need to set the type of field 'a' when using `default_factory`" def test_comparing(): @@ -1102,6 +1108,46 @@ class Config: assert SingletonFieldModel().singleton is SingletonFieldModel().singleton +def test_default_factory_called_once(): + """It should call only once the given factory by default""" + + 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_called_once_2(): + """It should call only once the given factory by default""" + + 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