some tests for keywords added in later drafts

[karenetheridge]

these are mostly the same as tests from draft2019-09, with the results
adjusted to accomodate for the keywords not being recognized. In most cases
this results in validation always being true, but in some cases (such as
contains + minContains) the results from existing keywords change their
outcomes as well.

[karenetheridge]

rebased.

[Julian]

To bikeshed the name (sorry), maybe "future-keywords" is a bit more correct than "unknown-keywords"? Presumably we should have some for a random nonexistent keyword (that all instances are valid), but I suspect that exists already in the suite somewhere.

[karenetheridge]

sure. Unknown is in the context of the current draft - "unknown to us", not "unknown to other drafts", but I have no strong feelings either way.

[karenetheridge]

I renamed the files from unknown-keywords.json to future-keywords.json.

I also added a file for 2019-09 containing a very very basic test of $dynamicRef (copied from ref.json). If/when we ever have some $recursiveRef tests in 2019-09 we can copy them into there with s/\$recursiveRef/\$dynamicRef/.

[Julian]

Not sure I follow the 2019-09 piece -- (apologies, probably I don't know what dynamicRef is, so if I should just look somewhere let me know? Is this a keyword that's going to be in the 2020 draft?)

[karenetheridge]

$dynamicRef is being defined in json-schema-org/json-schema-spec#930. actually I should add prefixItems too.

[ssilverman]

Shouldn’t these tests be in a different draft directory? Eg. draft2020-xx or something.

[karenetheridge]

Shouldn’t these tests be in a different draft directory? Eg. draft2020-xx or something.

No, tthey are in the right place. They are checking that these keywords are not recognized in draft2019-09, and the results will be different than this for draft2020-0X.

[ssilverman]

I’m not sure I see the purpose in adding new keywords to tests for all older drafts every time there’s a new draft. That, to me, is like requiring testing for all possible keywords that don’t exist in a draft. 🤷‍♂️
I dunno, I’d love to test that concept but that would require a ”negative” test concept.

[ssilverman]

Just to flesh this out a little bit more, I’ll just brain dump and think “aloud”: doesn’t this mandate testing for unmandated behaviour? For example, if a draft doesn’t say that the existence of an unknown keyword shouldn’t do something, doesn’t the existence of a test for that mandate something that’s unmandated? Does that make sense?

Bottom line is: maybe there’s a way to test unmandated keywords for each spec in a different and less tedious way. Just mulling here...

I’m not specifically against testing things that don’t belong in a spec, it’s just a lot to keep track of and it feels like it should be easier. That would be nice. :)

[awwright]

If an implementation doesn't know what $anchor is, then this isn't going to be false, it's going to emit an undefined reference error when it encounters {$ref: "#foo"}

[karenetheridge]

agreed, I'll take this out. I wrote this before the discussion on undefined-behaviour-not-being-the-same-as-invalid.

[Julian]

The drafts don't say keywords that are unknown should do nothing, they say they should be ignored -- and that implementations can add their own.

