Skip to content

$dynamicRefs not evaluated on error #1068

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

Closed
ikonst opened this issue Mar 24, 2023 · 10 comments
Closed

$dynamicRefs not evaluated on error #1068

ikonst opened this issue Mar 24, 2023 · 10 comments
Labels
Enhancement Some new desired functionality Error Reporting Issues related to clearer or more robust validation error reporting

Comments

@ikonst
Copy link
Contributor

ikonst commented Mar 24, 2023

If there are validation errors, a $dynamicRef appears not to be followed and thus not evaluated.

If such schema is set to disallow unevaluated properties, then validation would result in an unevaluated properties error in addition to the "culprit" error. In particular when using jsonschema.validate, the "best match" is the unevaluated property "error".

Example

jsonschema.validate(
    instance={
        'spam': 'spam',
    },
    schema={
        "$id": "file:///spam/ham.json",
        '$schema': 'https://json-schema.org/draft/2020-12/schema#',
        'unevaluatedProperties': False,
        "$dynamicRef": '#hovercraft',
        '$defs': {
            'does_not_matter': {
                "$dynamicAnchor": 'hovercraft',
                'properties': {
                    'spam': {'type': 'string'},
                },
            },
        },
        'required': ['ham'],
    }
)

Actual Result

jsonschema.exceptions.ValidationError: Unevaluated properties are not allowed ('spam' was unexpected)

Expected Result

jsonschema.exceptions.ValidationError: 'ham' is a required property

and if not for the missing ham property, the spam property would've been evaluated by the "hovercraft" definition, and thus there'd be no errors.

@Julian
Copy link
Member

Julian commented Mar 24, 2023

Thanks for the report -- I'll have a look at your example more carefully (probably not until after the release, so few weeks). Just to be sure immediately though, have you seen the discussions on the spec repo about unevaluatedProperties-in-the-face-of-other-errors? I don't recall whether this specific kind of example was discussed, or even what the spec says about it, but at the minute from my recollection a bunch of this behavior is somewhere between "not specified" and "contradictory" in the spec -- specifically, I don't think there are any examples where it changes the validation result (let me know if that's your intention but seems not) -- but there definitely are examples where it's either unspecified or worse as to whether one should get 1 error or 2 in cases using unevaluatedProperties -- so while I would hope to have as clear of an error message as possible out of best_match, which I think it sounds like is what you're asking for, that may not be fully feasible, needs thinking.

And any improvements in error message would likely have to come after there is some sort of more "native" way of supporting unevaluatedProperties -- its addition to the spec was a huge huge deal, it's a keyword like no other unfortunately :/ so supporting it at the minute is certainly done in a hacky way, and knowing what the right "permanent" interface for it looks like depends on how the spec evolves going forward and what kinds of other drastically different keywords we end up with.

But regardless this is likely very good to file, so appreciated certainly, and I suspect there's definitely at least one thing that should be addressable (just like I said, unless you care to take a stab, it'll take a bit for me to even think about it given some other priorities).

@Julian Julian added Enhancement Some new desired functionality Error Reporting Issues related to clearer or more robust validation error reporting labels Mar 24, 2023
@ikonst
Copy link
Contributor Author

ikonst commented Mar 24, 2023

I might be missing something about how $dynamicRef works: even though ham is missing (Error No 1), why would we not try to follow the $dynamicRef, thus causing Error No 2?

@Julian
Copy link
Member

Julian commented Mar 24, 2023

Which are errors 1 and 2, sorry?

We do indeed get two errors here in my implementation:

