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

Fix issues with validating schemas with "$id"s in @rjsf/validator-ajv8 #3232

Merged
merged 2 commits into from Nov 11, 2022

Conversation

nickgros
Copy link
Contributor

@nickgros nickgros commented Nov 5, 2022

@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

Reasons for making this change

Fixes #2821, fixes #3212

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

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

Choose a reason for hiding this comment

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

validate throws an error if you try to compile a schema that you've already compiled (based on $id).

This may also improve performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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"
);
});
Copy link
Contributor Author

@nickgros nickgros Nov 5, 2022

Choose a reason for hiding this comment

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

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.

@heath-freenome
Copy link
Contributor

Does this fix #3212 too?

@nickgros
Copy link
Contributor Author

nickgros commented Nov 11, 2022

Does this fix #3212 too?

Seems like it does. I'll link the issue and then merge.

@nickgros nickgros merged commit 397740e into rjsf-team:main Nov 11, 2022
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants