Adding tests for referencing different schema versions

[gregsdennis]

As requested here. Merging is probably dependent upon resolution of that issue.

Added test for:

• referencing draft 6 schema from a draft 4 schema
• referencing draft 4 schema from a draft 6 schema
• referencing draft 4 schema from a draft 7 schema

The referenced schemas include exclusiveMinimum which was a breaking change between drafts 4 & 6, so they should be a fairly good indicator that a validator supports referencing to different schema versions.

I have tested these with Manatee.Json.

(Feel free to cherry-pick out the .gitignore file. It's just something I added because I have to work on a Mac for now (ew).)

[awwright]

Which model (inclusion or delegation) is this testing, again?

[gregsdennis]

I think these tests would validate for the delegation model, but break for the inclusion model. In fact, you may go so far as to say they'd be invalid for the inclusion model, which makes the decision in json-schema-org/json-schema-spec#514 that much more important.

[awwright]

@gregsdennis Ok. So if I understand this right, either way, none of the existing tests would break because we've never tested this branch of behavior before.

[Julian]

That issue still has so much activity that I can't follow it at this point :), so apologies if this is still a dumb question, but while I can guess what this is after, does it really depend on remote retrieval? Are implementations not any more or less likely to do this incorrectly just from schemas within definitions inlined?

[gregsdennis]

@Julian I don't know that this was intended to actually be merged. For now, it serves more of a point in the context of the discussion.

Inlining these is specifically forbidden because $schema has been deemed to only be valid at the root of a document. These scenarios specifically test compatibility when a referenced schema is defined by a different draft than the schema referencing it.

[handrews]

I think we want 10 here, and validation outcome of false. If an implementation applies the local meta-schema to the remote schema and validates it, then it doesn't matter what the number is- the boolean exclusiveMinimum will fail validation against the draft-06 meta-schema.

However, if the implementation does not actually validate against the meta schema (and implementations are not required to do so), then it's possible that an implementation might just ignore the boolean exclusiveMinimum, which means it would improperly accept 10 as an instance.

[handrews]

If we make this 10 with a validation outcome of false then I think we catch the inconsistency, similar to the other cases.

[gregsdennis]

I just added that locally and my validator still passed. I'll update the PR with the new test case.

[gregsdennis]

@handrews I've added your test cases. They all pass for me.

[Julian]

Got it -- OK, I didn't recall that $schema had to be at the root of an object (actually, I don't think that's in the meta schema from last recollection, is it now?)

But in that case yeah seems reasonable enough.

[Julian]

The failed check by the way is telling you that unfortunately at the minute, any schema that's in remotes actually needs to be in two places, where you put it, but also in a second file.

[gregsdennis]

@Julian I don't understand your comment about the second file. Where else would I need to put it?

[Julian]

@gregsdennis it needs to go here too unfortunately: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/bin/jsonschema_suite#L46-L59

(Happy to explain the reasons why, and how we can fix that, which I think I also mentioned in another ticket, but yeah, for now it needs to go there too as a Python object [and no that's not related to my actual personal implementation :)]. If you need help there on exactly the syntax I can help of course)

[gregsdennis]

@Julian I think I missed something. I thought I got it, but fell short.

[awwright]

I just realized maybe we should have a separate tree, such as tests/latest/ for tests detailing effects of proposed changes to the draft.

[Julian]

@gregsdennis think you did manage to get at least my part now, but there's some JS stuff failing... will have a look to see if I can tell what that is (or maybe someone else knows).

[gregsdennis]

@Julian does the build actually eval the tests to make sure they're correct? If that's the case, then it may not be set up to handle referencing one schema version from another.

It might make sense to have these tests placed in a new file in the /optional folders.

[Julian]

It looks like that part is ajv related, so I don't actually know :) (though I hope not, the Python one does not IIRC, and though I've always had in mind to do so in a separate set of checks, this particular thing was always meant to be literally sanity checking for the tests themselves, not tests of implementation adherence.)

Presumably @epoberezkin might know what the failure means?

As for optional -- as I'd read it, they're not actually optional right? I think an implementation needs to have this behavior.

[epoberezkin]

Ajv requires different configuration depending on which draft(s) you want to support. Even though it's possible to configure it to support 4, 6 and 7 at the same time, because all test suites required supporting only one draft it was configured in this way - different ajv instance is used for each draft.

I can make tests pass, but

1. Given that these tests are for all validators, it is not fair to require that any validator supports more than one draft, so I don't think they should be merged in any of the draft suites regardless of What is "$ref" and how does it work? json-schema-spec#514 outcome.
2. If we still want to add tests for validators supporting multiple drafts I suggest creating a separate top level folder, say /tests/draft467, otherwise the validators that want to implement only draft7 won't be able to use draft7 folder without filtering which tests to use. At the very least it should be a separate file.

As for optional -- as I'd read it, they're not actually optional right? I think an implementation needs to have this behavior.

If the validator supports multiple drafts, then it should allow referencing schemas that use different drafts from each other. But supporting multiple drafts is not mandatory, so if not a separate top level folder, then putting it to optional also makes sense...

[Julian]

Got it, fair point, sounds reasonable to stick it in optional then.

[awwright]

I would say put it in an existing directory/tree and allow optional schemas to reference earlier meta-schemas.

…

On Dec 8, 2017 15:01, "Greg Dennis" ***@***.***> wrote: @epoberezkin <https://github.com/epoberezkin> I'm on board for putting these tests in a separate "multi-schema" folder, but I disagree with you on the point that it's unfair to require validators to support multiple drafts. If an implementor decides not to support a draft, then that's their choice. For instance Manatee.Json doesn't support draft 3, but it does allow referencing a supported draft from another supported draft (because that's how I interpreted the spec). It all hinges on the determined definition of $ref. *If* it's defined in such a way that an external schema can be any draft, then I would suggest that implementations need to support that functionality. Furthermore, I think it's more likely in the real world that this could occur, especially in a world where we share and catalog schemas <http://schemastore.org/json/>. See my example <json-schema-org/json-schema-spec#514 (comment)> . — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#210 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAatDTejWOm_4C6oB1VZnw03E7oZHeCaks5s-b_AgaJpZM4Q5ASB> .

[gregsdennis]

@awwright

allow optional schemas to reference earlier meta-schemas.

While I understand your intent here (older schema versions wouldn't be compatible with newer schema version), it doesn't address the upgrade scenario I mentioned where a referenced schema is upgraded to a new version. And to be fair, newer schema versions aren't necessarily compatible with older ones, either.

[epoberezkin]

@gregsdennis I also think these tests should be optional because they should pass only for validators that support multiple drafts - no validator must be required to support more than one draft to be compliant.

It has nothing to do with drafts being backward compatible or not.

[gregsdennis]

@epoberezkin correct, on two points: making these optional seems to be ideal, and validators only need to support those versions they wish to.

Regarding backwards compatibility, that was a response to @awwright's comment about earlier meta-schemas. I agree with you that this Isn't about backward compatibility, but rather schema independence at the $ref boundary.

[gregsdennis]

So now that #514 seems to be resolved, are we waiting on me to move these into an optional folder? Are we just closing this? What are the next steps?

[handrews]

@gregsdennis I think we'll keep this open and resolve it after #514 actually gets a PR in. Since it will technically be a change (the last time the behavior of $ref was explicitly defined, it was defined as inclusion), we'll need to decide how and if that shows up in the drafts where $ref's behavior was left a bit ambiguous.

I'd say rebase this on top of the current master (so the CI checks work again), and then we probably wait until we're closer to draft-08 going out or until (probably after the holidays) we have some consensus on whether we should be testing this with older drafts.

Clearly we would need to test draft-08 referencing older drafts. What about the other way around? What about draft-06 referencing draft-04? What about draft-08 referencing draft-06 referencing draft-04? We will nail that down during the PR process for #514 ( Everyone: PLEASE DO NOT DEBATE IT HERE!!!! )

If there's stuff you can do now about marking things as optional that's fine.

[epoberezkin]

@gregsdennis Yes, If it's in optional it should be ok. Please add it to the excluded tests here as "optional/something" (assuming it's in optional folder of draft-07 only), I would later amend the test code so it is executed (and works correctly).

[gregsdennis]

@epoberezkin I still seem to be missing something.

[epoberezkin]

@gregsdennis to make them work with Ajv you need to add remotes here: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/index.js#L7 and also add darft-04/06 meta-schemas to the instance that runs draft-07 tests here: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/index.js#L28

If the intention was to remove them and do not run them in Ajv, then they should be removed from the file https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/tests/draft7/refRemote.json, because now one test is present in two places.

Also, @Julian was against adding generic files to .gitignore (see #169 (comment)).

[gregsdennis]

@epoberezkin

1. Added the remotes to the list.
2. I added the meta-schemas somewhat manually (I think).
3. I'd like these tests to run, but failures should be more inconclusive rather than failing the build. That's what understand "optional" tests to be. I've added them to draft7/optional; forgot to remove from draft7/refRemotes.json.
4. Reset .gitIgnore

[gregsdennis]

@epoberezkin are we going to proceed with this? I need some help, if so.

[gregsdennis]

@Julian @epoberezkin I'm not sure what I need to do to make this build.

[Julian]

@gregsdennis will try and have a look what the story was here

[Julian]

Sorry for leaving this hanging :/ I saw it likely required some ajv understanding and therefore wouldn't be a 5 second thing for me either -- but @gregsdennis definitely haven't forgotten about it, will see if I can eventually get some time to decipher what this is about...

[Julian]

Bleh, and not close the issue accidentally...

[Julian]

@gregsdennis I don't know what the heck GH did (in combination with whatever I did) which made this PR close and not be able to be reopened, but we never added these. Any chance you're interested in resubmitting it?

[Julian]

We probably need to consider how to write it precisely, since an implementation may support any possible subset of previous drafts...

[gregsdennis]

I'll have a look tomorrow. 👍

[gregsdennis]

I'm wondering how scalable this kind of test is. Would we need to include tests for all of the draft combinations?

[Julian]

It doesn't scale very well, I think your implication is right, but in practice I suspect we can still be useful enough by simply starting with a few pairs of drafts as you did and deciding / waiting later to hear whether someone needs another combination we didn't yet add.

I.e. I think yeah ideally we'd have number-of-releases choose 2 scenarios which is 10 cases for {6, 7, 2019, 2020, next}, but if we just have 3 or 4 of the 10 which we think are the most likely then I think we're probably better off than not having it at all, personally. But yeah if you've got a better idea lemme know.

[gregsdennis]

I'd ultimately want both directions, too. Given two draft 6's, you can update either to 2020-12, and they shouldn't break in either case (assuming the implementation supports both 6 and 2020-12). So it's number-of-releases permute 2.

[Julian]

Fair enough though the same applies I think -- we don't need 20 to say we've improved things, if we just have 4-6 we're probably covering enough?

[gregsdennis]

I don't have this branch anymore. I'm going to have to rebuild it anyway.