Closed Bug 1490141 Opened 7 years ago Closed 3 years ago

Voluptuous gives confusing error messages for Any(...) schemas

Categories

(Firefox Build System :: Task Configuration, task)

Product:
Firefox Build System
Component:
Task Configuration
Type:
task

Tracking

(Not tracked)

Status:
RESOLVED DUPLICATE of bug 1652123

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.
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)
Flags: needinfo?(dustin)
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.
Flags: needinfo?(dustin)
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'.
Should I go ahead and file an issue for it on github?
Absolutely, please do!
Assignee: nobody → didwaniayashvardhan
Depends on: 1502253

YD: any update here?

Flags: needinfo?(didwaniayashvardhan)

I'll note that I fixed the major use of Any in-tree that was causing this error to impact us in Bug 1502253.

coop: I made a PR here - https://github.com/alecthomas/voluptuous/pull/368
Still awaiting response from @alecthomas

Flags: needinfo?(didwaniayashvardhan)

yd, it looks like alec made a comment and is looking for a more complete PR.

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.

You're the best! Do you want to make those changes in the Firefox source tree?

Yeah sure :)

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.

Flags: needinfo?(dustin)

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!

Flags: needinfo?(dustin)

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?

(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

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.

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

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Assignee: didwaniayashvardhan → nobody
Severity: normal → S3

We probably want to replace voluptuous rather than improve it. Bug 1652123 and https://github.com/taskcluster/taskgraph/issues/8.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.