Added deferred validators support to colander.All validator #70

Open
wants to merge 2 commits into from

6 participants

@igudym

It's about issue #69

@jayd3e
Pylons Project member

So I don't know about anyone else, but I vote we change the All validator to act like an iterable, making the default syntax for the 'validator' kwarg the same as the 'preparer' kwarg, in that it accepts a list of validators. This just seems like a more pythonic API to me. This would also make it far easier to deal with deferreds, b/c during a bind, we could just check if the value of self.validator is an iterable, if it is, we iterate through it and call all of the deferreds for the selected node. That's just my take on what should happen.

@jayd3e
Pylons Project member

So I guess in hindsight, it wouldn't make binding deferreds any easier, as that is effectively what you're doing atm. What I mentioned above would only have syntactical relevance.

@mcdonc
Pylons Project member

FWIW, I think @jayd3e is right, even though 2 weeks ago I didn't ;-) validator=(v1, v2) probably makes more sense than an All validator ever did.

@jayd3e
Pylons Project member

Haha yes! I used lists in soap for validator and preparer. I get why you did it that way tho, b/c formencode had an api like that correct? Any reason you would use a tuple instead of a list?

@jayd3e
Pylons Project member

Or I guess it should really be allowed to accept any interable.

@jayd3e
Pylons Project member

Can you accept this pull request @mcdonc?

@mcdonc
Pylons Project member

Sorry for the very late response to this. I'd actually prefer to not accept this pull request, and instead just deprecate All and tell people to use "SchemaNode(validator=(v1, v2), ...)" instead. Does anyone have the time to make such a feature work? I may release an 1.0a1 soon, but we could do it after that.

@jayd3e
Pylons Project member

If you're not in a rush, I can get this done after the 24th.

@mcdonc
Pylons Project member

I'd still like to do this, but now I'm handwringing about how to present errors if we change validator= to accept a sequence. colander.All() bundles up all validation errors into a composite error, but you had a problem with this the other day @jayd3e didnt you.

@jayd3e
Pylons Project member

Yah, specifically while using All() on a parent schema. I'm not quite sure how to do this consistently yet. The solution I eventually came to, was that I just now treat both situations completely separately. I use this composite validator when I need multiple validators on a parent schema:

class SchemaAll(colander.All):
    """
    The All validator doesn't work on a root SchemaNode. This is a replacement,
    that combines the Invalid exceptions appropriately.
    """
    def __call__(self, node, value):
        excs = []
        for validator in self.validators:
            try:
                validator(node, value)
            except ComboBreaker as e:
                excs.append(e)
                break
            except Invalid as e:
                excs.append(e)

        if excs:
            exc = Invalid(node)
            for e in excs:
                exc.add(e)
            raise exc

It allows me to throw an Invalid exception specific to single sub_node like so:

def validate_repeat_password(node, value):
    if value['password'] != value['repeat_password']:
        raise Invalid(node.get('repeat_password'), 'These passwords do not match')

While if the parent schema only needs a single validator, I will throw an Invalid exception specific to a single sub_node like so:

def validate_pass(node, value):
    db = node.bindings['db']

    # Get user
    auth_user = AuthUser.by_username(db, value['username'])
    if not auth_user or not auth_user.validate_password(value['password']):
        exc = Invalid(node)
        exc['password'] = 'Incorrect password'
        raise exc

As you can see separating the methods of achieving this is less than ideal. I could just wrap the single validator cases with my SchemaAll() to be more consistent, but that is kind of covering up the underlying issue here. But yah, would love to talk about how to handle this. In my opinion, we need to achieve five main things:

  1. Give users a way to attach more than one validator to any one SchemaNode() object through specifying a list/tuple of them.
  2. The structure of Invalid exceptions should mimic, the structure of the overall schema tree. Where each Invalid is associated with its own node.
  3. asdict() should return a dict, where multiple AND single errors attached to a node are represented as a list. i.e.:
{
    'username': ['non-existent username', 'too long']
}
  1. As opposed to flattening a dict, we should just maintain the identical structure for error reporting. So currently, colander does something like this:
{
    'solutions.0.revision': 'wrong revision number'
}

I purpose we make asdict() return this:

{
    'solutions': [{
        'revision': ['wrong revision number']
    }]
}
  1. Have a clear method(defined in the docs) of allowing a validator to make an exception for one of its children. This would solve the problem I was talking about yesterday.

I don't have specific methods of achieving some of these yet, but I just wanted to get your opinion on everything. I already wrote some of these features in https://github.com/jayd3e/soap/blob/master/soap/__init__.py. I copied Colander's software design in Soap almost exactly, and just changed the parts that I found kind of confusing in Colander. I also added the whole ability to create relationships between parent schemas, but we're just going to forget about that little addition b/c it was a complete disaster :). I think it's at-least worth a look at how I did it. Might be worth it to use some of the changes in Colander. I would be happy to implement whichever changes you want in Colander, as it is a very large portion of my site. I just need to know what direction you want to go with this.

@jayd3e
Pylons Project member

And optionally on the list should be a way to break the validation chain. I have a ComboBreaker exception that will break out of the validation loop in my code. This is helpful when you have validators down the chain that depend on earlier validators not throwing an error.

@tdavis

I was just hoping for this behavior and came around to see if an issue existed for it. The way I thought it should work was simply to let me pass an interable to validator and break the chain if any of them raise Invalid.

@jayd3e
Pylons Project member

@tdavis sometimes you want to be able to catch all existing errors in a schema. So if we were to break the chain of validation, some errors wouldn't be caught.

@tseaver
Pylons Project member

I believe #95 is @jayd3e's stab at implementing the sequence-of-validators bit. Can we close this PR in its favor?

@mmerickel
Pylons Project member

Since we've accepted colander.Any I'd prefer this PR over an iterable. A list of validators now means more than one thing and assuming All is not so great IMO.

@igudym any chance you'd be able to rebase this on master and add your name to CONTRIBUTORS.txt?

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