Skip to content

optional/content.json tests as assertions #363

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
Maldris opened this issue May 9, 2020 · 7 comments
Closed

optional/content.json tests as assertions #363

Maldris opened this issue May 9, 2020 · 7 comments

Comments

@Maldris
Copy link

Maldris commented May 9, 2020

Hi, I'm just wanting to check the intent here.

The tests in the draft2019-09/optional/content.json set treat the contentMediaType and other keywords as assertions, when their definition in the current draft states in section 8.1, paragraph 2:

They do not function as validation assertions; a malformed string-encoded document MUST NOT cause the containing instance to be considered invalid.

So I wanted to check that it was intended that these tests don't follow the spec's behaviour?

My intent for handling them in my implementation/use case is to treat them like format and have their behaviour be dependent on configuration/$vocabulary value, so this wont be a problem once fully implemented, but I felt it best to point out the inconsistency as I noticed it.

@karenetheridge
Copy link
Member

karenetheridge commented May 10, 2020

Agreed, these tests violate the spec. They should be moved copied to annotation-only tests where the validation result is always "true" (note that there aren't actually any "annotation" properties in the test results yet, so this would need to be added).

edit: I didn't see that these tests were already in the optional/ section. Since they are, these tests are valid, if they are marked in some way as requiring the 'content' vocabulary to be enabled. At a minimum there should be a $schema keyword, pointing to an extension of the standard metaschema that has the 'content' $vocabulary set to true. (However, annotation-only tests, where valid = true for all tests, would be fine in the main test directory.)

@Julian
Copy link
Member

Julian commented May 10, 2020

I haven't looked closely at this yet, but just opening the spec, the next section says:

Implementations MAY offer the ability to decode, parse, and/or validate the string contents automatically.

Have you seen that, and understand it to be about some other piece of functionality?

@awwright
Copy link
Member

awwright commented May 10, 2020

It seems appropriate to me that this exists in the optional tests, since this isn't a mandatory feature, but is a completely legitimate feature nonetheless.

However, the meaning of an "optional test" might be ambiguous, since it could mean anything from "vendor-specific extensions to JSON Schema Validation" to "mandatory behavior that's actually really difficult to implement in practice because of shortcomings in JSON parsers so we don't blame you if you fail these tests"

@karenetheridge
Copy link
Member

Implementations MAY offer the ability to decode, parse, and/or validate the string contents automatically.

The spec should probably be amended to clarify that this behaviour can be enabled by setting the appropriate $vocabulary value in the schema to true. I don't think it's valid for an implementation to turn these keywords into an assertion otherwise (and indeed the default metaschema has this vocabulary set to false, which pretty clearly indicates the desired behaviour here IMO).

@karenetheridge
Copy link
Member

I edited an earlier comment. I didn't see that these tests were in the optional/ directory.

@Maldris
Copy link
Author

Maldris commented May 11, 2020

I think @awwright has better described my issue than I did. The reason for it being in the optional folder is somewhat ambiguous and should probably be annotated (perhaps with a "$comment"). As it could be for optional vocabulary (as in the case of format in 2019-09), etc. Describing the reason for it being optional may help

@karenetheridge, your comment

Implementations MAY offer the ability to decode, parse, and/or validate the string contents automatically.

The spec should probably be amended to clarify that this behaviour can be enabled by setting the appropriate $vocabulary value in the schema to true. I don't think it's valid for an implementation to turn these keywords into an assertion otherwise (and indeed the default metaschema has this vocabulary set to false, which pretty clearly indicates the desired behaviour here IMO).

is inline with how I was intending to implement my behaviour of the content vocabulary, so its encouraging to see someone else mentioning this approach.

My reporting the issue was intended to simply ensure that the current content was intended the way it is written, and not an accident or error.

Thank you for addressing this.

@Maldris Maldris closed this as completed May 11, 2020
@Julian
Copy link
Member

Julian commented May 11, 2020

The work to start documenting why things are optional is #25 and is started in #345. Definitely happy to see reviewers on that.

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