Voluptuous gives confusing error messages for Any(...) schemas
Categories
(Firefox Build System :: Task Configuration, task)
Tracking
(Not tracked)
People
(Reporter: dustin, Unassigned, Mentored)
References
Details
We use Voluptuous to do schema validation in the task-graph generation code (https://searchfox.org/mozilla-central/source/taskcluster/taskgraph). In particular, we often have schemas that are sort of like C/C++ unions: they can have any of a few forms, determined by a single field that gives the name of the form. For example, https://searchfox.org/mozilla-central/rev/9e7995b3c384945740985107a4de601b27904718/taskcluster/taskgraph/transforms/task.py#221 The problem is, if you get something wrong in a task definition, you get a super-confusing error message from voluptuous, because it doesn't know which of the Any(..) alternatives you meant. It doesn't know about the field that discriminates between the alternatives (in the example linked above, `implementation`). Here's a boiled-down replication recipe: $ cat test.py from voluptuous import Any from taskgraph.util.schema import ( validate_schema, Schema, ) sch = Schema({ # this field is keyed by 'type' 'implementation': Any({ 'type': 'A', 'a-value': str, }, { 'type': 'B', 'b-value': int, }, { 'type': 'C', 'c-value': bool, }) }) # should be OK validate_schema(sch, { 'implementation': { 'type': 'A', 'a-value': 'hello', }, }, "example 1") validate_schema(sch, { 'implementation': { 'type': 'B', 'b-value': 13, }, }, "example 2") # but this one gives a very confusing error message, because Voluptuous does # not realize this is a (malformed) type-C implementation. Most confusingly, # it says the value for 'type' is not allowed, when really it is! validate_schema(sch, { 'implementation': { 'type': 'C', 'c-value': None, }, }, "example 3") $ ./mach python test.py Let's figure out a way to solve this -- either just within the Firefox source or, better, as a fix to voluptuous itself.
Comment 1•6 years ago
|
||
I looked around a bit. I think this is where the issue is - https://github.com/alecthomas/voluptuous/blob/master/voluptuous/validators.py#L251 Validation of Any(..) is done in the order they are written - first it is validated against A, then B, then C. In the above example, each of these validations fail, and a `Invalid` exception is raised. Interestingly the `error` (that is returned in the end) is updated only when it is more nested in the dict than the already existing error. For the above example the validation error for all three types is at the same level of the dict. The error for type A is returned as that is the first to be validated. # Gives the same confusing error validate_schema(sch, { 'implementation': { 'type': 'B', 'b-value': None, }, }, "example 3") # Gives the correct error - `expected str for dictionary value @ data['implementation']['a-value']` validate_schema(sch, { 'implementation': { 'type': 'A', 'a-value': None, }, }, "example 3") The fix has to be in voluptuous, but I am not sure what is should be. Maybe we could return the one with shortest `errors` list (https://github.com/alecthomas/voluptuous/blob/master/voluptuous/schema_builder.py#L536)
Reporter | ||
Comment 2•6 years ago
|
||
What about adding a discriminant option: Any(alternatives, discriminant=lambda value, alternatives: alternative). That function would return one of the given alternatives, based on value. So for the simple example something like def discriminant(value, alternatives): type = value.get('type') for alternative in alternatives: if alternative['type'] == type: return alternative Then when validating, rather than trying to match the options sequentially, it would use the discriminant function to figure out which one to apply. I can see one possible objection from the voluptuous developers: this option changes the behavior of Any as it doesn't really mean "matches any of" -- it now means "matches the right one of" depending on how the discriminant is implemented. If that's the case, maybe this could be a new class ("Union"?). Alternately, Any could continue to validate the way it always has, and the discriminant could be used only when generating error messages.
Comment 3•6 years ago
|
||
Yes, Any() uses the first validated value, and they will be opposed to changing that behavior. Using discriminants for error messages is a good idea. Even without that, Any() should have been made to return the error of the closest alternative, which it currently does not. Discriminants will surely give the users control over how they define the 'closest alternative'.
Reporter | ||
Updated•6 years ago
|
Comment 8•6 years ago
|
||
I'll note that I fixed the major use of Any
in-tree that was causing this error to impact us in Bug 1502253.
Comment 9•6 years ago
|
||
coop: I made a PR here - https://github.com/alecthomas/voluptuous/pull/368
Still awaiting response from @alecthomas
Reporter | ||
Comment 10•6 years ago
|
||
yd, it looks like alec made a comment and is looking for a more complete PR.
Comment 11•5 years ago
|
||
Dustin, The PR was merged today.
Now we can add a discriminant function with Union or Any, similar to this and it should return the correct error.
Reporter | ||
Comment 12•5 years ago
|
||
You're the best! Do you want to make those changes in the Firefox source tree?
Comment 14•5 years ago
|
||
Just to be sure, this is the type
you want to be identified.
If it's not, can you tell me which keys in the schema you want voluptous to identify.
Reporter | ||
Comment 15•5 years ago
|
||
Look at that! It seems like we've worked around this issue with @payload_builder
https://hg.mozilla.org/mozilla-central/file/tip/taskcluster/taskgraph/transforms/task.py#l340
However, it looks like it could be useful in taskcluster/taskgraph/transforms/fetch.py
!
Comment 16•5 years ago
|
||
I made the change in fetch.py, but I am unable to figure how to run tests in taskgraph only.
Also, do we have a substitute for IRC yet?
Comment 17•5 years ago
|
||
(In reply to Yashvardhan Didwania [:ydidwania] from comment #16)
I made the change in fetch.py, but I am unable to figure how to run tests in taskgraph only.
./mach python-test --subsuite taskgraph
Also, do we have a substitute for IRC yet?
Not yet, but until we do we expect #taskcluster or #ci to be good for taskgraph related questions
Comment 18•5 years ago
|
||
Another good test is to run ./mach taskgraph full
as well, to ensure nothing broke. And you can adjust stuff in-gecko at taskcluster/ci/**
to make some invalid fetches things to see how voluptuous would error out.
Comment 19•5 years ago
|
||
Thank you Justin. I was able to run these tests. It turns out there hasn't been a release after I added the discriminant argument in voluptuous so we'll have to wait till that is done
Comment 20•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.
Updated•2 years ago
|
Comment 21•2 years ago
|
||
We probably want to replace voluptuous rather than improve it. Bug 1652123 and https://github.com/taskcluster/taskgraph/issues/8.
Description
•