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

export and validate: Substantially improve validation errors #1134

Merged
merged 6 commits into from
Jan 27, 2023

Conversation

tsibley
Copy link
Member

@tsibley tsibley commented Jan 24, 2023

All errors are reported in the case of subschemas inside a "oneOf" validator, such as used by our "tree" property. Previously only the top-level error was reported and it was worse than useless as it was incredibly long and contained no actionable .

Suberrors are grouped by the validator arm they failed, making it clear why each element of a "oneOf" failed.

Errors avoid being overly long by consistently shortening output that might be long. Different shortening strategies are used for different values in an attempt to preserve the most useful information.

Paths in error messages now index into the given JSON being validated, not the JSON Schema. This disconnect was confusing as the schema path refers to properties that don't exist in the JSON being validated.

Resolves #1044.

Testing

  • Manually tested various error message situations
  • Added doctests
  • CI passes

Checklist

  • Add a message in CHANGES.md summarizing the changes in this PR that are end user focused. Keep headers and formatting consistent with the rest of the file.

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Base: 68.08% // Head: 68.14% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (59b5505) compared to base (85d2edd).
Patch coverage: 80.00% of modified lines in pull request are covered.

❗ Current head 59b5505 differs from pull request most recent head a8ea3dd. Consider uploading reports for the commit a8ea3dd to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1134      +/-   ##
==========================================
+ Coverage   68.08%   68.14%   +0.06%     
==========================================
  Files          62       62              
  Lines        6690     6725      +35     
  Branches     1641     1646       +5     
==========================================
+ Hits         4555     4583      +28     
- Misses       1829     1836       +7     
  Partials      306      306              
Impacted Files Coverage Δ
augur/validate.py 72.67% <75.00%> (+0.24%) ⬆️
augur/__version__.py 100.00% <100.00%> (ø)
augur/io/json.py 86.76% <100.00%> (+1.76%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rneher
Copy link
Member

rneher commented Jan 24, 2023

thanks, @trs! this is much more helpful:

      .tree.children[…].node_attrs.accession "England/Qatar/2012" failed pattern validation for "^[0-9A-Za-z-_.]+$"
      .tree.children[…].node_attrs.accession "D998/15" failed pattern validation for "^[0-9A-Za-z-_.]+$"
      .tree.children[…].node_attrs.accession "Jordan-N3/2012" failed pattern validation for "^[0-9A-Za-z-_.]+$"
    validation for arm 1: {"type": "array", "minItems": 1, "items": {"$ref": "#/$defs/tree"}}
      .tree {"name": "NODE_0000000", "node_attrs": {"div": 0…} failed type validation for "array"
Validation of 'auspice/mers2023.json' failed.

augur/validate.py Show resolved Hide resolved
@emmahodcroft
Copy link
Member

Thanks so much for taking this up Tom! I can remember a time when our validation errors were helpful (this is a long time ago), so it's great to get them back to something that one can actually interpret! 🙌

Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing up all the things I missed when I copied over the json module 🙏

@tsibley tsibley force-pushed the trs/better-validation-errors branch from 59b5505 to 750db2b Compare January 27, 2023 23:47
tsibley added a commit that referenced this pull request Jan 27, 2023
  • Names the specific utility functions
  • Uses the full commit SHA instead of abbreviating
  • Uses an absolute repo URL instead of a relative one
  • Notes the licensing of subsequent modifications
  • Uses rST syntax to demarcate the included ID3C license
It refers to shorten(), which we didn't copy over from ID3C.  This
docstring is based on the shorten() docstring from ID3C (at the same
commit as the rest of the file when we copied it into Augur).
Doctests must be in their own block.  rST was interpreting these as
badly-formed blockquotes.
All errors are reported in the case of subschemas inside a "oneOf"
validator, such as used by our "tree" property.  Previously only the
top-level error was reported and it was worse than useless as it was
incredibly long and contained no actionable information.

Suberrors are grouped by the validator arm they failed, making it
clear why each element of a "oneOf" failed.

Errors avoid being overly long by consistently shortening output that
might be long.  Different shortening strategies are used for different
values in an attempt to preserve the most useful information.

Paths in error messages now index into the given JSON being validated,
not the JSON Schema.  This disconnect was confusing as the schema path
refers to properties that don't exist in the JSON being validated.

Resolves <#1044>.
@tsibley tsibley force-pushed the trs/better-validation-errors branch from a8ea3dd to 0f4b655 Compare January 27, 2023 23:53
@tsibley tsibley merged commit 5400626 into master Jan 27, 2023
@tsibley tsibley deleted the trs/better-validation-errors branch January 27, 2023 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
5 participants