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

Rework schema errors #862

Merged
merged 5 commits into from Jul 11, 2018

Conversation

Projects
None yet
3 participants
@lafrech
Member

lafrech commented Jul 1, 2018

I tried to address issues raised in #857 (comment) by refactoring the code.

The first commit (9f84247) changes the error structure in a corner case, but in a good way. It could be considered a bugfix. I can send a separate PR for this one if needed.

The other commits are about factorizing the code that stores schema errors by adapting store_error (that normally stores field errors) while it is currently duplicated in Unmarshaller. I'm happy with this rework (not "this is so perfect" happy, but rather "this is much better" happy), except one test is broken.

When store_error receives a dict as message, it does not merge it, nor does it append it to the current error messages list. It just uses it as error message.

        if isinstance(messages, dict):
            errors[field_name] = messages

Hence the test I had to modify:

-        assert errors['_schema'][0] == {'code': 'invalid_field'}
+        assert errors['_schema'] == {'code': 'invalid_field'}

This differs from the behaviour of the code duplicated in Unmarshaller. And changing this in store_error would break a lot of field errors tests.

While working on this, I ended up facing the issues described in #441. I think I'll either follow-up there or create a separate issue.

@lafrech lafrech force-pushed the dev_rework_schema_errors branch from f4e2c24 to cbf1ee8 Jul 2, 2018

@lafrech

This comment has been minimized.

Member

lafrech commented Jul 2, 2018

Rebased.

@sloria

This comment has been minimized.

Member

sloria commented Jul 3, 2018

I like the refactoring done here; the addition of store_validation_error makes sense.

Re: the changed test: I think it's an OK change. There is a valid use case for using dictionaries (rather than strings) as individual error messages. marshmallow-jsonapi, for example, does this. But this use case is still supported by this PR, since the user can pass a list of dictionaries to ValidationError. Also, I think you're right to point out that this change could set up #441 nicely.

Because this is a breaking change, it'd be good to have one more set of eyes on this.

@sloria sloria requested a review from deckar01 Jul 3, 2018

@lafrech

This comment has been minimized.

Member

lafrech commented Jul 3, 2018

Thanks for the feedback.

This PR does not really address #441. It is just a small step towards refactoring the error structure. Maybe it will help by centralizing the problem in one single piece of code. Regarding #441, I'd like to list all cases (different types like string, list, dict,... and which fields returns what) to get a clearer view and decide what should be valid or not. But that can be in a further step.

self.error_field_names.append(field_name)
errors = self.get_errors(index=index)
# Warning: Mutation!
if isinstance(error.messages, dict):
errors[field_name] = error.messages

This comment has been minimized.

@deckar01

deckar01 Jul 3, 2018

Member

When store_error receives a dict as message, it does not merge it ... This differs from the behaviour of the code duplicated in Unmarshaller. And changing this in store_error would break a lot of field errors tests.

It looks like fields were originally (8340fca5) allowed to append dicts to the error list, but in a seemingly unrelated bug fix (4698fe51) it was regressed.

It's probably fine to overwrite some schema errors since field errors have been overwritten for so long, but it might be worth investigating what errors this is hiding. When the field error tests fail, is it because there are extra errors that the tests don't expect, or does it violate core expectations about the structure of the error? Are the hidden errors important?

@lafrech

This comment has been minimized.

Member

lafrech commented Jul 10, 2018

For the record, while working on the error structure (#441), I noticed that this PR fixes (what I think is) a corner case issue.

I modified a test in test_deserialization as a quick way of testing the output when errors are raised from different validators:

    def test_nested_single_deserialization_to_dict(self):
        class SimpleBlogSerializer(Schema):
            title = fields.String()
            author = fields.Nested(SimpleUserSchema, required=True)
            @validates('author')
            def validate_author(self, value):
               if value != 42:
                   raise ValidationError('The answer to life the universe and everything.')
            @validates_schema(skip_on_field_errors=False)
            def validate_schema(self, data):
                raise ValidationError('This will never work', 'author')

        blog_dict = {
            'title': 'Gimme Shelter',
            'author': {'name': 'Mick', 'age': 'lol', 'email': 'mick@stones.com'},
        }
        result = SimpleBlogSerializer().load(blog_dict)

Without this PR, the error returned is

ValidationError: {'author': {'_field': ['The answer to life the universe and everything.'], 'age': ['Not a valid number.'], '_schema': ['This will never work']}}

which is wrong: there shouldn't be _schema, here.

With this change, I get

ValidationError: {'author': {'age': ['Not a valid number.'], '_field': ['The answer to life the universe and everything.', 'This will never work']}}

which is the expected output.

@sloria

This comment has been minimized.

Member

sloria commented Jul 10, 2018

@deckar01 Do you see any blockers to merging this?

@deckar01

This comment has been minimized.

Member

deckar01 commented Jul 10, 2018

Feel free to ignore #862 (comment). It is similar to existing behavior and it doesn't seem to be bothering anyone. Looks good to me otherwise.

@sloria

This comment has been minimized.

Member

sloria commented Jul 10, 2018

Thanks @deckar01. @lafrech, care to update the changelog and upgrading guide?

@lafrech lafrech force-pushed the dev_rework_schema_errors branch from cbf1ee8 to c3ddbfb Jul 11, 2018

@lafrech

This comment has been minimized.

Member

lafrech commented Jul 11, 2018

I rebased and updated the changelog. @sloria, do you see anything to add to the upgrading guide?

I still don't like the way store_error works, and the breaking change underlined by @deckar01 makes it slightly worse, but it is not that bad as long as we get this sorted for 3.0.

I think this should be dealt with in a wider reflection and I hope #879 will help clarifying things.

@@ -4,11 +4,19 @@ Changelog
3.0.0b13 (unreleased)
+++++++++++++++++++++
Bug fixes:
- Errors reported by a schema leval validator for a field in a ``Nested`` field

This comment has been minimized.

@sloria

sloria Jul 11, 2018

Member

Typos: "schema-level"; "is" -> "are"

This comment has been minimized.

@lafrech

lafrech Jul 11, 2018

Member

Oh, sorry. Fixed.

@sloria

This comment has been minimized.

Member

sloria commented Jul 11, 2018

It would be good to document the changed behavior with raise ValidationError({...}) in the upgrading guide. Certainly not a blocker to merging this if you don't have time right now, though.

@lafrech lafrech force-pushed the dev_rework_schema_errors branch from c3ddbfb to 96b33bb Jul 11, 2018

@lafrech

This comment has been minimized.

Member

lafrech commented Jul 11, 2018

Yeah, time and not sure how to formulate this.

I'd better leave this to you when you get the time.

@sloria sloria merged commit aa32c41 into dev Jul 11, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details

@sloria sloria deleted the dev_rework_schema_errors branch Jul 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment