Skip to content

Fixes unique and equal checks #820

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.

Sign up for GitHub

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
wants to merge 4 commits into from
Closed

Fixes unique and equal checks #820

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5ce1de7
Julian/jsonschema#686: Fixes unique and equal checks
nezhar Jun 14, 2021
3e5781d
Add test for list_equal and dict_equal
nezhar Jun 28, 2021
4ef44a7
Add test case with custom sequance type
nezhar Jun 29, 2021
27661b2
Extend sequance and mapping check
nezhar Jul 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 56 additions & 18 deletions jsonschema/_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,46 @@ def ensure_list(thing):
return thing


def dict_equal(one, two):
"""
Check if two dicts are the same using `equal`
"""
if len(one.keys()) != len(two.keys()):
return False

for key in one:
if key not in two:
return False
if not equal(one[key], two[key]):
return False

return True


def list_equal(one, two):
"""
Check if two lists are the same using `equal`
"""
if len(one) != len(two):
return False

for i in range(0, len(one)):
if not equal(one[i], two[i]):
return False

return True


def equal(one, two):
"""
Check if two things are equal, but evade booleans and ints being equal.
"""
if isinstance(one, list) and isinstance(two, list):
return list_equal(one, two)

if isinstance(one, dict) and isinstance(two, dict):
return dict_equal(one, two)

return unbool(one) == unbool(two)


Expand All @@ -181,25 +217,27 @@ def uniq(container):
"""
Check if all of a container's elements are unique.

Successively tries first to rely that the elements are hashable, then
falls back on them being sortable, and finally falls back on brute
force.
Successively tries first to rely that the elements are being sortable
and finally falls back on brute force.
"""

try:
return len(set(unbool(i) for i in container)) == len(container)
except TypeError:
try:
sort = sorted(unbool(i) for i in container)
sliced = itertools.islice(sort, 1, None)
for i, j in zip(sort, sliced):
if i == j:
sort = sorted(unbool(i) for i in container)
sliced = itertools.islice(sort, 1, None)

for i, j in zip(sort, sliced):
return not list_equal(list(i), list(j))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will now likely fail for triply-nested non-list containers.

It also will now do a lot of copying (every object will always be copied).

It's a bit better if we instead just use isinstance(collections.Sequence or collections.Mapping I suspect.

But yeah please check the additional nesting, I suspect we need even better tests that uncover that. Really the nice thing would be a hypothesis test that generates arbitrarily nested containers which it puts 0s and Falses inside and ensures they're always considered unique. If you know how to do that that'd be great, otherwise yeah I suspect at least another manual test is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right, a tripple nesting caused new failures, this was related to the equal function where the behavior was not extended. I refactored the tests and added some more cases as I'm not really sure how to create a data generator here and I'm afraid this makes debugging harder.


except (NotImplementedError, TypeError):
seen = []
for e in container:
e = unbool(e)

for i in seen:
if isinstance(i, dict) and isinstance(e, dict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line too I'm suspicious of if it still contains isinstance(, dict) -- it'll fail for non-dict mappings then, so we need to exercise that case as well.

Let me know if that makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been removed and it makes use of the equal function instead.

if dict_equal(i, e):
return False
elif i == e:
return False
except (NotImplementedError, TypeError):
seen = []
for e in container:
e = unbool(e)
if e in seen:
return False
seen.append(e)

seen.append(e)
return True
160 changes: 0 additions & 160 deletions jsonschema/tests/test_jsonschema_test_suite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,36 +145,6 @@ def leap_second(test):
"$ref prevents a sibling $id from changing the base uri"
),
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="[0] and [false] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="[1] and [true] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="nested [0] and [false] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="nested [1] and [true] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description='{"a": false} and {"a": 0} are unique',
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description='{"a": true} and {"a": 1} are unique',
)(test)
),
)

Expand Down Expand Up @@ -240,36 +210,6 @@ def leap_second(test):
subject="refRemote",
case_description="base URI change - change folder in subschema",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="[0] and [false] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="[1] and [true] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="nested [0] and [false] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="nested [1] and [true] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description='{"a": false} and {"a": 0} are unique',
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description='{"a": true} and {"a": 1} are unique',
)(test)
),
)

Expand Down Expand Up @@ -351,56 +291,6 @@ def leap_second(test):
subject="refRemote",
case_description="base URI change - change folder in subschema",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="[0] and [false] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="[1] and [true] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="nested [0] and [false] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="nested [1] and [true] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description='{"a": false} and {"a": 0} are unique',
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description='{"a": true} and {"a": 1} are unique',
)(test)
or skip(
message=bug(686),
subject="const",
case_description="const with [false] does not match [0]",
)(test)
or skip(
message=bug(686),
subject="const",
case_description="const with [true] does not match [1]",
)(test)
or skip(
message=bug(686),
subject="const",
case_description='const with {"a": false} does not match {"a": 0}',
)(test)
or skip(
message=bug(686),
subject="const",
case_description='const with {"a": true} does not match {"a": 1}',
)(test)
),
)

Expand Down Expand Up @@ -506,55 +396,5 @@ def leap_second(test):
"validation of binary-encoded media type documents"
),
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="[0] and [false] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="[1] and [true] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="nested [0] and [false] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description="nested [1] and [true] are unique",
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description='{"a": false} and {"a": 0} are unique',
)(test)
or skip(
message=bug(686),
subject="uniqueItems",
description='{"a": true} and {"a": 1} are unique',
)(test)
or skip(
message=bug(686),
subject="const",
case_description="const with [false] does not match [0]",
)(test)
or skip(
message=bug(686),
subject="const",
case_description="const with [true] does not match [1]",
)(test)
or skip(
message=bug(686),
subject="const",
case_description='const with {"a": false} does not match {"a": 0}',
)(test)
or skip(
message=bug(686),
subject="const",
case_description='const with {"a": true} does not match {"a": 1}',
)(test)
),
)
Loading