WIP: Add Field#validator for registering field validation methods

[sloria]

Implements #367

from marshmallow import Schema, fields

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

    @quantity.validator
    def validate_quantity(self, value):
        if quantity > 30:
            raise ValidationError('Must not be less than 30.')

TODO

• Docs
• Mark @validates for PendingDeprecation
• Fix override behavior (see WIP: Add Field#validator for registering field validation methods #368 (comment))

[justanr]

Not that I forsee a huge problem, but won't this bind the validator to the first schema it's on, rather than to the inheritance chain, unless I missed some Juju in SchemaMeta. I can't test my hunch as I'm mobile, but I think this shouldn't produce the expected results...

class MySchema(Schema):
    bar = DecimalField()

    @bar.validator
    def error(self, *a, **k):
        raise ValidationError(self.__class__.__name__)


class SubSchema(MySchema):
    pass

SubSchema.dump(dotteddict(bar=4)).errors 

But I could be wrong. Another sticky part could be child schemas that have stricter validation needs. How would this react to:

class SubSchema(MySchema):
    @MySchema.bar.validates
    def stricter a validation(...):
        ...

[justanr]

Pretend that last bit is a valid python name. Stupid autocorrect.

[sloria]

@justanr You raise valid concerns in #368 (comment) (which I'm moving out here so this discussion doesn't get lost if the code changes).

So we have 2 cases to address:

1. Validator methods should be bound to the schema they were defined on (not parent classes).
2. Validator methods should be override-able.

Since Field#_add_to_schema gets called on Schema instantiation--not in SchemaMeta--case 1 turns out to be a non-issue. I've verified the behavior in this test

marshmallow/tests/test_decorators.py

Line 376 in b30b705

def test_method_validator_inheritance(self):

.

Case 2 is currently broken. Overriding a validator method in a concrete Schema class will not prevent its parent validator method from being invoked.

Also, I had to change SchemaMeta such that fields were accessible as class variables (by passing pop=False here: https://github.com/marshmallow-code/marshmallow/blob/b30b705b836a86f722a89c8cf11b9d01ac17091d/marshmallow/schema.py#L97)This is a breaking change, but without it, we can't do @MySchema.bar.validate. @jmcarp Do you recall why we decided to remove fields as class variables in the first place?

[justanr]

I missed that sneaky assert in that test case. I have always found it odd that fields aren't accessible as properties - though, I can see the logic about that if it's possible to access them in a broken state that way.

I guess my last question would be since one instance is shared through the inheritance chain. Would validators registered on child schemas be run when the parent runs the validate method?

Going back to my second example, would

MySchema().dump(...)

Run the stricter_validation from the child?

Sorry to ask a bunch of questions, just trying to stem potential edge cases before they're actual issues.

[jmcarp]

I submitted some test fixes in #400, but now that I'm thinking about this proposal, I have a few reservations. Or maybe I'm just confused--it's hard to tell the difference.

• I think the decision to keep fields off schema classes happened before I started using marshmallow, but it makes sense to me--if fields are available off the schema, we have to worry about conflicts with other class and instance variables. It seems like we'd have to keep users from naming fields extra, loads, opts, etc., since those names are reserved by Schema.
• As implemented in WIP: Add Field#validator for registering field validation methods #368, a decoration like @MySchema.bar.validates in a class that subclasses MySchema can do potentially bad things, since it mutates the field object on MySchema. If subclass SubSchema adds a validator to a field in MySchema, and we then create another subclass of MySchema called SubSchema2, the second subclass will also use the validator added by its sibling, since all three schema classes refer to the same field object.

Assuming I'm not off-base so far, I'm wondering if we should take field objects back off the schema and not allow schema subclasses to add new validators to fields defined in their parents, since both behaviors lead to edge cases and possibly counter-intuitive behavior. Would that make this interface too hard to use?

[sloria]

@jmcarp You make a good case for keeping fields off of schema classes. I am still unsure how to answer the question about validators defined on subclasses. I'll have to give this more thought, but we definitely should prevent subclasses from adding validators to parents.

[justanr]

@sloria Not sure how willing you are to do this, but instance variables defined in metaclasses are available to classes but not to instances of those classes. It's an interesting effect of the type system and potentially useful for this, but could cause a lot of confusion. Classes can have things defined as one name and instances can overwrite those and provide their own values for them, not affecting the class at all.

For an example: https://gist.github.com/justanr/52301d3af774f714a80d

The Builder.of is only available as an attribute on the Builder class but not instances of it.

Something to mull over at least. I'll also note, I've not verified that this is intended, documented or compatible between Python 2 and 3.

[justanr]

@sloria Some thoughts after mulling this over for a while.

By default, child schemas inherit their parents' validators. So, what do the following semantics mean:

class MySchema(Schema):
    bar = DecimalField()

    @bar.validator
    def error(self, *a, **k):
        raise ValidationError(self.__class__.__name__)


class SubSchema(MySchema):
    @MySchema.bar.validator
    def stricter(self, *a, **k):
         pass

Additionally, is it possible to have multiple validators bound to a single field:

class MySchema(Schema):
    bar = DecimalField()

    @bar.validator
    def error(self, *a, **k):
        raise ValidationError

     @bar.validator
     def other(self, *a, **k):
          pass

I know in Python 2 this would always be non-deterministic registering, but it's possible to avoid in Python 3 by specifying __prepare__ = lambda *a, **k: OrderedDict() in SchemaMeta.

If the behavior is that, no you should register one and one validator callable per field, and child validators should override their parents for their own instances, I think a solution is possible using some metaclass magic.

A very basic (and potentially wrong) example could be:

class Field(object):
    def __init__(self, *args, **kwargs):
         self.validators = {}

    def validator(self, func):
        func.__validator_for__ = self
        return func

    def _run_validators(self, schema, value):
        # wrong for finding parent validators, we should look to py3 functools._find_impl for inspiration
        registered = self.validators.get(type(schema), lambda schema, value: value)
        registered(schema, value)

class SchemaMeta(type):
    def __init__(cls, name, bases, attrs):
        super(cls, cls).__init__(cls, name, bases, attrs)

        for possible in attrs.values():
            field = getattr(possible, '__validator_for__', None)
            if not field:
                continue

            field.validators[cls] = possible

If the behavior is that, yes you can register multiple validation methods for a single field, then the solution is still possible but trickier because now we also need to walk the MRO of the Schema in question and call all parent validators in MRO.

[sloria]

Closing this for now, as I don't plan on working on this in the immediate future (at least for 3.0 final). Thanks @justanr and @jmcarp for the feedback you gave on this; we'll will definitely refer back to your comments if we decide to pick this back up.