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

Field validator as method is buggy/inconsistent #391

Closed
martinstein opened this Issue Feb 11, 2016 · 12 comments

Comments

Projects
None yet
5 participants
@martinstein

martinstein commented Feb 11, 2016

Let's take the example from the docs at http://marshmallow.readthedocs.org/en/latest/quickstart.html#field-validators-as-methods

If you run this:

from marshmallow import Schema, fields, validates, ValidationError

class ItemSchema(Schema):
    quantity = fields.Integer()

    @validates('quantity')
    def validate_quantity(self, value):
        if value < 0:
            raise ValidationError('Quantity must be greater than 0.')
        if value > 30:
            raise ValidationError('Quantity must not be greater than 30.')

schema = ItemSchema()
result, errors = schema.load({'quantity': 40})
print("result:", result) 
print("errors:", errors)

you get the result:

result: {'quantity': 40}
errors: {'quantity': ['Quantity must not be greater than 30.']}

instead of the empty result {} (because validation failed).

Just for comparison, the behavior is correct for validation via lambda-function:

class ItemSchema2(Schema):
    quantity = fields.Integer(validate=lambda x: 0 <= x <= 30)

schema = ItemSchema2()
result, errors = schema.load({'quantity': 40})
print("result:", result)
print("errors:", errors)

This time you get the expected result:

result: {}
errors: {'quantity': ['Invalid value.']}
@sloria

This comment has been minimized.

Member

sloria commented Feb 14, 2016

Thanks for reporting @martinstein . I would certainly review and merge a PR addressing this inconsistency.

@jmcarp

This comment has been minimized.

Contributor

jmcarp commented Feb 21, 2016

It looks like this inconsistency might get fixed for free once #368 is merged and validates is deprecated.

@vuonghv

This comment has been minimized.

Contributor

vuonghv commented Feb 24, 2016

@sloria @jmcarp
Do you think we really need a PR to fix this issue because validates is deprecated in the future (#368)?

@sloria

This comment has been minimized.

Member

sloria commented Feb 25, 2016

Even though validates will be deprecated after #368, I would gladly review and merge a PR fixing the inconsistency.

@maximkulkin

This comment has been minimized.

Contributor

maximkulkin commented Jul 18, 2016

Isn't that if validation fails, the contents of result does not make sense and could just be garbage/inconsistent? Where can I find documentation on guarantees of what result will be in case of errors?

@vuonghv

This comment has been minimized.

Contributor

vuonghv commented Jul 18, 2016

I think that removing the invalid values from result will make getting all valid values easier by iterating result's keys. We do not have to check if the key is in errors or not.

@maximkulkin

This comment has been minimized.

Contributor

maximkulkin commented Jul 18, 2016

Valid values should be determined by absence of errors for that path. How will removing fields help detecting good values? Do you need ANOTHER validator to distinguish values that have all fields from values that do not have all fields?

Also, what do you do with those valid values in the event of invalid values presence? Are you making this up or there is a good use case for that?

@vuonghv

This comment has been minimized.

Contributor

vuonghv commented Jul 19, 2016

@maximkulkin Lets come back to the main purpose of this issue. We want to be consistent in returning result and errors, regardless of the ways we use validators (validates decorator or pass function to the validate param in Field's __init__).
Do you think it is a good idea?

@maximkulkin

This comment has been minimized.

Contributor

maximkulkin commented Jul 19, 2016

I just think there is no value in being consistent in uselessness and doing anything for that matter.

@martinstein

This comment has been minimized.

martinstein commented Jul 21, 2016

@vuonghv Thanks for providing this pull request. Fixing this inconsistency would be great.

@maximkulkin If you disagree with the original behavior of removing invalid values from results, I think that should be part of a separate issue/discussion. vuonghv's pull request is a clear improvement since it makes marhsmallow's behavior more consistent. Let's not fall into the trap where we ignore improvements in one area just because there might be disagreement in another area.

So I think this is definitely good to be merged.

@maximkulkin

This comment has been minimized.

Contributor

maximkulkin commented Jul 21, 2016

Please then mark this as backward incompatible as it changes current behavior.

@sloria

This comment has been minimized.

Member

sloria commented Jul 22, 2016

This is a bug, since the expected behavior is that @validates is consistent with field validators. Therefore, the PR is considered a fix rather than a breaking change.

@sloria sloria closed this Jul 22, 2016

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