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

Don't ignore all exceptions during coercions for nullable fields #490

Merged
merged 1 commit into from May 6, 2019

Conversation

@nikhaldi
Copy link
Contributor

@nikhaldi nikhaldi commented May 2, 2019

Only ignore exceptions if a value for a nullable field is not null.
Previously all exceptions during custom coerction were ignored with nullable
fields. The developer and/or user most likely wants to know about
exceptions that happen during coercion - this leads to hard to debug
problems otherwise.

The purpose of the e is not TypeError condition was unclear. As far as I
know this condition can never be false (e is an instance of an exception,
not an exception class).

I think this is still in line with the original intent of this code, which
was to make common coercions like float work with nullable fields out
of the box: #269

@nikhaldi
Copy link
Contributor Author

@nikhaldi nikhaldi commented May 2, 2019

The Travis failure for test_nested_error_paths is only on Python 3.5. Unclear to me what the problem is. It doesn't reproduce for me locally on 3.5. I'm going go force push a noop change to get Travis to rerun.

Loading

@nikhaldi nikhaldi force-pushed the fix/nullable-coercions branch from f5ece6b to 7921688 May 2, 2019
@nikhaldi
Copy link
Contributor Author

@nikhaldi nikhaldi commented May 2, 2019

The same test passed on the second try, so just appears to be flaky. The failure was https://travis-ci.org/pyeve/cerberus/jobs/527412260

Loading

Copy link
Member

@funkyfuture funkyfuture left a comment

thanks for bringing this up. could you please also update the CHANGES.rst and add yourself to the AUTHORS file?

Loading

cerberus/tests/test_normalization.py Outdated Show resolved Hide resolved
Loading
@@ -170,6 +170,15 @@ def test_nullables_dont_fail_coerce():
assert_normalized(document, document, schema)


def test_nullables_fail_coerce_on_non_null_values(validator):
schema = {'foo': {'coerce': lambda x: 1 / 0, 'nullable': True, 'type': 'integer'}}
document = {'foo': None}
Copy link
Member

@funkyfuture funkyfuture May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already covered in the previous test.

Loading

Copy link
Contributor Author

@nikhaldi nikhaldi May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I feel the repetition is a good idea here because the new behavior is about the difference between these two cases (the value is None or not).

Loading

Copy link
Member

@funkyfuture funkyfuture May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not saying you're wrong, but we don't do that elsewhere (oh dear, formulating this makes me feel old). i'm gonna amend this when merging.

Loading

@@ -723,7 +723,7 @@ def __normalize_coerce(self, processor, field, value, nullable, error):
try:
return processor(value)
except Exception as e:
if not nullable and e is not TypeError:
if not nullable or value is not None:
Copy link
Member

@funkyfuture funkyfuture May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i find this term more obvious: not (nullable and value is None)

Loading

Copy link
Member

@funkyfuture funkyfuture May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second thought: should the coercion get tried at all when the part in brackets is true?

Loading

Copy link
Contributor Author

@nikhaldi nikhaldi May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the condition.

Not sure there's an obvious answer to whether the coercion should be tried or not. Somebody might want to coerce None to something else? Or would the validation in that case fail at a later stage anyway?

Loading

Copy link
Contributor Author

@nikhaldi nikhaldi May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that NOT trying the coercion for nullable fields when the value is None would fix this issue: #427

To be honest, I don't feel I'm in a position to make the right call here. So happy to make this change or not.

Loading

Copy link
Member

@funkyfuture funkyfuture May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for digging this out. that issue concluded to postpone changes until the 2.0 release; and i think i shall document that this still needs to be layed out design-wise.

please don't mark this conversation as resolved for visibility.

Loading

@funkyfuture
Copy link
Member

@funkyfuture funkyfuture commented May 3, 2019

The same test passed on the second try, so just appears to be flaky.

yes, Python 2.7 and 3.5 sort errors kind of erraticly. no worries.

Loading

@nikhaldi nikhaldi force-pushed the fix/nullable-coercions branch from 7921688 to 2797119 May 6, 2019
Copy link
Contributor Author

@nikhaldi nikhaldi left a comment

Thanks for the quick review! I updated AUTHORS and CHANGES too

Loading

cerberus/tests/test_normalization.py Outdated Show resolved Hide resolved
Loading
@@ -170,6 +170,15 @@ def test_nullables_dont_fail_coerce():
assert_normalized(document, document, schema)


def test_nullables_fail_coerce_on_non_null_values(validator):
schema = {'foo': {'coerce': lambda x: 1 / 0, 'nullable': True, 'type': 'integer'}}
document = {'foo': None}
Copy link
Contributor Author

@nikhaldi nikhaldi May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I feel the repetition is a good idea here because the new behavior is about the difference between these two cases (the value is None or not).

Loading

@@ -723,7 +723,7 @@ def __normalize_coerce(self, processor, field, value, nullable, error):
try:
return processor(value)
except Exception as e:
if not nullable and e is not TypeError:
if not nullable or value is not None:
Copy link
Contributor Author

@nikhaldi nikhaldi May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the condition.

Not sure there's an obvious answer to whether the coercion should be tried or not. Somebody might want to coerce None to something else? Or would the validation in that case fail at a later stage anyway?

Loading

@nikhaldi nikhaldi force-pushed the fix/nullable-coercions branch from 2797119 to 299bd92 May 6, 2019
Only ignore exceptions if a value for a nullable field is not null.
Previously all exceptions during custom coerction were ignored with nullable
fields. The developer and/or user most likely wants to know about
exceptions that happen during coercion - this leads to hard to debug
problems otherwise.

The purpose of the `e is not TypeError` condition was unclear. As far as I
know this condition can never be false (`e` is an instance of an exception,
not an exception class).

I think this is still in line with the original intent of this code, which
was to make common coercions like `float` work with nullable fields out
of the box: pyeve#269
@nikhaldi nikhaldi force-pushed the fix/nullable-coercions branch from 299bd92 to c51059d May 6, 2019
@nikhaldi
Copy link
Contributor Author

@nikhaldi nikhaldi commented May 6, 2019

Side note, test_nested_error_paths has failed 4 out of 5 times on Travis for me now. I'm not going to force a Travis rerun again until I know this is ready to merge.

Loading

@funkyfuture
Copy link
Member

@funkyfuture funkyfuture commented May 6, 2019

has failed 4 out of 5 times on Travis for me now

now, that's bad luck. but just ignore it.

Loading

@funkyfuture funkyfuture merged commit c51059d into pyeve:master May 6, 2019
1 check passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants