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

Incorrect Revert "by default, "format" only annotates, not validates"? #660

Closed
Relequestual opened this issue Mar 28, 2023 · 5 comments · Fixed by #663
Closed

Incorrect Revert "by default, "format" only annotates, not validates"? #660

Relequestual opened this issue Mar 28, 2023 · 5 comments · Fixed by #663
Labels
good first issue An issue that is a good candidate for new contributors missing test A request to add a test to the suite that is currently not covered elsewhere.

Comments

@Relequestual
Copy link
Member

Relequestual commented Mar 28, 2023

PR #515 reverted/removed some tests. I believe this was in error and should not have been merged.

In the PR, @karenetheridge argues...

According to the spec, "$vocabulary": { "...format-assertion": false }
does not mean "do not assert formats", but rather "if you don't support
format-assertion, you can still load the schema. but if you do support it,
feel free to assert that behaviour if you wish".

That's correct. They continue...

As such, the data in the removed tests could result in either a true OR a
false result. If we wish to explicitly test "formats are only annotations, and
NOT assertions" behaviour, we will have to define a custom metaschema and
reference that with the $schema keyword, where the metaschema contains:

"$vocabulary": {
...
"https://json-schema.org/draft/2020-12/vocab/format-annotation": true,
}

and does NOT mention the format-assertion vocabulary whatsoever.

This would be a reasonable test of the $vocabulary keyword, too.

When we inspect the 2020-12 meta-schema, and the 2019-09 meta-schema, we actually find the following $vocabulary declarations...

    "$vocabulary": {
        "https://json-schema.org/draft/2020-12/vocab/core": true,
        "https://json-schema.org/draft/2020-12/vocab/applicator": true,
        "https://json-schema.org/draft/2020-12/vocab/unevaluated": true,
        "https://json-schema.org/draft/2020-12/vocab/validation": true,
        "https://json-schema.org/draft/2020-12/vocab/meta-data": true,
        "https://json-schema.org/draft/2020-12/vocab/format-annotation": true,
        "https://json-schema.org/draft/2020-12/vocab/content": true
    },

https://github.com/json-schema-org/json-schema-spec/blob/769daad75a9553562333a8937a187741cb708c72/schema.json#L4-L12

Git histroy shows that the line format-annotation: true was comitted 2020-11-24, while PR #515 was opened 2021-09-10.

(I note that the linked commit was done outside of a PR and comitted directly, by me. Bad previous me. However, I did find the justification: json-schema-org/json-schema-spec#1027 (comment), so I wasn't totally rogue.)

Based on these facts, I think that PR needs to be reverted, and the tests should be included.

I found this issue when looking for these tests and not finding them.

(This Issue may be related, but I'm not 100% sure: #495)

@gregsdennis
Copy link
Member

Yeah, I agree that these tests are correct and should be added back.

The vocab entry that Karen suggests in #515 is already part of the meta-schema.

@Relequestual Relequestual added missing test A request to add a test to the suite that is currently not covered elsewhere. good first issue An issue that is a good candidate for new contributors labels Mar 30, 2023
@Julian
Copy link
Member

Julian commented Mar 30, 2023

Sounds good to me too thanks for figuring this out.

@karenetheridge
Copy link
Member

karenetheridge commented Apr 2, 2023

When we inspect the 2020-12 meta-schema, and the 2019-09 meta-schema, we actually find the following $vocabulary declarations...

No, we don't. What you describe is true for 2020-12, but not for 2019-09, because we hadn't split the format vocabulary into the annotation and assertion varieties yet.

Therefore, the problem that #515 describes is only an issue for 2019-09, and only that version's tests should have been modified.

That is:

  • format assertion-vs-annotation tests are indeterminate for 2019-09, and should be removed (and relegated solely to the optional/format directory)
  • format assertion-vs-annotation tests are valid in the main directory for 2020-12, and can be safely restored (where all formats are valid: true, because the format-annotation vocabulary is in effect).

@karenetheridge
Copy link
Member

I have prepared a PR to fix.

karenetheridge added a commit that referenced this issue Apr 2, 2023
This reverts the parts of commit 0173a08
that pertain to draft2020-12 and draft-next.

Resolves #660.
@Relequestual
Copy link
Member Author

No, we don't. What you describe is true for 2020-12, but not for 2019-09, because we hadn't split the format vocabulary into the annotation and assertion varieties yet.

You are totally correct. My apologies. I saw the date in URIs in the commits and made a bad assumption. I should have validated that assumption. The commit was after the 2019-09 release... should have been obvious.

Relequestual referenced this issue in json-schema-org/json-schema-spec Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue An issue that is a good candidate for new contributors missing test A request to add a test to the suite that is currently not covered elsewhere.
Projects
None yet
4 participants