Skip to content

feat: add validationErrors to schema mismatch errors#641

Open
MoLow wants to merge 3 commits into
fastify:mainfrom
MoLow:add-validation-errors
Open

feat: add validationErrors to schema mismatch errors#641
MoLow wants to merge 3 commits into
fastify:mainfrom
MoLow:add-validation-errors

Conversation

@MoLow

@MoLow MoLow commented Aug 20, 2023

Copy link
Copy Markdown
Contributor

when using a schema that includes anyOf or oneOf 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

@MoLow

MoLow commented Aug 20, 2023

Copy link
Copy Markdown
Contributor Author

CC @mcollina @ivan-tymoshenko

@ivan-tymoshenko ivan-tymoshenko left a comment

Copy link
Copy Markdown
Member

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?

@MoLow

MoLow commented Aug 20, 2023

Copy link
Copy Markdown
Contributor Author

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 anyOf or oneOf we go over all the possible schemas and if validation passes we use that schema for serialization. for the case where non of the possibilities matches - each of the errors is relevant I think

@ivan-tymoshenko

Copy link
Copy Markdown
Member

Oh, I see. Can you modify the validate function to also return errors when validation fails and put the error acumulation directly into anyof/oneOf block please? Reseting errors globaly might be a problem for some cases in the future.

@MoLow MoLow force-pushed the add-validation-errors branch from fe701e4 to 04d0b3c Compare August 27, 2023 07:38
@MoLow MoLow requested a review from ivan-tymoshenko August 27, 2023 07:38

@Eomm Eomm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice addition 👍🏼

Comment thread test/any.test.js Outdated
Comment thread lib/validator.js

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@MoLow

MoLow commented Aug 31, 2023

Copy link
Copy Markdown
Contributor Author

@ivan-tymoshenko @mcollina is there anything else missing to get this merged?

Comment thread index.js Outdated
Comment thread index.js
Comment on lines +901 to +903
else throw Object.assign(new TypeError(\`The value of '${schemaRef}' does not match schema definition.\`), {
validationErrors: errors
})

Copy link
Copy Markdown
Contributor

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?

Suggested change
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 <aras.abbasi@googlemail.com>
Comment thread lib/validator.js
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could this be faster?

Suggested change
if (this.ajv.errors && Array.isArray(errors)) errors.push(...this.ajv.errors)
Array.isArray(errors) && Arrry.prototype.push.apply(errors, this.ajv.errors)

Comment thread test/any.test.js
t.fail('should throw')
} catch (err) {
t.equal(err.message, 'The value of \'#\' does not match schema definition.')
t.same(err.validationErrors, [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This would be then:

Suggested change
t.same(err.validationErrors, [
t.same(err.cause, [

Comment thread test/any.test.js
t.fail('should throw')
} catch (err) {
t.equal(err.message, 'The value of \'#/properties/data\' does not match schema definition.')
t.same(err.validationErrors, [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dito

Suggested change
t.same(err.validationErrors, [
t.same(err.cause, [

Copy link
Copy Markdown
Member

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:

https://github.com/fastify/fastify/blob/9b3ca58097954c4df20b253906ed6697461e2486/lib/handleRequest.js#L109

@Uzlopak Uzlopak left a comment

Copy link
Copy Markdown
Contributor

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants