Skip to content

Move "dependencies" to applicability group #528

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
epoberezkin opened this issue Jan 7, 2018 · 11 comments
Closed

Move "dependencies" to applicability group #528

epoberezkin opened this issue Jan 7, 2018 · 11 comments

Comments

@epoberezkin
Copy link
Member

epoberezkin commented Jan 7, 2018

As requested by @handrews in #513 (comment)

The fact you can rewrite a keyword using if/then/else/required doesn't mean that it makes the keyword itself asserting any conditions. This schema:

{
  "properties": {
    "foo": {"type": "number"}
  }
}

can be rewritten as

{
  "if": {"required":["foo"]},
  "then": {
    "properties": {
      "foo": {"type": "number"}
    }
  }
}

"properties" keyword still belongs to the applicability group, even though you can rewrite it using "required", same as "dependencies".

Compare keywords definitions (ignoring property syntax of "dependencies" for now, I'll come back to it later):

"properties" (http://json-schema.org/latest/json-schema-validation.html#rfc.section.6.5.4):

Validation succeeds if, for each name that appears in both the instance and as a name within this keyword's value, the child instance for that name successfully validates against the corresponding schema.

"dependencies" (http://json-schema.org/latest/json-schema-validation.html#rfc.section.6.5.7):

If the dependency value is a subschema, and the dependency key is a property in the instance, the entire instance must validate against the dependency value.

So, for "properties", if a property name appears in the instance, you have to apply a corresponding subschema to the child instance.

For "dependencies", if a property name appears in the instance, you have to apply a corresponding subschema to the current instance.

The logic of "properties" and "dependencies" only differs in that the former applies subschemas to the child instance, and the latter to the current instance. Neither of them is making assertions on their own.

Now back to the property syntax of "dependencies". We have two possible points of view here:

  1. We can consider it just a syntactic sugar that allows to use <array of strings> as a shortcut for {"required": <array of strings>}. It's not dissimilar from the boolean form of the schema, where you can use false as a shortcut for {"not":{}}. In this case, "dependencies" keyword still applies a certain schema to the current instance, without making any assertions of its own.
  2. We can interpret it is as a separate thing completely that makes its own assertions. In this case the property syntax does not belong to applicability group. But the downside of this view is that it creates an overlap with "required" keyword by allowing two ways of asserting that property is required.

Regardless which way of looking at property syntax of "dependencies" we prefer, the schema syntax of "dependencies" still belongs to the applicability group, as it only uses a defined logic to apply subschemas to the current instance, without making any assertions of its own, which is consistent with how applicability is defined.

@erayd
Copy link

erayd commented Jan 7, 2018

I agree that dependencies should be classified as an applicability keyword.

It does have something of a hybrid nature, but because the assertion it makes is which property definitions (or which dependency schemas) should be considered applicable, and it has to live somewhere, applicability seems to me like a more logical home for it.

@handrews
Copy link
Contributor

handrews commented Jan 8, 2018

Locking this as it is an offshoot of #513.

@json-schema-org json-schema-org locked as too heated and limited conversation to collaborators Jan 8, 2018
@handrews
Copy link
Contributor

handrews commented Jan 8, 2018

Unlocking this as 513 has been definitively resolved.

Anyone re-opening of 513's topics in this issue will get a warning if it seems accidental. If the focus of the comment is purely revisiting 513, I will remove the comment to keep this issue on-topic (and we will determine a consistent policy on this via #529).

@json-schema-org json-schema-org unlocked this conversation Jan 8, 2018
@handrews
Copy link
Contributor

handrews commented Jan 8, 2018

I can see the view of the schema form of dependencies as an applicator, without a real assertion component. While it's not how I have looked at the keyword in the past, I accept the argument that "if the property is present then apply the subschema" is already part of applicator behavior.

However, I am vehemently against the string array form of dependencies going into core, and I have long thought that having these two forms of dependencies is a problem. In part because one is more of an applicator (and, based on arguments here, really just as much of a pure applicator as anything else) and the other is unquestionably an assertion.

This is part of what led me to mark dependencies for possible future deprecation now that we have if, which is tracked in #442. I had originally proposed removing the schema form and keeping the string array form, but responses to #442 have indicated that maybe the schema form is the one worth keeping.

I could get behind the idea of keeping the schema form of dependencies under that name, and considering it to purely be an applicator keyword (and therefore moving to core along with the others listed in #513). And then either dropping the string array form entirely, or coming up with another keyword (propertyDependencies?) for the string array form which would stay in the validation spec with the other assertions.

Thoughts?

@handrews handrews added this to the draft-08 milestone Jan 8, 2018
@erayd
Copy link

erayd commented Jan 8, 2018

I could get behind the idea of keeping the schema form of dependencies under that name, and considering it to purely be an applicator keyword (and therefore moving to core along with the others listed in #513). And then either dropping the string array form entirely, or coming up with another keyword (propertyDependencies?) for the string array form which would stay in the validation spec with the other assertions.

This is pretty much my ideal outcome. dependencies as it exists currently is a nasty hybrid thing, and splitting it this way makes things much cleaner. dependencies as a schema-only keyword is essentially identical to the way properties works now, except that it applies to the schema containing the properties rather than to the schema within the property.

I don't think that the string form of dependencies is necessary - {"required":["myKeyword"]} is fairly obvious and easy to remember. However, is there some merit in perhaps keeping that functionality around (in a different keyword) for the sake of making life easier for new users?

@epoberezkin
Copy link
Member Author

propertyDependencies

or "dependentProperties"? I always get confused what depends on what...

@handrews
Copy link
Contributor

handrews commented Jan 9, 2018

"dependentRequired"? Really, the string array form is a shortcut for "required" so maybe we could make that more obvious in the name.

@epoberezkin
Copy link
Member Author

epoberezkin commented Jan 10, 2018

or "requiredDependencies" :) If I am correct and the property value is the list of "dependencies" of the property key, than "requiredDependencies" is better (also closer to the current name that implies the same). If it's the opposite (the key is a "dependency" of each property in the list), then "dependentRequired" is better. As I wrote, I am confused, but I now think it's the first... Now we can spend another 100 comments arguing about the definition of the word "dependency" ;) (that's a joke).

As it was

{
  "exclusiveMaximum": ["maximum"]
}

then, probably, "maximum" is a dependency of "exclusiveMaximum" (i.e. "maximum" is required to define "exclusiveMaximum"). So "requiredDependencies" makes the most sense to me now :) But whatever.

@epoberezkin
Copy link
Member Author

@handrews And yes, I agree that

the string array form is a shortcut for "required" so maybe we could make that more obvious in the name.

conditional "required", sort of.

@handrews
Copy link
Contributor

handrews commented Mar 4, 2018

Let's go with:

  • Restricting dependencies to the schema form and classifying it as an applicator
  • Creating requiredDependencies for the string form and classifying it as an assertion

We can keep kicking the requiredDependencies name around between now and publication (which will be a while given the volume of PRs needed) since it's easy enough to change. But requiredDependencies seems to be the best option (I agree with you on the directionality).

@handrews
Copy link
Contributor

handrews commented Apr 4, 2018

Merged #573.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants