Fix 5338 & 5373: compile precompiled validators with the form's customMergeAllOf - #5371
heath-freenome wants to merge 6 commits into
Conversation
Size limit reportSizes are minified + brotli, measured by size-limit at A package listed twice is measured both as installed and, on the second row, as its own code with its dependencies excluded. Peer dependencies are always excluded.
|
jimmycallin
left a comment
jimmycallin
left a comment
There was a problem hiding this comment.
Review of the fix-5338 changes against fix-4385-schema-context. I verified the first four inline comments with scratch vitest probes on this head. The last two are nits.
63e7bf5 to
1fa40cf
Compare
| let merged: S; | ||
| try { | ||
| merged = context.customMergeAllOf(resolvedSchema); | ||
| } catch { |
There was a problem hiding this comment.
This catch swallows every error from the merge without a trace.
The form's own merge path logs could not merge subschemas in allOf before it falls back. Here the compile just leaves out every sub-schema that merge would have produced. If the user's customMergeAllOf has a bug that throws, say a TypeError on some allOf shape, the compile script succeeds. The form then fails later at runtime with No precompiled validator function was found, and nothing points back at the merge. A console.warn like the one in the non-expand branch would make it visible at compile time.
There was a problem hiding this comment.
Gone. The merge now goes through mergeAllOf(), which already does logOnce('could not merge subschemas in allOf:\n', 'warn', e) and returns merged: false, so the compile warns in the same place and with the same message the form does, and drops the allOf the way the form drops it.
There's a test for the drop as well: a throwing merge keeps the rest of the schema's own sub-schemas and leaves the allOf's out, which is what the form renders.
edaa1c2 to
33a06fa
Compare
7dd953a to
2ddbfd0
Compare
Addresses the review of #5371. A form always merges an `allOf`, so the sub-schemas it validates against are the merged schema's. `expandAllBranches` returned the unmerged branches instead, which hold neither the sub-schemas a `customMergeAllOf` rewrote nor the ones that only exist once the merge has brought a property together with the `patternProperties` matching it. Gating a second, merge-aware path on `customMergeAllOf`, as this branch did, left the second case broken whenever no custom merge was passed, and re-resolved the merged schema in a way the form does not, which is what made the recursion guard necessary. Expanding all branches now merges the `allOf`, and drops one it cannot merge, through the same code the form uses, so the two cannot diverge. The branch variants were also what `resolveSchema()`'s permutation of the `allOf` members multiplied: a root `allOf` of six three-item `allOf`s went from 15,631 merges in 60.2s to 7 in under a millisecond. Every `schemaParser()` snapshot is unchanged, so the branches were contributing nothing to the parsed map. Also in the parser: - Each `oneOf`/`anyOf` option is parsed rather than having its `properties` read off, so an option whose sub-schemas come from its own `allOf`, conditions or dependencies is reached. A parsed set held by identity keeps a property the options share from being resolved once per option. - The `patternProperties` and `additionalProperties` schemas are parsed. A key they describe is only stubbed into `properties` once the form data has it, which a parse has none of, so nothing reached them. - Every position of a tuple `items` is parsed, with `additionalItems` only for a tuple, matching what `ArrayField` renders. - Two schemas that share an `$id` and differ no longer fail the parse. The first is compiled and answers for the rest, which is how both the precompiled and the regular validators look a schema up. `SchemaParserOptions` is a `Pick` of `SchemaContext` rather than an interface `SchemaContext` extends, leaving the context type as v7 has it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks @jimmycallin — this was a genuinely useful review. Every one of the eleven live findings reproduced, including your timings. Pushed as 18639b9, with per-thread replies above. The review changed the shape of the fix. Your "the same gap exists with the default merge" comment is the root cause: a form always merges an if (ALL_OF_KEY in resolvedSchema) {
- // resolve allOf schemas
- if (expandAllBranches) {
- const { allOf, ...restOfSchema } = resolvedSchema;
- return [...(allOf as S[]), restOfSchema as S];
- }
The two Why narrowing what gets compiled is safeDropping the branches removes schemas from the compiled map, and under-compiling throws at runtime, so this is the part I did not want to argue from reading alone:
Two pre-existing defects this review surfacedBoth outside this PR; say the word and I'll open issues.
CI is green locally on this commit: lint, knip, build (16 projects), typecheck and test (15 projects), plus |
| const valueSchemas: unknown[] = Object.values(schema[PROPERTIES_KEY] ?? {}); | ||
| // An additional property is only stubbed into `properties` once the form data has a key for it, which the parse has | ||
| // none of, so the schema a form renders those keys with is reached here instead | ||
| valueSchemas.push(...Object.values(schema[PATTERN_PROPERTIES_KEY] ?? {}), schema[ADDITIONAL_PROPERTIES_KEY]); |
There was a problem hiding this comment.
Pattern/additional property schemas are parsed raw, but the form merges them through customMergeAllOf first
When the form has data for an extra key, stubExistingAdditionalProperties() builds that key's schema with retrieveSchema({ allOf: Object.values(matchingProperties) }). That allOf goes through the context's customMergeAllOf, even when only one pattern matches. If several patterns overlap, they're merged into one schema. The parser here pushes each raw patternProperties value on its own and never builds that merged schema.
Repro: patternProperties: { '^x': { properties: { choice: CHOICE } } } with titleChoiceMergeAllOf, plus form data { x1: { choice: 'b' } }. The form validates against the titled options. Those options were never compiled, so you get No precompiled validator function was found (#5338 again, through patternProperties). Two overlapping patterns that each have a oneOf fail the same way, even with the default merge.
Suggested fix: parse { allOf: [pattern] } for each pattern, and { allOf: [...] } for each overlapping set, so the parser's path matches the form's.
There was a problem hiding this comment.
Confirmed, and this was the important one. Fixed in 03d3185.
Your repro reproduces exactly. With the fix removed, the new end-to-end test in both validator packages fails with:
Error: No precompiled validator function was found for the given schema for "1691cd08"
the same hash #5338 itself reports, now reached through patternProperties instead of a top-level allOf.
parseValueSchemas() now pushes { allOf: [...combination] } for each non-empty combination of patternProperties, which is the same shape stubExistingAdditionalProperties() hands retrieveSchema(), down to the single pattern a lone match makes. The raw pattern values are no longer parsed, since a form resolves nothing from one on its own. A parse has no form data, so it cannot know which key matches which patterns and has to cover all 2^n - 1 of them. Measured cost, with a merge that counts its calls:
| patterns | merges | ms |
|---|---|---|
| 1 | 1 | 1 |
| 2 | 3 | 0 |
| 4 | 15 | 0 |
| 6 | 63 | 1 |
| 8 | 255 | 4 |
One correction on the second half: I could not get the default merge to fail this way. shallowAllOfMerge({ allOf: [p1, p2] }) where both define choice with a oneOf returns the first one's options:
merged = {"properties":{"choice":{"oneOf":[{"const":"a"},{"const":"b"}]}}}so the merged schema's options are a single pattern's, which is parsed on its own either way. It is a customMergeAllOf rewriting the merge that produces options nothing else reaches. I enumerate the combinations regardless — a custom merge can rewrite any subset, and that is the case this PR exists for — but say the word if you would rather pay the 2^n only when a customMergeAllOf is present.
Tests: parses each combination of patternProperties a key can match, as the form merges every one it matches (asserts the merge is called with [first], [second], [first, second]), parses the allOf merged for a key its patternProperties match with it, and the end-to-end covers the sub-schemas of the merge the form makes for that key in both validators, off the shared SCHEMA_MERGED_FOR_PATTERN_KEY fixture.
| if (!existing) { | ||
| this.schemaMap[key] = identifiedSchema; | ||
| } else if (!deepEquals(existing, identifiedSchema)) { | ||
| if (ownId !== undefined) { |
There was a problem hiding this comment.
A schema with an $id that comes in several variants now just warns, and the precompiled validator gives wrong answers for every variant after the first
getFirstMatchingOption() builds augmentedSchema = { ...option, ...requiresAnyOf } with required deleted. relaxOptionsForScoring() makes a copy with additionalProperties: false turned into true. Both keep the option's $id. Take oneOf: [{ $ref: '#/definitions/Cat' }] with Cat = { $id: 'Cat', properties: {...}, additionalProperties: false }. The parser sees two different Cat schemas. It used to throw. Now it keeps only the first (strict) one. At runtime getValidator() looks up by $id, so the relaxed scoring call from omitExtraData's handleOneOf runs against the strict function. That gives false negatives, which is exactly what relaxOptionsForScoring exists to prevent. The augmented variant also validates without its anyOf-of-required.
The only sign of this is a one-time warning. The cleaner fix is lower down: strip $id in the code that builds these variants (getFirstMatchingOption, relaxOptionsForScoring, mergeSchemas of option + parent), so each variant gets its own hash key, instead of dropping variants here.
There was a problem hiding this comment.
Agreed, and your fix is the better one — I took it and reverted my drop-and-warn. Fixed in 03d3185.
getFirstMatchingOption() now drops the $id from the schema it augments, and relaxOptionsForScoring() from the relaxed copy. Each variant is keyed by its content, all of them get compiled, and ParserValidator.addSchema() is back to the loud Two different schemas exist with the same key rather than keeping the first. The doc paragraph and changelog bullet about first-wins are gone with it.
This also fixes the runtime side you point at, not just the parse. Your Cat example, parsed:
-42e24cca => {"type":"object","properties":{"name":{...}},"additionalProperties":false,"anyOf":[{"required":["name"]}]}
-3b33eb27 => {"type":"object","properties":{"name":{...}},"additionalProperties":true,"anyOf":[{"required":["name"]}]}
Both now exist, each under its own key, so the relaxed scoring call from omitExtraData's handleOneOf gets its own function instead of the strict one. No warning, nothing dropped.
One exception I had to carve out: the junk option. precompiledValidator.isValid() recognises it by schema[ID_KEY] === JUNK_OPTION_ID and returns false without a compiled function, and getClosestMatchingOption() puts it through getFirstMatchingOption(), where it takes the augmentation path (it has properties). A blanket delete made the augmented junk schema look like an ordinary one and threw No precompiled validator function was found. So:
if (augmentedSchema[ID_KEY] !== JUNK_OPTION_ID) {
delete augmentedSchema[ID_KEY];
}A symbol marker on JUNK_OPTION would survive the spread and remove the special case, but that changes a public constant and both validators' isValid(), so I left it for its own PR unless you want it here.
Not covered, and worth its own issue: MultiSchemaField calls isValid(retrievedOptions[selectedOption], ...) on a resolved option, which can carry a $ref'd definition's $id while differing from it, and the parser never captures that one at all. Different defect from this finding.
Tests: parses each variant of an option that has an $id under a key of its own (the parse throws without the fix), parses the relaxed variant of an $id option that has no properties under a key of its own (the case where the option reaches isValid() unaugmented, which is what makes the relaxOptionsForScoring() half load-bearing), and calls isValid() with the $id dropped from the option it augments, keeping only the junk option's.
| } | ||
| // resolve allOf schemas. A form always merges an `allOf`, so even `expandAllBranches` merges it: the subschemas it | ||
| // validates against are the merged schema's, which the unmerged branches need not hold -- a `customMergeAllOf` can | ||
| // rewrite them, and only the merged schema has the properties its `patternProperties` apply to |
There was a problem hiding this comment.
Merging a property with its matching patternProperties still resolves that property with expandAllBranches off
Now that expand mode merges allOf, more schemas reach the PROPERTIES_KEY && PATTERN_PROPERTIES_KEY block below (around line 770), which is exactly the case the PR wants to cover. That block calls retrieveSchemaInternal(context, { allOf: [prop, ...matching] }, rootSchema, getByPath(rawFormData, key), undefined, ...). Passing undefined means expandAllBranches is false. So for a pattern-matched property:
resolveConditionkeeps only the branch picked byParserValidator.isValid(), which always returnsfalse, so it always takeselse;processDependenciesskips every dependency, because the form data has no key for it.
Repro: properties: { p: { if: {...}, then: { properties: { c: { oneOf: [...] } } } } } with patternProperties: { '^p$': {...} }. The then branch's oneOf options are never compiled, so the precompiled validator throws once the user's data meets the if.
Fix: pass expandAllBranches through, and use flatMap over the results instead of [acc.properties[key]] =.
There was a problem hiding this comment.
Confirmed. Fixed in 03d3185, both halves as you suggested.
Proof it reproduces — the same schema parsed with and without the patternProperties:
WITH pattern: 2 entries (the root, and the `if` condition)
WITHOUT pattern: 4 entries (the root, the condition, and both `then` oneOf options)
The then branch's options are compiled only when nothing merges the property, exactly as you describe.
expandAllBranches is passed through now, and the reduce fans the parent out over the property's variants instead of taking [0]:
withMergedProperties = Object.keys(resolvedSchema.properties!).reduce(
(schemas: S[], key) =>
schemas.flatMap((schemaSoFar) => {
const matchingProperties = getMatchingPatternProperties(schemaSoFar, key);
if (Object.keys(matchingProperties).length === 0) {
return schemaSoFar;
}
return retrieveSchemaInternal<T, S, F>(
context,
{ allOf: [schemaSoFar.properties![key], ...Object.values(matchingProperties)] } as S,
rootSchema,
getByPath<T>(rawFormData, key),
expandAllBranches,
...
).map((mergedProperty) => ({
...schemaSoFar,
properties: { ...schemaSoFar.properties, [key]: mergedProperty },
}));
}),
[{ ...resolvedSchema, properties: { ...resolvedSchema.properties } } as S],
);stubExistingAdditionalProperties() moved inside a flatMap over the result for the same reason. With expandAllBranches false each inner call returns exactly one schema, so every form path is unchanged — the fan-out is paid only by the parser, and only for a property that a patternProperties entry matches.
Tests: should expand the branches of a property merged with the patternProperties that match it in retrieveSchemaTest.ts, which asserts both branches come back, and parses the conditional branches of a property its patternProperties also match in the parser tests. Both fail with expandAllBranches put back to undefined.
| // An option can hold an `allOf`, conditions or dependencies of its own, which only parsing the option resolves. | ||
| // The schema is parsed alongside its options rather than being taken as covered by them, since merging an option | ||
| // into the schema can replace one of the schema's own subschemas (a property's `oneOf`, say) | ||
| for (const option of resolveAnyOrOneOfSchemas<T, S, F>(context, localSchema, rootSchema, true)) { |
There was a problem hiding this comment.
Each option now runs retrieveSchemaInternal() again over the parent's properties, so the identity parsed set doesn't save anything when the parent has patternProperties
Each option is mergeSchemas(remaining, item), so it carries the parent's properties and patternProperties. parseSchema(option) runs the property/pattern merge again for every option. Each merge makes fresh property objects, so state.parsed (which checks identity) never matches them, and they get parsed once per option. Every merged option and property is also pushed onto recurseList, where findIndex(deepEquals) is O(n²) over large schemas.
Suggested fix: key recurseList/parsed by hashForSchema() in a Set<string>. That covers both content and identity, and turns the linear deepEquals scan into O(1) lookups.
There was a problem hiding this comment.
Confirmed, with one deviation on the key. Fixed in 03d3185.
Measured on a schema with 150 properties each holding a 3-option oneOf, a patternProperties entry and 20 top-level options:
| state | ms | entries |
|---|---|---|
identity Set + recurseList.findIndex(deepEquals) |
86 | 471 |
content-keyed Set<string> |
32 | 471 |
Same 471 sub-schemas, 2.7x faster, and all 13 schemaParser snapshots byte-identical.
The deviation: I key by sortedJSONStringify() rather than hashForSchema(). Same O(1) lookup — it is the string hashForSchema() hashes, and it compares as deepEquals() does here since both ignore symbol keys — but the 32-bit hash can collide, and a collision in a recursion guard is silent: the colliding schema is treated as already walked and its sub-schemas are never parsed, surfacing much later as No precompiled validator function was found. addSchema() at least throws on a hash collision. The cost is memory, one string per walked schema instead of a reference; happy to switch to the hash if you would rather have that.
Both sets are content-keyed now, which also means the parsed guard catches an option that carries the parent's property objects even after a merge has rebuilt them — the case your comment describes, which identity missed.
| * the hash of the schema to schema/sub-schema. | ||
| * | ||
| * @param rootSchema - The root schema to parse for sub-schemas used by `isValid()` calls | ||
| * @param [options={}] - The `SchemaParserOptions` to parse with; pass the same `customMergeAllOf` the form uses, so the |
There was a problem hiding this comment.
The schemaParser() API reference wasn't updated
packages/docs/docs/api-reference/utility-functions.md (### schemaParser) still lists only rootSchema under Parameters. It also still says the parser recurses "through properties and items". It doesn't mention the new options/customMergeAllOf parameter or the newly parsed patternProperties, additionalProperties, tuple items and additionalItems. The ajv8/ata references and the upgrade guide were updated; this page was missed.
There was a problem hiding this comment.
Confirmed — missed it. Fixed in 03d3185.
### schemaParser now lists the options parameter, and the recursion sentence names what it actually walks: properties, the patternProperties and additionalProperties a form renders the keys they describe with, and items including every tuple position and additionalItems. It also documents that a key the patternProperties match is parsed as the merge of every pattern matching it, and corrects "keyed by the hash" to the $id-or-hash the lookup actually uses.
The two validator API references had the same stale [parserOptions={}] bullet; both now describe the single CompileValidatorOptionsType<S> from the next comment.
| schema: S, | ||
| output: string, | ||
| options: CustomValidatorOptionsType = {}, | ||
| parserOptions: SchemaParserOptions<S> = {}, |
There was a problem hiding this comment.
parserOptions is a second trailing options bag, right after this changelog's breaking change that removed exactly that kind of call shape
The createPrecompiledValidator() entry above says its options were folded into one object "so a call supplying only the merge no longer has to pass two undefineds". Here, a caller who only wants the merge has to write compileSchemaValidators(schema, output, {}, { customMergeAllOf }) (and compileSchemaValidatorsCode(schema, {}, {...})). The same customMergeAllOf then has to be passed separately to createPrecompiledValidator(). A customMergeAllOf field on the existing options (or one options object shared by compile and create) would make the call simpler and keep the two calls from drifting apart.
There was a problem hiding this comment.
Fair, and the contradiction with the entry right above it is the part that settles it. Fixed in 03d3185.
Both packages gained a CompileValidatorOptionsType<S> that extends their CustomValidatorOptionsType with the customMergeAllOf, and compileSchemaValidators() / compileSchemaValidatorsCode() take that one bag:
-compileSchemaValidators(yourSchema, output, {}, { customMergeAllOf });
+compileSchemaValidators(yourSchema, output, { customMergeAllOf });I put it on a new type rather than on CustomValidatorOptionsType itself, since that one is also what customizeValidator() takes, where a customMergeAllOf means nothing — the regular validator gets it from the form's SchemaContext.
createPrecompiledValidator() still takes its own: one call runs in the compile script and the other in the app, so there is no one object to share. The docs now say to give the same customMergeAllOf to both, and the compile options' JSDoc says so at the field.
SchemaParserOptions stays as schemaParser()'s own parameter type; the validator packages just hold a customMergeAllOf and hand it over.
| addSchema(schema: S, hash: string) { | ||
| const key = schema[ID_KEY] ?? hash; | ||
| const ownId = schema[ID_KEY]; | ||
| const key = ownId ?? hash; |
There was a problem hiding this comment.
$id: '' gets a different key here than at runtime
Here key = ownId ?? hash, so $id: '' maps to the key '', and because ownId !== undefined, its variants also take the new drop-and-warn path. The precompiled validator's getValidator() uses schema[ID_KEY] || hashForSchema(schema), so at runtime it looks up the hash. The compiled function sits under '' and the lookup misses, giving No precompiled validator function was found. || here (and if (ownId)) would match the runtime lookup.
There was a problem hiding this comment.
Confirmed. Fixed in 03d3185 — it is || now, and the comment says why.
Proof, a oneOf option carrying $id: '', printing the key each schema is mapped under against the key a validator would look it up by:
before: key="" runtimeKey="-258051b4"
after: key="511cd14b" runtimeKey="511cd14b"
Every key matches its runtime lookup now. The if (ownId !== undefined) branch you mention is gone entirely with the drop-and-warn, so there is no second place for '' to take the wrong path.
Test: calling isValid() with an empty $id maps the schema under its hash, as a validator looks it up, which fails with ?? put back.
| - Fixed `schemaParser()` skipping the sub-schemas that only a `oneOf`/`anyOf` option has: the `properties` and `items` of the option itself, and anything its own `allOf`, conditions or dependencies resolve to, since an option's `properties` were read off directly rather than the option being parsed. Each option is now parsed in full, and the parent schema's own properties are no longer parsed again for every option ([#5338](https://github.com/rjsf-team/react-jsonschema-form/issues/5338)) | ||
| - Fixed `schemaParser()` parsing only an `items` that is a single schema, leaving the positions of a tuple `items` and the `additionalItems` schema unparsed even though `ArrayField` renders them ([#5338](https://github.com/rjsf-team/react-jsonschema-form/issues/5338)) | ||
| - `schemaParser()` keeps the first of two schemas that share an `$id` and differ, warning once, rather than throwing `Two different schemas exist with the same key`. A precompiled validator function is looked up by `$id` when there is one, so the first schema compiled under it answers for every variant of it the resolution produces ([#5338](https://github.com/rjsf-team/react-jsonschema-form/issues/5338)) | ||
| - Fixed `schemaParser()` leaving the `additionalProperties` and `patternProperties` schemas unparsed. A key they describe is only stubbed into `properties` once the form data has it, which a parse has none of, so a `oneOf` under either threw `No precompiled validator function was found for the given schema` as soon as a user added such a key. The `additionalItems` of an `items` that is not a tuple is no longer parsed, since `ArrayField` renders nothing from one ([#5338](https://github.com/rjsf-team/react-jsonschema-form/issues/5338)) |
There was a problem hiding this comment.
The changelog says something that was never true
"The additionalItems of an items that is not a tuple is no longer parsed". The old parser never parsed additionalItems at all. It only followed a non-array, non-boolean items. So no behaviour was removed, and the sentence (and the matching test name) implies a change that didn't happen. Suggest dropping the sentence.
There was a problem hiding this comment.
Confirmed and dropped. Fixed in 03d3185.
The old parser, at bb96e6ab5:
if (ITEMS_KEY in s && !Array.isArray(s.items) && typeof s.items !== 'boolean') {
parseSchema<T, S, F>(context, recurseList, rootSchema, s.items as S);
}additionalItems is never read, so nothing was removed and the sentence is gone.
I kept the test. Its name — parses no additionalItems of an items that is not a tuple, as a form renders none — states the behavior rather than claiming a change, and it locks the else if (ITEMS_KEY in schema) branch so a later "parse everything" edit cannot quietly start compiling a sub-schema ArrayField renders nothing from. Happy to rename it if it still reads as a changelog claim to you.
| for (const valueSchema of valueSchemas) { | ||
| // A boolean or missing sub-schema describes no value of its own, so there is nothing in it to parse | ||
| if (isObject(valueSchema)) { | ||
| parseSchema<T, S, F>(context, state, rootSchema, valueSchema as S); |
There was a problem hiding this comment.
Avoidable type casts in new code
valueSchema as S here, and allOf as RJSFSchema[], mergeSchemas(acc, s) as RJSFSchema and o as RJSFSchema in test/testUtils/customMergeAllOfData.ts and schemaParser.test.ts. These casts are avoidable. Typing valueSchemas as (S | boolean | undefined)[] and narrowing with a type-guarding isObject (or typeof v === 'object') would remove the cast.
There was a problem hiding this comment.
Fixed in 03d3185, all but one.
parseValueSchemas() narrows with a local type guard, as you suggested, so there is no cast at the call:
function isSchemaObject<S extends StrictRJSFSchema = RJSFSchema>(valueSchema: unknown): valueSchema is S {
return isObject(valueSchema);
}isObject() on its own narrows to GenericObjectType, which is what forced the as S; this narrows to the schema the parser walks and carries the "a boolean or missing sub-schema describes no value of its own" rationale in one place. Pushing { allOf: patterns } through the same unknown[] means that one needs no cast either.
In the fixture and the test, allOf as RJSFSchema[] and o as RJSFSchema are gone — const { allOf = [], ... } plus isObject narrowing on the members, which also makes the boolean-subschema case explicit instead of implicitly casting it away.
The one I left is mergeSchemas(acc, s) as RJSFSchema. mergeSchemas() is declared (obj1: GenericObjectType, obj2: GenericObjectType) and returns the accumulator, so the cast is the signature's rather than the call's — retrieveSchema.ts casts it the same way (mergeSchemas(remaining, item) as S). Making it generic would be a nice cleanup but it is a public API and the comment inside it says the wider type causes "a bunch of type errors downstream", so not in this PR.
Addresses the review of #5371. A form always merges an `allOf`, so the sub-schemas it validates against are the merged schema's. `expandAllBranches` returned the unmerged branches instead, which hold neither the sub-schemas a `customMergeAllOf` rewrote nor the ones that only exist once the merge has brought a property together with the `patternProperties` matching it. Gating a second, merge-aware path on `customMergeAllOf`, as this branch did, left the second case broken whenever no custom merge was passed, and re-resolved the merged schema in a way the form does not, which is what made the recursion guard necessary. Expanding all branches now merges the `allOf`, and drops one it cannot merge, through the same code the form uses, so the two cannot diverge. The branch variants were also what `resolveSchema()`'s permutation of the `allOf` members multiplied: a root `allOf` of six three-item `allOf`s went from 15,631 merges in 60.2s to 7 in under a millisecond. Every `schemaParser()` snapshot is unchanged, so the branches were contributing nothing to the parsed map. Also in the parser: - Each `oneOf`/`anyOf` option is parsed rather than having its `properties` read off, so an option whose sub-schemas come from its own `allOf`, conditions or dependencies is reached. A parsed set held by identity keeps a property the options share from being resolved once per option. - The `patternProperties` and `additionalProperties` schemas are parsed. A key they describe is only stubbed into `properties` once the form data has it, which a parse has none of, so nothing reached them. - Every position of a tuple `items` is parsed, with `additionalItems` only for a tuple, matching what `ArrayField` renders. - Two schemas that share an `$id` and differ no longer fail the parse. The first is compiled and answers for the rest, which is how both the precompiled and the regular validators look a schema up. `SchemaParserOptions` is a `Pick` of `SchemaContext` rather than an interface `SchemaContext` extends, leaving the context type as v7 has it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
18639b9 to
3ddc356
Compare
|
Thanks @jimmycallin — second round answered in 03d3185, pushed on top of the two commits you reviewed so the diff against what you read is clean. All nine reproduce; all nine are fixed.
The one that mattered most is #1. Removing the fix makes the new end-to-end test in both validator packages throw #2 made the PR smaller. You were right that the fix belongs where the variants are built, not where the parse gives up on them. Two places I diverged, both argued in the inline replies:
Also surfaced, not fixed here: Every fix is mutation-checked individually. |
|
Thanks @jimmycallin — this round was the most useful yet. All 15 reproduce; six were real bugs, three of them mine from the last round.
Mutation evidence, each with the hash you quoted:
Two corrections to the findings, both narrow: #9, the ata half. #2, one test I inverted rather than kept. Two things I'd rather you decided than me, both on the cap:
And one new pre-existing defect, now in "Not fixed here": a Locally green: |
Addresses the review of #5371. A form always merges an `allOf`, so the sub-schemas it validates against are the merged schema's. `expandAllBranches` returned the unmerged branches instead, which hold neither the sub-schemas a `customMergeAllOf` rewrote nor the ones that only exist once the merge has brought a property together with the `patternProperties` matching it. Gating a second, merge-aware path on `customMergeAllOf`, as this branch did, left the second case broken whenever no custom merge was passed, and re-resolved the merged schema in a way the form does not, which is what made the recursion guard necessary. Expanding all branches now merges the `allOf`, and drops one it cannot merge, through the same code the form uses, so the two cannot diverge. The branch variants were also what `resolveSchema()`'s permutation of the `allOf` members multiplied: a root `allOf` of six three-item `allOf`s went from 15,631 merges in 60.2s to 7 in under a millisecond. Every `schemaParser()` snapshot is unchanged, so the branches were contributing nothing to the parsed map. Also in the parser: - Each `oneOf`/`anyOf` option is parsed rather than having its `properties` read off, so an option whose sub-schemas come from its own `allOf`, conditions or dependencies is reached. A parsed set held by identity keeps a property the options share from being resolved once per option. - The `patternProperties` and `additionalProperties` schemas are parsed. A key they describe is only stubbed into `properties` once the form data has it, which a parse has none of, so nothing reached them. - Every position of a tuple `items` is parsed, with `additionalItems` only for a tuple, matching what `ArrayField` renders. - Two schemas that share an `$id` and differ no longer fail the parse. The first is compiled and answers for the rest, which is how both the precompiled and the regular validators look a schema up. `SchemaParserOptions` is a `Pick` of `SchemaContext` rather than an interface `SchemaContext` extends, leaving the context type as v7 has it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9523ec5 to
0cbab37
Compare
Addresses the review of #5371. A form always merges an `allOf`, so the sub-schemas it validates against are the merged schema's. `expandAllBranches` returned the unmerged branches instead, which hold neither the sub-schemas a `customMergeAllOf` rewrote nor the ones that only exist once the merge has brought a property together with the `patternProperties` matching it. Gating a second, merge-aware path on `customMergeAllOf`, as this branch did, left the second case broken whenever no custom merge was passed, and re-resolved the merged schema in a way the form does not, which is what made the recursion guard necessary. Expanding all branches now merges the `allOf`, and drops one it cannot merge, through the same code the form uses, so the two cannot diverge. The branch variants were also what `resolveSchema()`'s permutation of the `allOf` members multiplied: a root `allOf` of six three-item `allOf`s went from 15,631 merges in 60.2s to 7 in under a millisecond. Every `schemaParser()` snapshot is unchanged, so the branches were contributing nothing to the parsed map. Also in the parser: - Each `oneOf`/`anyOf` option is parsed rather than having its `properties` read off, so an option whose sub-schemas come from its own `allOf`, conditions or dependencies is reached. A parsed set held by identity keeps a property the options share from being resolved once per option. - The `patternProperties` and `additionalProperties` schemas are parsed. A key they describe is only stubbed into `properties` once the form data has it, which a parse has none of, so nothing reached them. - Every position of a tuple `items` is parsed, with `additionalItems` only for a tuple, matching what `ArrayField` renders. - Two schemas that share an `$id` and differ no longer fail the parse. The first is compiled and answers for the rest, which is how both the precompiled and the regular validators look a schema up. `SchemaParserOptions` is a `Pick` of `SchemaContext` rather than an interface `SchemaContext` extends, leaving the context type as v7 has it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0cbab37 to
dbc5e59
Compare
Addresses the review of #5371. A form always merges an `allOf`, so the sub-schemas it validates against are the merged schema's. `expandAllBranches` returned the unmerged branches instead, which hold neither the sub-schemas a `customMergeAllOf` rewrote nor the ones that only exist once the merge has brought a property together with the `patternProperties` matching it. Gating a second, merge-aware path on `customMergeAllOf`, as this branch did, left the second case broken whenever no custom merge was passed, and re-resolved the merged schema in a way the form does not, which is what made the recursion guard necessary. Expanding all branches now merges the `allOf`, and drops one it cannot merge, through the same code the form uses, so the two cannot diverge. The branch variants were also what `resolveSchema()`'s permutation of the `allOf` members multiplied: a root `allOf` of six three-item `allOf`s went from 15,631 merges in 60.2s to 7 in under a millisecond. Every `schemaParser()` snapshot is unchanged, so the branches were contributing nothing to the parsed map. Also in the parser: - Each `oneOf`/`anyOf` option is parsed rather than having its `properties` read off, so an option whose sub-schemas come from its own `allOf`, conditions or dependencies is reached. A parsed set held by identity keeps a property the options share from being resolved once per option. - The `patternProperties` and `additionalProperties` schemas are parsed. A key they describe is only stubbed into `properties` once the form data has it, which a parse has none of, so nothing reached them. - Every position of a tuple `items` is parsed, with `additionalItems` only for a tuple, matching what `ArrayField` renders. - Two schemas that share an `$id` and differ no longer fail the parse. The first is compiled and answers for the rest, which is how both the precompiled and the regular validators look a schema up. `SchemaParserOptions` is a `Pick` of `SchemaContext` rather than an interface `SchemaContext` extends, leaving the context type as v7 has it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
16e1e41 to
18c89b8
Compare
|
Follow-up in A root schema with ajv8 and ata both fixed, with a regression test in each that fails on the old code. The retrieved-option scoring shared one Dropped the Two more that reproduce identically at the base commit, so not from this PR, but worth recording:
Happy to open issues for those two alongside the Still green locally: |
Addresses the review of #5371. A form always merges an `allOf`, so the sub-schemas it validates against are the merged schema's. `expandAllBranches` returned the unmerged branches instead, which hold neither the sub-schemas a `customMergeAllOf` rewrote nor the ones that only exist once the merge has brought a property together with the `patternProperties` matching it. Gating a second, merge-aware path on `customMergeAllOf`, as this branch did, left the second case broken whenever no custom merge was passed, and re-resolved the merged schema in a way the form does not, which is what made the recursion guard necessary. Expanding all branches now merges the `allOf`, and drops one it cannot merge, through the same code the form uses, so the two cannot diverge. The branch variants were also what `resolveSchema()`'s permutation of the `allOf` members multiplied: a root `allOf` of six three-item `allOf`s went from 15,631 merges in 60.2s to 7 in under a millisecond. Every `schemaParser()` snapshot is unchanged, so the branches were contributing nothing to the parsed map. Also in the parser: - Each `oneOf`/`anyOf` option is parsed rather than having its `properties` read off, so an option whose sub-schemas come from its own `allOf`, conditions or dependencies is reached. A parsed set held by identity keeps a property the options share from being resolved once per option. - The `patternProperties` and `additionalProperties` schemas are parsed. A key they describe is only stubbed into `properties` once the form data has it, which a parse has none of, so nothing reached them. - Every position of a tuple `items` is parsed, with `additionalItems` only for a tuple, matching what `ArrayField` renders. - Two schemas that share an `$id` and differ no longer fail the parse. The first is compiled and answers for the rest, which is how both the precompiled and the regular validators look a schema up. `SchemaParserOptions` is a `Pick` of `SchemaContext` rather than an interface `SchemaContext` extends, leaving the context type as v7 has it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
330a8eb to
c16e1a7
Compare
heath-freenome
changed the title
| return; | ||
| } | ||
| state.parsed.add(parsedKey); | ||
| const { [ALL_OF_KEY]: allOf, ...withoutAllOf } = schema; |
There was a problem hiding this comment.
An allOf reached through if/then or dependencies loses its unmerged entries (regression vs 9af375731)
The unmerged allOf walk only runs when the allOf sits on the schema handed to parseSchema(). Now that retrieveSchemaInternal() merges even when expanding all branches, an allOf that only appears after resolution (a then/else, a dependency) is merged inside it, and the parser never sees the entries. A merge that leaves them in place (your identityMergeAllOf) then throws at runtime. Before this PR, the expanded-branch return covered that case.
const c = { customMergeAllOf: identityMergeAllOf };
const viaCond = { type: 'object', properties: { u: { if: { required: ['zz'] }, then: SCHEMA_UNMERGED_ALL_OF, else: SCHEMA_UNMERGED_ALL_OF } } };
omitExtraData({ validator: precompiled(viaCond, c), ...c }, viaCond, viaCond, { u: UNMERGED_ALL_OF_FORM_DATA });
const viaDep = { type: 'object', properties: { t: { type: 'string' } }, dependencies: { t: SCHEMA_UNMERGED_ALL_OF } };
createSchemaUtils({ validator: precompiled(viaDep, c), ...c }, viaDep).omitExtraData(viaDep, { t: 'x', ...UNMERGED_ALL_OF_FORM_DATA }); 9af375731 c16e1a7ba
if/then ok THREW No precompiled validator function ... for "-4fca741c"
dependencies ok THREW ... for "-4fca741c"
Suggested fix: also walk the allOf of each localSchema that retrieveSchemaInternal() returns (for example, parseSchema() each entry when ALL_OF_KEY in localSchema), rather than only the input's. That covers every way of reaching an allOf, and state.parsed already stops repeats.
There was a problem hiding this comment.
Confirmed and fixed. Both variants threw -4fca741c at c16e1a7ba, exactly as you have them.
Taken as suggested, with one adjustment: the walk runs on each localSchema that retrieveSchemaInternal() returns as well as on the schema handed in, not instead of it. Your ALL_OF_KEY in localSchema check only fires for a merge that leaves the entries in place — identityMergeAllOf here — and with the default merge the resolved schema has no allOf at all while the declared one does, so dropping the input-level walk loses the case the existing parses the entries of an allOf alongside the merge test covers. Both now go through one parseUnmergedAllOf(), and state.parsed stops the repeats as you said it would.
New test: parses the entries of an allOf only reached through a condition or a dependency, which runs both your shapes. Removing the localSchema call fails it.
| const retrievedOptions = anyOrOneOf.flatMap((item) => | ||
| // Each option gets its own copy of the list, since a `$ref` one of them resolves is not one a sibling has | ||
| // already been through, the same reason the `properties` loop of `resolveAllReferences()` copies it | ||
| retrieveSchemaInternal<T, S, F>(context, item, rootSchema, formData, true, [...recurseList]), |
There was a problem hiding this comment.
A retrieved option keeps the declared option's $id but not its content, so ParserValidator rejects it (regression for ata)
retrievedOptions are passed to getFirstMatchingOption(). An option without properties goes through the plain isValid(option) branch with its $id unchanged. If retrieval changes the option (merges its allOf, or picks an if branch), ParserValidator.addSchema() sees two different schemas under one key and throws.
{ type: 'object', properties: { v: { oneOf: [{ $id: 'str', allOf: [{ type: 'string' }, { minLength: 1 }] }, { type: 'number' }] } } }
{ type: 'object', properties: { v: { oneOf: [{ $id: 'str', type: 'string', if: { minLength: 3 }, then: { maxLength: 9 }, else: { pattern: 'a' } }, { type: 'number' }] } } } 9af375731 c16e1a7ba
schemaParser() ok THREW Two different schemas exist with the same key str!
ata compile...Code() ok THREW (same)
ajv8 already fails these at compile with schema with key or id already exists on both commits, so for ajv8 this isn't new. ata compiled them before this PR.
Suggested fix: derive the $id for retrieved options too. Run withVariantId() on a retrieved option that isn't deep-equal to the declared one, or apply it in the isValid(option) branch of getFirstMatchingOption(). That's the same reasoning withVariantId() already uses for the augmented and relaxed variants.
There was a problem hiding this comment.
Confirmed and fixed your way. Both shapes threw Two different schemas exist with the same key str! at c16e1a7ba.
withVariantId() is now applied in the isValid(option) branch of getFirstMatchingOption(), so the plain branch derives an $id the way the augmented one already did, with the junk option still carved out.
That surfaced a second-order problem worth recording: relaxOptionsForScoring() already derives one, so scoring a relaxed option derived a variant of a variant and produced bare?rjsf=99e78ad?rjsf=99e78ad. Consistent between the parse and the runtime, since both go through this function, but nonsense. withVariantId() now replaces a suffix it has already added rather than appending, so it is idempotent and the two paths name one schema.
This changed an existing test. parses the relaxed variant of an $id option that has no properties under a key of its own asserted schemaMap.bare exists, on the premise your comment overturns — that an option with no properties is scored as it stands so its own $id names it. It now asserts both the strict and the relaxed forms are keyed bare?rjsf=… and that the bare key is gone.
| getFirstMatchingOption<T, S, F>(context, formData, relaxed, rootSchema, discriminator); | ||
| // `MultiSchemaField` scores the options it has retrieved rather than the ones the schema declares, so an option | ||
| // that resolves into something else -- one that is an `allOf`, say -- is scored in that resolved form too | ||
| const retrievedOptions = anyOrOneOf.flatMap((item) => |
There was a problem hiding this comment.
Retrieved options are scored with {} form data, so an option with additionalProperties still throws once the user adds a key (pre-existing, but this block claims to cover it)
MultiSchemaField retrieves each option with the real form data. stubExistingAdditionalProperties() then adds every additional key to properties, and getFirstMatchingOption() adds that key to the requiresAnyOf augmentation. So the hash (or, with $id, the ?rjsf= suffix) depends on what keys the user has, and a parse that has no form data can't enumerate them.
const rootSchema = { type: 'object', properties: { pet: { oneOf: [
{ type: 'object', properties: { meow: { type: 'string' } }, additionalProperties: { type: 'string' } },
{ type: 'object', properties: { bark: { type: 'string' } } },
] } } };
const formData = { meow: 'x', extra: 'y' };
schemaUtils.getClosestMatchingOption(formData, options.map((o) => schemaUtils.retrieveSchema(o, formData)), 0);
// THREW No precompiled validator function was found for the given schema for "9461a84"
// with `$id: 'cat'` on the first option: ... for "cat?rjsf=9461a84"This throws the same way on 9af375731, so it isn't a regression. But the comment here says retrieved options are covered, and they aren't for this shape. Either narrow the comment and the changelog bullet, or open a follow-up issue. Scoring options with stubbed additional keys can't be precompiled without leaving the stubbed keys out of the augmentation.
There was a problem hiding this comment.
Confirmed, pre-existing, and not fixed — the comment and the changelog are narrowed instead, which is the first of the two options you offered.
I could not see a way to precompile it. The stubbed keys are in the option's own properties by the time it is scored, so the schema itself varies with the user's data, not just the augmentation; leaving the stubs out of the anyOf would not change the hash. A parse has no form data and cannot enumerate them.
The code comment now ends:
It retrieves with the real form data, which this has none of, so an option whose retrieved form depends on the data is still reached only in the forms an empty retrieval produces: a key an option's
additionalPropertiesdescribes is stubbed into itspropertiesonce the user adds one, and no parse can enumerate those.
and the changelog bullet carries the same caveat. Happy to open the follow-up issue — it belongs with the other four still outstanding.
| context, | ||
| { allOf: [properties[key], ...Object.values(matchingProperties)] } as S, | ||
| rootSchema, | ||
| getByPath<T>(rawFormData, key), |
There was a problem hiding this comment.
recurseList resets to [] here, so a recursive schema with patternProperties overflows the stack (pre-existing; this call was rewritten in the PR)
const schema = {
definitions: { node: { type: 'object', properties: { child: { $ref: '#/definitions/node' } }, patternProperties: { '^c': { type: 'object' } } } },
$ref: '#/definitions/node',
};
schemaParser(schema); // Maximum call stack size exceeded
retrieveSchema({ validator }, schema, schema, {}); // Maximum call stack size exceededchild matches ^c. The { allOf: [child, pattern] } resolves the $ref back to node, which has child and the pattern again, and the undefined here starts every level with an empty recursion list. It fails the same way on 9af375731, and the form hits it as well as the parser. This line was rewritten anyway, so passing recurseList through costs nothing.
There was a problem hiding this comment.
Confirmed — overflowed in both schemaParser() and retrieveSchema() at c16e1a7ba, and at the base.
Passing recurseList through does not work, and it took two tries to find out why. Worth writing down, because the trap is not visible from this line.
resolveAllReferences() gives each property its own [...recurseList] (line 402) and then merges every child's list back into the caller's (lines 417–421). resolveSchema() then re-enters retrieveSchemaInternal() with that same array. So by the time a property is merged with its matching patterns, the list holds what its siblings resolved, and a copy taken here copies the contamination:
{ type: 'object',
definitions: { x: { type: 'string' }, y: { maxLength: 7 } },
properties: { aa: { $ref: '#/definitions/x' }, other: { $ref: '#/definitions/y' } },
patternProperties: { '^a': { $ref: '#/definitions/y' } } }| seed | properties.aa |
|---|---|
| v7 (fresh list) | { type: 'string', maxLength: 7 } |
recurseList |
{ type: 'string', $ref: '#/definitions/y' } |
[...recurseList] |
{ type: 'string', $ref: '#/definitions/y' } |
| shipped | { type: 'string', maxLength: 7 } |
other resolves #/definitions/y first, so aa's pattern reads as a cycle and a literal $ref reaches SchemaField and customMergeAllOf with the constraint dropped. That is a rendering regression, strictly worse than the overflow it was fixing.
What shipped seeds the merge with the one reference this schema was itself reached through — its RJSF_REF_KEY — and a fresh copy of it per key, nothing from recurseList:
| case | v7 | shipped |
|---|---|---|
sibling-resolved $ref in a pattern |
correct | correct |
self-recursive $ref under a matched key |
overflow | ok |
mutual recursion (a.cb → b, b.ca → a, both ^c) |
overflow | overflow |
No case is worse than v7 and the one you reported is fixed. Mutual recursion still overflows, identically at the base — I'd rather that went in the issue alongside the recursive-$ref-in-oneOf overflow than get a second seeding rule here.
Three tests, one per property of the fix: resolves a $ref in a patternProperties entry that a sibling property resolved first, …for every key it matches, not just the first (the per-key copy — the nested call mutates the seed), and parses a recursive $ref under a key its patternProperties match. Each of the three seeds above fails a different one.
| /** The options a schema is compiled into precompiled validator functions with: everything `customizeValidator()` takes, | ||
| * plus the form's `customMergeAllOf`, since the sub-schemas that get compiled are the ones that merge produces | ||
| */ | ||
| export interface CompileValidatorOptionsType< |
There was a problem hiding this comment.
CompileValidatorOptionsType is defined twice and repeats SchemaParserOptions
This interface is copied word for word into validator-ata/src/types.ts:63, and its one added field is the SchemaParserOptions<S> this PR adds to @rjsf/utils. Using type CompileValidatorOptionsType<S> = CustomValidatorOptionsType & SchemaParserOptions<S> in both packages, or one shared definition, keeps the doc comment and the field in one place. Otherwise the next option added to SchemaParserOptions has to be mirrored by hand in two validators.
There was a problem hiding this comment.
Correct, done. Both packages now have
export type CompileValidatorOptionsType<S extends StrictRJSFSchema = RJSFSchema> = CustomValidatorOptionsType &
SchemaParserOptions<S>;so the field and its doc comment live in SchemaParserOptions only, and the next option added there needs no mirroring. The changelog bullet in each package says so rather than carrying a separate bullet for the type's shape.
| const key = schema[ID_KEY] ?? hash; | ||
| // An empty `$id` names nothing, and a validator looks a schema up by `schema[ID_KEY] || hashForSchema(schema)`, so | ||
| // one is mapped under its hash here too or the lookup finds nothing compiled | ||
| const key = schema[ID_KEY] || hash; |
There was a problem hiding this comment.
The "$id or hash" key rule is now restated in 8 places across 4 packages
The same rule now lives as schema[ID_KEY] || hashForSchema(schema) plus a near-identical comment in ajv8 validator.ts:263 and precompiledValidator.ts:95, ata validator.ts:141/235 and precompiledValidator.ts:96, cfworker validator.ts:144/227, and the comment here. The bug this round fixed came from those copies drifting (?? in some, nothing in others). A schemaKey(schema) helper exported from @rjsf/utils, next to hashForSchema, would enforce what this comment only describes, and the copied comments could go.
There was a problem hiding this comment.
Correct, done — this is the one I'd have regretted skipping, since you are right that the drift is where the ?? family came from.
schemaKey() is a new @rjsf/utils export next to hashForSchema:
export function schemaKey<S extends StrictRJSFSchema = RJSFSchema>(schema: S): string {
return schema[ID_KEY] || hashForSchema<S>(schema);
}All seven code sites call it — ajv8's two, ata's three, cfworker's two — and the copied comments are gone with them. ParserValidator.addSchema() is the eighth: it took a precomputed hash only to apply the same rule, so it now takes addSchema(schema) and calls schemaKey() itself. The class is not exported from the package index and nothing outside src/parser/ calls it, so I treated that as internal; say if you'd rather keep the signature.
The doc comment states the rule your comment described — same $id ⇒ same validation, and a derivation that changes meaning derives its own — and points at withVariantId(), which is the other half you asked for in the third pass. It's in the API reference and the v7 upgrade guide too.
ajv8's rawValidation() is deliberately not on the list: it keys AJV's own cache by $id or object reference, which is a different mechanism.
| const retrievedOptions = anyOrOneOf.flatMap((item) => | ||
| // Each option gets its own copy of the list, since a `$ref` one of them resolves is not one a sibling has | ||
| // already been through, the same reason the `properties` loop of `resolveAllReferences()` copies it | ||
| retrieveSchemaInternal<T, S, F>(context, item, rootSchema, formData, true, [...recurseList]), |
There was a problem hiding this comment.
An option with a schema dependencies is only scored with the dependency applied, so data without the key still throws
retrieveSchemaInternal(item, …, true) goes through processDependencies(), which applies every dependency when expanding (expandAllBranches || getByPath(formData, dependencyKey) !== undefined, retrieveSchema.ts:999) and never returns the option with the dependency left out. MultiSchemaField retrieves the option with the real data, so when the key is absent it scores the option with dependencies dropped and nothing merged. That schema doesn't match the hash of the declared option (which still carries dependencies) or of the expanded one (which carries c).
const schema = {
oneOf: [
{ type: 'object', properties: { a: { type: 'string' } }, dependencies: { a: { properties: { c: { type: 'number' } } } } },
{ type: 'object', properties: { d: { type: 'string' } } },
],
};
// formData { d: 'x' }: options.map((o) => retrieveSchema(o, formData)), then getClosestMatchingOption(formData, retrieved, 0)schemaParser(schema) doesn't contain this, so the precompiled validator throws No precompiled validator function was found while rendering:
{"type":"object","properties":{"a":{"type":"string"}},"anyOf":[{"required":["a"]}]}
On 9af3757 the same schema misses this one and the applied variant, so it isn't a regression. It is the case this block is meant to cover, though. When expanding, processDependencies() could return the schema without the dependency next to the applied one. That would cover this without a second retrieval here.
There was a problem hiding this comment.
Confirmed and fixed, but not in processDependencies() — that shape is unaffordable.
Returning the unapplied variant next to the applied one multiplies per key, since each key branches into both states and the next key branches off both:
| dependency keys | schemaParser() |
map entries |
|---|---|---|
| 8 | 7 ms | 1 |
| 12 | 106 ms | 1 |
| 16 | 2147 ms | 1 |
One entry at every size. The 2 ** k intermediate merges reach no isValid() call at all — a oneOf in the base schema augments from its own properties, so which dependencies merged in doesn't change it, and a oneOf inside a dependency value is now reached directly (your next comment). It is pure cost.
So resolveDependencies() adds the all-unapplied variant once, after processDependencies() has returned, and only while expanding:
if (!expandAllBranches || applied.some((appliedSchema) => deepEquals(appliedSchema, resolvedSchema))) {
return applied;
}
return [...applied, resolvedSchema];16 keys: 1 ms. That covers your repro and the common case — a form before the user has filled any dependency key in. The subsets in between are the 2 ** k and are not covered; the comment says so rather than implying they are.
New test: parses an option with its dependency left unapplied, as a form scores it before the key is filled in. It asserts the exact schema, because objectContaining matched the declared option through its leftover dependencies key and passed under mutation — my mistake, caught on the mutation check.
| const { [ALL_OF_KEY]: allOf, ...withoutAllOf } = schema; | ||
| if (allOf) { | ||
| // A form does not validate only against the merge of an `allOf`: `getObjectDefaults()` reads a nested object's | ||
| // unmerged `properties`, and `omitExtraData()` reads the entries a merge leaves in place. So the entries and what |
There was a problem hiding this comment.
omitExtraData()'s scoring of a dependency's oneOf is still never parsed (pre-existing)
This walk is justified by what omitExtraData() reads. But handleDependencies() → handleOneOf() (omitExtraData.ts:318 / 252) also scores a dependency's oneOf options through getClosestMatchingOption(), which validates each one augmented. The parser reaches those options only through withExactlyOneSubschema(). That validates the { type: 'object', properties: { [key]: … } } condition schemas, never the options themselves.
The playground's schemaDependencies sample with its own formData misses all three:
{"properties":{"Do you have any pets?":{"enum":["No"]}},"anyOf":[{"required":["Do you have any pets?"]}]}
{"properties":{"Do you have any pets?":{"enum":["Yes: One"]},"How old is your pet?":{"type":"number"}},"anyOf":[…]}
{"properties":{"Do you have any pets?":{"enum":["Yes: More than one"]},"Do you want to get rid of any?":{"type":"boolean"}},"anyOf":[…]}
So a precompiled form with omitExtraData throws on submit. It's the same on 9af3757, so it's not from this PR and probably belongs in a follow-up issue. I'm noting it because this comment and the changelog read as though omitExtraData() is now covered.
There was a problem hiding this comment.
Confirmed and fixed. Your playground repro missed all three options at c16e1a7ba and omitExtraData() threw -5c27b707 on submit.
parseSchema() now parses each schema dependencies value in its own right, before resolution gets to it:
// `omitExtraData()` applies a schema dependency by walking the dependency's own schema and scoring any `oneOf` it
// declares, where resolution only ever validates the conditions `withExactlyOneSubschema()` builds out of it
for (const dependencyValue of Object.values(schema[DEPENDENCIES_KEY] ?? {})) {That routes the dependency's oneOf through resolveAnyOrOneOfSchemas()'s expand branch, which is the same augmented and relaxed scoring handleOneOf() does, so all three options are collected.
This is the one change that moves snapshots: two of the thirteen grow, by exactly those augmented options. The snapshot diff for the whole PR is insertions only (154 0) — the other eleven are byte-identical and nothing left a compiled map.
Tests: parses the options of a dependency's oneOf, which omitExtraData() scores in @rjsf/utils, and an end-to-end omitExtraData() through a precompiled validator in both validator packages.
| ); | ||
| const schemaUtils = createSchemaUtils({ validator }, rootSchema); | ||
| const formData = { meow: 'x' }; | ||
| const options = (rootSchema.properties!.pet as RJSFSchema).oneOf as RJSFSchema[]; |
There was a problem hiding this comment.
! and as are back in the new tests
(rootSchema.properties!.pet as RJSFSchema).oneOf as RJSFSchema[] shows up here, in validator-ata/test/compileSchemaValidatorsCode.test.ts:237 and in validator-ajv8/test/validator.test.ts:271 (.pick). The fixtures live in parsedSchemaData.ts, so exporting each option list as an RJSFSchema[] and building the root schema from it would remove all three, the same way the earlier test casts were removed.
There was a problem hiding this comment.
Correct, done, and the fixtures route you suggested is what I took.
ONE_OF_ALL_OF_REF_OPTIONS is exported from parsedSchemaData.ts as an RJSFSchema[] and SCHEMA_ONE_OF_ALL_OF_REF is built from it, so both compileSchemaValidatorsCode.test.ts sites just map over the export. The third, in validator-ajv8/test/validator.test.ts, has a schema local to the file, so the same shape applies locally: a const pickOptions: RJSFSchema[] the root schema is built from.
No ! or as left in any of the three. grep -rn "properties!\." packages/validator-*/test is empty.
…llOf A precompiled validator only has functions for the sub-schemas schemaParser() collected, keyed by their hash. The parser had no way to receive a customMergeAllOf, so a form whose merge produced different sub-schemas threw "No precompiled validator function was found for the given schema". schemaParser() takes an optional SchemaParserOptions, holding the customMergeAllOf, and parses with a SchemaContext built from it. SchemaContext now extends SchemaParserOptions. compileSchemaValidators() and compileSchemaValidatorsCode() in @rjsf/validator-ajv8 and @rjsf/validator-ata take the same options as a trailing argument. When given a customMergeAllOf, retrieveSchemaInternal()'s expand-all path also returns each allOf merged with it and resolved the rest of the way, so the parser collects the sub-schemas the form validates against. Also fixed while here: - schemaParser() took a schema as covered by the options it resolves to, so a property that every oneOf/anyOf option redefines was parsed only as each option spells it, never as the schema's own. - Upgraded ata-validator to ^1.32.0. The ^1.23.0 v7 pins resolves to standalone bundles that throw "_re1 is not defined" for a schema with patternProperties, so the precompiled ata validator could not validate one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> # Conflicts: # CHANGELOG-v7.md # Conflicts: # CHANGELOG-v7.md # Conflicts: # packages/validator-ata/package.json # pnpm-lock.yaml # Conflicts: # CHANGELOG-v7.md # Conflicts: # CHANGELOG-v7.md
Addresses the review of #5371. A form always merges an `allOf`, so the sub-schemas it validates against are the merged schema's. `expandAllBranches` returned the unmerged branches instead, which hold neither the sub-schemas a `customMergeAllOf` rewrote nor the ones that only exist once the merge has brought a property together with the `patternProperties` matching it. Gating a second, merge-aware path on `customMergeAllOf`, as this branch did, left the second case broken whenever no custom merge was passed, and re-resolved the merged schema in a way the form does not, which is what made the recursion guard necessary. Expanding all branches now merges the `allOf`, and drops one it cannot merge, through the same code the form uses, so the two cannot diverge. The branch variants were also what `resolveSchema()`'s permutation of the `allOf` members multiplied: a root `allOf` of six three-item `allOf`s went from 15,631 merges in 60.2s to 7 in under a millisecond. Every `schemaParser()` snapshot is unchanged, so the branches were contributing nothing to the parsed map. Also in the parser: - Each `oneOf`/`anyOf` option is parsed rather than having its `properties` read off, so an option whose sub-schemas come from its own `allOf`, conditions or dependencies is reached. A parsed set held by identity keeps a property the options share from being resolved once per option. - The `patternProperties` and `additionalProperties` schemas are parsed. A key they describe is only stubbed into `properties` once the form data has it, which a parse has none of, so nothing reached them. - Every position of a tuple `items` is parsed, with `additionalItems` only for a tuple, matching what `ArrayField` renders. - Two schemas that share an `$id` and differ no longer fail the parse. The first is compiled and answers for the rest, which is how both the precompiled and the regular validators look a schema up. `SchemaParserOptions` is a `Pick` of `SchemaContext` rather than an interface `SchemaContext` extends, leaving the context type as v7 has it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r it
A form renders a key its `patternProperties` match with the merge of every
pattern matching it, which `stubExistingAdditionalProperties()` resolves as
`{ allOf: [...matching] }`. The parser pushed each pattern's own schema
instead, so a `customMergeAllOf` that rewrites a sub-schema of that merge
never saw it and the form threw `No precompiled validator function was
found for the given schema`. Each combination of patterns is now parsed as
that `allOf`, since a parse has no form data to tell it which key matches
which patterns.
Merging a property with the `patternProperties` matching it also resolved
that property with the branch expansion off, even while expanding all
branches for the parser, so its conditions collapsed to their `else` and
its dependencies were skipped. `expandAllBranches` is passed through and
the parent fans out over the property's variants.
`getFirstMatchingOption()` and `relaxOptionsForScoring()` kept the `$id` of
the option they derive a schema from. A validator caches the function it
compiles under that `$id`, so every variant was answered by the function
compiled for the first of them -- the relaxed variant validating as
strictly as the one it was meant to relax. Both drop it now, so each
variant is keyed by its content and each is compiled, and `ParserValidator`
goes back to failing loudly on a genuine key collision rather than keeping
the first and warning. The junk option keeps its `$id`, which the
precompiled validators answer it by.
A schema whose `$id` is the empty string was mapped under that `$id`, where
no validator looks for it: both the precompiled and the regular ones fall
back to the hash for one.
`compileSchemaValidators()` and `compileSchemaValidatorsCode()` take the
`customMergeAllOf` in their existing options rather than in a second
trailing bag, as the new `CompileValidatorOptionsType`.
The parser keys the schemas it has walked by content rather than scanning a
list with `deepEquals()`, which cost quadratically in a schema's properties
and options: 150 properties and 20 options parse in 32ms rather than 86ms,
to the same 471 sub-schemas.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s both A form does not validate only against the merge of an `allOf`: `getObjectDefaults()` reads a nested object's unmerged `properties`, and `omitExtraData()` reads the entries a merge leaves in place, scoring the `oneOf` options it finds in each. Merging the `allOf` when expanding all branches therefore stopped compiling sub-schemas a form still asks about, which threw `No precompiled validator function was found for the given schema` for a rewriting or a partial `customMergeAllOf`. `parseSchema()` now parses the entries and the rest of the schema alongside the merge. `resolveAnyOrOneOfSchemas()` scored only the options a schema declares, where `MultiSchemaField` scores the ones it has retrieved, so an option that is a `$ref` to an `allOf` definition was never compiled in the form that gets validated. Each option's retrieved form is now scored too. `withExactlyOneSubschema()` returned no schema at all when expanding a dependency's `oneOf` that no option qualifies for, and the empty list reached `getAllPermutationsOfXxxOf()` as `undefined`. Such a `oneOf` is now ignored, as it already is when the form data picks no single option. The variants of the properties merged with their matching `patternProperties` are varied one property at a time rather than in every combination, so `k` two-branch properties make `k + 1` variants and not `2 ** k`, and the `properties` object is rebuilt once rather than once per matched key, which had cost the form's own resolution quadratically in them: 2000 properties went from 5.6ms to 603ms, and is now 7.4ms. `getFirstMatchingOption()` and `relaxOptionsForScoring()` derive the `$id` of the variant they build rather than dropping the option's: `<the option's>?rjsf=<the variant's hash>` names the variant while keeping the option's as the base a relative `$ref` left inside it resolves against, which dropping it broke. `schemaParser()` reports a schema with more than 16 `patternProperties` rather than enumerating the `2 ** n - 1` combinations of them. The regular validators key a schema whose `$id` is the empty string by its hash, as the precompiled ones do. For ajv8 that stopped two such schemas sharing one compiled function; for ata and cfworker, which re-check the cached schema, it stops them evicting each other, in `rawValidation()` as well as `isValid()`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`handleSchemaUpdate()` registered the root schema under `rootSchema[ID_KEY] ?? ROOT_SCHEMA_PREFIX`, so a root carrying `$id: ''` registered under the empty name while `withIdRefPrefix()` went on rewriting every local `$ref` to resolve against `__rjsf_rootSchema`. Nothing was registered there, so each `$ref` into such a root failed to compile and `isValid()` answered `false` after logging `can't resolve reference`. An empty `$id` names nothing, so the root falls back to the prefix as one with no `$id` does. `@rjsf/validator-cfworker` always used the prefix and was unaffected. Each `oneOf`/`anyOf` option gets its own copy of the `recurseList` when it is retrieved for scoring: a `$ref` one option resolves is not one a sibling has already been through, which is why the `properties` loop of `resolveAllReferences()` copies it too. Drops the `ata-validator` changelog bullet: the rebase onto v7 brought `^1.40.1` in through #5422, so this branch no longer bumps the dependency and the versions the bullet named were both stale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourth review round. Six fixes, two narrowed claims and one cleanup. An `allOf` is merged inside `retrieveSchemaInternal()`, so one reached through a `then`/`else` or a dependency was merged before the parse saw it and the entries `getObjectDefaults()` and `omitExtraData()` read unmerged were never collected. They are now reached on what resolution returns as well as on the schema the parse was handed. `getFirstMatchingOption()` scored an option with no `properties` as it stands, keeping the option's `$id`. `MultiSchemaField` scores the options it has retrieved, so an option whose retrieval changes it is a different schema under one key. Deriving the `$id` is also idempotent now, so relaxing an option and then scoring the relaxed one names one schema rather than stacking a second `?rjsf=` suffix on. Merging a property with its matching `patternProperties` restarted the list of resolved references, so a recursive `$ref` under a key one of those patterns matches overflowed the stack, in the form's own resolution as well as the parser's. The merge is seeded with the one reference this schema was itself reached through, and with a fresh copy of it per key. Not with `recurseList`: `resolveAllReferences()` merges the references each property resolved back into the list it was given, so that list -- and a list shared between the keys -- holds what another key resolved, which reads as a cycle and leaves this key holding a literal `$ref` in place of what it resolves to. Mutual recursion between two schemas through their `patternProperties` still overflows, as it does on v7; this makes no case worse than v7 and fixes the self-recursive one. Expanding all branches applied every dependency, where a form leaves one unapplied until its key has a value and scores an option in that form. The unapplied variant is returned alongside the applied ones. Doing this per key, as suggested, is the product of the two states: 16 dependency keys took 1ms -> 2147ms and produced no further schema, since the intermediate merges reach no `isValid()` call. Only the all-unapplied variant is added, which is 1ms again; the subsets in between are not covered and the comment says so. `omitExtraData()` scores the options of a dependency's `oneOf` itself, where resolving the dependency only validates the conditions `withExactlyOneSubschema()` builds out of them, so each schema dependency is now parsed in its own right. `schemaKey()` replaces the eight copies of `schema[$id] || hash` across `ParserValidator` and the three validators, which is where the `??` family of bugs came from, and `CompileValidatorOptionsType` is `CustomValidatorOptionsType & SchemaParserOptions<S>` in both validators rather than an interface restating the parser's options in each. Two claims narrowed rather than fixed, both pre-existing. An option whose `additionalProperties` stubs the keys a user adds into its `properties` hashes differently per key, which no parse can enumerate; the comment and the changelog say so instead of claiming retrieved options are covered. The tuple-`items` casts are gone from the three tests that had them, the option lists being exported from the fixtures. Every new test fails when its fix is reverted. All 13 `schemaParser()` snapshots are insertions only -- 11 byte-identical, 2 grown by exactly the dependency `oneOf` options -- so nothing left a compiled map. lint, knip, build, test and cs-check green, `@rjsf/utils` and all three validators at 100% coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c16e1a7 to
a49fd2d
Compare
|
Thanks @jimmycallin — fourth round answered in
Mutation evidence, each with the error you quoted:
Two I did not take as written#4 — passing
No case is worse than v7. Mutual recursion still overflows, identically at the base. #7 — the unapplied variant per key multiplies. Returning it from Also in this roundThe changelog went from 30 bullets to 11 — nine Still not fixed hereThree for issues, with #3 above: mutual recursion through
|
This branch has not been deployed
2 participants
Reasons for making this change
Fixes #5338. Fixes #5373. Rebased onto
v7now that #5340, whoseSchemaContextthis builds on, has merged.A precompiled validator only has functions for the sub-schemas
schemaParser()collected, keyed by their$idor hash. The parser had no way to receive acustomMergeAllOf, so when a form's merge produced sub-schemas the default merge doesn't, the form threwNo precompiled validator function was found for the given schema.API changes
SchemaParserOptions<S>type in@rjsf/utils, aPickofSchemaContextholdingcustomMergeAllOf, so a form'sSchemaContextcan be passed in its place.schemaParser(rootSchema, options?)parses with aSchemaContextbuilt from the options.CompileValidatorOptionsType<S>in@rjsf/validator-ajv8and@rjsf/validator-ata, which is theirCustomValidatorOptionsTypeplus thecustomMergeAllOfthe schema is parsed with.compileSchemaValidators(schema, output, options?)andcompileSchemaValidatorsCode(schema, options?)take it in place of theCustomValidatorOptionsTypethey took, so compiling with only a merge takes one options object rather than an empty one followed by a second. It only adds an optional field, so existing calls type-check and behave unchanged; compiling with{ customMergeAllOf }, merging the way the form's does, covers the sub-schemas it merges.Note that the same
customMergeAllOfhas to reachcreatePrecompiledValidator()as well, since the validator resolves the root schema it is handed with it.validation.mdnow says so.The parser reads an
allOfboth merged and as it standsschemaParser()walks the schema withexpandAllBranches, which returned anallOf's unmerged branches and nothing else. A form renders the merged schema, so the branches held neither the sub-schemas acustomMergeAllOfrewrote (which is #5338) nor the ones that only exist once the merge has brought a property together with thepatternPropertiesmatching it — which happens with the default merge too. Expanding all branches now merges theallOfthe way the form does, and drops one it cannot merge the way the form does.But a form does not read an
allOfonly through that merge, which is what @jimmycallin caught this round:getObjectDefaults()walks a nested object's unmergedpropertiesunlessdefaultFormStateBehavior.allOf === 'populateDefaults', andgetClosestMatchingOption()scores those options as written.omitExtraData()walks the entries a merge leaves in place and scores each entry's options.Both threw on the merge-only parse. So
parseSchema()records the entries and what the schema declares besides them alongside the merge:That stays additive:
knestedallOfs cost2k + 1merges (5, 9, 13, 17, 21 for k = 2, 4, 6, 8, 10), not a product. Only the parser passesexpandAllBranches: true, so none of this changes form behavior — only what gets compiled.This also removes a cost that predates the PR:
resolveSchema()returns the cartesian product of itsallOfmembers' resolutions, which the branch variants multiplied. A rootallOfof six three-itemallOfs went from 15,631 merges in 60.2s to 13 in under a millisecond, faster than the pre-PR baseline of 64ms.Also fixed in the parser
oneOf/anyOfoption is parsed rather than having itspropertiesread off, so an option whose sub-schemas come from its ownallOf, conditions or dependencies is reached.MultiSchemaFieldscores it. That component scoresoptions.map((opt) => retrieveSchema(opt, formData)), so the commononeOf: [{ $ref: Cat }]withCat = { allOf: [...] }is validated as the merge of thatallOf, which the parse never recorded. It threw before this PR as well as during it;resolveAnyOrOneOfSchemas()now scores each option's retrieved form too, as-is and relaxed.patternPropertiesandadditionalPropertiesschemas are parsed, which is Precompiled validators miss sub-schemas under patternProperties and additionalProperties #5373. A key they describe is only stubbed intopropertiesonce the form data has it, and a parse has no form data, so nothing reached them; aoneOfunder either threw as soon as a user added such a key. Both of that issue's variants are verified fixed here: at thev7base they throw on the hash it names,-4c020a92, thepatternPropertiesone throughgetDefaultFormState()and theadditionalPropertiesone a step later, whereMultiSchemaFieldscores the retrieved options.patternPropertiesmatch is rendered with the merge of every pattern matching it, whichstubExistingAdditionalProperties()resolves as{ allOf: [...matching] }. Each combination of patterns is parsed as thatallOfrather than as the patterns themselves, since a parse has no form data to tell it which key matches which patterns. There are2^n - 1of those, so a schema with more than 16patternPropertiesis now reported with a message saying why rather than parsed for minutes.itemsis parsed, withadditionalItemsonly for a tuple, matching whatisFixedItems()gates inArrayField.oneOf/anyOfoption redefines is parsed as the schema's own as well as as each option spells it. Fix 4385: pass customMergeAllOf and defaultFormStateBehavior through a SchemaContext #5340 fixed the neighbouring half of this; this is the remainder.deepEquals(), which cost quadratically in a schema's properties and options: 150 properties with a 3-optiononeOfeach and 20 top-level options parse in 32ms rather than 86ms, to the same 471 sub-schemas.A derived schema carries an
$idthat names it, not the one it came fromA validator caches the function it compiles for a schema under that schema's
$id, so a schema RJSF derives from one must not keep it unchanged or the two share a function:getFirstMatchingOption()validates an object option against an augmented copy — ananyOfof itsrequiredkeys added,requireddeleted — andrelaxOptionsForScoring()scores it withadditionalProperties: falsewidened totrue. Both kept the option's$id, so each variant was answered by the function compiled for the first of them, which made the relaxed variant validate as strictly as the one it exists to relax. In the regular validator as much as the precompiled one.<the option's $id>?rjsf=<the variant's hash>. Simply dropping the$id, which is what the first pass at this did, loses the base URI that a relative$refleft inside the option resolves against — a$ref: 'b.json'under$id: 'http://example.com/a.json'stopped resolving, and the option silently failed to match. The query keeps the base while naming the variant.$idunchanged:precompiledValidator.isValid()recognises it by that$idand answers it without a compiled function at all.ParserValidator.addSchema()keeps failing loudly on a genuine key collision rather than needing to keep the first of two and warn.$idis the empty string was mapped under that$id, where the precompiled validators do not look.@rjsf/validator-ajv8had the same bug inisValid(), where two such schemas shared one compiled function;@rjsf/validator-ataand@rjsf/validator-cfworkerre-check the cached schema, so for them the shared key only made two such schemas evict each other. All now key by hash when the$idis empty, inrawValidation()as well asisValid().??sat inhandleSchemaUpdate()in ajv8 and ata: a root schema carrying$id: ''registered under the empty name whilewithIdRefPrefix()went on rewriting local$refs to resolve against__rjsf_rootSchema, so every$refinto such a root failed to compile andisValid()answeredfalseafter loggingcan't resolve reference. Both fall back toROOT_SCHEMA_PREFIXnow, which is what cfworker already did.A property merged with its
patternPropertiesis resolved with the branch expansion onMerging a property with the
patternPropertiesmatching it callsretrieveSchemaInternal()again, and passedundefinedforexpandAllBrancheseven when the parser had asked for every branch. So that property's conditions collapsed to theelsethatParserValidator.isValid()always returningfalsepicks, and its dependencies were skipped for want of form data: aoneOfin thethenbranch was never compiled, and a form whose data meets theifthrew.expandAllBranchesis passed through, and the branches are varied one property at a time. The first pass took the cartesian product of them, which cost2 ** kparent variants forktwo-branch properties (2.75s at k=16) and produced an identical parsed map, sinceisValid()is only ever handed an option, a condition or a dependency and never a whole parent. One at a time givesk + 1. Thepropertiesobject is also rebuilt once rather than once per matched key, which had made the form's own resolution quadratic: 2000 properties against one^ppattern went 5.6ms → 603ms → 7.4ms. With the expansion off, each inner call returns exactly one schema, so the result on every form path is unchanged.An expansion that qualifies no option no longer crashes
Under
expandAllBranches,withExactlyOneSubschema()returned[]when a dependency'soneOfhas no option naming the dependency key, because the "ignore thisoneOf" path was guarded on!expandAllBranches. The empty list reachedgetAllPermutationsOfXxxOf(), which pushedlist[0]—undefined— andObject.getOwnPropertySymbols(undefined)threwCannot convert undefined or null to object. Such aoneOfis now ignored, as it already is when the form data picks no single valid option.Not fixed here
Both predate this change and want their own issues:
expandAllBranchesnever produces the variant of anifwithout anelse, or of adependenciesentry, with the branch not applied, so a form whose data fails theifvalidates against a schema that was never compiled. Fixing it adds a variant perifand per dependency key — the combinatorial cost this PR removes — so it needs its own perf budget.oneOfoption that carries an$idand resolves into different content —{ $id: 'opt1', allOf: [...] }, say — makesschemaParser()throwTwo different schemas exist with the same key opt1. Same family as the derived-$idfix above: the resolved option doesn't get an$idof its own the way the augmented and relaxed ones now do. Reproduces identically at the pre-PR base.Dropped in the rebase onto v7
The optional trailing
rootSchemaonprocessRawValidationErrors(), and the precompiled validators passing their own, are gone: #5340'sensureSameRootSchema()resolves the validator's own root schema with itscustomMergeAllOf, andFormsuppliesgetCustomValidateFormData()so the defaults handed tocustomValidateare computed by the form. With the parameter removed the whole ajv8 suite still passed, so it was unreachable.precompiledValidator.tsandprocessRawValidationErrors.tsare untouched by this PR in both packages.Checklist
validation.md, the ajv8 and ata API reference, theSchemaContextsection and "New types" list of the v7 upgrade guide)CHANGELOG-v7.md)Testing
@rjsf/utilsschemaParsertests: anallOfmerged with acustomMergeAllOfand with the default one, the entries of anallOfa merge leaves in place, the unmerged options alongside the merged ones, a merge that throws, a booleanallOfentry, apatternPropertiesmatch, a mergedallOfwhosepatternPropertiesthen applies, an option whose sub-schemas come only from its ownallOf, option-onlypropertiesanditems, a property every option redefines, tupleitemsandadditionalItems,patternProperties/additionalPropertiesoptions, a schema with 17patternPropertiesbeing reported, and a merge-count assertion that pins the nested-allOfcost at2k + 1.@rjsf/utilsschemaParsertests for the derived$id: each variant of an$id-carrying option is compiled under a key of its own, both for an option withproperties(where the augmentation is what reachesisValid()) and for one without (where the relaxed copy is), and the derived key keeps the option's$idas its base.@rjsf/utilsParserValidatortest: a schema whose$idis the empty string is mapped under its hash, as a validator looks it up.@rjsf/utilsgetFirstMatchingOptiontest:isValid()is called with an$idderived from the schema it augments, and with the junk option's kept. It reads the schema from a recording validator that implementsValidatorType, so it holds for the real validators the shared suite also runs it against.@rjsf/utilsretrieveSchematests: expanding all branches merges anallOf, drops one it cannot merge, expands the branches of a property merged with thepatternPropertiesmatching it, varies those branches one property at a time rather than in every combination, and ignores a dependencyoneOfthat qualifies no option.@rjsf/validator-ajv8validatortests: an option whose relative$refresolves against its own$idstill matches once augmented, and a schema whose$idis the empty string gets its own compiled function.validatortests: a$refinto a root schema whose$idis the empty string resolves.compileSchemaValidatorstests: the options,customMergeAllOfincluded, reachcompileSchemaValidatorsCode(), and default to{}without them.compileSchemaValidatorsCodetests: the issue's repro throughgetDefaultFormState(), compiled without and with thecustomMergeAllOf; the same for a key only thepatternPropertiesmatch;validateFormData()with acustomValidate, both on submit and under live validation; and four end-to-end cases for the sub-schemas a form validates against unmerged — a nestedallOfthroughgetObjectDefaults(), the entries an identity merge leaves in place throughomitExtraData(), an option retrieved from a$refto anallOfthroughgetClosestMatchingOption(), and a key twopatternPropertiesboth match. The shared fixtures live inutils/test/testUtils/customMergeAllOfData.tsandutils/test/testUtils/parsedSchemaData.ts.Every fix is mutation-checked: removing it makes its test fail, with the hash the reported repro named —
-4c020a92for the unmerged nestedallOf,-4fca741cfor the entries left in place,-1ca39cdffor the retrieved$ref-to-allOfoption,73557177for two patterns matching one key, and1691cd08for the originalpatternPropertiescase.No snapshot changed — all 13
schemaParser()snapshots,superSchemaandSCHEMA_WITH_ALLOF_CANNOT_MERGEincluded, are byte-identical.Locally green:
lint,knip,build(16 projects), the roottypecheck, andtest(15 projects), with@rjsf/utilsand the validator packages at their enforced 100% coverage, pluscs-check.🤖 Generated with Claude Code