Skip to content

fix: accept tuple items whose schema has no type keyword - #869

Open
maximilliangrand wants to merge 1 commit into
fastify:mainfrom
maximilliangrand:fix/tuple-items-without-type
Open

fix: accept tuple items whose schema has no type keyword#869
maximilliangrand wants to merge 1 commit into
fastify:mainfrom
maximilliangrand:fix/tuple-items-without-type

Conversation

@maximilliangrand

Copy link
Copy Markdown

fix: accept tuple items whose schema has no type keyword

The contract that is violated

In JSON Schema, an items array is tuple validation: each subschema constrains the element at the same index. A subschema with no type keyword places no type constraint on that element — {} accepts every value, and so do const, enum, oneOf/anyOf/allOf, if/then/else and the boolean schema true.

fast-json-stringify does 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)

const build = require('fast-json-stringify')
const Ajv = require('ajv')

const schema = {
  type: 'array',
  items: [
    { anyOf: [{ type: 'string' }, { type: 'number' }] }
  ]
}
const data = ['foo']

console.log('fast-json-stringify', require('fast-json-stringify/package.json').version)
console.log('ajv says data is valid:', new Ajv({ strict: false }).validate(schema, data))
console.log(build(schema)(data))
fast-json-stringify 7.0.1
ajv says data is valid: true
<anonymous_script>:78
            throw new Error(`Item at 0 does not match schema definition.`)
            ^

Error: Item at 0 does not match schema definition.
    at anonymous0 (eval at build (.../node_modules/fast-json-stringify/index.js:233:23), <anonymous>:78:19)

The same happens for items: [{}], items: [{ const: 'foo' }], items: [{ enum: ['a', 'b'] }], items: [{ oneOf: [...] }], items: [{ allOf: [...] }], items: [{ if: ..., then: ... }] and items: [true].

A common real-world shape hits it. TypeBox emits exactly this for a tuple containing a union:

const { Type } = require('typebox')
const schema = Type.Tuple([Type.Union([Type.String(), Type.Number()]), Type.Literal('x')])
// {"type":"array","additionalItems":false,"items":[{"anyOf":[{"type":"string"},{"type":"number"}]},
//  {"type":"string","const":"x"}],"minItems":2}
build(schema)(['a', 'x'])   // Error: Item at 0 does not match schema definition.

Root cause

index.js:850buildArrayTypeCondition(type, accessor) has no fallback when type is undefined. Its switch handles null/string/integer/number/boolean/object/array and, in default, an array of types. For an item schema with no type, condition is never assigned and the function returns undefined.

The tuple call site interpolates that return value straight into the guard:

if (${i} < arrayLength) {
  if (${buildArrayTypeCondition(item.type, value)}) {
    ...
  } else {
    throw new Error(`Item at ${i} does not match schema definition.`)
  }
}

