accept None as value for all optional fields #1307

PrettyWood · 2020-03-13T01:33:13Z

Change Summary

All optional list fields with None as value should be valid except if there is a custom validation
to handle this case.

ℹ️ Test skipped on python 3.6

        if unused_constraints:
            raise ValueError(
>               f'On field "{field_name}" the following field constraints are set but not enforced: '
                f'{", ".join(unused_constraints)}. '
                f'\nFor more details see https://pydantic-docs.helpmanual.io/usage/schema/#unenforced-field-constraints'
            )
E           ValueError: On field "bar" the following field constraints are set but not enforced: min_items. 
E           For more details see https://pydantic-docs.helpmanual.io/usage/schema/#unenforced-field-constraints

Related issue number

closes #1295

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 for details)

codecov · 2020-03-13T03:45:10Z

Codecov Report

Merging #1307 into master will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master     #1307      +/-   ##
===========================================
+ Coverage   99.89%   100.00%   +0.10%     
===========================================
  Files          21        21              
  Lines        3708      3737      +29     
  Branches      731       739       +8     
===========================================
+ Hits         3704      3737      +33     
+ Misses          2         0       -2     
+ Partials        2         0       -2

Impacted Files	Coverage Δ
pydantic/types.py	`100.00% <100.00%> (ø)`
pydantic/errors.py	`100.00% <0.00%> (ø)`
pydantic/fields.py	`100.00% <0.00%> (ø)`
pydantic/typing.py	`100.00% <0.00%> (ø)`
pydantic/decorator.py	`100.00% <0.00%> (ø)`
pydantic/validators.py	`100.00% <0.00%> (ø)`
pydantic/schema.py	`100.00% <0.00%> (+0.55%)`	⬆️
pydantic/utils.py	`100.00% <0.00%> (+1.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 938335c...d226cc3. Read the comment docs.

PrettyWood · 2020-03-13T03:50:55Z

pydantic/fields.py

@@ -705,6 +705,10 @@ def _apply_validators(
    ) -> 'ValidateReturn':
        for validator in validators:
            try:
+                # optional field set to None should be valid unless a custom validator handles this case
+                if validator.__name__ in DEFAULT_VALIDATORS_NAMES and v is None and not self.required:


I'm not really satisfied with the check on name. But the validators are wrapped so no easy way to directly check that. I'm open to proposals

I'm afraid I'm not happy with this. This is the most performance critical part of the whole of pydantic.

I think we can fix this bug without modifying this funciton.

Instead modify ConstrainedList to something like

# This types superclass should be List[T], but cython chokes on that... class ConstrainedList(list): # type: ignore # Needed for pydantic to detect that this is a list __origin__ = list __args__: List[Type[T]] # type: ignore min_items: Optional[int] = None max_items: Optional[int] = None item_type: Type[T] # type: ignore @classmethod def __get_validators__(cls) -> 'CallableGenerator': yield cls.list_length_validator @classmethod def __modify_schema__(cls, field_schema: Dict[str, Any]) -> None: update_not_none(field_schema, minItems=cls.min_items, maxItems=cls.max_items) @classmethod def list_length_validator(cls, v: 'List[T]', field) -> Optional['List[T]']: if v is None and not field.required: return None v = list_validator(v) v_len = len(v) if cls.min_items is not None and v_len < cls.min_items: raise errors.ListMinLengthError(limit_value=cls.min_items) if cls.max_items is not None and v_len > cls.max_items: raise errors.ListMaxLengthError(limit_value=cls.max_items) return v

I'm afraid I'm not happy with this. This is the most performance critical part of the whole of pydantic.

I think we can fix this bug without modifying this funciton.

I definitely understand ! I was not satisfied either. But I wanted to tackle the whole problem instead of just the ConstrainList one. As a matter of fact it seems it's not needed! Sorry for the overkill

samuelcolvin · 2020-03-18T13:44:02Z

changes/1307-prettywood.md

@@ -0,0 +1 @@
+fix : value None should be valid for all optional fields by default


Suggested change

fix : value None should be valid for all optional fields by default

Allow `None` as input to all optional fields

Thanks ! Just added list fields as the fix is scoped to ConstrainedList

samuelcolvin · 2020-03-18T13:45:43Z

pydantic/fields.py

@@ -705,6 +705,10 @@ def _apply_validators(
    ) -> 'ValidateReturn':
        for validator in validators:
            try:
+                # optional field set to None should be valid unless a custom validator handles this case
+                if validator.__name__ in DEFAULT_VALIDATORS_NAMES and v is None and not self.required:


I'm afraid I'm not happy with this. This is the most performance critical part of the whole of pydantic.

I think we can fix this bug without modifying this funciton.

samuelcolvin · 2020-03-18T19:51:05Z

pydantic/fields.py

@@ -705,6 +705,10 @@ def _apply_validators(
    ) -> 'ValidateReturn':
        for validator in validators:
            try:
+                # optional field set to None should be valid unless a custom validator handles this case
+                if validator.__name__ in DEFAULT_VALIDATORS_NAMES and v is None and not self.required:


Instead modify ConstrainedList to something like

# This types superclass should be List[T], but cython chokes on that... class ConstrainedList(list): # type: ignore # Needed for pydantic to detect that this is a list __origin__ = list __args__: List[Type[T]] # type: ignore min_items: Optional[int] = None max_items: Optional[int] = None item_type: Type[T] # type: ignore @classmethod def __get_validators__(cls) -> 'CallableGenerator': yield cls.list_length_validator @classmethod def __modify_schema__(cls, field_schema: Dict[str, Any]) -> None: update_not_none(field_schema, minItems=cls.min_items, maxItems=cls.max_items) @classmethod def list_length_validator(cls, v: 'List[T]', field) -> Optional['List[T]']: if v is None and not field.required: return None v = list_validator(v) v_len = len(v) if cls.min_items is not None and v_len < cls.min_items: raise errors.ListMinLengthError(limit_value=cls.min_items) if cls.max_items is not None and v_len > cls.max_items: raise errors.ListMaxLengthError(limit_value=cls.max_items) return v

samuelcolvin · 2020-03-18T19:51:57Z

tests/test_main.py

@@ -1025,3 +1026,21 @@ class FunctionModel(BaseModel):

    m = FunctionModel()
    assert m.uid is uuid4
+
+
+@pytest.mark.skipif(sys.version_info < (3, 7), reason='field constraints are set but not enforced with 3.6 and above')


surely this can be removed then?

As a matter of fact it seems it can't :/
The problem lies here

def go(type_: Any) -> Type[Any]: if is_literal_type(annotation) or isinstance(type_, ForwardRef) or lenient_issubclass(type_, ConstrainedList): return type_ origin = getattr(type_, '__origin__', None)

where typing.List.__origin__ is list on python 3.7+ but None on python 3.6
Not sure if we want to have a get_origin method to handle python 3.6 or if there is a backport of any kind

PrettyWood · 2020-03-18T22:59:10Z

pydantic/types.py

        yield cls.list_length_validator

    @classmethod
    def __modify_schema__(cls, field_schema: Dict[str, Any]) -> None:
        update_not_none(field_schema, minItems=cls.min_items, maxItems=cls.max_items)

    @classmethod
-    def list_length_validator(cls, v: 'List[T]') -> 'List[T]':
+    def list_length_validator(cls, v: 'Optional[List[T]]', field: 'ModelField') -> 'Optional[List[T]]':  # type: ignore[name-defined] # noqa: E501,F821


@samuelcolvin Thank you for the review ! Just a question on the mypy policy as I could

not set the type of field and just add # type: ignore

same but explaining the ignored error but then add a noqa for length

do some refactoring to avoid circular imports (overkill probably)

let it like that

you just needed to import ModelField, imports inside if TYPE_CHECKING: don't cause circular import problems.

PrettyWood force-pushed the fix/none-mix-max-items branch 3 times, most recently from 25cceab to 3eb1614 Compare March 13, 2020 03:45

PrettyWood commented Mar 13, 2020

View reviewed changes

samuelcolvin reviewed Mar 18, 2020

View reviewed changes

PrettyWood added 2 commits March 18, 2020 22:54

ignore pyenv generated file

84b2f21

Allow None as input to all optional list fields

2fa1ed2

PrettyWood force-pushed the fix/none-mix-max-items branch from 3eb1614 to 2fa1ed2 Compare March 18, 2020 22:54

PrettyWood commented Mar 18, 2020

View reviewed changes

PrettyWood and others added 2 commits March 19, 2020 00:05

skip test on python 3.6

ca36cf5

fix mypy error, line length

d226cc3

samuelcolvin merged commit 2c05415 into pydantic:master Apr 15, 2020

PrettyWood deleted the fix/none-mix-max-items branch April 15, 2020 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

accept None as value for all optional fields #1307

accept None as value for all optional fields #1307

PrettyWood commented Mar 13, 2020 •

edited

codecov bot commented Mar 13, 2020 •

edited

PrettyWood Mar 13, 2020

samuelcolvin Mar 18, 2020

samuelcolvin Mar 18, 2020

PrettyWood Mar 18, 2020

samuelcolvin Mar 18, 2020

PrettyWood Mar 18, 2020 •

edited

samuelcolvin Mar 18, 2020

samuelcolvin Mar 18, 2020

samuelcolvin Mar 18, 2020

PrettyWood Mar 18, 2020 •

edited

PrettyWood Mar 18, 2020

samuelcolvin Apr 15, 2020

		@@ -0,0 +1 @@
		fix : value None should be valid for all optional fields by default

	fix : value None should be valid for all optional fields by default
	Allow `None` as input to all optional fields

accept None as value for all optional fields #1307

accept None as value for all optional fields #1307

Conversation

PrettyWood commented Mar 13, 2020 • edited

Change Summary

Related issue number

Checklist

codecov bot commented Mar 13, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrettyWood Mar 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrettyWood Mar 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrettyWood commented Mar 13, 2020 •

edited

codecov bot commented Mar 13, 2020 •

edited

PrettyWood Mar 18, 2020 •

edited

PrettyWood Mar 18, 2020 •

edited