So I think the point actually is good here -- I do see good reasons to add these for reasons you probably are saying (so that implementations can ensure they don't leak backwards functionality if they want that guarantee for themselves)

But an implementation is free to make the reverse choice, and add additional keywords to older drafts, and it's still valid for that draft (e.g. someone can add $anchor or whatever other custom keyword to draft 7 they feel like for their specific implementation)

Which essentially means these tests probably need to go in optional, since yeah, a valid implementation may still not pass them.

Does that make sense?

[karenetheridge]

The drafts don't say keywords that are unknown should do nothing, they say they should be ignored

Ok, sure, but "do nothing" is the same as "ignored", isn't it? :)

But an implementation is free to make the reverse choice, and add additional keywords to older drafts

Yes and no -- some draft revisions are not backportable to previous drafts without changing existing behaviour that would violate the spec -- for example, contains now behaves differently when minContains is present, vs. it not being there. Should we allow a draft7 implementation to behave differently when it sees a minContains?

At least now (starting with draft2019-09), modifications to the core specification should be made more clear, by the presence of the $vocabulary keyword in a metaspec. So that means that a schema with "$schema": "https://json-schema.org/draft/2019-09/schema" should never allow additional keywords to alter behaviour, because the schema is saying "I follow the draft2019-09 metaschema, with its $vocabulary provisions and no more". In that light I should add the $schema keyword to the schemas in tests/draft2019-09/future-keywords.json. As for the tests for previous drafts, the concept of additional vocabularies did not exist yet, so what should we assume for behaviour there?

[Julian]

Ok, sure, but "do nothing" is the same as "ignored", isn't it? :)

Yes sorry, all I mean is "they don't say 'you must not implement additional behavior", they just say "don't blow up" essentially -- if that's clear we can use whichever terminology :D.

(I don't know about the rest yet, I have to think a bit more carefully, probably about each example individually, as you say).

[Julian]

Merged (after rebasing and moving as per the above). Thanks!

[karenetheridge]

I'm sad that the "squash!" commits made it into the main branch without being squashed :/ I left them unsquashed in the PR to make it easier to review, and the intention was to clean up the branch prior to merging.

[karenetheridge]

The last commit added to this pull request made the tests incorrect. The tests were in the non-optional section before and all marked as "valid": true -- that is, that the keywords are ignored. But the last commit moved them to optional/, with the intent that it is optional whether the implementation chooses to actually implement these keywords (that is, to not ignore them, and to sometimes return false instead of true).

[Julian]

See the discussion above (which you agreed with) -- that was where they belonged, as an implementation is indeed allowed to implement the keywords even on older drafts before they were part of the spec.

[Julian]

Sorry, glancing at this again given I left it open on a tab, perhaps what you're referring to by "incorrect" is specifically draft2019-09, with what you'd said previously:

At least now (starting with draft2019-09), modifications to the core specification should be made more clear, by the presence of the $vocabulary keyword in a metaspec. So that means that a schema with "$schema": "https://json-schema.org/draft/2019-09/schema" should never allow additional keywords to alter behaviour, because the schema is saying "I follow the draft2019-09 metaschema, with its $vocabulary provisions and no more".

which it seems we indeed didn't hash out here, but which I still believe is correct as merged. The spec (in 2019-09 and newer) does not say an implementation may not add keywords itself. The vocabulary mechanism allows schema authors to tell the implementation they require support for some vocabulary, but does not prohibit the implementation from offering other additional supported keywords. If that's what you disagree with, can you point out where in the spec says that to you?

I just double checked, and in support of the above, the spec says in §6.5:

Additional schema keywords and schema vocabularies MAY be defined by any entity. Save for explicit agreement, schema authors SHALL NOT expect these additional keywords and vocabularies to be supported by implementations that do not explicitly document such support.

i.e. a schema author cannot depend on it, but an implementation may offer it and enable it.

And in §4.3.3:

While keywords can be supported outside of any vocabulary, there is no analogous mechanism to indicate individual keyword usage.

Both of which to me confirm the above, but happy to be wrong if you see something else.

[karenetheridge]

Okay there's two things being conflated here, and I think you missed the second point:

• first, you're saying that it's acceptable for earlier drafts to decide to understand and process keywords from later drafts. That may be the case, but I think it's worth discussing (as was the case for adding $schema to all tests that came up recently), because it seems pretty strange to me.
• second, assuming the first point is all totally fine, the tests are wrong. When I wrote these tests I took keyword tests from future drafts and changed the results all to true, to demonstrate that the keywords would be ignored. You didn't readjust the valid/invalid results of these tests when you moved them, so if an implementation did choose to understand these keywords, the results are now wrong for that expected result.

It really should be required to run new tests against an implementation to confirm that the results are satisfactory before merging, otherwise it's way too easy for mistakes to be made and incorrect tests can get merged.

[Julian]

I did not miss the second point. The tests are correct as is, and are meant to catch exactly the same scenario you originally sent them for -- any implementation which knows it does not implement future keywords will enable the optional tests and ensure the same thing you originally filed this for.

Any implementation which does implement newer keywords will not enable the optional test file. It's true such an implementation would benefit from an exactly conflicting file with "true/false behaviored" results. Feel free to PR one, but I don't think any implementations really do such a thing, so it doesn't seem high priority to add. The point was simply an implementation is allowed to do so, so the tests cannot go in the required directory.

[karenetheridge]

Feel free to PR one

No thank you. I would object to such a PR.

I don't see the sense in a set of optional tests that all return true. The tests aren't really saying anything at all. "Do this. Maybe. Or not. Whatever. We don't care." What is the point?

This isn't what I intended with this PR at all. But I submitted it over two years ago, so I'm not really sure now what I was thinking at the time. I would have appreciated a chance to revisit it before it was merged, let alone changed in a way that I didn't intend.

[Julian]

The original intent of the PR was to give implementations a chance to prevent leakage. That intent is preserved, it simply is opt-in, in order to comply with the spec.

I'm not sure what to say about revisiting -- please do take the time to revisit any of the PRs you have opened beside this one, I pinged you on a few to review. I'll be merging or closing them in the next few days so now's the time.

There's no real reason to fret doing this before or after though -- if something is wrong, we can fix it going forward. In the case of this PR, everything's fine as far as I see, unless there's something in the spec saying it isn't.

[karenetheridge]

I pinged you on a few to review. I'll be merging or closing them in the next few days so now's the time.

I haven't gotten any pings. As I subscribe to all these repositories, all emails for them go into one folder and an explicit ping is lost. If I appear unresponsive on something, feel free to poke me in slack.

I've moved them all to draft until I have a chance to look at them again.

[karenetheridge]

#559 reverts this PR.

[Julian]

I can't tell if you're not reading my comments. #559 is incorrect, as I explained above in my last comment, which you must have seen? Going to close it. Happy to explain further if it's unclear.

[handrews]

I noticed the back and forth through the github slack channel and thought I'd take a look. @Julian I have to say it's not clear to me. For one thing, I can't figure out why this PR is in a closed state but apparently merged? And there was a move of something to somewhere? Is that move commit ea68a4f and was it done in a separate PR somewhere?

Assuming that's correct, is the debate here about whether they make sense in "optional" vs somewhere else?

To me these make more sense in a "future" directory as they're not really testing spec-mandated optional behavior, but perhaps I don't understand what "optional" is supposed to mean?

[handrews]

I'm also confused about the process here. Did you request changes from @karenetheridge , @Julian ? This is why I was asking about the other commit being in a PR as I was trying to understand who reviewed that further change.

[Julian]

I can't figure out why this PR is in a closed state but apparently merged?

GitHub is confused by the rebase yes.

Assuming that's correct, is the debate here about whether they make sense in "optional" vs somewhere else?

Yes, essentially. Optional is conflated (which is a known issue that's intended to be fixed, see e.g. #361 or #345 or a few other issues. We need to fix it, but today optional essentially has any test that an implementation may or may not need to run depending either on its implementation language or choices.

Did you request changes from @karenetheridge , @Julian ?

Yes, awhile back, and then just made them myself to merge this.

[handrews]

Yes, awhile back, and then just made them myself to merge this.

Oh. I see that "awhile back" was in 2020, and there doesn't appear to have been a check-in about it before merging. I have to say if someone had done that to one of my PRs on the spec repo I'd feel a bit miffed.

I'm still trying to understand what's going on as I don't see where @karenetheridge agreed to the move, as there was only one comment between you proposing that move and you doing it. But I also can't quite figure out if she was outright disagreeing, or on what grounds, so maybe I'm just missing a lot here. I get the contains example, but not the implication for the choice of directory.

This all seems to be related to the ongoing murky conversations about cross-draft behavior. Perhaps it would be reasonable to back this out as proposed in #559 so we can get consensus? I suspect the JSON Schema world can wait another week for these cases.

[Julian]

I took that comment as agreement, though indeed I'd completely forgotten we didn't address 2019-09 (though again I believe the newer specs to be no different).

I'm happy to back this out while we discuss, @karenetheridge can you elaborate on whatever you feel needs changing?

[Julian]

Let's discuss on #561 so we're not using a closed PR.

[karenetheridge]

I'm still trying to understand what's going on as I don't see where @karenetheridge agreed to the move, as there was only #376 (comment) between you proposing that move and you doing it.

I hadn't been to this ticket in two years. If I had been poked about it I would have been happy to revisit it, clean up the "squash!" commits that should have never been merged in this form, and run the tests against my implementation again to see if I still agreed with how they were written. I would have made some substantial changes here had I had a chance to review it.