-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Optimizations to improve performance on complex conditional schemas #2466
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
Changes from 7 commits
eef551d
51387a0
fca3592
ba15fd3
75665ca
23288ae
4cf5f7f
78fc62c
58d3335
5e7a08d
667903b
ab36d1e
b00f49f
833006a
8796c9d
b6c53c8
f72a62c
75c4d84
a08c563
07a43c8
52747fb
123c480
964f537
544133b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -249,7 +249,8 @@ function SchemaFieldRender(props) { | |
const FieldTemplate = | ||
uiSchema["ui:FieldTemplate"] || registry.FieldTemplate || DefaultTemplate; | ||
let idSchema = props.idSchema; | ||
const schema = retrieveSchema(props.schema, rootSchema, formData); | ||
var schema = retrieveSchema(props.schema, rootSchema, formData); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming all |
||
|
||
idSchema = mergeObjects( | ||
toIdSchema(schema, null, rootSchema, formData, idPrefix), | ||
idSchema | ||
|
@@ -403,7 +404,6 @@ function SchemaFieldRender(props) { | |
</FieldTemplate> | ||
); | ||
} | ||
|
||
class SchemaField extends React.Component { | ||
shouldComponentUpdate(nextProps, nextState) { | ||
return !deepEquals(this.props, nextProps); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
import React from "react"; | ||
import * as ReactIs from "react-is"; | ||
import mergeAllOf from "json-schema-merge-allof"; | ||
import fill from "core-js-pure/features/array/fill"; | ||
import union from "lodash/union"; | ||
import jsonpointer from "jsonpointer"; | ||
|
@@ -189,6 +188,8 @@ function computeDefaults( | |
} else if ("default" in schema) { | ||
// Use schema defaults for this node. | ||
defaults = schema.default; | ||
} else if ("const" in schema) { | ||
defaults = schema.const; | ||
} else if ("$ref" in schema) { | ||
// Use referenced schema defaults for this node. | ||
const refSchema = findSchemaDefinition(schema.$ref, rootSchema); | ||
|
@@ -666,15 +667,16 @@ export function resolveSchema(schema, rootSchema = {}, formData = {}) { | |
} else if (schema.hasOwnProperty("dependencies")) { | ||
const resolvedSchema = resolveDependencies(schema, rootSchema, formData); | ||
return retrieveSchema(resolvedSchema, rootSchema, formData); | ||
} else if (schema.hasOwnProperty("allOf")) { | ||
} else if (schema["allOf"]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the reason for this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think I changed that per a comment on the previous PR. Could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switching that line to |
||
return { | ||
...schema, | ||
allOf: schema.allOf.map(allOfSubschema => | ||
retrieveSchema(allOfSubschema, rootSchema, formData) | ||
), | ||
}; | ||
} else { | ||
// No $ref or dependencies attribute found, returning the original schema. | ||
// No $ref, dependencies, or allOf attribute found, so there's nothing to resolve. | ||
// Returning the original schema. | ||
return schema; | ||
} | ||
} | ||
|
@@ -697,28 +699,68 @@ export function retrieveSchema(schema, rootSchema = {}, formData = {}) { | |
return {}; | ||
} | ||
let resolvedSchema = resolveSchema(schema, rootSchema, formData); | ||
if ("allOf" in schema) { | ||
try { | ||
resolvedSchema = mergeAllOf({ | ||
...resolvedSchema, | ||
allOf: resolvedSchema.allOf, | ||
}); | ||
} catch (e) { | ||
console.warn("could not merge subschemas in allOf:\n" + e); | ||
const { allOf, ...resolvedSchemaWithoutAllOf } = resolvedSchema; | ||
return resolvedSchemaWithoutAllOf; | ||
|
||
while ("if" in resolvedSchema) { | ||
// Note that if and else are key words in javascript so extract to variable names which are allowed | ||
var { | ||
if: expression, | ||
then, | ||
else: otherwise, | ||
...resolvedSchemaLessConditional | ||
} = resolvedSchema; | ||
|
||
var conditionalSchema = isValid(expression, formData, rootSchema) | ||
? then | ||
: otherwise; | ||
|
||
if (conditionalSchema) { | ||
conditionalSchema = resolveSchema( | ||
conditionalSchema, | ||
rootSchema, | ||
formData | ||
); | ||
} | ||
resolvedSchema = mergeSchemas( | ||
resolvedSchemaLessConditional, | ||
conditionalSchema || {} | ||
); | ||
} | ||
|
||
let allOf = resolvedSchema.allOf; | ||
|
||
if (allOf) { | ||
for (var i = 0; i < allOf.length; i++) { | ||
let allOfSchema = allOf[i]; | ||
|
||
// if we see an if in our all of schema then evaluate the if schema and select the then / else, not sure if we should still merge without our if then else | ||
if ("if" in allOfSchema) { | ||
allOfSchema = isValid(allOfSchema.if, formData, rootSchema) | ||
? allOfSchema.then | ||
: allOfSchema.else; | ||
} | ||
|
||
if (allOfSchema) { | ||
allOfSchema = resolveSchema(allOfSchema, rootSchema, formData); // resolve references etc. | ||
resolvedSchema = { | ||
...mergeSchemas(resolvedSchema, allOfSchema), | ||
allOf: undefined, | ||
}; | ||
} | ||
} | ||
} | ||
|
||
const hasAdditionalProperties = | ||
resolvedSchema.hasOwnProperty("additionalProperties") && | ||
resolvedSchema.additionalProperties !== false; | ||
|
||
if (hasAdditionalProperties) { | ||
return stubExistingAdditionalProperties( | ||
resolvedSchema = stubExistingAdditionalProperties( | ||
resolvedSchema, | ||
rootSchema, | ||
formData | ||
); | ||
} | ||
|
||
return resolvedSchema; | ||
} | ||
|
||
|
@@ -998,7 +1040,7 @@ export function toIdSchema( | |
const idSchema = { | ||
$id: id || idPrefix, | ||
}; | ||
if ("$ref" in schema || "dependencies" in schema || "allOf" in schema) { | ||
if ("$ref" in schema || "dependencies" in schema) { | ||
const _schema = retrieveSchema(schema, rootSchema, formData); | ||
return toIdSchema(_schema, id, rootSchema, formData, idPrefix); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ function createAjvInstance() { | |
multipleOfPrecision: 8, | ||
schemaId: "auto", | ||
unknownFormats: "ignore", | ||
useDefaults: true, | ||
}); | ||
|
||
// add custom formats | ||
|
@@ -300,17 +301,27 @@ export function withIdRefPrefix(schemaNode) { | |
*/ | ||
export function isValid(schema, data, rootSchema) { | ||
try { | ||
// add the rootSchema ROOT_SCHEMA_PREFIX as id. | ||
// add the rootSchema. if it has no $id, use ROOT_SCHEMA_PREFIX as the cache key. | ||
// 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. | ||
return ajv | ||
.addSchema(rootSchema, ROOT_SCHEMA_PREFIX) | ||
.validate(withIdRefPrefix(schema), data); | ||
let rootSchemaAdded; | ||
if (rootSchema.$id) { | ||
rootSchemaAdded = !!ajv.getSchema(rootSchema.$id); | ||
} else { | ||
rootSchemaAdded = !!ajv.getSchema(ROOT_SCHEMA_PREFIX); | ||
} | ||
|
||
if (!rootSchemaAdded) { | ||
if (rootSchema.$id) { | ||
ajv.addSchema(rootSchema); | ||
} else { | ||
ajv.addSchema(rootSchema, ROOT_SCHEMA_PREFIX); | ||
} | ||
} | ||
return ajv.validate(withIdRefPrefix(schema), data); | ||
} catch (e) { | ||
console.warn("Encountered error while validating schema", e); | ||
return false; | ||
} finally { | ||
// make sure we remove the rootSchema from the global ajv instance | ||
ajv.removeSchema(ROOT_SCHEMA_PREFIX); | ||
Comment on lines
-312
to
-314
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the root schema being removed (before my change)? Since we now need to run the validator frequently to evaluate conditional subschemas, keeping the root schema in the cache is essential for performance, especially for large schemas. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've figured it out: this function uses the same ajv instance as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One cause of performance issues here is that we can't take advantage of AJV's cache because Another thing I've noticed: we get different behavior with the current implementation if the root schema has an $id. When an $id is present, I think AJV totally ignores the root schema prefix when we call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As for |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what's happening here with these docs changes, they are unrelated to the PR and are also out commented. I guess this should be reverted?