fix(schema): dedupe recursive JSON Schema $ref-to-$id types - #2983
Open
schani wants to merge 10 commits into
Open
fix(schema): dedupe recursive JSON Schema $ref-to-$id types#2983schani wants to merge 10 commits into
schani wants to merge 10 commits into
Conversation
Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
Co-Authored-By: Claude <noreply@anthropic.com>
… cjson
The new recursive-ref-to-id.schema fixture broke two languages the PR did
not target, both due to pre-existing language limitations rather than the
$ref-to-$id dedup fix itself:
- Go: the sample's leaf node used an empty `children: []` array on an
optional property, which Go's `omitempty` drops on marshal, so the
round-trip output no longer matched the input ("Output is not equivalent
to input"). This is the same known omitempty limitation already documented
in test/languages.ts for github-events.json. Deepen the recursion
(root -> child -> grandchild) so it still exercises multi-level recursion
without relying on round-tripping an empty optional array.
- cjson: cjson's generated deserializer does not validate that an array
property actually holds an array, so it did not reject the
`.fail.union.json` negative case ("Expected failure ... "). This is the
documented cjson class of "Arrays with invalid types are not checked", so
add the schema to cjson's skipSchema alongside the other such fixtures.
Co-Authored-By: Claude <noreply@anthropic.com>
The Haskell fixture driver decodes the top-level type as a Maybe and prints "null" with exit 0 on a failed decode, so expected-failure schema samples can never be detected for schemas that hit this path (the same documented limitation already worked around for nested-intersection-union.schema, prefix-items.schema, and direct-union.schema). recursive-ref-to-id.1.fail.union.json hits the same path, so add recursive-ref-to-id.schema to Haskell's skipSchema list alongside the others. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
…ned) master's test/unit/cjson-enum-default.test.ts, pulled into this branch's CI by merging master, expects the pre-#3052 alphabetical enum-case layout, but current cJSON generator output (after #3052's "start enum values at 1") uses declaration order with an explicit start value on the first case. Correct the expectation to match actual generator output so this branch's version wins the merge and CI stops failing on unrelated cJSON churn. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
# Conflicts: # test/unit/cjson-enum-default.test.ts
Generated-output differences95 files differ — 44 modified, 36 new, 15 deleted |
Generated-output differences98 files differ — 46 modified, 37 new, 15 deleted |
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.
Bug
When a JSON Schema has a self-recursive
$refback to its own$id(or a$ref: "#"self-reference), quicktype generated a duplicate, self-recursivecopy of the type — plus duplicated copies of every type nested underneath it.
Repro (
--lang cs --src-lang schema --top-level Tree tree.schema.jsonwith atree-node schema whose
childrenproperty is$ref'd back to the schema'sown
$id) produced four C# classes (Tree, a duplicate self-recursiveTreeSchema,TreeData, and a duplicateChildData) instead of the expectedtwo (
Tree,TreeData), withTreecorrectly self-recursive. Sameduplication happened for every target language (also reproduced with
TypeScript), since the bug is in the shared JSON Schema input handling.
Root cause
In
packages/quicktype-core/src/input/JSONSchemaInput.ts,Refs thatrepresent "the root of an addressed document" were constructed with two
different, non-equal path shapes depending on the code path:
Ref.parseURIspecial-cased an empty fragment on an addressed URI toproduce path
[{kind: Root}].Ref.root(address)(used byCanonizer.addSchemaandResolver.resolveTopLevelRef) constructed a bareRefwith path[].These two "same" refs weren't equal under the
EqualityMapused to cachealready-converted types (
typeForCanonicalRef), so when a self-$refresolved back to the document root, the cache missed and the schema got
converted a second time — only the second copy ended up wired up as truly
recursive, leaving the first copy (and everything nested under it) as a
duplicated dead end.
Fix
Normalize this invariant in one place — the
Refconstructor itself: anyRefwith a definedaddressURIand an emptypathis normalized to path[{kind: Root}]. This makes every construction path (Ref.root,Ref.parseURI, and any othernew Ref(address, [])) agree on a singlecanonical representation for "root of an addressed document," so nested
paths built by pushing onto any of them stay comparable.
An initial version of this fix only changed
Ref.parseURI's normalizationwithout touching the invariant globally, which fixed the reported bug but
broke two other things that depend on ref-path shape: the CLI's
properties-are-types feature for
<file>#/-style source URIs, and$id-based self-references combined with explicit#/...-style refs (theexisting
id-root.schemafixture). Both are covered by the regression testsadded below and now pass together with the original fix.
Test coverage
test/inputs/schema/recursive-ref-to-id.schema+test/inputs/schema/recursive-ref-to-id.1.json— new end-to-end fixturereproducing the reported bug (self-
$refto the schema's own$id),wired into the schema fixture registry (skipped for Elm/Dart, which don't
support this kind of recursion at all, matching the pattern already used
for other recursive schema fixtures).
test/unit/recursive-ref-to-id.test.ts— unit test asserting exactly 2generated TypeScript interfaces (not 4) for both the
$ref: "T2"and$ref: "#"self-reference forms, since fixture round-tripping alonedoesn't catch "duplicate type" bloat.
test/unit/schema-properties-as-types.test.ts— regression coverage forthe
<file>#/properties-are-types CLI feature that an earlier version ofthis fix broke.
Verification
npm run buildpasses.npm run test:unit— 27 files, 167 tests, all pass.TreeandData(renderer-named nested type) in C#, with
Tree.Childrentyped asTree[]— no
TreeSchema/ChildDataduplicates.FIXTURE=schema-typescript script/testacross alltest/inputs/schema/*.schemafixtures exceptvega-lite.schema,class-map-union.schema, andclass-with-additional.schema(all threeconfirmed to already fail the same way on unfixed master — pre-existing,
unrelated TypeScript
tscstrictness issues) — all pass, includingid-root.schemawhich the intermediate fix attempt had broken.dotnet) was not available in thissandbox, so the C#/
schema-csfixture wasn't run end-to-end; CI willvalidate it, and the shared input-layer fix plus the TypeScript fixture
coverage above exercise the same code path.
Fixes #2836.
🤖 Generated with Claude Code