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

Annotation output for propertyDependencies #1367

Open
gregsdennis opened this issue Dec 15, 2022 · 5 comments
Open

Annotation output for propertyDependencies #1367

gregsdennis opened this issue Dec 15, 2022 · 5 comments
Milestone

Comments

@gregsdennis
Copy link
Member

propertyDependencies was originally proposed as a replacement for a complex and verbose pattern to apply a schema to an instance based on the value of a property, similarly to OpenAPI's discriminator, but with more native JSON-Schema-like functionality. It was added to draft-next (post-2020-12) here.

Proposed as a replacement for this pattern, one could expect that it would work the same in all cases, however I added a test to the Suite recently to cover the interation between propertyDependencies and unevaluatedProperties:

{
  "$schema": "https://json-schema.org/draft/next/schema",
  "propertyDependencies": {
    "foo": {
      "foo1": {
        "properties": {
          "bar": true
        }
      }
    }
  },
  "unevaluatedProperties": false
}

This is supposed to be analogous to:

{
  "$schema": "https://json-schema.org/draft/next/schema",
  "if": {
    "properties": {
      "foo": { "const": "foo1" }
    }
  },
  "then": {
    "properties": {
      "bar": true
    }
  },
  "unevaluatedProperties": false
}

In the latter schema, unevaluatedProperties can "see into" /if to recognize that the foo property is declared. If the value of foo is foo1, then the /if subschema passes and produces an annotation that foo is a known property, so unevaluatedProperties subsequently ignores the property and passes validation.

However, as it is currently defined, propertyDependencies produces no annotations. That means, for the former schema, unevaluatedProperties wouldn't know about the foo property, so it would produce an error. Therefore the current definition fails to meet the explicit purpose of this keyword: a replacement for this common if/then, discriminator-like pattern.

This could be solved by requiring propertyDependencies to produce an annotation of the properties listed within it (just like properties does). (We would also need to update unevaluatedProperties to consider that annotation.)

If we maintain that propertyDependencies does not produce an annotation, then the author is required to define the property elsewhere:

{
  "$schema": "https://json-schema.org/draft/next/schema",
  "properties": {
    "foo": true // or an enum or something
  },
  "propertyDependencies": {
    "foo": {
      "foo1": {
        "properties": {
          "bar": true
        }
      }
    }
  },
  "unevaluatedProperties": false
}

I believe this counteracts the verbosity reduction benefit that propertyDependencies provides.

NOTE Interactions with other keywords, including unevaluatedProperties was not explored in the original proposal.

@gregsdennis gregsdennis added this to the draft-next milestone Dec 15, 2022
@karenetheridge
Copy link
Member

I agree completely.

@jdesrosiers
Copy link
Member

I don't think transformations are relevant to how propertyDependencies should behave. They are informative in showing the value of the keyword by showing what users would have to do if it didn't exist, but propertyDependencies isn't and shouldn't be defined by any particular transformation. I don't mean that as an argument one way or another, just that I'd like to come to a decision on this without being influenced by how one transformation out of several possible transformations behaves.

I'm not convinced that not producing annotations results in more verbose schemas. It's possible to construct a schema where that's true, but it wouldn't be a very useful schema. The schema from the example says that "foo" can be absolutely any value of any type, but if it's a string and its value is "foo1", then apply the schema. The "any value of any type" part is what I consider unrealistic. In any real schema, "foo" is going to need to be more fully defined anyway. It's going to have valid values, or at least be constrained to a string. So, you're not just defining "foo" for the sake of unevaluatedProperties, it needs to be defined anyway. Even if there is some edge-case where it would make sense to leave "foo" completely unconstrained, I think it would be so rare as to not have much influence on what we decide here. Please enlighten me if you can think of some use-case that shows another perspective.

Something we haven't mentioned yet is that if propertyDependencies produces annotations, it will affect the definition of additionalProperties as well as unevaluatedProperties. If we have good reason, that's probably fine, but so far I don't see a compelling reason and I'd rather not touch those (especially additionalProperties) if we don't have to.

Although I can see how some people might expect to see propertyDependencies produce annotations, I don't see a benefit in doing so. It makes processing propertyDependencies, additionalProperties, and unevaluatedProperties a little more complicated to implement, but doesn't, IMO, provide a benefit that justifies that added complexity.

@karenetheridge
Copy link
Member

Something we haven't mentioned yet is that if propertyDependencies produces annotations, it will affect the definition of additionalProperties as well

additionalProperties isn't defined exclusively in terms of annotations right now, but "other properties that are defined in the same schema" can encapsulate the properties in propertyDependencies easily enough, so I see no reason not to do so unless there is a compelling reason? Doing so would be more consistent IMO.

@gregsdennis
Copy link
Member Author

To extend @karenetheridge note, unevaluatedProperties also doesn't need to change in order for propertyDependencies to produce annotations.

The specification says this for unevaluatedProperties:

The behavior of this keyword depends on the annotation results of adjacent keywords that apply to the instance location being validated. Specifically, the annotations from "properties", "patternProperties", and "additionalProperties", which can come from those keywords when they are adjacent to the "unevaluatedProperties" keyword. Those three annotations, as well as "unevaluatedProperties", can also result from any and all adjacent in-place applicator (Section 10.2) keywords. This includes but is not limited to the in-place applicators defined in this document.

unevaluatedProperties is already defined to consider annotations from all other applicator keywords, including ones that aren't in this document. That would even include my data keyword. (I should make a test for that.)

@gregsdennis
Copy link
Member Author

Note: propertyDependencies is being extracted to a separate proposal document. Once that goes through, PRs against the keyword should be made against that document, not Core.

@gregsdennis gregsdennis added the proposal Initial discussion of a new idea. A project will be created once a proposal document is created. label Jun 18, 2024
@gregsdennis gregsdennis removed the proposal Initial discussion of a new idea. A project will be created once a proposal document is created. label Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

3 participants