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

Voluptuous cannot support multi-field validation #124

Closed
jessedhillon opened this issue Aug 7, 2015 · 12 comments · Fixed by #341
Closed

Voluptuous cannot support multi-field validation #124

jessedhillon opened this issue Aug 7, 2015 · 12 comments · Fixed by #341

Comments

@jessedhillon
Copy link

Hi @alecthomas, it looks like Voluptuous is not super active, but I thought I'd raise an architectural point with you. It seems like there's no way to write a validation which depends on the value of another field. For example, there's no way to require that a field called password and confirm_password actually match. Or that a field called size actually refers to a size that is in stock for the product referenced by product_id.

Am I wrong, and if so, how would I do those kinds of validations? If not, are there any plans to support them?

@alecthomas
Copy link
Owner

Yes, this is definitely a limitation of Voluptuous. I haven't come up with a good design solution, but I'd be interested in adding it if something elegant presented itself.

@jessedhillon
Copy link
Author

The most obvious way to me would be to introduce the concept of events, and have an after_validate event which can be subscribed to by anyone who needs whole schema validation. I see it working one of two ways here, either you use some sort of event subscription interface to attach listeners:

def passwords_match(values):
  if values['password'] != values['confirm_password']:
    raise Invalid(...) 
s = Schema(...)
s.on('after_validate', passwords_match)

Or you could use inheritance:

class SignupSchema(Schema):
  def after_validate(self, values):
    if values['password'] != values['confirm_password']:
      raise Invalid(...)

s = SignupSchema(...)  # same constructor

@jason-curtis
Copy link
Contributor

jason-curtis commented Dec 21, 2015

Can't this be done trivially with All() and a custom validator function?

def passwords_must_match(passwords):
    if passwords['password'] != passwords['password_again']:
        raise Invalid('passwords must match')
    return passwords

s=Schema(All(
    {'password':str, 'password_again':str},
    passwords_must_match
))

# valid
s({'password':'123', 'password_again':'123'})

# raises MultipleInvalid: passwords must match
s({'password':'123', 'password_again':'and now for something completely different'})

This leads to a reasonably clean pattern where you use All to first validate the general structure of your document (by passing in a regular schema dict) and then move on to your more special rules that involve more than one field.

Extending a little further to the password length-checking case:

def password_min_length(passwords):
    if len(passwords['password']) < passwords['min_length']:
        raise Invalid('password should be at least {} characters'.format(passwords['min_length']))
    return passwords

s2=Schema(All(
    {'password':str, 'password_again':str, 'min_length':int},
    passwords_must_match,
    password_min_length
))

# MultipleInvalid: password should be at least 5 characters
s2({'password':'123', 'password_again':'123', 'min_length': 5})

@cjw296
Copy link
Contributor

cjw296 commented Feb 23, 2016

@thatneat: 👍 :-)

@aaronkavlie-wf
Copy link

@thatneat I had the same need (to compare start/end dates in a schema), and used All as the solution. That prevents accumulation of errors though. The schema dictionary is validated, then (if there are no errors) the custom validator is run. You get errors from one or the other — not both.

@jason-curtis
Copy link
Contributor

Yeah, that is a limitation of this approach. I think it's actually a good thing in the cases I've seen - for instance, it means that your validate_start_and_end_dates_in_correct_order() function does not have to worry about the fact that the inputs may not even be valid dates. If the second-tier validator got run in any case, you'd want to write it with guarding logic about, for instance, the case where one date is valid but not the other, etc, so that you didn't end up throwing all kinds of exceptions. If the dates aren't valid in the first place, the more complex validator isn't going to do you any good anyway.

I suppose you could write a different version of All that goes ahead and tries all the validators even if an earlier one failed, but that's not how it works out of the box. The output of each validator in an All is passed to the next so that downstream validators can take advantage of any validation and coercion that might be happening upstream.

@aaronkavlie-wf
Copy link

Yeah, this is true... in a theoretically multi-field API it could be in All on just the "end" field — but it would have to have the output of the "start" field conversion as an input, somehow.

@jason-curtis
Copy link
Contributor

Ah, I see, do you have other fields as well that shouldn't affect whether the validate_start_and_end_dates_in_correct_order validator runs? I'm guessing your situation is something like:

s=Schema(All(
    {'password': str, 'password_again': str, 'totally_unrelated': int},
    passwords_must_match
))

# Ideally, this would be MultipleInvalid: 
# passwords don't match _and_ 'totally_unrelated' should be a number.
# Instead, you just get the 'totally_unrelated' error and it looks like the passwords are fine.
s({
    'password': 'abc',
    'password_again': 'does not match',
    'totally_unrelated': 'not a number'
})

I agree that there doesn't seem to be a way to do that well. Would the alternative version of All that I suggested work? like an IndependentAll which doesn't do chaining and raises MultipleInvalid instead of Invalid?

@aaronkavlie-wf
Copy link

In your example the chaining isn't necessary. With date comparisons, if All wraps the whole schema (as in your examples), then the chaining is needed for the date conversion (or you have to convert in your date comparison function as well).

@jason-curtis
Copy link
Contributor

I should have asked this earlier, but could you provide an illustrative example of what's wrong and what your ideal interface would be?

@aaronkavlie-wf
Copy link

Sorry I never got to this, the project I was using voluptuous on is no longer active.

@m-aciek
Copy link

m-aciek commented Jun 22, 2018

Shouldn't All pattern be documented and the issue closed then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants