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

Fix bug with [False] and [0] equal #688

Closed
wants to merge 2 commits into from
Closed

Fix bug with [False] and [0] equal #688

wants to merge 2 commits into from

Conversation

nomnoms12
Copy link

#686
Hey. I tried to fix the error. Hope this helps.

An additional isinstance check is needed because Python considers [False] and [0] equal.
I also had to remove the sort to avoid comparison errors.

@Julian
Copy link
Member

Julian commented May 21, 2020

Hi! Thanks! Very happy to see you take a shot at this.

We need to add a test for this upstream to the test suite (here: https://github.com/json-schema-org/JSON-Schema-Test-Suite) so the first step here is to add one to that. You can test out whether that test exercises the bug by adding it directly to the json directory here in this repository -- that directory is just a literal copy of that test suite git repository.

Also, as for the fix itself, I suspect what we have in your diff will fix the specific instance @Zac-HD found (thanks to him as usual :), but intuitively seems unlikely to be correct I think -- it will likely fail again once a single additional layer of nesting is introduced. In order to fix it once and for all, the code in this area essentially tries to do lots of lazy unwrapping -- but give it a shot with both adding a test and then adding one that's doubly nested, I think you may see the trick here.

@Zac-HD
Copy link
Member

Zac-HD commented May 22, 2020

@Julian - you might want to steal my encode_canonical_json() function; it serialises JSON objects to strings which are equal if-and-only-if JSONschema considers the objects equal. Not suitable for runtime use where performance is an issue, but it's great for testing!

@nomnoms12
Copy link
Author

nomnoms12 commented May 22, 2020

@Julian Hello again :)

I tried to fix this error by implementing breadth-first search algorithm to apply the unbool function to every element in the container. In code, this looks pretty complicated because I had to process dictionaries and lists at the same time, but the implementation is non-recursive and I think it should be better than dumping to a string.

I'd love to hear what you think about it.

@Julian
Copy link
Member

Julian commented May 22, 2020

Thanks! Will have a look -- one immediate thing that pops out though is that jsonschema supports arbitrary container types (via TypeChecker) so we can't type check on list and dict concretely -- we'd in theory have to use the collections.abc types.

If the existing implementation doesn't fail on some test (which it seems it doesn't), we probably should add a test (now just in this library, not upstream) to make sure we don't forget that we can have any type there, not just lists and dicts.

But will look more closely at the strategy you used, and definitely thanks again for trying to tackle this.

@nomnoms12 nomnoms12 closed this May 27, 2020
Julian added a commit that referenced this pull request Oct 11, 2023
ba52c48a Merge pull request #689 from skryukov/add-schema-keyword-to-required-tests
d0c602a7 Add $schema keyword to required tests
20f1f52c Merge pull request #688 from spacether/feat_updates_python_exp_impl
b087b3ca Updates implmentation

git-subtree-dir: json
git-subtree-split: ba52c48ac0725d1d5323d8083a5149e1d20e8a40
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

Successfully merging this pull request may close these issues.

3 participants