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 SchemaError for enum schema with unique elements #529

Closed
Zac-HD opened this issue Feb 23, 2019 · 7 comments
Closed

Incorrect SchemaError for enum schema with unique elements #529

Zac-HD opened this issue Feb 23, 2019 · 7 comments
Labels
Bug Something doesn't work the way it should.

Comments

@Zac-HD
Copy link
Member

Zac-HD commented Feb 23, 2019

# Draft7 for convenience; the same applies to 3, 4, and 6.
# We can have a unique list of things that compare equal, e.g. this works:
jsonschema.Draft7Validator.check_schema({'enum': [False, 0.0]})

# But wrap them in a one-element list, and we get a `SchemaError` instead!
jsonschema.Draft7Validator.check_schema({'enum': [[False], [0.0]]})

There are IMO two bugs here:

  1. JSON values are being tested for equality using Python object semantics, rather than comparing them as JSON (e.g. via a canonical encoding).

  2. The json-schema specification says

     The value of this keyword MUST be an array.
     This array SHOULD have at least one element.
     Elements in the array SHOULD be unique.
    

    Validation therefore ought to allow schema authors to use empty or non-unique arrays, because the RFC 2119 definition of SHOULD explicitly permits ignoring such clauses after considering the circumstances.

@Zac-HD
Copy link
Member Author

Zac-HD commented Feb 23, 2019

I also just want to say thanks for the jsonschema library - the interface is easy to use, and the fantastic error messages when I misused or misunderstood something have saved so much time debugging.

I stumbled over this when a build failed for my hypothesis-jsonschema library, which generates arbitrary instances matching a schema for testing. In this case it was also generating arbitrary schemas!

@Julian
Copy link
Member

Julian commented Feb 23, 2019

Woohoo, thanks for writing the hypothesis thing, been on my list for awhile but never gotten to it (oddly got to it for protobuf first).

That first one does look like a bug, though jsonschema, despite its name, doesn't deal at all with JSON, it does use Python semantics and objects, and just avoids the Python behavior when needed (because it'd clash with JSON). E.g., it evades issubclass(bool, int) for doing type checks that JSON Schema defines. Seems like uniqueItems is likely missing the same sort of evasion.

The second one also looks like a bug but not here :), because all jsonschema does is use the official meta schemas. E.g. here for enum you'll see that despite the language of the spec, it does refuse any instances with non-unique items (or an empty array). That metaschema is yeah just the official one essentially (copied from e.g. https://github.com/json-schema-org/json-schema-spec/blob/draft-07/schema.json#L139-L144). The right thing to do here then is probably for us to file a bug in the spec repo, and then once we merge it there jsonschema will just stop raising those.

I also just want to say thanks for the jsonschema library - the interface is easy to use, and the fantastic error messages when I misused or misunderstood something have saved so much time debugging.

Thank you! That's very very good to hear :) particularly about the error messages. There's still lots to do to make them even better I suspect, but yeah glad they helped.

@Julian Julian added the Bug Something doesn't work the way it should. label Feb 23, 2019
Julian added a commit that referenced this issue Feb 25, 2019
It says that these two conditions are recommendations, but can be
ignored if someone feels like it.

Thanks @Zac-HD.

Refs: #529
@Julian
Copy link
Member

Julian commented Feb 25, 2019

OK, have pushed a local fix for the metaschema bugs here... Gonna have to set aside some time to start moving tests upstream to help us move it into the spec repo too.

@Julian
Copy link
Member

Julian commented Feb 25, 2019

Hee hee I also just realized that the second bug is likely impossible to fix for arbitrary types, so will need to think about what to do about that.

@Zac-HD
Copy link
Member Author

Zac-HD commented Aug 14, 2019

I just checked this, and it's fixed as of jsonschema==3.0.2 🎉

@Zac-HD Zac-HD closed this as completed Aug 14, 2019
Julian added a commit that referenced this issue Jan 4, 2022
20fb14bde Merge pull request #537 from jimmylewis/removeInvalidTest
d07162bcf Remove invalid draft6 test using draft7 constructs
26f74befc Add org badges
6e87b5bdb Merge pull request #529 from yakimun/remove-non-canonical-uris
c5ba7b28f Replace non-canonical URIs with canonical ones
8d96d15fc Merge pull request #527 from johnnoone/patch-2
b7f657f77 Update vocabulary.json
e82bfdfa5 s/future/next/ in schema reference

git-subtree-dir: json
git-subtree-split: 20fb14bde8f3c45a8933cd13283557c1b1fdf5b5
@Julian
Copy link
Member

Julian commented Nov 1, 2022

I mentioned this briefly on json-schema-org/json-schema-spec#717 (comment) but just for archaeological purposes, this change was just partially "backed out" for drafts 3 and 4 where indeed the original behavior (MUST) was correct, those drafts contain stronger language. It's definitely correct on newer ones though (so thanks again for filing it!)

@erohmensing
Copy link

@Julian thanks for the "for archeological purposes", rest assured it's worth adding as people still run into this strange mismatch years later 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something doesn't work the way it should.
Projects
None yet
Development

No branches or pull requests

3 participants