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

Maintain json schema error properties when using custom validation #1608

Closed
wants to merge 4 commits into from
Closed

Maintain json schema error properties when using custom validation #1608

wants to merge 4 commits into from

Conversation

RoboPhred
Copy link

@RoboPhred RoboPhred commented Feb 26, 2020

This is a fix for 1.x. After it lands, I will cherry-pick to 2.x and adjust as nescessary.

Reasons for making this change

As described in #1596

Enabling custom validation strips json schema errors of useful properties such as "property", "params", and "schemaPath". All properties save "stack" are removed, preventing the error consumer from knowing where the error occurred or what rule it validated.

Additionally, custom errors do not provide a "property" prop, so the error consumer likewise does not know what property is encountering the custom error, just that the error happened 'somewhere'.

This PR fixes both these issues:

JSON Schema errors now keep their properties in the presence of custom validation. This is done by not regenerating the entire error list, but instead keeping the old list and appending the custom errors to this list.

For custom errors, this PR modifies toErrorList to track the path of the error and generate a property prop containing the property path to the property that encountered the error.

I am not adding documentation, as the actual format of the onError errors array seems to be undocumented. I can attempt to fill in some of the gaps here if you want to adopt the current ajv error objects as official api.

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@epicfaace
Copy link
Member

@RoboPhred thanks for the PR. Could you actually make this PR to master instead? Would like to keep updates to v1.x to be minimal changes such as security vulnerability fixes if possible.

Additionally, can you add a test that ensures that the error properties are maintained?

@bmmpt
Copy link

bmmpt commented Nov 27, 2020

@epicfaace Any reason why this has gone stale? Is it the lack of unit tests?

@bmmpt
Copy link

bmmpt commented Dec 9, 2020

@epicfaace thanks for the thumbs up and forgive my persistence. I don't mean to be pushy, but I didn't really get an answer to any of my questions.

@epicfaace
Copy link
Member

Additionally, can you add a test that ensures that the error properties are maintained?

Yeah, it's the lack of unit tests, and also the fact that this PR isn't to the master branch.

@bmmpt
Copy link

bmmpt commented Dec 9, 2020

@epicfaace Thanks for the quick response. This is something that I will need pretty soon and was even considering forking and rolling with forked code until this is completed. Instead of that, as soon as I have a few minutes I will add the missing tests if no one beats me to it.

@bmmpt
Copy link

bmmpt commented Feb 18, 2021

@epicfaace I have been too busy to be able to get to this, but I asked a colleague of mine to look into it and he created a PR into master: #2242

@epicfaace
Copy link
Member

Additionally, can you add a test that ensures that the error properties are maintained?

Yeah, it's the lack of unit tests, and also the fact that this PR isn't to the master branch.

@bmmpt , sorry, I forgot about this PR at the time of the comment (#2002). I'm going to close this PR for now, @RoboPhred , so we can focus on the PR #2002 to master.

@epicfaace epicfaace closed this Mar 10, 2021
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