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

Support encountering of a "deprecated": true keyword by propagating a deprecation warning #1170

Open
edgarrmondragon opened this issue Sep 28, 2023 · 7 comments
Labels
Dialects v2 Issues which will likely be addressed as part of reworked dialect support Enhancement Some new desired functionality

Comments

@edgarrmondragon
Copy link

edgarrmondragon commented Sep 28, 2023

New in draft 2019-09 The deprecated keyword is a boolean that indicates that the instance value the keyword applies to should not be used and may be removed in the future.[1]

Usage example:

from jsonschema import validators

schema = {
    "properties": {
        "id": {"type": "integer"},
        "name": {"type": "string", "deprecated": True},
    },
}
instance={"id": 1, "name": "abc"}
validator = validators.Draft202012Validator(schema, check_deprecated=True)

validator.validate(instance)  # raises ValidationError

I can start working on a PR if the maintainers are interested and the suggested API makes sense.

@edgarrmondragon edgarrmondragon changed the title Warn when using schema features annotated with "deprecated": true Raise validation error when using schema features annotated with "deprecated": true Sep 28, 2023
@sirosen
Copy link
Member

sirosen commented Sep 28, 2023

Just to pull in some of the broader context:
This comes from a check-jsonschema ticket, to which I said basically "cool but I don't know how to do this!" 🙂

It may very well be possible today. If so, a doc example would be awesome.

My first thought on the proposed interface is that it's a little odd that this would produce a validation error when the keyword's meaning is "warning like".
I think this problem, in the JSON Schema parlance, is a matter of collecting annotations (I'd love to get confirmation that I'm correct about this). In that context, it's not necessary that jsonschema raises exceptions or emits warnings, but rather that it has a way of communicating back to the caller which deprecations were hit.

I'm also not sure that this merits a special keyword argument. My ideal is that this fits in as part of a more generic mechanism like... (Just spitballing)

annotations = []
validator.validate(schema, instance, collect_annotations=(annotations, "deprecated"))
print(annotations)  # [{...}]

@Julian
Copy link
Member

Julian commented Sep 28, 2023

My first thought on the proposed interface is that it's a little odd that this would produce a validation error when the keyword's meaning is "warning like".

This definitely immediately discards any solution or implementation where a ValidationError is raised or returned. The interpretation of getting a ValidationError is "the specification says your instance is invalid". For the deprecated keyword, that simply isn't the specified behavior.

