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

Add support for date, time, and date-time validators, and support a greater range of schema ids. #121

Closed
wants to merge 17 commits into from
Closed

Add support for date, time, and date-time validators, and support a greater range of schema ids. #121

wants to merge 17 commits into from

Conversation

yspreen
Copy link

@yspreen yspreen commented Apr 21, 2021

Extending the work on #118​.

We had an issue where our schema id is

http://json-schema.org/draft/2020-12/schema#

instead of

https://json-schema.org/draft/2020-12/schema"

So this PR makes IDs more forgiving.

@yspreen
Copy link
Author

yspreen commented Apr 21, 2021

Maybe someone can help me interpret the travis error message? I'm not sure what's going wrong here

@kylef
Copy link
Owner

kylef commented Apr 21, 2021

I've introduced a few related changes:

  • ed6041e - Error out when an unknown meta schema is referenced to prevent unexpected behaviour or confusion.
  • 519db4c - Bumped the default schema version to the latest
  • 78cee2a - Normalise the schema comparison so that empty fragment differences are equal.

From the notes in http://json-schema.org/specification-links.html, it sounds like the newer meta schemas are not expected to be available over http, and are only available that way due to implementation details (limitations of GitHub pages). Thus I'm not sure about that one:

NOTE: All meta-schema URIs now use https://. While currently also available over plain HTTP due to the limitations of GitHub pages and the need to keep prior drafts available over HTTP, only the HTTPS URIs should be used.

@yspreen
Copy link
Author

yspreen commented Apr 22, 2021

Thanks for the feedback!

I'm specifically using this library as part of a huge european project with many stakeholders. There is no control from my side over how they generate the schema JSON. This is what it currently looks like, I think they auto generate it somehow: https://github.com/ehn-digital-green-development/ehn-dgc-schema/blob/main/DGC.schema.json

They did just move to draft 7... so I do still have to test with that one. Previously they used http://json-schema.org/draft/2020-12/schema#

I'll check if #118 is compatible with the most recent schema. If not, we'll have to keep using our own fork. It'd be awesome to benefit from the contributions to this project continuously and work with an approved PR though!

@kylef
Copy link
Owner

kylef commented Apr 22, 2021

Draft 7 is http, the specs after that (2019-09, 2020-12) are https:

$ http http://json-schema.org/draft-07/schema | jq -r '.["$schema"]'
http://json-schema.org/draft-07/schema#

$ http https://json-schema.org/draft/2019-09/schema | jq -r '.["$schema"]'
https://json-schema.org/draft/2019-09/schema

$ http https://json-schema.org/draft/2020-12/schema | jq -r '.["$schema"]'
https://json-schema.org/draft/2020-12/schema

Sounds like master would now count for your case, and that your implementation is correct in the generator (albeit that the fragment strictness tripped up JSONSchema.swift).

@yspreen
Copy link
Author

yspreen commented Apr 22, 2021

Great! But let me ask this: What's the benefit of this strictness? IMO this PR still is a net positive, and changing back to 2020-12 is currently an open discussion on our project. Who knows if we fulfill the spec then or if I'll have to re-open this PR here...

I'd vouch heavily for the robustness principle, since it would've prevented me from even landing on the issues page of this repo: "be conservative in what you send, be liberal in what you accept"

@kylef
Copy link
Owner

kylef commented Apr 22, 2021

What's the benefit of this strictness?

To be consistent with the JSON Schema specification which states that HTTPS should be used. If all implementations behave different to the spec that breaks interoperability and makes it harder in the long run for schema authors, such as you move to a different tool and then it breaks and it may be non-obvious that it has broken.

I'd suggest having the discussion with JSON Schema writers, if they agree then it can be in the spec so all implementations can follow, the wording they have right now states that HTTPS should be used as I've mentioned. If the referenced clause was removed from json-schema.org and that http would be valid for 2020-12 then we should implement it here so that users can have full interoperability across implementations. Instead of a different and inconsistent experience depending on the tool.

I'd vouch heavily for the robustness principle, since it would've prevented me from even landing on the issues page of this repo

The change I've made in ed6041e would have prevented that, as the validator would have errored out for meta schema's that it doesn't know how to validate. The validator would be robust in the sense that no unexpected behaviour would have occured, also if you asked to validate with a version of JSON Schema that was not supported (like draft 3).

changing back to 2020-12

Changing to 2020-12 is fine, just that spec has a https:// in it per the json-schema.org clause.

@yspreen
Copy link
Author

yspreen commented Apr 23, 2021

This is not about spec, it goes deeper than that. The robustness principle (should someone choose to conform to it, and it is a choice, not a law) states

In other words, programs that send messages to other machines (or to other programs on the same machine) should conform completely to the specifications, but programs that receive messages should accept non-conformant input as long as the meaning is clear.

I don't know about you, but I think it's pretty clear messaging when someone is referring to a schema, regardless if the string is either

http://json-schema.org/draft-07/schema#,
https://json-schema.org/draft-07/schema#, or
http://json-schema.org/draft-07/schema.

But maybe that's just me.

@kylef
Copy link
Owner

kylef commented Apr 23, 2021

I don't know about you, but I think it's pretty clear messaging when someone is referring to a schema, regardless if the string is either

http://json-schema.org/draft-07/schema#,
https://json-schema.org/draft-07/schema#, or
http://json-schema.org/draft-07/schema.

The top and bottom will work, the middle one will not. It will currently give you an error that metaschema is not supported. I agree with supporting https where the spec was written with http and the metaspec is also available over https would make sense. I do not agree with automatic downgrade when the JSON Schema website states the spec shouldn't be available over http.

I do not see how this pull request is more robust than the existing implementation, it imposes more limitations and removes existing support. This pull request removes support for schema lookup via http for draft 4, 6 and 7 which is counter to the JSON Schema spec which states those as the URL for the metadata with http, this is a backwards breaking change.

$ http http://json-schema.org/draft-07/schema | jq -r '.["$id"]'
http://json-schema.org/draft-07/schema#

The lookup should always work with the exact $id or id declared in the spec. That's a bug making it less robust.

@kylef kylef closed this Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants