Less Specific Validation Errors for "oneOf" Schemas - Take 2

[dtantsur]

Moving this out of #977 since that bug was a user error apparently.

We're seeing a significant regression in user messages, presumably because of bd5ea73. Granted, it's hard to provide good error messages for something like

{'anyOf': [{'additionalProperties': False,
                'properties': {'hostname': {'type': 'string'},
                               'network_type': {'enum': ['managed',
                                                         'unmanaged'],
                                                'type': 'string'},
                               'port_id': {'type': 'string'},
                               'switch_id': {'type': 'string'},
                               'switch_info': {'type': 'string'}},
                'required': ['port_id', 'switch_id'],
                'type': 'object'},
               {'additionalProperties': False,
                'properties': {'hostname': {'type': 'string'},
                               'network_type': {'enum': ['managed',
                                                         'unmanaged'],
                                                'type': 'string'},
                               'port_id': {'type': 'string'},
                               'switch_id': {'type': 'string'},
                               'switch_info': {'type': 'string'}},
                'required': ['port_id', 'hostname'],
                'type': 'object'},
               {'additionalProperties': False,
                'properties': {'hostname': {'type': 'string'},
                               'network_type': {'enum': ['unmanaged'],
                                                'type': 'string'},
                               'port_id': {'type': 'string'},
                               'switch_id': {'type': 'string'},
                               'switch_info': {'type': 'string'}},
                'required': ['network_type'],
                'type': 'object'},
               {'additionalProperties': False, 'type': 'object'}]}

in all case, but e.g. all of them have 'additionalProperties': False. Previously (<= 4.7 I think) we used to have Additional properties are not allowed, now we only have ... is not valid under any of the given schemas.

Another pretty unfortunately example from our API:

STANDARD_TRAITS = # ... some list
CUSTOM_TRAIT_PATTERN = "^CUSTOM_[A-Z0-9_]+$"
TRAITS_SCHEMA = {'anyOf': [
    {'type': 'string', 'minLength': 1, 'maxLength': 255,
     'pattern': CUSTOM_TRAIT_PATTERN},
    {'type': 'string', 'enum': STANDARD_TRAITS},
]}

{
    'type': 'object',
    'properties': {
        'description': {'type': ['string', 'null'], 'maxLength': 255},
        'extra': {'type': ['object', 'null']},
        'name': TRAITS_SCHEMA,
        'steps': {'type': 'array', 'items': api_utils.DEPLOY_STEP_SCHEMA,
                  'minItems': 1},
        'uuid': {'type': ['string', 'null']},
    },
    'required': ['steps', 'name'],
    'additionalProperties': False,
}

With CUSTOM_AAAAA (a really long string) we used to have 'CUSTOM_AAAAA....' is too long, now again the generic error message. It basically means that we need to accept unspecific API errors or migrate away from jsonscheme.

[Julian]

In and of itself producing less specific error messages isn't a bug, it's precisely the intended behavior of the change there (and produces better error messages in many other cases), so there'd need to be some specific heuristic one could use to again figure out a better error for the cases you're presenting. Do you have a suggested one?

[dtantsur]

I'd be fine with a way to opt-out of the change in #977 (i.e. going back to the 4.7 behavior).

[dtantsur]

Another data point: I tried rewriting the first schema to use deeper anyOf:

    {'additionalProperties': False,
     'anyOf': [{'anyOf': [{'required': ['port_id', 'switch_id']},
                          {'required': ['port_id', 'hostname']}],
                'properties': {'hostname': {'type': 'string'},
                               'network_type': {'enum': ['managed',
                                                         'unmanaged'],
                                                'type': 'string'},
                               'port_id': {'type': 'string'},
                               'switch_id': {'type': 'string'},
                               'switch_info': {'type': 'string'}}},
               {'properties': {'hostname': {'type': 'string'},
                               'network_type': {'enum': ['unmanaged'],
                                                'type': 'string'},
                               'port_id': {'type': 'string'},
                               'switch_id': {'type': 'string'},
                               'switch_info': {'type': 'string'}},
                'required': ['network_type']}],
     'type': 'object'}

but now it rejects any properties, including the ones defined here... I guess you cannot test properties inside anyOf this way?

[dtantsur]

I did get some progress with rewriting the second schema to

TRAITS_SCHEMA = {
    'type': 'string', 'minLength': 1, 'maxLength': 255,
    'anyOf': [
        {'pattern': CUSTOM_TRAIT_PATTERN},
        {'enum': STANDARD_TRAITS},
    ]
}

Now it at least handles the lengths correctly, but not the failing patterns. My reasoning would be to put pattern above enum with the logic: if it is not one of the predefined values, it must match the pattern, so the final error message is about the pattern. YMMV.

[Julian]

I'd be fine with a way to opt-out of the change in #977 (i.e. going back to the 4.7 behavior).

If you want to indiscretely recurse into applicators you certainly still can by doing something like:

error = best_match(...)
while error.context:
    error = best_match(error.context)

but now it rejects any properties, including the ones defined here... I guess you cannot test properties inside anyOf this way?

Yes, that's the defined behavior of additionalProperties, but may mean you want to read about unevaluatedProperties.

[dtantsur]

Yes, that's the defined behavior of additionalProperties

Sorry, I must be missing the place where it says that anyOf cannot be used with it.

may mean you want to read about unevaluatedProperties.

Same result with 'unevaluatedProperties': False.

I'll see what the manual recursion gives me, thanks!

[Julian]

It's:

Validation with "additionalProperties" applies only to the child values of instance names that do not appear in the annotation results of either "properties" or "patternProperties".

which basically means additionalProperties applies to properties defined as a "sibling" keyword, not somewhere nested inside a subschema that's inside anyOf.

[dtantsur]

Gotcha. And with unevaluatedProperties it will descend into subschemas, but if none of them match, it will report a wrong error ("unexpected fields" even for fields defined in all subschemas).

[Julian]

Yep exactly.

[dtantsur]

Thank you again! With your incredible help I think I managed to produce error messages I want: https://review.opendev.org/c/openstack/ironic/+/855166. Do you think it's worth an FAQ item? Otherwise feel free to close this issue.

[Julian]

My pleasure, glad to hear.

Do you think it's worth an FAQ item?

I suspect a FAQ entry would end up highly specific to the use case there -- but there are 2 open tickets to enhance best_match yet again with better heuristics (#698 and #812). It may be worth trying your schema again after one or both of those are done, perhaps they cover your case (and if not as I say as long as we can define some well-defined heuristic best_match is always open to improving).