-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat(rest): add support for ajv-errors #4623
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @mastermunj! Not sure if it's out-of-scope but I think we'll need some documentation on the new feature. Maybe we could add the docs under Model > Property decorator or Error handling.
Hi @achrinza, added docs for Model > Property Decorator and Parsing Requests > Custom Error Messages. |
@@ -190,6 +191,7 @@ function createValidator( | |||
{}, | |||
{ | |||
allErrors: true, | |||
jsonPointers: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed for ajv-errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, allErrors
& jsonPointers
both are needed by ajv-errors
as documented in their usage docs.
When not provided, they're enabled from within ajv-errors
at https://github.com/epoberezkin/ajv-errors/blob/master/index.js#L4-L8
Benefits of jsonPointers are listed at ajv errors -> Templates
@@ -333,7 +333,7 @@ describe('validateRequestBody', () => { | |||
it('nested $ref in reference', () => { | |||
const details: RestHttpErrors.ValidationErrorDetails[] = [ | |||
{ | |||
path: '.accounts[0].address.city', | |||
path: '/accounts/0/address/city', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it introduced by jsonPointers: true,
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is because of jsonPointers: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Implements #2146
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