Fix issues with validating schemas with "$id"s in @rjsf/validator-ajv8 #3232
Conversation
@rjsf/validator-ajv8 - BREAKING: No longer attempt to validate data against invalid schemas - Use pre-compiled validator from Ajv if the schema has an $id - isValid: Use rootSchema.$id over the default value if it exists (Ajv uses it anyway) - isValid: Log warning on compilation error
| try { | ||
| this.ajv.validate(schema, formData); | ||
| if (compiledValidator === undefined) { | ||
| compiledValidator = this.ajv.compile(schema); | ||
| } | ||
| compiledValidator(formData); |
There was a problem hiding this comment.
validate throws an error if you try to compile a schema that you've already compiled (based on $id).
This may also improve performance.
There was a problem hiding this comment.
Do we want to cache the compiled validator using a Map with the schema as a key? I'm not sure how much time it takes to compile it, but if it is a big schema it might save time on subsequent validations?
There was a problem hiding this comment.
I'd rather use the Ajv cache if possible, one less thing for us to get wrong. Though that might not work for conditional logic where we may create various subschemas that have no $id
| // TODO: A function should be called if the root schema changes so we don't have to remove and recompile the schema every run. | ||
| // make sure we remove the rootSchema from the global ajv instance | ||
| this.ajv.removeSchema(ROOT_SCHEMA_PREFIX); | ||
| this.ajv.removeSchema(rootSchemaId); |
There was a problem hiding this comment.
I'd prefer if we had something like validator.setRootSchema(...), which I think could help performance for complex if/then/else cases. But that's a bigger project for another day.
There was a problem hiding this comment.
If I remember how it is used, isValid is used primarily for multi-schema use cases. Not sure if the rootSchema is always the same. We could also consider caching it in a Map in the future too.
There was a problem hiding this comment.
I think the rootSchema is always the same, unless the Form schema prop changes. It's just provided here to resolve refs and definitions.
| if (this.ajv.getSchema(rootSchemaId) === undefined) { | ||
| this.ajv.addSchema(rootSchema, rootSchemaId); | ||
| } |
There was a problem hiding this comment.
Note that if rootSchema has an $id, the second parameter is ignored.
| // @ts-expect-error - accessing private Ajv instance to verify compilation happens once | ||
| const compileSpy = jest.spyOn(validator.ajv, "compile"); | ||
| compileSpy.mockClear(); | ||
|
|
||
| // Call isValid twice with the same schema | ||
| validator.isValid(schema, formData, rootSchema); | ||
| validator.isValid(schema, formData, rootSchema); | ||
|
|
||
| expect(compileSpy).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
I hate doing this but I can't think of a better way to test this behavior
| it("should return an error list", () => { | ||
| expect(errors).toHaveLength(2); | ||
| expect(errors[0].name).toEqual("type"); | ||
| expect(errors[0].property).toEqual(".properties.foo.required"); | ||
| // TODO: This schema path is wrong due to a bug in ajv; change this test when https://github.com/ajv-validator/ajv/issues/512 is fixed. | ||
| expect(errors[0].schemaPath).toEqual( | ||
| "#/definitions/stringArray/type" | ||
| ); | ||
| expect(errors[0].message).toEqual("must be array"); | ||
|
|
||
| expect(errors[1].stack).toEqual( | ||
| expect(errors).toHaveLength(1); | ||
| expect(errors[0].stack).toEqual( | ||
| "schema is invalid: data/properties/foo/required must be array" | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This is a breaking change compared to the v4/validator-ajv6 behavior: If the schema is invalid, don't attempt to validate the data.
I made this change because otherwise we would have to do something like
try {
compiledSchema = this.ajv.compile(schema)
compiledSchema.validate(data)
} catch (e1) {
try {
this.ajv.validate(schema, data)
} catch (e2) {
compilationError = e2;
}
}IMO this was getting too complex to support a case that probably shouldn't be happening.
|
Does this fix #3212 too? |
Seems like it does. I'll link the issue and then merge. |
@rjsf/validator-ajv8
Reasons for making this change
Fixes #2821, fixes #3212
Checklist
npm run test:updateto update snapshots, if needed.