fix(schema): respect "required" for properties inside oneOf/anyOf#3041
Open
schani wants to merge 4 commits into
Open
fix(schema): respect "required" for properties inside oneOf/anyOf#3041schani wants to merge 4 commits into
schani wants to merge 4 commits into
Conversation
The oneOf/anyOf `required` fix (#1104) can make a self-referential property required, e.g. FacetSpec/RepeatSpec in vega-lite require a nested `spec`. Previously such members were optional and the C++ renderer stored them as `std::shared_ptr` (heap indirection) via the `isCycleBreakerType` path in `isOptionalAsValuePossible`, which broke the cycle. A required member skipped that path and was emitted by value (`Spec spec;`), producing an incomplete recursive type that fails to compile. Mirror the optional behaviour: a required class-typed member that is a cycle-breaker type now also gets heap indirection, so recursive required members compile. Verified all 71 schema-cplusplus fixtures pass, including vega-lite. Also scope out the new `required-in-any-of.schema` for Elixir, which — like the existing `strict-optional.schema`/`required.schema` — cannot enforce required struct keys at runtime, so its `.fail.strict-optional` sample does not fail as the fixture expects. Co-Authored-By: Claude <noreply@anthropic.com>
# Conflicts: # test/languages.ts
Generated-output differences154 files differ — 77 modified, 53 new, 24 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
requiredhad no effect on properties reached through JSON SchemaoneOf/anyOf. A property required in the branch where it's declared would come out optional in generated code, while the exact same property/requiredshape outside ofoneOf/anyOfcorrectly stayed required.Repro (from the issue,
--src-lang schema --lang typescript):{ "properties": { "anyof": { "anyOf": [ { "type": "object", "properties": { "name": {"type": ["string","null"]} }, "required": ["name"], "additionalProperties": false }, { "type": "object", "properties": { "size": {"type": "string"} }, "required": [], "additionalProperties": false } ] } } }Before:
After:
Root cause
quicktype's default pipeline flattens a union of several object (
ClassType) branches — which is howoneOf/anyOfalternatives are represented internally — into a single merged interface (FlattenUnions.ts→UnifyClasses.ts). The property-merging logic (getCliquePropertiesinUnifyClasses.ts) unconditionally marked a property optional whenever it was absent from any one of the merged branches, without considering that the branch it's missing from might be a closed schema (additionalProperties: false) that simply doesn't declare it — so the presence/absence heuristic (correct for merging differently-shaped plain JSON samples) silently dropped therequiredinformation declared for JSON SchemaoneOf/anyOfalternatives.Fix
schemaSetOperationTypeAttributeKindtype attribute (packages/quicktype-core/src/attributes/Schema.ts) thatJSONSchemaInput.tsstamps on the union type it builds foroneOf/anyOf(convertOneOrAnyOf).FlattenUnions.tschecks for that attribute on the unions it's about to merge and, when present, tellsUnifyUnionBuilder/getCliqueProperties(UnifyClasses.ts) to preserve a property'srequired-ness instead of forcing it optional purely because a closed sibling branch (additionalProperties: false) doesn't declare it. Properties still become optional when the branch that lacks them is "open" (could carry the property as untyped additional data) or when the property is itself optional within the branch(es) that do declare it.Test coverage
Added
test/inputs/schema/required-in-any-of.schema(a two-branchanyOf, one branch requiringname, the other requiring nothing and being closed) plus.1.json(valid) and.2.fail.strict-optional.json(an object missingname, expected to fail under thestrict-optionalfeature — TypeScript's schema fixture already declares that feature). This is picked up automatically by the existingschema-typescriptfixture.Verification
required-in-any-of.schema) fails on unfixed code, passes after the fix.QUICKTEST=true FIXTURE=schema-typescript script/test— all 71 cases pass.npx vitest run test/unit— 170 tests pass.npm run buildpasses.name: null | string;instead ofname?: null | string;.Fixes #1104
🤖 Generated with Claude Code