Skip to content

[question] format.json #434

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

Closed
epoberezkin opened this issue Sep 11, 2020 · 8 comments
Closed

[question] format.json #434

epoberezkin opened this issue Sep 11, 2020 · 8 comments

Comments

@epoberezkin
Copy link
Member

There is both format.json in the main folder and in format folder in optional - what is the reason?

@Relequestual
Copy link
Member

Everything in format.json in the main folder is expected to pass.
I'm not actually sure this belongs in the main folder, as any knowledge of the format keyword only comes from the optionally supported vocabulary.
Feels like a reasonable argument in my mind for that set of tests to be moved to the optional folder.

@Julian
Copy link
Member

Julian commented Sep 11, 2020

That's maybe true now in 2019-09 but was not in previous drafts -- the point of that file was indeed that it is not ok to error and/or fail previously, all the things in there must pass in all modes (even when format isn't being validated).

Whereas the optional tests pass only if you're validating format.

In 2019-09 maybe that's not the case anymore I'm sure you both may know, but I thought I recall the reverse was true? I.e. that if you didn't enable format vocabulary, even the invalid formats MUST now pass by default, which is an argument to me to move all the existing tests to the required file and turn them all valid.

@epoberezkin
Copy link
Member Author

Actually, it might be helpful to have the tests for anything that is supposed to pass because something is ignored grouped in a separate folder.

The option to throw exception on anything ignored was asked a lot, and it will be flipped in the next Ajv version (throw by default when schema is processed, ignore during validation with option). So if these tests were grouped in say "ignored" folder alongside "optional" I could test that they all pass with option and throw exception without (currently I have to run all tests with this option).

It doesn't only include formats - also ref siblings, unknown keywords, additionalItems without items, then without if, etc. Currently I just run them all with option, but having tests that pass because something is ignored grouped together would help differentiating it in the implementations.

@Julian
Copy link
Member

Julian commented Sep 11, 2020

I agree it's a common request and that implementations like ajv may want to offer it -- but it's technically against the spec, right? (Unless you're writing a different meta schema to implement it).

But the suite basically is testing the behavior mandated by the spec -- the spec says what's in format.json must pass in all modes -- if you're annotation-only, it passes regardless, and if you're in assertion mode, because all that's there is actually valid.

It may be convenient to somehow allow implementations to implement modes like what you're describing and have tests in the suite to ensure they're working, but it'd likely have to work the opposite way to be written in a spec-compliant way -- where we have a second file with tests that'd be flagged as erroring (e.g. via the mechanism described in #361) for implementations that want to implement behavior like what you're describing (or decide to do so by default).

@Julian
Copy link
Member

Julian commented Sep 11, 2020

I'm not actually sure this belongs in the main folder, as any knowledge of the format keyword only comes from the optionally supported vocabulary.

@Relequestual correct me if I'm wrong, but I don't think this is true. format as an annotation (in draft2019-09, certainly even in previous drafts) is used even in the core metaschema, and the spec talks about it. So regardless, an implementation must pass silently anything in that file -- otherwise the core metaschema itself is invalid.

E.g.: https://json-schema.org/draft/2019-09/json-schema-core.html#rfc.section.8

(using "pattern" to ensure that it is validated even when "format" functions purely as an annotation, as explained in the Validation specification.

@karenetheridge
Copy link
Member

karenetheridge commented Sep 16, 2020

I'm not actually sure this belongs in the main folder, as any knowledge of the format keyword only comes from the optionally supported vocabulary.

They absolutely do belong there, because automatically validating the 'format' keyword is a common thing for implementations to do, so it's a good thing to test for. (An implementation can of course have a treat-formats-as-validations configuration option, but when running the spec tests, that option must be turned off in order to be spec-compliant.)

We should add a '$comment' to these tests though, explaining why they aren't in the optional/ folder -- that is, there are some schemas in that test file that would fail if validate-formats was turned on (and referencing the optional/format/* tests where the same tests exist with "valid":false), and therefore these tests are confirming that the implementation has a mode to turn that off.

@karenetheridge
Copy link
Member

I was looking at these tests again today...

We could move all the optional/ tests (that is, the tests that actually test format validation and have both true and false values in them) into the main section if we added an explicit $schema keyword to all of them, which referenced a new metaspec in remotes/ that explicitly declared the format vocabulary as true. We have no tests of metaschemas (other than the main specification metaspec itself) nor $vocabulary anywhere, and this is an easy way of testing the functionality of those keywords. I might PR that up soon...

@Julian
Copy link
Member

Julian commented Jun 24, 2021

I think this was answered above (so we can close this) but obviously if not lemme know.

@Julian Julian closed this as completed Jun 24, 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

No branches or pull requests

4 participants