-
-
Notifications
You must be signed in to change notification settings - Fork 208
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: add validationErrors to schema mismatch errors #641
base: master
Are you sure you want to change the base?
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.
I like the idea, but I don't uunderstand the purpose of accumulating errors. Why we can't throw them if validation fails without accumulating?
with |
Oh, I see. Can you modify the |
fe701e4
to
04d0b3c
Compare
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.
Nice addition 👍🏼
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
@ivan-tymoshenko @mcollina is there anything else missing to get this merged? |
else throw Object.assign(new TypeError(\`The value of '${schemaRef}' does not match schema definition.\`), { | ||
validationErrors: 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.
Why not use cause
and avoid the Object.assign?
else throw Object.assign(new TypeError(\`The value of '${schemaRef}' does not match schema definition.\`), { | |
validationErrors: errors | |
}) | |
else throw new TypeError(\`The value of '${schemaRef}' does not match schema definition.\`, { | |
cause: errors | |
}) |
Co-authored-by: Uzlopak <[email protected]>
return this.ajv.validate(schemaRef, data) | ||
validate (schemaRef, data, errors) { | ||
const valid = this.ajv.validate(schemaRef, data) | ||
if (this.ajv.errors && Array.isArray(errors)) errors.push(...this.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.
could this be faster?
if (this.ajv.errors && Array.isArray(errors)) errors.push(...this.ajv.errors) | |
Array.isArray(errors) && Arrry.prototype.push.apply(errors, this.ajv.errors) |
t.fail('should throw') | ||
} catch (err) { | ||
t.equal(err.message, 'The value of \'#\' does not match schema definition.') | ||
t.same(err.validationErrors, [ |
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.
This would be then:
t.same(err.validationErrors, [ | |
t.same(err.cause, [ |
t.fail('should throw') | ||
} catch (err) { | ||
t.equal(err.message, 'The value of \'#/properties/data\' does not match schema definition.') | ||
t.same(err.validationErrors, [ |
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.
dito
t.same(err.validationErrors, [ | |
t.same(err.cause, [ |
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.
i think the rationale here was to call it as fastify with ajv implementation:
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.
I hope my remarks make sense :)
Looking forward
when using a schema that includes
anyOf
oroneOf
and it fails to find a match the error is pretty worthless (in my case it is usually'The value of \'#\' does not match schema definition.'
)and it is not trivial to debug since stack trace points to anonymous functions composed at runtime.
this sets the validation errors on the error before throwing it. if the performance penalty is too big I suggest it should be behind a
verbose
option