-
-
Notifications
You must be signed in to change notification settings - Fork 584
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
uniqueItems
check only validates the first two entries for types with a __len__
implementation
#866
Comments
Thanks for the detailed report. Any chance you're interested in a PR? The first step would be adding some tests to the official test suite here: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/tests/draft2020-12/uniqueItems.json (along with the 2019 draft). After that your suggested change looks like what was intended as well. |
@Julian, I'm interested in submitting a PR in the official test suite at https://github.com/json-schema-org/JSON-Schema-Test-Suite. I got changes to the tests that can trigger the current issue. My understanding is that I should make the changes in the tests of all drafts that support the |
Great! Yes please, I believe IIRC all drafts have uniqueItems |
@Julian, I see that the changes from the test suite are already merged here. If it's OK with you, I'll work on a PR to fix the current issue. |
Yep, sounds great thanks again! |
This reverts commit ed4693c.
After updating to a newer version of
jsonschema
, we now find that it does not notice violations of theuniqueItems
constraint on lists.By stepping through the code, I think I've found the issue in this code (originally introduced in this commit in this PR, but now found here):
Notice that the for loop will always return on the first iteration, without examining subsequent values.
This code looks like it's supposed to be seeing if any two successive elements is the same, but with the return statement it would only detect unique constraint violations between the first two elements.
I think this bug has escaped notice so far for two reasons:
__len__
implementation. If aTypeError
orNotImplementedError
is raised then the "slow, brute force" path runs - and it does not have the same bug.I wonder if this code is instead supposed to be:
For what it's worth, here's some noddy test code I put together to explore the issue:
And here is the output I got:
The text was updated successfully, but these errors were encountered: