Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(validation): accept null for optional union types. #24

Conversation

edwardgeorge
Copy link
Contributor

Validating a field that is an optional union will fail when the value is null:

>>> field = Field(annotation=Optional[Union[int,str]])
>>> field.select_validator()
>>> field.validate(None)
---------------------------------------------------------------------------

molten/validation/field.py in validate(self, value)
    181         if value is None:
    182             if not is_optional:
--> 183                 raise FieldValidationError("this field cannot be null")
    184
    185             return value

FieldValidationError: this field cannot be null

The cause of this is that when determining if a field is optional in
is_optional_annotation the simply checks the second argument of the Union.

This works when the annotation is Optional[X] which expands to
Union[X, NoneType] but not, for instance, with Optional[Union[X, Y]] which
expands to Union[X, Y, NoneType].

We could check the last argument to Union but that would still fail on the,
slightly more contrived, example of Union[Optional[X], Y] which expands to
Union[X, NoneType, Y] and for which null would be perfectly valid.

Therefore, in this implementation, is_optional_annotation is dropped as
that involves iterating over the arguments to determine if one is NoneType
and we also need to iterate over the arguments in extract_optional_annotation
to produce the "inner annotation" so I do both in the same place now instead.
is_optional_annotation is not referenced anywhere else in the project, but
as this affects what is possibly the external API this is possibly a breaking
change.

Adds tests for the examples in test_fields.py.

BREAKING CHANGE: removes is_optional_annotation from molten.typing

Validating a field that is an optional union will fail when the value is null:

```
>>> field = Field(annotation=Optional[Union[int,str]])
>>> field.select_validator()
>>> field.validate(None)
---------------------------------------------------------------------------

molten/validation/field.py in validate(self, value)
    181         if value is None:
    182             if not is_optional:
--> 183                 raise FieldValidationError("this field cannot be null")
    184
    185             return value

FieldValidationError: this field cannot be null
```

The cause of this is that when determining if a field is optional in
`is_optional_annotation` the simply checks the second argument of the Union.

This works when the annotation is `Optional[X]` which expands to
`Union[X, NoneType]` but not, for instance, with `Optional[Union[X, Y]]` which
expands to `Union[X, Y, NoneType]`.

We could check the _last_ argument to `Union` but that would still fail on the,
slightly more contrived, example of `Union[Optional[X], Y]` which expands to
`Union[X, NoneType, Y]` and for which `null` would be perfectly valid.

Therefore, in this implementation, `is_optional_annotation` is dropped as
that involves iterating over the arguments to determine if one is `NoneType`
and we also need to iterate over the arguments in `extract_optional_annotation`
to produce the "inner annotation" so I do both in the same place now instead.
`is_optional_annotation` is not referenced anywhere else in the project, but
as this affects what is possibly the external API this is possibly a breaking
change.

Adds tests for the examples in `test_fields.py`.

BREAKING CHANGE: removes `is_optional_annotation` from `molten.typing`
@Bogdanp
Copy link
Owner

Bogdanp commented Nov 13, 2018

Thanks! It looks like this breaks on 3.6 (since typing changed slightly between 3.6 and 3.7) so that has to be addressed. I'll merge it once you do.

@edwardgeorge
Copy link
Contributor Author

ah, i'll fix it up, thanks.

@Bogdanp
Copy link
Owner

Bogdanp commented Nov 24, 2018

Hey, @edwardgeorge any update on this? Want me to pick it up?

@edwardgeorge
Copy link
Contributor Author

Hi, @Bogdanp I've been away from a computer for a couple of weeks whilst moving. feel free to pick up, I probably won't be able to return to it until next week.

@Bogdanp Bogdanp closed this Jan 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants