fix: accept tuple items whose schema has no type keyword - #869
Open
maximilliangrand wants to merge 1 commit into
Open
fix: accept tuple items whose schema has no type keyword#869maximilliangrand wants to merge 1 commit into
type keyword#869maximilliangrand wants to merge 1 commit into
Conversation
`buildArrayTypeCondition` returned `undefined` when a tuple item schema had
no `type` keyword, so the generated guard was `if (undefined)` and every
value was rejected with `Item at N does not match schema definition.`.
This affected every item schema that constrains the value without a `type`:
`{}`, `const`, `enum`, `oneOf`/`anyOf`/`allOf`, `if`/`then`/`else` and
boolean schemas. A TypeBox `Type.Tuple([Type.Union([...])])` could never be
serialized.
An item schema without `type` puts no type constraint on the value, so the
guard now emits `true`. Generated code for item schemas that do declare a
type is byte-identical.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: accept tuple items whose schema has no
typekeywordThe contract that is violated
In JSON Schema, an
itemsarray is tuple validation: each subschema constrains the element at the same index. A subschema with notypekeyword places no type constraint on that element —{}accepts every value, and so doconst,enum,oneOf/anyOf/allOf,if/then/elseand the boolean schematrue.fast-json-stringifydoes the opposite: it rejects every value at such a tuple position. The serializer compiled for that schema is not merely wrong for some inputs — it can never produce output at all.Repro (published
fast-json-stringify@7.0.1)The same happens for
items: [{}],items: [{ const: 'foo' }],items: [{ enum: ['a', 'b'] }],items: [{ oneOf: [...] }],items: [{ allOf: [...] }],items: [{ if: ..., then: ... }]anditems: [true].A common real-world shape hits it. TypeBox emits exactly this for a tuple containing a union:
Root cause
index.js:850—buildArrayTypeCondition(type, accessor)has no fallback whentypeisundefined. Itsswitchhandlesnull/string/integer/number/boolean/object/arrayand, indefault, an array of types. For an item schema with notype,conditionis never assigned and the function returnsundefined.The tuple call site interpolates that return value straight into the guard:
so the emitted source is literally
if (undefined). Confirmed against 7.0.1 withmode: 'debug':Note that
item.typeis read afterbuildValue()has run, andbuildValue()back-fillsschema.typefrominferTypeByKeyword(). That is whyitems: [{ properties: {...} }]works whileitems: [{ anyOf: [...] }]does not —inferTypeByKeywordhas nothing to infer from an applicator keyword.Which call site actually runs
buildArrayTypeConditionis called from two places inbuildArray:index.js:719(extracted-function path) andindex.js:806(inlined path). In practice every array compiles through the extracted-function path atindex.js:719. The branch guard atindex.js:670isand
context.rootSchemaId = schema.$id ||_fjs_root${schemaIdCounter++}`` (index.js:161) is never empty, so `schemaId !== ''` is always true and the first arm is always taken.The inlined path below it (
index.js:762–847) is therefore currently unreachable. Two independent checks:c8reports every statement in that block as uncovered, onmainand on this branch.throw new Error('INLINE ARRAY PATH REACHED')at the top of that block still leaves the whole suite at 494 passing, 0 failing.Since the fix is inside
buildArrayTypeConditionitself, both call sites are corrected; the second one is fixed by symmetry only and cannot be exercised today. I am not claiming test coverage for it.Fix
Give
buildArrayTypeConditionthe missing fallback: notypekeyword means no type constraint, so the guard istrue.if (Array.isArray(type)) { const conditions = type.map((subType) => { return buildArrayTypeCondition(subType, accessor) }) condition = `(${conditions.join(' || ')})` + } else { + // The item schema has no `type` keyword, so it does not constrain the + // type of the item at all. That is the case for `{}`, `const`, `enum`, + // `oneOf`/`anyOf`/`allOf`, `if`/`then`/`else` and boolean schemas. + // Without this branch the condition would be `undefined` and every + // value would be rejected. + condition = 'true' }Why not just remove the check? (prior art)
You have twice merged a full removal of
buildArrayTypeCondition, and neither ever shipped:next(07f07c8).next(377288f), approved by @mcollina and @gurgunday.Neither commit is an ancestor of
main:07f07c8survives only onnext-old, and377288fis not reachable frommain,next-old,v5.xorv1.x. Bothnextlines were abandoned, so the check is still shipping in 7.0.1.Removing the check outright is semver-major — it was called breaking then and it still is. This PR is deliberately the non-breaking subset: it keeps the runtime type check for item schemas that declare a type and only supplies the fallback that was missing for those that do not. It unblocks untyped tuple items on
maintoday and does not preclude landing #706 in a future major. This also matches @ivan-tymoshenko's ruling in #662: "IMO this check is redundant, but dropping it would be a breaking change, so we should support this."Evidence
Fail on a clean baseline. With
index.jsrestored fromorigin/main(grep -c "condition = 'true'" index.js→0) and the new tests in place:all three failing with
Item at N does not match schema definition.Pass with the fix, full suite green (
npm test;mainis 491 before the 3 new tests):npm run lintexits 0 with no output.Output is unchanged for everything that worked before. I dumped
mode: 'debug'code onorigin/mainand on this branch for six schemas — a typed 2-tuple; a$idtuple withadditionalItems; a non-tuple array; a tuple item withtype: ['string', 'null']; a tuple nested in an object property; a tuple item typedobject— and diffed:Round-trip oracle. For 12 schema+data pairs that
ajvaccepts, I serialized with this branch,JSON.parsed the result, re-validated it against the same schema anddeepStrictEqual'd it against the input. 11 of 12 threw onorigin/main; all 12 pass here.mode: 'standalone'verified working for an untyped tuple item too.Regression surface
The only behaviour that changes is the guard for a tuple position whose item schema has no
type. Onmainthat guard isif (undefined), i.e. it rejects 100% of inputs, so nothing that previously produced output can change: there is no input for which the old code returned a string and the new code returns a different one. Codegen for item schemas that do declare a type is byte-identical (above).What does change is
throw→ some output, and that output follows the library's existing conventions rather than new ones. I diffed unmodifiedmainagainst this branch over the relevant inputs; in every row below the "after" value is exactly what the non-tuple equivalent already produces onmaintoday:items: [...]beforemainitems: [{}], valueundefined/ a function / aSymbol/{ toJSON () { return undefined } }[undefined]items: {}already gives[undefined]items: [{ const: 'foo' }], value'bar'["foo"]items: { const: 'foo' }already gives["foo"]items: [{ enum: ['a'] }], value'zzz'["zzz"]items: { enum: ['a'] }already gives["zzz"]items: [false], value1[1]items: falsealready gives[1,2];properties: { a: false }already gives{"a":1}Two of these deserve to be called out rather than discovered in review:
An untyped tuple item can now emit the literal text
undefined, which is invalid JSON, wheremainthrew a cleanError. This is the pre-existingjson += JSON.stringify(value)gap, not a new one: on unmodifiedmain,items: {}(non-tuple),properties: { a: {} }anditems: [{ type: 'string' }] + additionalItems: truealready produceundefinedin their output for the same values. The change makes tuple positions consistent with them. Fixing that gap properly is a separate concern (it also affectsadditionalItems: trueandadditionalProperties: true) and I have deliberately not bundled it here — happy to open it separately if you want it addressed.const/enumare not enforced, matchingmain's non-tuple behaviour:fast-json-stringifyis a serializer, not a validator, anditems: { const: 'foo' }already rewrites'bar'to"foo"whileitems: { enum: ['a'] }already passes'zzz'through untouched.An item schema with an unknown
typestill fails at build time inisValidSchemabefore reaching this code — I checked'bogus',123andnull, all rejected withschema is invalid: data/items must be object,boolean— so the newelsebranch cannot mask a bad type.Security
No regex, loop, parser or allocator is touched. The emitted literal is the constant
'true', not schema-derived text, so nothing new is interpolated into generated source. Values newly reaching serialization go through the existingbuildValuepaths —anyOf/oneOftuple items use the samevalidator.validate("<ref>", value)call non-tuple positions already use — so no input class reaches any validator or serializer it could not already reach through a non-tuple schema.What I could not verify
mode: 'debug'diff shows the hot path for typed tuples is byte-identical, and the newly-working path had no previous throughput to compare against.prefixItemsis not supported by this library (Support of JSON Schema draft 2020-12 #627), so I did not touch it.index.js:762–847) is unreachable today, so my change to the shared helper is verified only through the extracted-function call site.Checklist
test/array.test.js)npm testpassesnpm run lintpasses