But indeed I think this at least relies on or benefits from annotation support (which I think has no current open issue because no one has ever asked for its functionality! even though it's been on my radar).

If we did have such an interface, I do indeed think this would be implementable on top of it.

I'm also not sure that this merits a special keyword argument. My ideal is that this fits in as part of a more generic mechanism like... (Just spitballing)

(I mentioned this on the PR, but indeed, agree.)

@Julian Julian changed the title Raise validation error when using schema features annotated with "deprecated": true Support encountering of a "deprecated": true keyword by propagating a deprecation warning Sep 28, 2023
@Julian Julian added the Enhancement Some new desired functionality label Sep 28, 2023
@sirosen
Copy link
Member

sirosen commented Sep 29, 2023

I'm looking at this again with ~24 more hours to think about it. I think the CLI to look at, which would help drive this, would be something akin to this:
--annotations 'warn:deprecated,warn:readOnly', --annotations 'error:writeOnly'
To mean that you can validate that a document doesn't contain writeOnly fields, etc.
And the default logically falls out: --annotations 'warn:deprecated'.

Supporting multiple annotations smoothly and simultaneously is therefore an important feature and has relevance, in that I'd like jsonschema to provide a mechanism for collecting the annotations, and then check-jsonschema can take responsibility for warning or erroring.

So I have two questions, as someone who might try to implement this:

  1. what's the ideal API to provide?
  2. what is the behavior, per the spec, in tricky cases?

(1) is easy to understand as a question, hard to answer, but isn't even really relevant if we know of nasty cases which are likely to block any efforts. Plus, if there are especially complex cases, that may drive what the ideal API looks like.
For (2), here's the tricky case I have in mind:

# schema.json
{
  "oneOf": [
    {"deprecated": true},
    {"type": "integer"}
  ]
}
# instance1.json
"1"
# instance2.json
2

Everything can be matched by the deprecated:true schema. instance1.json definitely matches it and that seems to be intentional. instance2.json, however, matches both branches, and only one of them is deprecated.
Do we consider this when collecting annotations?
What if the two elements of the oneOf were in the opposite order?

I don't see an answer to this in the JSON Schema docs, so I'm left scratching my head.

@Julian
Copy link
Member

Julian commented Sep 29, 2023

what's the ideal API to provide?

The reason this hasn't happened already (this being "support annotations") is essentially because so far the answer I have to this question is a large backwards incompatible change.

Specifically -- modern versions of JSON Schema are now stateful. Annotations are the representation of that state.

Historically, jsonschema's interface for validator functions (implementations of keywords) has the signature:

def keyword_function(validator_object, keyword_value, instance, full_schema):
    ...

but this is... wrong for stateful worlds!

Instead the "right API" is:

def keyword_function(annotator, keyword_value, instance, full_schema):
    ...

where an annotator essentially represents the thus far-collected state for the current validation process, and has methods like .add_annotation, and things for seeing what annotations are collected so far.

So it's quite a delicate change!

Literally my best thought so far is to bundle this with other changes related to wanting to better support vocabularies, and essentially deprecate all the existing validator objects (Draft202012Validator etc.) in favor of new locations which support that new API -- so e.g. jsonschema.dialects.Draft202012Validator obsoletes jsonschema.validators.Draft202012Validator and calls its keywords with the new signature I mentioned rather than the old one.

If you're saying you're interested in playing around, I'd be trying to do a POC of the above, with essentially a completely private extension interface since.. even upstream it's not yet really firm what JSON Schemas "real" compute model is, how complex keywords can get, whether they support complicated ordering mechanisms (for which keyword has to be processed first), etc. Your example is perhaps even a simple example of this sort of thing, I don't honestly remember what the answer is but I can think harder if you'd like :)

So if you want to POC it, I can certainly give some opinions.

If you want to try something less ambitious, it's possible indeed that this could be done more "localized", especially if the goal is to just support deprecated.

@edgarrmondragon
Copy link
Author

Thanks both for starting the discussion, and sorry for the noise with the PR!

In particular, my use case was something like

{
  "type": "string",
  "description": "The type of value this setting contains",
  "oneOf": [
    {
      "enum": [
        "oauth",
        "date_iso8601",
        "file",
        "email",
        "integer",
        "options",
        "object",
        "array",
        "boolean",
        "string"
      ]
    },
    {
      "enum": [
        "hidden",
        "password"
      ],
      "deprecated": true
    }
  ]
}

@Julian
Copy link
Member

Julian commented Sep 29, 2023

sorry for the noise with the PR

Definitely no need for apologies, thanks for the issue.

@sirosen
Copy link
Member

sirosen commented Sep 29, 2023

+1 for offering thanks for opening the issues! Having a clear use-case to drive discussion is often worth a thousand design meetings!

I'm not sure I can reasonably commit myself to a big project at least until December, so there's no pressure to give me a lot of support. I didn't realize how difficult this would be when I mentioned that I might give it a try -- I was imagining a purely additive change.

I'm still interested in a full solution for this, but in the meantime, maybe we can do something as a useful stopgap.

check-jsonschema could -- as a jsonschema consumer -- inject ValidationErrors in response to a user-provided flag.
i.e. check-jsonschema --show-me-the-deprecation-errors (flag TBD, in case that's unclear 😜 ) could essentially inject the change from #1171 into validation.

There's already check-jsonschema --fill-defaults which has a warning/caveat that it could surprise you if used with polymorphic keywords like allOf. That feels similar to the CLI having options to do things which might be spec violations, as long as the user explicitly opts-in.

I'll think about this more on the "only in the CLI" side of the house ( python-jsonschema/check-jsonschema#331 ).

@Julian Julian added the Dialects v2 Issues which will likely be addressed as part of reworked dialect support label Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dialects v2 Issues which will likely be addressed as part of reworked dialect support Enhancement Some new desired functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants