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

Restore exclusive* dependencies, update metaschema #189

Closed
wants to merge 1 commit into from

Conversation

handrews
Copy link
Contributor

This restores dependencies that got dropped somewhere between
draft 04 and 05 (not clear where), superseding both the 4th
commit of #168 and also #185.

It adds a reasonable restriction that the boolean vs numeric
forms not be mixed for exclusiveMinimum and exclusiveMaximum,
and implements the correct meta-scheam for this. Which needs
a oneOf to corectly group the dependencies, but no longer needs
an extra allOf or anyOf to allow minimum and maximum to change
types independently.

@awwright
Copy link
Member

Imo I don't think this is necessary, see #185 (comment).

In short, there is no "correct" usage of the boolean form anymore (it's specifically called out with SHOULD NOT, the "boolean" form is kept in to allow reverse compatibility), so I don't think it makes sense to put further descriptive effort in how to use it.

And if this gets taken out (as is also indicated in a comment), this becomes a non-issue entirely.

@handrews
Copy link
Contributor Author

so I don't think it makes sense to put further descriptive effort in how to use it.

There's no effort, it's just keeping what was there before. Don't degrade functionality while we have it at all.

And if this gets taken out (as is also indicated in a comment), this becomes a non-issue entirely.

If you want to take it out, file an issue. Unless/until such an issue is accepted, the goal is to implement what we have.

@Relequestual Relequestual added this to the Meta-schema draft-05 milestone Dec 13, 2016
"exclusiveMinimum": [ "minimum" ]
}
}
],
"maxLength": { "$ref": "#/definitions/positiveInteger" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is plain wrong. Firstly there is typo (type:'maximum'). A bigger concern is that exclusiveMaximum can have boolean form and exclusiveMinimum can be numeric, while this meta-schema won't allow it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach addresses it: #185 (comment)
Also anyOf is always better when it is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epoberezkin Fixed typo, thanks.

What you suggest in that comment on #185 is essentially what I had in #168 which was rejected. This was my attempt to split the difference between that and the refusal to use "dependencies" at all that defines @awwright's approach in #185.

In Draft 4 you can only do the boolean form. We have discussed (but not, IIRC, agreed on) eventually dropping the boolean form and using only numeric forms.

What is your use case for mixing numeric and boolean in the same schema object? That would be a very confusing schema to me. Note that if you use the boolean form in the root schema and numeric form in a child schema, or a $ref's schema, this meta-schema allows that just fine. It's only mixing the approaches in the same object that it avoids. In this case the "oneOf" is specifically to prevent mixing in a single object.

@epoberezkin
Copy link
Member

epoberezkin commented Dec 20, 2016

@handrews

What you suggest in that comment on #185 is essentially what I had in #168 which was rejected.

Not sure I understand why it was rejected (I've read the comments and still I don't follow). What you have there makes sense.

What is your use case for mixing numeric and boolean in the same schema object? That would be a very confusing schema to me.

It's not about the use-case. It's about meta-schema being more strict that I-D, while I think it should be other way around (I-D stricter than meta-schema or the same).

We either have to add in I-D that mixing boolean and numeric form in the same schema is not allowed (maybe I missed it there?) or to remove this restriction from meta-schema. I am ok with either option, but not with meta-schema being more strict than I-D.

Given that the intention is to deprecate boolean form I don't see any problem with dropping dependencies completely and saying in I-D that boolean form without minimum/maximum is ignored (it would also be consistent with stray "additionalItems" being ignored without "items" rather than making the schema invalid).

@epoberezkin
Copy link
Member

We either have to add in I-D that mixing boolean and numeric form in the same schema is not allowed

And we have to keep in mind that it would be an additional (and unnecessary) violation of "independence" principle.

@handrews
Copy link
Contributor Author

@epoberezkin

Not sure I understand why it was rejected (I've read the comments and still I don't follow).

You and me both.

We either have to add in I-D that mixing boolean and numeric form in the same schema is not allowed (maybe I missed it there?)

Line 250 about boolean "exclusiveMaximum" includes:

and "exclusiveMinimum" (if present) MUST also be a boolean.

and line 276 includes the corresponding language in the other direction.

or to remove this restriction from meta-schema. I am ok with either option, but not with meta-schema being more strict than I-D.

Yes, I agree that the meta-schema should not be more strict than the I-D. That is not what this schema is attempting. This is just bringing the meta-schema in alignment with how the spec should be (the dependency was previously removed without discussion or approval).

We absolutely should not drop dependencies. That is a regression from past behavior and it does not do us (spec writers) any good, while at the same time removing a useful sanity check for schema authors. I am just baffled by you and awwright trying to make life more difficult for schema writers. Putting this in costs us nothing an provides a benefit that we already had and some of us found useful.

Stop trying to make life harder for schema authors. If you want to just drop the boolean forms to make it simpler then let's just drop the boolean forms.

@epoberezkin
Copy link
Member

epoberezkin commented Dec 20, 2016

Line 250 about boolean "exclusiveMaximum" includes: and "exclusiveMinimum" (if present) MUST also be a boolean.

Yes, sorry, missed that.

We absolutely should not drop dependencies. That is a regression from past behavior and it does not do us (spec writers) any good, while at the same time removing a useful sanity check for schema authors. I am just baffled by you and awwright trying to make life more difficult for schema writers. Putting this in costs us nothing an provides a benefit that we already had and some of us found useful.

agreed. I was only concerned by inconsistently between I-D and meta-schema. If they are consistent it's fine then.

We definitely need meta-schemas, they are used a lot. And they should be as close as possible to I-D, no argument about it.

I can only repeat that I prefer the approach in #168, that doesn't impose that additional restriction. But whatever :).

If you want to just drop the boolean forms to make it simpler then let's just drop the boolean forms.

I'd rather keep boolean form.

@epoberezkin
Copy link
Member

Why don't we add a requirement to have items present when additionalItems are used? (and change the I-D as well). It is kind of related issue - no reason to allow schemas where additionalItems are simply ignored. I know it's a separate issue...

@handrews
Copy link
Contributor Author

I can only repeat that I prefer the approach in #168, that doesn't impose that additional restriction. But whatever :).

I am fine with either that approach or this one, I'm only against the approach in #185. This was an attempt to compromise between #168 and #185. I can pull the version from #168 out into its own PR if we want to have all three options posted.

Why don't we add a requirement to have items present when additionalItems are used?

Can you file that as a separate issue? This set of conflicting PRs is more than complicated enough as it is without pulling in new ideas, no matter how consistent those ideas are.

This restores dependencies that got dropped somewhere between
draft 04 and 05 (not clear where).

It adds a reasonable restriction that the boolean vs numeric
forms not be mixed for exclusiveMinimum and exclusiveMaximum,
and implements the correct meta-scheam for this.  Which needs
a oneOf to corectly group the dependencies, but no longer needs
an extra allOf or anyOf to allow minimum and maximum to change
types independently.
@epoberezkin
Copy link
Member

epoberezkin commented Dec 28, 2016

I have been thinking about it. I think that it is very important that meta-schema should be clear in intention and not only validate schemas according to the spec, but also be self-evident. So I do believe that if we want to express "dependency" then we should use "dependencies" keyword.

In addition to that, spec should determine meta-schema and not the other way around, while this approach required changing the spec. That should have been discussed/agreed separately.

So the problems with all three approaches are:

#189 (this one) - changes the spec, no dependencies keyword
#168 - no dependencies keyword
#185 (comment) - just wrong (it doesn't actually require maximum to be present...), changes the spec (requirement not to have maximum if exclusiveMaximum is present)

The only approach free from all the shortcomings would be:

"dependencies": {
  "exclusiveMaximum": {
    "anyOf": [
      { "properties": { "exclusiveMaximum": { "type": "number" } } },
      { "required": ["maximum"] }
    ]
  },
  "exclusiveMinimum": {
    "anyOf": [
      { "properties": { "exclusiveMinimum": { "type": "number" } } },
      { "required": ["minimum"] }
    ]
  }
}

The above can be read as "if exclusiveMaximum is present and it's boolean then the maximum keyword MUST BE present too". Same about exclusiveMinimum.

I can make a PR if it seems better than alternatives @awwright @Relequestual @handrews

EDIT: simplified meta-schema above

@epoberezkin
Copy link
Member

Also given that allowing both maximum and exclusiveMaximum as numbers in the schema (as the spec currently does), we are making implementers' life unnecessarily more complex (and we will have to write a couple more tests about how they behave together as well). Why can't we prohibit maximum if exclusiveMaximum is present?

In we can agree on that, "anyOf" should simply become "oneOf" in the schema above.

@handrews
Copy link
Contributor Author

#189 (this one) - changes the spec, no dependencies keyword

@epoberezkin what do you mean by this? The dependencies keyword is on lines 83-86. My entire point with this PR has been that it needs to use dependencies. But it only needs to use dependencies when using the boolean form. If the exclusive keywords use the numeric form, they are independent and do NOT incur dependencies, which was the point of introducing the numeric form in the first place.

Why can't we prohibit maximum if exclusiveMaximum is present?

To me that is more complex to implement. I have a function per keyword, which returns true or false. The complex cases are where I have to look at two or more keywords at once.

@epoberezkin
Copy link
Member

epoberezkin commented Dec 28, 2016

@epoberezkin what do you mean by this? The dependencies keyword is on lines 83-86. My entire point with this PR has been that it needs to use dependencies

@handrews Sorry, I meant that there is no dependencies on the top level. It is much clearer to use "dependencies" for all context dependencies, rather than some boolean expression, that validates correctly but obscures the intention. Having dependencies on the top level would allow to merge them all - exclusive, additionalItems (if #209 gets some traction) and $ref (@awwright's suggestion #194 (comment)).
The fact that nobody of us noticed missing properties keyword in this PR (it should wrap exclusiveMaximum/Minimum) is just another confirmation that it is obscure. If you fix it it will be:

"oneOf": [
  {
    "properties": {
      "exclusiveMaximum": { "type": "number" },
      "exclusiveMinimum": { "type": "number" }
    }
  },
  {
    "properties": {
      "exclusiveMaximum": { "type": "boolean", "default": false },
      "exclusiveMinimum": { "type": "boolean", "default": false }
    },
    "dependencies": {
      "exclusiveMaximum": [ "maximum" ],
      "exclusiveMinimum": [ "minimum" ]
    }
  }
]

To me that is more complex to implement.

I didn't mean on implementation level, only on meta-schema level. It is as simple as replacing anyOf with oneOf in my meta-schema and adding dependencies for maximum/minimum:

"dependencies": {
  "maximum": { "not": { "required": ["exclusiveMaximum"] } },
  "minimum": { "not": { "required": ["exclusiveMinimum"] } },
  "exclusiveMaximum": {
    "oneOf": [
      { "properties": { "exclusiveMaximum": { "type": "number" } } },
      { "required": ["maximum"] }
    ]
  },
  "exclusiveMinimum": {
    "oneOf": [
      { "properties": { "exclusiveMinimum": { "type": "number" } } },
      { "required": ["minimum"] }
    ]
  }
}

Why should it affect implementation in any way?

I also think we still can consider dropping boolean form from draft 6 completely. Implementations still can decide whether to support boolean form for backward compatibility, in the same way as some validators support required: true from draft3, but I'm not sure I remember why/how we decided that it is a good idea to keep both forms... It would simplify draft 6 meta-schema even further:

"dependencies": {
  "maximum": { "not": { "required": ["exclusiveMaximum"] } },
  "minimum": { "not": { "required": ["exclusiveMinimum"] } },
  "exclusiveMaximum": { "not": { "required": ["maximum"] } },
  "exclusiveMinimum": { "not": { "required": ["minimum"] } }
}

Which meta-schema is used for schema validation will be determined by $schema keyword, so implementation can still be more flexible than the spec and support draft4 for backward compatibility.

@handrews
Copy link
Contributor Author

I also think we still can consider dropping boolean form from draft 6 completely. Implementations still can decide whether to support boolean form for backward compatibility, in the same way as some validators support required: true from draft3, but I'm not sure I remember why/how we decided that it is a good idea to keep both forms...

@awwright wanted it: #77 (comment)

If you and @awwright are OK with dropping the boolean forms entirely, I am 100% fine with that, just settle on it and let me know.

Otherwise...


The fact that nobody of us noticed missing properties keyword in this PR (it should wrap exclusiveMaximum/Minimum) is just another confirmation that it is obscure.

No, it's another confirmation that everyone is too busy arguing other things to actually read the PR and give me proper feedback. I personally accidentally leave properties out a lot. It's just something that I often forget and I can't believe you are using my personal foible as a global argument against everything. On those grounds, properties itself is too obscure.

It is much clearer to use "dependencies" for all context dependencies, rather than some boolean expression, that validates correctly but obscures the intention.

This is definitely your opinion. I have a lot of trouble following your version of the meta-schema here. To me, mine is very simple. "either the exclusive keywords are numbers, OR they are booleans and require the corresponding non-exclusive keywords." That's all it says. you can directly translate that sentence to match the schema structure:

OR [
    { exclusive keywords are numbers },
    AND [
        { exclusive keywords are booleans },
        { matching non-exclusive keywords must appear }
    ]
]

that's how mine works.

Yours uses a lot of negated "required"s, and while I do understand how they work, they are far from the clearest aspect of JSON Schema.

@epoberezkin
Copy link
Member

they are far from the clearest aspect of JSON Schema.

just see "required" as a synonym of "present"...

@handrews
Copy link
Contributor Author

just see "required" as a synonym of "present"...

I said it's not clear (meaning intuitive). I didn't say I didn't know how to read it.

You're making this epic argument about clarity, and I'm trying to point out that clarity is a matter of opinion. What you find unclear I find clear, and what I find clear you find unclear. We both have a lot of experience with other users of JSON Schema so neither of us are just making this up. It is a legitimate difference in views on what constitutes clear schema writing.

@handrews
Copy link
Contributor Author

I've filed #210 as a version of this that just removes the boolean form entirely, and request that further use of dependencies with respect to the exclusive* keywords be tracked in #209 or as their own issue. Whatever we decide to do with dependencies can be done as a follow-on to #210. If we go with #210 instead of one of these others.

@handrews
Copy link
Contributor Author

#210 got merged so this is no longer needed

@handrews handrews closed this Dec 29, 2016
@handrews handrews deleted the excl branch January 5, 2017 19:44
@gregsdennis gregsdennis added clarification Items that need to be clarified in the specification and removed Type: Bug labels Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Items that need to be clarified in the specification Priority: High
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants