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

feat: add validationErrors to schema mismatch errors #641

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MoLow
Copy link
Contributor

@MoLow MoLow commented Aug 20, 2023

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
Copy link
Contributor Author

MoLow commented Aug 20, 2023

CC @mcollina @ivan-tymoshenko

Copy link
Member

@ivan-tymoshenko ivan-tymoshenko left a 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?

@MoLow
Copy link
Contributor Author

MoLow commented Aug 20, 2023

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

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

Nice addition 👍🏼

test/any.test.js Outdated Show resolved Hide resolved
lib/validator.js Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@MoLow
Copy link
Contributor Author

MoLow commented Aug 31, 2023

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

index.js Outdated Show resolved Hide resolved
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
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>
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
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)

t.fail('should throw')
} catch (err) {
t.equal(err.message, 'The value of \'#\' does not match schema definition.')
t.same(err.validationErrors, [
Copy link
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, [

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

Copy link
Contributor

@Uzlopak Uzlopak left a 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

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.

None yet

5 participants