Issue 147

[anakhalil]

#147
What do you think?

[anakhalil]

I am working on try to pass an optional argument that will be evaluated. i.e.
errMessage: "'$fqdn' is invalid."
which would print as:
-host.test.com is invalid

[anakhalil]

The message can look like:
''$value' is incorrect
$value will be replaced by the instance data. I need to make a change so that it will use path and resolve $value rather then just substituting err.instance.

One issue is that top level properties will return all instance data, if $value is used within the custom error message.

[anakhalil]

Going to leave it here until I get some feedback. Its quite simple now. If the custom message (value of errMessage) contains $iData then it will replace $iData with instance data.

Of course there are some limitations to this:

• it will replace all occurrences of $iData (can limit this with count)
• you can no longer use $iData in the message
• if the instance data is top level then it will print all instance data

Use cases:

hosts:
  type: string
  pattern: -regex-
  errMessage: "'$iData' does not match the regex pattern. Please see the docs"

Will print:
FAIL ...... 'bad*host*' does not match the regex pattern. Please see the docs ...

or

hosts:
  type: string
  pattern: -regex-
  errMessage: "Config has bad host. Please see the docs"

Will print:
FAIL ...... Config has bad host. Please see the docs ...

[aggle-baggle]

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...

[aggle-baggle]

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))

[anakhalil]

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.

[aggle-baggle]

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

[aggle-baggle]

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

[aggle-baggle]

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.

[PhillSimonds]

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.

[aggle-baggle]

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!

[PhillSimonds]

Sounds good, thank you! :)

[aggle-baggle]

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!

[PhillSimonds]

Thanks for putting this PR in! Sorry again for my delay in getting back. It's looking good. Beyond comments in the PR body I have a couple of thoughts:

1. Can we add tests to the tests/ folder for this functionality?
2. Can we make sure to use snake case for variable names in both the YAML data and the python code (e.g. errMessage should be in the form err_message?
3. Can we add a snippit to the documentation?

[PhillSimonds]

Happy New Year! Sorry for the delay in getting back with you. Thank you so much for opening this!

• I like the idea of using $variable for specific replacement rather than $iData
• I'm having trouble thinking about a case where an error at the root would be more useful than that built-in jsonschema error. I like the idea of evaluating length than letting the built in error bubble up, and just documenting that custom errors at root aren't supported. We could also add a warning message on schema load to ensure the user is aware that a custom error message at root will be skipped.
• We could just print the root error message without replacement and/or add a $root keyword. On initial pass, I don't like this idea for two reasons: 1. The behavior is applied differentially depending on where the error message exists. We replace sometimes but not other times. 2. The $root is a bit of magic, it's not quite intuitive what's going on there, where you can use $root and where you cant. Both of those reasons make me think it'd introduce complexity to the user in a way that doesn't seem worth the trade off of adding this functionality to me.