>>> from jsonschema import Draft202012Validator
>>> schema={
...         "$id": "file:///spam/ham.json",
...         '$schema': 'https://json-schema.org/draft/2020-12/schema#',
...         'unevaluatedProperties': False,
...         "$dynamicRef": '#hovercraft',
...         '$defs': {
...             'does_not_matter': {
...                 "$dynamicAnchor": 'hovercraft',
...                 'properties': {
...                     'spam': {'type': 'string'},
...                 },
...             },
...         },
...         'required': ['ham'],
...     }
>>> print(list(Draft202012Validator(schema).iter_errors({'spam': 'spam'})))
[<ValidationError: "Unevaluated properties are not allowed ('spam' was unexpected)">, <ValidationError: "'ham' is a required property">]

@ikonst
Copy link
Contributor Author

ikonst commented Mar 24, 2023

I also get 2 errors. What I mean to say is that if I remove the required directive, then I get zero errors. In other words, even though ham is required, why wouldn't it still evaluate $dynamicRef and thus evaluate spam?

@Julian
Copy link
Member

Julian commented Mar 24, 2023

Sorry, can you be more specific about which example? Here, leaving everything the same but removing the required, one still gets an error:

>>> from jsonschema import Draft202012Validator
... schema={
...         "$id": "file:///spam/ham.json",
...         '$schema': 'https://json-schema.org/draft/2020-12/schema#',
...         'unevaluatedProperties': False,
...         "$dynamicRef": '#hovercraft',
...         '$defs': {
...             'does_not_matter': {
...                 "$dynamicAnchor": 'hovercraft',
...                 'properties': {
...                     'spam': {'type': 'string'},
...                 },
...             },
...         },
...     }
... print(list(Draft202012Validator(schema).iter_errors({'spam': 'spam'})))
[<ValidationError: "Unevaluated properties are not allowed ('spam' was unexpected)">]

@ikonst
Copy link
Contributor Author

ikonst commented Mar 24, 2023

Let me try again once I'm back at my keyboard. The crux of this report is skipping evaluation on error, not a suboptimal "best match".

@Julian
Copy link
Member

Julian commented Mar 24, 2023

Got it -- yep feel free to follow up -- I should say we pass all the upstream tests, so if indeed we find one that's correct according to the spec but incorrectly fails then the first step will be adding a test to the official suite. Certainly not impossible that that's the case though.

@ikonst
Copy link
Contributor Author

ikonst commented Mar 25, 2023

This issue was an attempt to reproduce another issue minimally, but at this point I can't see the forest for the trees, so let's close it.

@ikonst ikonst closed this as completed Mar 25, 2023
@ikonst
Copy link
Contributor Author

ikonst commented Mar 26, 2023

BTW, I managed to reproduce this (my complex case involved two schemas and $dynamicAnchor... 😵 ), but it all boiled down to a TIL on my part — that unevaluatedProperties includes properties that aren't 'unknown' but failed validation.

Simple case:

import jsonschema

schema = {
    'properties': {
        'foo': {
            'type': 'object',
            'required': ['bar'],
        },
    },
    'unevaluatedProperties': False,
}
instance = {
    'foo': {},
}

validator_cls = jsonschema.validators.validator_for(schema)
validator = validator_cls(schema)
for err in validator.iter_errors(instance):
    print('-' * 80)
    print(err)

This is clearly documented in the docs, and overall not that surprising given unevaluatedProperties being intended to address composition, but also means that it's not strictly a "better additionalProperties" and can be misleading if you think of it as a "typo-prevention mechanism".

One thing I realized is that strategically positioning unevaluatedProperties affects which error is chosen as "best":

 {
    'unevaluatedProperties': False,  # <-- 'unevaluated property' is best error
    'properties': {
        'foo': {
            'type': 'object',
            'required': ['bar'],
        },
    },
    'unevaluatedProperties': False,   # <-- 'required' is best error
}

@Julian
Copy link
Member

Julian commented Mar 26, 2023

Thanks for the follow up. The last bit on position probably reflects that at the minute they're considered equally important, but suggestions for improvements to the best match heuristics are always welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Some new desired functionality Error Reporting Issues related to clearer or more robust validation error reporting
Projects
None yet
Development

No branches or pull requests

2 participants