so the emitted source is literally if (undefined). Confirmed against 7.0.1 with mode: 'debug':

          if (undefined) {
            throw new Error(`Item at 0 does not match schema definition.`)

Note that item.type is read after buildValue() has run, and buildValue() back-fills schema.type from inferTypeByKeyword(). That is why items: [{ properties: {...} }] works while items: [{ anyOf: [...] }] does not — inferTypeByKeyword has nothing to infer from an applicator keyword.

Which call site actually runs

buildArrayTypeCondition is called from two places in buildArray: index.js:719 (extracted-function path) and index.js:806 (inlined path). In practice every array compiles through the extracted-function path at index.js:719. The branch guard at index.js:670 is

if (context.recursivePaths.has(fullPath) || context.buildingSet.has(schema) || schemaId !== '') {

and 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:762847) is therefore currently unreachable. Two independent checks:

  • c8 reports every statement in that block as uncovered, on main and on this branch.
  • Inserting 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 buildArrayTypeCondition itself, 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 buildArrayTypeCondition the missing fallback: no type keyword means no type constraint, so the guard is true.

       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:

Neither commit is an ancestor of main: 07f07c8 survives only on next-old, and 377288f is not reachable from main, next-old, v5.x or v1.x. Both next lines 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 main today 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.js restored from origin/main (grep -c "condition = 'true'" index.js0) and the new tests in place:

✖ tuple items without a type keyword accept any value (0.386334ms)
✖ mixed tuple without a type keyword, with $id and nested in an object (0.554792ms)
✖ tuple items with a type keyword still reject mismatching values (0.260708ms)
ℹ tests 34
ℹ pass 31
ℹ fail 3

all three failing with Item at N does not match schema definition.

Pass with the fix, full suite green (npm test; main is 491 before the 3 new tests):

ℹ tests 494
ℹ pass 494
ℹ fail 0
ℹ skipped 0
ℹ todo 0
Targets:    1 passed, 1 total
Test files: 1 passed, 1 total
Assertions: 12 passed, 12 total

npm run lint exits 0 with no output.

Output is unchanged for everything that worked before. I dumped mode: 'debug' code on origin/main and on this branch for six schemas — a typed 2-tuple; a $id tuple with additionalItems; a non-tuple array; a tuple item with type: ['string', 'null']; a tuple nested in an object property; a tuple item typed object — and diffed:

$ diff code-before.txt code-after.txt && echo IDENTICAL
IDENTICAL

Round-trip oracle. For 12 schema+data pairs that ajv accepts, I serialized with this branch, JSON.parsed the result, re-validated it against the same schema and deepStrictEqual'd it against the input. 11 of 12 threw on origin/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. On main that guard is if (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 throwsome output, and that output follows the library's existing conventions rather than new ones. I diffed unmodified main against this branch over the relevant inputs; in every row below the "after" value is exactly what the non-tuple equivalent already produces on main today:

input tuple items: [...] before tuple after non-tuple equivalent on main
items: [{}], value undefined / a function / a Symbol / { toJSON () { return undefined } } throws [undefined] items: {} already gives [undefined]
items: [{ const: 'foo' }], value 'bar' throws ["foo"] items: { const: 'foo' } already gives ["foo"]
items: [{ enum: ['a'] }], value 'zzz' throws ["zzz"] items: { enum: ['a'] } already gives ["zzz"]
items: [false], value 1 throws [1] items: false already gives [1,2]; properties: { a: false } already gives {"a":1}

Two of these deserve to be called out rather than discovered in review:

  1. An untyped tuple item can now emit the literal text undefined, which is invalid JSON, where main threw a clean Error. This is the pre-existing json += JSON.stringify(value) gap, not a new one: on unmodified main, items: {} (non-tuple), properties: { a: {} } and items: [{ type: 'string' }] + additionalItems: true already produce undefined in their output for the same values. The change makes tuple positions consistent with them. Fixing that gap properly is a separate concern (it also affects additionalItems: true and additionalProperties: true) and I have deliberately not bundled it here — happy to open it separately if you want it addressed.

  2. const/enum are not enforced, matching main's non-tuple behaviour: fast-json-stringify is a serializer, not a validator, and items: { const: 'foo' } already rewrites 'bar' to "foo" while items: { enum: ['a'] } already passes 'zzz' through untouched.

An item schema with an unknown type still fails at build time in isValidSchema before reaching this code — I checked 'bogus', 123 and null, all rejected with schema is invalid: data/items must be object,boolean — so the new else branch 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 existing buildValue paths — anyOf/oneOf tuple items use the same validator.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

  • I ran the suite only on Node 26.7.0 / macOS. CI covers Node 20/22/24/26 on ubuntu/macos/windows, so my local run matches one CI cell; the other cells are unverified by me. The change is pure string codegen with no platform-dependent behaviour.
  • I did not benchmark. The 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.
  • Draft 2020-12 prefixItems is not supported by this library (Support of JSON Schema draft 2020-12 #627), so I did not touch it.
  • The inlined codegen path (index.js:762847) is unreachable today, so my change to the shared helper is verified only through the extracted-function call site.

Checklist

  • Tests added (test/array.test.js)
  • npm test passes
  • npm run lint passes
  • No version bump
  • No public API change, no docs change needed

`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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant