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

Fix additionalProperties error with errorDataPath #674

Merged

Conversation

lehni
Copy link
Contributor

@lehni lehni commented Jan 12, 2018

As discussed with @epoberezkin in #671 (comment), here now the PR to add correct error messages for additionalProperties validation errors when errorDataPath is set to 'property'

@lehni
Copy link
Contributor Author

lehni commented Jan 15, 2018

This probably should need a test for it too, but I couldn't figure out where to put it, and couldn't find a similar one for the same behavior for required here.

@epoberezkin
Copy link
Member

@lehni well spotted the test was missing for the other change. The tests should be somewhere here https://github.com/epoberezkin/ajv/blob/797dfc8c2b0f51aaa405342916cccb5962dd5f21/spec/errors.spec.js#L45 and here https://github.com/epoberezkin/ajv/blob/797dfc8c2b0f51aaa405342916cccb5962dd5f21/spec/errors.spec.js#L145 (for "required" errors).

At the moment it tests only dataPaths, should also check messages.

@lehni lehni force-pushed the fix/additional-properties-message branch from 42fcfa6 to e3f3e57 Compare January 24, 2018 22:22
@lehni lehni force-pushed the fix/additional-properties-message branch from e3f3e57 to 4a12ca3 Compare January 24, 2018 22:40
@lehni
Copy link
Contributor Author

lehni commented Jan 24, 2018

@epoberezkin thanks for the pointer. Here now my added tests:

4a12ca3#diff-831dd6f83c36a271e002b7770f3dbac9R67

I hope these suffice?

Also, please let me know if 'is an invalid additional property' sounds like a good message to you. The wording can change. Alternatives to invalid are illegal or unsupported

@epoberezkin epoberezkin merged commit 1f75c51 into ajv-validator:master Jan 26, 2018
@epoberezkin
Copy link
Member

@lehni thank you. I've merged. Maybe just "is an additional property"?

@lehni
Copy link
Contributor Author

lehni commented Jan 26, 2018

I think the message should somehow state that additional properties aren't allowed. should NOT have additional properties implied that. But how to phrase it for the message that is directly attached to the property?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants