Validation meta-schema: Implement "exclusiveMaximum"/"exclusiveMinimum" changes

[awwright]

Issue #77

[handrews]

I know you don't like how I wrote it in #168, but what you have here flatly contradicts the way the exclusive fields have always been declared in the meta-schema and does not implement the current specification.

Yes, the current specification produces a complex meta-schema, but that is the correct meta-schema and we don't just ignore the complexity because it is aesthetically displeasing.

[awwright]

@handrews Which language is being contradicted?

The only requirements (the ones with capital MUST or REQUIRED) are that it be a number or a boolean. There's no actual requirement that boolean exclusiveMinimum be accompanied by minimum, so I removed that.

[handrews]

There's no actual requirement that boolean exclusiveMinimum be accompanied by minimum, so I removed that.

With no issue filed. With no discussion. Nothing. You just don't like it so you took it out.

[awwright]

@handrews The json-schema-validation document doesn't specify that boolean exclusiveMinimum be accompanied by minimum, is that correct?

If so, is it fair to remove "dependencies" from the meta-schema?

[handrews]

If so, is it fair to remove "dependencies" from the meta-schema?

I'm not sure what "fair" is supposed to mean here. While the specification does not explicitly declare that, the meaning is clear both from the description of the behavior and the previous existence of the "dependencies" entry in the meta-schema. It was put in for a reason- namely to be helpful for ensuring a valid schema.

There are many situations where "nonsense" schemas are allowed because they have some benefit in complex schemas, particularly in complex re-use patterns.

But there is no benefit whatsoever to allowing a boolean exclusive keyword without the corresponding numeric boundary keyword. Having the meta-schema enforce that, whether the specification calls it out or not, is a UX benefit to schema authors.

[Relequestual]

@handrews While I agree that it would be a better UX for schema authors, the validation spec doesn't actually specify that one means the other is required. Maybe it should be, but also the default action seems to be to ignore anything which doesn't make sense, and make a best effort.

We want to build the meta-schema for draf-5 as close to the spec as possible, as writ. It's obvious that in this instance that exclusiveMinimum should require minimum, but the spec doesn't say this, so it should be legal.

There should (if there isn't alread) be an issue to fix the langauge to either make one key word require another, or to state that exclusive should be ignored if minimum isn't present.

I've had similar issues in the other spec I'm working on; assumed definitions of things that don't line up with what's actually written. I was very strict, others were not. Ambiguities should be fixed, and that's an issue with draft-5, but the meta-schema shoudn't make assumptions this way.

I'll reiterate, the ambiguity issue should still be addressed for draft 6.

--

AND, the way in which these issues and PRs have been managed is far from ideal. @awwright I'm firmly against closing someone else issues or PRs unless it's clear they would want me to do so, or an amicable resolution has been agreed upon. Quite frankly doing so is offensive, especially with effort on #168 . It's not the behaviour the community deserves.

[Relequestual]

Why has this been removed? It's in the spec document.

[Relequestual]

Same as previous.

[Relequestual]

@awwright Also, I agree with @handrews, removing the clase

If exclusiveMinimum is present, minimum MUST also be present.

was a terrible idea. Why do that?! You say:

There's no actual requirement that boolean exclusiveMinimum be accompanied by minimum, so I removed that.

I can't tell if you mean "there's no requirement in the draft-5 validation document I released" (which is true), or you mean, "when I evaluated draft-4 validation, I considered that it shouldn't be required". In which case, @handrews is right to be annoyed at this changed in draft-5 and I would have also fed back on this if I'd seen the removal of that clause, because it opens a clear ambiguity.

In my previous comment #185 (comment), I didn't realise that it was in draft-4 and was actually removed for draft-5... =/

As per https://tools.ietf.org/html/draft-fge-json-schema-validation-00#section-5.1.3 (draft-4 validation)

This highlights the need for the changes you make to be accountable and agreed upon.

[awwright]

@Relequestual I'm not sure who changed that. The initial commit in this repository has that language already removed, and I don't recall making any changes before I copied the repository over verbatim. If that was me, it was a change made well over a year ago in the old repository (that I don't have commit access to).

To be clear, it's entirely possible it was me, I don't know anyone else who was working on the document, I just have no clue how.

[handrews]

@awwright @Relequestual I fixed both the spec and meta-schema together in #189

[awwright]

As I'm doing more looking around, there were different versions of the "validation" document circulated around, some that were edited since the draft-fge-json-schema-validation-00 release. Some have this language, some don't. I can't find a copy that looks exactly similar to the one I imported, but I'm guessing it's one of the ones that didn't. I'll keep poking. It's entirely possible there might have been a repository that someone deleted.

In any event, the document already says

Schemas SHOULD NOT use the boolean form.

While I can't find any commits where I removed the "dependencies" language, I did remove or reduce a lot of other requirements language, since usage of MUST or REQUIRED is supposed to be limited to promoting interoperability. And similarly here, I don't think it makes sense to add a best-practice requirement on how to use a feature that's explicitly a discouraged practice.

Also consider the boolean form was kept in to facilitate reverse compatibility. There is no correct way to use the boolean form anymore. And if we take it out entirely, this becomes a non-issue.

[handrews]

I don't think it makes sense to add a best-practice requirement on how to use a feature that's explicitly a discouraged practice.

We're not adding anything. There is also no reason to take away a useful constraint that was present and correct. As long as we are keeping it for backwards compatibility it should remain identical when used.

And if we take it out entirely, this becomes a non-issue.

Of course, but we're not doing that right now. If you want to do that, open a new issue for it.

[awwright]

#189 is probably a more useful forum for this, but I'll just clarify as far as I can tell, there was never a strong opinion on the "dependencies" requirement to begin with. draft-zyp-json-schema-03 does not have this language. And I find it sort of silly that everyone seemed cool with the current wording (lacking the requirement) until it was uncovered it might have been changed recently.

[handrews]

I find it sort of silly that everyone seemed cool with the current wording (lacking the requirement) until it was uncovered it might have been changed recently.

I was not, I just didn't realize that it had been removed. See @Relequestual's point about opportunity for review- whether you removed it or not, it was neither discussed nor noted in the change log.

And if you haven't figured it out, I do have a strong opinion on "dependencies". Its presence is useful for validation and it doesn't hurt anything to have it there.

[epoberezkin]

It could be:

"dependencies": {
  "exclusiveMaximum": {
    "anyOf": [
      {
        "properties": {
          "exclusiveMaximum": { "type": "number" },
          "maximum": false
        }
      },
      {
        "properties": {
          "exclusiveMaximum": { "type": "boolean" },
          "maximum": true
        }
      }
    ]
  }
}

[epoberezkin]

And with if/the/else it would look more elegant :)

"dependencies": {
  "exclusiveMaximum": {
    "if": { "properties": { "exclusiveMaximum": { "type": "boolean" } } },
    "then": { "required": ["maximum"] },
    "else": { "not": { "required": ["maximum"] } }
  }
}

the else branch requires that "maximum" keyword is absent when "exclusiveMaximum" is present, which can be dropped as a requirement - maybe having them both is ok.

[awwright]

I gotta say, these solutions make me feel a lot better about pushing this forward, and how well JSON Schema is designed in general.

[Relequestual]

Just so I'm clear, which is the associated issue for this PR?

[handrews]

@Relequestual

Just so I'm clear, which is the associated issue for this PR?

Both this PR and #189 are solutions for issue #77

[awwright]

I updated the topic body, and the #77 title to reflect the solution.

[handrews]

@awwright since #210 has been merged is it OK to close this? If there's something here you want to keep pursuing it would probably be easier as a new issue and/or PR.