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

Add line numbers to xml validation errors #630

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

csc-jm
Copy link
Collaborator

@csc-jm csc-jm commented Oct 27, 2022

Description

Before this PR, validation error reasons when validating an XML file would be something:
Unexpected child with tag 'BAD_ELEMENT' at position 2.
This PR makes those more understandable so they'd instead come out as:
Unexpected child with tag 'BAD_ELEMENT' at line 34.

Also the changes made here enable the validator to find all the errors from the file at once.

Related issues

Fixes #91

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update (this needs a follow up PR)

Changes Made

  • changed the xml validator to add line numbers to error messages
  • updated existing unit tests

Testing

  • Unit Tests

Mentions

@csc-jm csc-jm force-pushed the feature/add-line-numbers-to-validation-errors branch from c31713d to 5cab919 Compare October 27, 2022 15:42
@csc-jm
Copy link
Collaborator Author

csc-jm commented Oct 27, 2022

When researching possibilities for this, I thought I had found a simple solution with xmlschema.assertValid() as shown here where you could get something like this:

xmlschema.assertValid(doc2)
Traceback (most recent call last):
  ...
lxml.etree.DocumentInvalid: Element 'c': This element is not expected. Expected is ( b )., line 1

However, after implementing this vulture complained that using lxml.etree is not safe anymore. It would have also required schemas to be loaded in a different way, which would have resulted in a bigger refactor. So the solution in this PR is best I could come up with without refactoring things too much.

@csc-jm csc-jm marked this pull request as ready for review October 27, 2022 16:08
@csc-jm csc-jm self-assigned this Oct 27, 2022
@csc-jm csc-jm added the enhancement New feature or request label Oct 27, 2022
@csc-jm csc-jm changed the title Manually add line numbers to xml validation errors Add line numbers to xml validation errors Oct 27, 2022
@csc-jm csc-jm force-pushed the feature/add-line-numbers-to-validation-errors branch from 5cab919 to 6c6108e Compare October 28, 2022 08:44
docs/specification.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@csc-felipe csc-felipe left a comment

Choose a reason for hiding this comment

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

Good work putting the line numbers there ^^ Much appreciated.

The API error responses are quite well standardized (since #433), although the validation errors were not following it. The detail field is always a str, following the JSON Problem Details specification https://www.rfc-editor.org/rfc/rfc7807. It would be nice to stick with it.

Would it make sense to change it in this PR, as you are already proposing a change to the validation error message?

This makes the error messages slightly more descriptive.
Also this enables the validator to parse all the errors
from the file at once.
@csc-jm csc-jm force-pushed the feature/add-line-numbers-to-validation-errors branch from 6c6108e to 2ce8086 Compare November 1, 2022 17:53
@csc-jm
Copy link
Collaborator Author

csc-jm commented Nov 1, 2022

Ok, I altered the new functionality slightly so it collects the error messages and concatenates them in the same string of the reason key separated by an endline. It's not ideal but things will hopefully be fixed as the whole response will need some reformatting from its prior state. I opened the issue #641 for a follow up.

Copy link
Contributor

@csc-felipe csc-felipe left a comment

Choose a reason for hiding this comment

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

lgtm

@blankdots blankdots linked an issue Nov 2, 2022 that may be closed by this pull request
@csc-felipe csc-felipe merged commit 97ab973 into develop Nov 2, 2022
@csc-felipe csc-felipe deleted the feature/add-line-numbers-to-validation-errors branch November 2, 2022 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add line numbers to xml validation error messages
3 participants