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

Rework validation error structure #1026

Merged
merged 16 commits into from Nov 30, 2018

Conversation

Projects
None yet
2 participants
@lafrech
Copy link
Member

lafrech commented Oct 26, 2018

This PR implements #441 using the merge function implemented in #442, and it addresses a few of the concerns expressed in #441 and #879.


New validation error specification:

  • A ValidationError has a message and a field_name. If field_name is not specified, the error applies to the schema and is stored under the _schema key.

  • message can be a dict, a string or a list. Passing a string is a shortcut for passing a list with that string as unique element. When message is a dict, its keys are supposed to be field names, or an equivalent hierarchy (like in the dict messages generated by List or Field), so that several dict messages can be deep-merged together.

  • It is not possible anymore to raise a ValidationError with a message applying to several fields by passing a message as string and a list of filed names. This is a special case of a schema-level error with the messages expressed as a dict.

  • When a Nested field has both errors in its fields and errors in itself, its own errors are not stored under _field but under _schema. From client perspective, schema or field is an implementation detail.


Fixes:

  • Fixes the case where a pre/post_dump/load validator returns an error as dict.
  • Allows two schema-level validators to return errors as dict on the same field. The errors messages are merged.
  • Rewrite test_passing_original_data to test only what it is meant to and not use a wrong error structure. There was a long argument in #441/#442 about whether the {'code': 'invalid_field'} message was wrong or not, and whether forbidding such a message would be a breaking change. It is wrong according to the specifications described above, which may be a breaking change (we're changing the error structure, the values expected in a ValidationError) but that's fine, as it is "breaking changes" season.
  • index = index if index_errors else None is factorized which should provide a ridiculously marginal performance gain in most cases but also ensure index_errors is respected all along, even in case of invalid input type error.

@lafrech lafrech added this to the 3.0 milestone Oct 26, 2018

@lafrech lafrech requested review from sloria and deckar01 Oct 26, 2018

@lafrech lafrech force-pushed the dev_rework_validation_error branch from 3373096 to b0fcd3f Oct 28, 2018

@sloria

This comment has been minimized.

Copy link
Member

sloria commented Oct 28, 2018

On first glance, the implemented behavior looks desirable. I think this is the right direction to take.

Fixes the case where a pre/post_dump/load validator returns an error as dict.

Can you clarify this?

Should we prevent keys for unknown fields to passed to ValidationError?

This PR allows arbitrary keys to be passed:

from marshmallow import Schema, fields, pre_load, ValidationError

class MySchema(Schema):
    foo = fields.Field()

    @pre_load
    def always_error(self, data):
        raise ValidationError({
            'foo': 'foo error',
            'unknown': '<-- unknown key',
        })


MySchema().load({'foo': 42})
# marshmallow.exceptions.ValidationError: {'foo': 'foo error', 'unknown': '<-- unknown key'}
@lafrech

This comment has been minimized.

Copy link
Member Author

lafrech commented Oct 28, 2018

Fixes the case where a pre/post_dump/load validator returns an error as dict.

Can you clarify this?

This is the third commit of the PR ("Fix handling dict error on field in pre/post_dump/load", 0aeee44).

I figured a decorator (pre/post_dump/load) could raise errors similar to schema errors, by passing a message and an optional field name, and the message could be a dict. Without the change I made in normalized_message, the field name is lost if the message is a dict.

I realized afterwards that the logic is the same in normalized_message and store_error, so the comment in store_error applies.

        # field error  -> store/merge error messages under field name key
        # schema error -> if string or list, store/merge under _schema key
        #              -> if dict, store/merge with other top-level keys

Should we prevent keys for unknown fields to passed to ValidationError?

I don't see any problem with this. I think this is something you purposely allowed (#273).

@sloria

This comment has been minimized.

Copy link
Member

sloria commented Oct 28, 2018

Without the change I made in normalized_message, the field name is lost if the message is a dict.

Ah, I see. Thanks for the clarification.

I don't see any problem with this. I think this is something you purposely allowed (#273).

Ah, good catch. Forgot about that issue. OK, I think the behavior is fine as is.

@lafrech

This comment has been minimized.

Copy link
Member Author

lafrech commented Oct 28, 2018

Note that as opposed to the proposal in #442, I wouldn't consider merge_errors public API as I don't see the benefit for this. I didn't underscore the name, but it is not documented and not advertised as public.

Also I didn't copy the ValidationErrorBuilder. It's never too late to add something like that if we feel the need, but I wanted to keep this minimal, it is already an important change, and in most cases, creating the dict should be easy enough not to need this.

@lafrech lafrech force-pushed the dev_rework_validation_error branch from b0fcd3f to d1c424d Oct 28, 2018

@sloria

This comment has been minimized.

Copy link
Member

sloria commented Oct 28, 2018

I wouldn't consider merge_errors public API as I don't see the benefit for this.

Agreed. All of marshmallow.marshalling is considered private API.

It's never too late to add something like that if we feel the need, but I wanted to keep this minimal,

👍

@sloria

This comment has been minimized.

Copy link
Member

sloria commented Oct 28, 2018

@lafrech Is there anything left to do with this PR except for update the upgrading docs and changelog?

@lafrech

This comment has been minimized.

Copy link
Member Author

lafrech commented Oct 28, 2018

Nothing, I believe. I wasn't totally satisfied until the last rework. Now, I think it is complete.

It would be nice to have @deckar01's eye on this since he commented on #441/#442.

@lafrech lafrech force-pushed the dev_rework_validation_error branch from af1be3d to 106aa46 Oct 28, 2018

@lafrech lafrech force-pushed the dev_rework_validation_error branch from 106aa46 to a3df236 Nov 6, 2018

@lafrech

This comment has been minimized.

Copy link
Member Author

lafrech commented Nov 6, 2018

Rebased.

Shall we merge this or shall we wait for another reviewer?

@lafrech lafrech force-pushed the dev_rework_validation_error branch from a3df236 to fb614a6 Nov 15, 2018

@lafrech

This comment has been minimized.

Copy link
Member Author

lafrech commented Nov 15, 2018

Rebased.

I'd be glad if we could merge this so as to limit merge conflicts.

The sooner it is merged and beta released, the sooner we get feedback...

@sloria

sloria approved these changes Nov 30, 2018

Copy link
Member

sloria left a comment

Took a second look and the behavior looks correct.

Had a couple nits but those definitely aren't blockers.

Let's get this merged!

(recursively). Format is the same as accepted by :exc:`ValidationError`.
Returns new error messages.
"""
if errors1 is None:

This comment has been minimized.

Copy link
@sloria

sloria Nov 30, 2018

Member

I think this can be simplified to

Suggested change
if errors1 is None:
if not errors1:

Same with L263.

if errors2 is None:
return errors1
if isinstance(errors1, list):
if not errors1:

This comment has been minimized.

Copy link
@sloria

sloria Nov 30, 2018

Member

This conditional shouldn't be necessary.

@sloria sloria merged commit 0efbd7f into dev Nov 30, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@sloria sloria deleted the dev_rework_validation_error branch Nov 30, 2018

@lafrech

This comment has been minimized.

Copy link
Member Author

lafrech commented Nov 30, 2018

Thanks @sloria for reviewing and merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.