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.

Sign up for GitHub

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

Issue 147 #148

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from
Draft

Issue 147 #148

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f092290
add custom error to schema validation
anakhalil Dec 20, 2022
04dacaa
make message generic
anakhalil Dec 20, 2022
886971d
replace placeholder with instance data
anakhalil Dec 20, 2022
0f55cfd
Simplify message tag
anakhalil Dec 21, 2022
42e2d6e
Adding tests for custom error handling
aggle-baggle Feb 10, 2023
b113e89
Update README.md
aggle-baggle Feb 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion schema_enforcer/schemas/jsonschema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,14 @@ def validate(self, data, strict=False):
for err in validator.iter_errors(data):

has_error = True
self.add_validation_error(err.message, absolute_path=list(err.absolute_path))

if 'errMessage' in err.schema:
message = err.schema['errMessage']
message = message.replace("$iData", str(err.instance))
Copy link

@aggle-baggle aggle-baggle Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nice work! You could even make the iData variable dynamic and couple it to the user defined data structure so the user can then use the property key prepended with $:

Suggested change
message = err.schema['errMessage']
message = message.replace("$iData", str(err.instance))
keyName = '$' + err.absolute_path[-1]
message = err.schema['errMessage']
if keyName in message:
message = message.replace(keyName, str(err.instance))

Then then schema would look like:

fqdn:
  type: string
  pattern: regex
  errMessage: $fqdn is not valid, please see docs...
ip_address:
  type: string
  format: ipv4
  errMessage: $ip_address is not valid, please see docs...
etc...

Copy link

Choose a reason for hiding this comment

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

I think you could even do it on one line:

Suggested change
message = err.schema['errMessage']
message = message.replace("$iData", str(err.instance))
message = err.schema['errMessage']
message = message.replace('$' + err.absolute_path[-1], str(err.instance))

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion!
I am only worried about how you index the path. Since:

The deque can be empty if the error happened at the root of the instance.

Copy link

@aggle-baggle aggle-baggle Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Oh good catch, I guess the potential IndexError could be caught with a try/except or if len(err.absolute_path) > 0 or similar...

Copy link

@aggle-baggle aggle-baggle Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Been doing a bit of testing with the following test schema:

type: "object"
errMessage: Root error
additionalProperties: false
properties:
  data:
    type: "object"
    properties:
      fqdn:
        type: string
        pattern: regex
        errMessage: "'$fqdn' is not valid. Please see https://confluence.test.com/schema"
      ip_address:
        type: string
        format: ipv4
        errMessage: "'$ip_address' is not valid. Please see https://confluence.test.com/schema"

When validating the following data the IndexError: deque index out of range error is observed:

data:
  fqdn: host
  ip_address: 1.2.3.400
data2:
  fqdn: host
  ip_address: 1.2.3.500

The error observed:

File "/usr/lib/python3.9/site-packages/schema_enforcer/schemas/jsonschema.py", line 67, in validate
    message = message.replace('$' + err.absolute_path[-1], str(err.instance))
IndexError: deque index out of range

One solution that seems to work is to ignore the errMessage key at the root level i.e. if err.absolute_path evaluates to an empty deque:

            message = err.message

            if len(err.absolute_path) > 0 and 'errMessage' in err.schema:
                message = str(err.schema['errMessage']).replace('$' + err.absolute_path[-1], str(err.instance))

            self.add_validation_error(message, absolute_path=list(err.absolute_path))

Then you get the built-in error messages back from JSON Schema at the root level, and custom error message processing for anything nested:

# schema-enforcer validate
FAIL | [ERROR] Additional properties are not allowed ('data2' was unexpected) [FILE] .//test.yml [PROPERTY]
FAIL | [ERROR] 'host' is not valid. Please see https://confluence.test.com/schema [FILE] .//test.yml [PROPERTY] data:fqdn
FAIL | [ERROR] '1.2.3.400' is not valid. Please see https://confluence.test.com/schema [FILE] .//test.yml [PROPERTY] data:ip_address

Copy link

@aggle-baggle aggle-baggle Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hey Phill, no problem!

So I've tried quite a few schemas in my local testing and using $instance does indeed seem to work fine in every scenario I've looked at. One thing to bear in mind is that it will only return the parent error (this is the default behaviour of Schema Enforcer anyway). When the parent error is the result of sub-errors when using schema composition keywords like anyOf, the ValidationError.context attribute can be used to return the sub-errors. But returning these sub-errors in SE is crossing over into a whole different feature request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Acknowledged, thanks for all the testing.

It would be awesome to get those tests into unit tests to validate future changes don't break things, and that different custom message cases work as we expect them to.

Copy link

Choose a reason for hiding this comment

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

Hi Phill,

I would like to open a new PR for this feature as @anakhalil has moved on to a new adventure and likely won't have the time to flesh out the unit/integration tests required.

Hopefully this will be fairly soon!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thank you! :)

Copy link

Choose a reason for hiding this comment

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

I've pushed some commits to this PR that introduce some unit tests for custom error message handling and also a README snippet. There were a few hurdles with the unit tests, for some reason I could not get it to work with the schema broken up into separate files, so had to adjust it so it was all in one file. Even then it would not allow me to use references to definitions from within the same file.

However the tests do pass. I tried to make the tests in line with the existing ones and not interfere with them so there is some repeated code. Hopefully it's a good starting point anyway!

else:
message = err.message

self.add_validation_error(message, absolute_path=list(err.absolute_path))

if not has_error:
self.add_validation_pass()
Expand Down