Skip to content

Ensure proper sorting of list in error message #940

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

Merged
merged 1 commit into from
May 5, 2022

Conversation

ssbarnea
Copy link
Contributor

@ssbarnea ssbarnea commented May 5, 2022

Fixes problem where error message was not consistent between runs, even when the inputs were the same (file to be validated and the schema to use).

This should make the outcomes predictable and eliminate the random factor from it. In fact the sorting already happens in 3 other places inside the library but that specific location was missed.

This was found while working on ansible/ansible-lint#2035 and where we observed that the output did very randomly between two runs:

"Additional properties are not allowed ('when', 'pki_certificates', 'pki_realms', 'pki_authorities', 'pki_private_groups_present', 'pki_routes' were unexpected)"
"Additional properties are not allowed ('pki_private_groups_present', 'when', 'pki_authorities', 'pki_certificates', 'pki_realms', 'pki_routes' were unexpected)"

Not listing the allowed properties alphabetically is also not human-friendly as it make quite hard to spot specific entries, especially as the size of these list can grow considerably.

@robherring
Copy link
Contributor

+1, but a similar change has been rejected before. See PR #774

@ssbarnea
Copy link
Contributor Author

ssbarnea commented May 5, 2022

+1, but a similar change has been rejected before. See PR #774

Ouch... I hope it was due to some misunderstanding because the ability to provide consistent error message instead of random ones should not even need to be explained.

@Julian
Copy link
Member

Julian commented May 5, 2022

Sorin - please be a bit more humble about your requests, your last message isn't worded nicely. Read the linked issue. My first inclination is indeed to wontfix this again for the same reason, but I'm happy to consider a change, or even reconsider the closed issue as long as it has justification and addresses what's in #774, not simply "it's obvious".

@cidrblock
Copy link

If we can do something here I think it would be a real win. I won't pretend to know the solution, but our use of json schemas continues to increase and we love the library.

I can think of quite a few places where the errors are being presented right back to the user, so if consistent ordering is possible IMHO, it would be great.

@Julian
Copy link
Member

Julian commented May 5, 2022

In fact the sorting already happens in 3 other places inside the library but that specific location was missed.

Is certainly a good reason. Probably indeed plenty to merge, thanks. It's just a bit unfortunate as a maintainer, because:

where the errors are being presented right back to the user,

which it seems is the key driver, is on slightly tenuous grounds:

The following are not considered public API and may change without notice: [...] the exact wording and contents of error messages; typical reasons to rely on this seem to involve downstream tests in packages using jsonschema. These use cases are encouraged to use the extensive introspection provided in jsonschema.exceptions.ValidationErrors instead to make meaningful assertions about what failed rather than relying on how what failed is explained to a human.

where if you're going to present to a human the message fine, but the rest of the fields are what equality should be based on, and if that's blocked on #119, then there's an open PR (#856) that seems to just need CI to pass to be merged.

But OK, fine. Merging. Thanks for the PR and comments.

@Julian
Copy link
Member

Julian commented May 5, 2022

The 3.11 failure in CI is spurious (and I probably should just disable testing on 3.11), but the other one is not -- can you address that one?

Fixes problem where error message was con consistent between runs.
@ssbarnea ssbarnea force-pushed the fix/error-order branch from 3439f09 to eef7417 Compare May 5, 2022 16:37
@ssbarnea
Copy link
Contributor Author

ssbarnea commented May 5, 2022

@Julian thanks! I also updated the tests. You might want to configure github to require GHA approval only for new-github users, that proved to be more enough for us to prevent CI abuse. That involves editing https://github.com/python-jsonschema/jsonschema/settings/actions and selecting "Require approval for first-time contributors who are new to GitHub".

@codecov-commenter
Copy link

Codecov Report

Merging #940 (eef7417) into main (7949bf6) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #940   +/-   ##
=======================================
  Coverage   97.02%   97.02%           
=======================================
  Files          20       20           
  Lines        3295     3295           
  Branches      458      458           
=======================================
  Hits         3197     3197           
  Misses         77       77           
  Partials       21       21           
Impacted Files Coverage Δ
jsonschema/tests/test_validators.py 98.09% <ø> (ø)
jsonschema/_utils.py 97.61% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7949bf6...eef7417. Read the comment docs.

@Julian
Copy link
Member

Julian commented May 5, 2022

Thanks (I made that GH setting change in the background just as you said it hah.) Will merge in a few. Appreciated.

@Julian Julian merged commit 4374b1b into python-jsonschema:main May 5, 2022
@ssbarnea ssbarnea deleted the fix/error-order branch May 5, 2022 21:17
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.

5 participants