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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Expand Up @@ -21,10 +21,13 @@ should change the heading of the (upcoming) version to include a major version b
- Fix Vite development server [#3228](https://github.com/rjsf-team/react-jsonschema-form/issues/3228)

## @rjsf/validator-ajv8
- BREAKING CHANGE: Disable form data validation for invalid JSON Schemas. Use @rjsf/validator-ajv6 if you need to validate against invalid schemas.
- Fix additionalProperties validation [#3213](https://github.com/rjsf-team/react-jsonschema-form/issues/3213)
- Report all schema errors thrown by Ajv. Previously, we would only report errors thrown for a missing meta-schema. This behavior is unchanged for @rjsf/validator-ajv6.
- Disable Ajv strict mode by default.
- Add RJSF-specific additional properties keywords to Ajv to prevent errors from being reported in strict mode.
- For JSON Schemas with `$id`s, use a pre-compiled Ajv validation function when available.
- No longer fail to validate inner schemas with `$id`s, fixing [#2821](https://github.com/rjsf-team/react-jsonschema-form/issues/2181).

# 5.0.0-beta.12

Expand Down
59 changes: 42 additions & 17 deletions packages/validator-ajv8/src/validator.ts
@@ -1,4 +1,4 @@
import Ajv, { ErrorObject } from "ajv8";
import Ajv, { ErrorObject, ValidateFunction } from "ajv8";
import toPath from "lodash/toPath";
import isObject from "lodash/isObject";
import clone from "lodash/clone";
Expand Down Expand Up @@ -245,22 +245,35 @@ export default class AJV8Validator<
schema: RJSFSchema,
formData?: T
): { errors?: Result[]; validationError?: Error } {
let validationError: Error | undefined = undefined;
let compilationError: Error | undefined = undefined;
let compiledValidator: ValidateFunction | undefined;
if (schema["$id"]) {
compiledValidator = this.ajv.getSchema(schema["$id"]);
}
try {
this.ajv.validate(schema, formData);
if (compiledValidator === undefined) {
compiledValidator = this.ajv.compile(schema);
}
compiledValidator(formData);
Comment on lines 253 to +257
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
Member

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

} catch (err) {
validationError = err as Error;
compilationError = err as Error;
}

if (typeof this.localizer === "function") {
this.localizer(this.ajv.errors);
}
const errors = this.ajv.errors || undefined;
let errors;
if (compiledValidator) {
if (typeof this.localizer === "function") {
this.localizer(compiledValidator.errors);
}
errors = compiledValidator.errors || undefined;

// Clear errors to prevent persistent errors, see #1104
this.ajv.errors = null;
// Clear errors to prevent persistent errors, see #1104
compiledValidator.errors = null;
}

return { errors: errors as unknown as Result[], validationError };
return {
errors: errors as unknown as Result[],
validationError: compilationError,
};
}

/** This function processes the `formData` with an optional user contributed `customValidate` function, which receives
Expand Down Expand Up @@ -366,25 +379,37 @@ export default class AJV8Validator<
* false otherwise. If the schema is invalid, then this function will return
* false.
*
* @param schema - The schema against which to validate the form data * @param schema
* @param formData- - The form data to validate
* @param schema - The schema against which to validate the form data
* @param formData - The form data to validate
* @param rootSchema - The root schema used to provide $ref resolutions
*/
isValid(schema: S, formData: T, rootSchema: S) {
const rootSchemaId = rootSchema["$id"] ?? ROOT_SCHEMA_PREFIX;
try {
// add the rootSchema ROOT_SCHEMA_PREFIX as id.
// then rewrite the schema ref's to point to the rootSchema
// this accounts for the case where schema have references to models
// that lives in the rootSchema but not in the schema in question.
const result = this.ajv
.addSchema(rootSchema, ROOT_SCHEMA_PREFIX)
.validate(this.withIdRefPrefix(schema), formData);
if (this.ajv.getSchema(rootSchemaId) === undefined) {
this.ajv.addSchema(rootSchema, rootSchemaId);
}
Comment on lines +393 to +395
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.

const schemaWithIdRefPrefix = this.withIdRefPrefix(schema) as S;
let compiledValidator: ValidateFunction | undefined;
if (schemaWithIdRefPrefix["$id"]) {
compiledValidator = this.ajv.getSchema(schemaWithIdRefPrefix["$id"]);
}
if (compiledValidator === undefined) {
compiledValidator = this.ajv.compile(schemaWithIdRefPrefix);
}
const result = compiledValidator(formData);
return result as boolean;
} catch (e) {
console.warn("Error encountered compiling schema:", e);
return false;
} finally {
// 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);
Comment on lines +410 to +412
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
Member

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.

}
}

Expand Down