fix(json-schema): stop merged type descriptions overriding property descriptions#3056
Open
schani wants to merge 10 commits into
Open
fix(json-schema): stop merged type descriptions overriding property descriptions#3056schani wants to merge 10 commits into
schani wants to merge 10 commits into
Conversation
…escriptions (#2801) quicktype's type unifier merges structurally identical types (e.g. every bare map-of-string like {additionalProperties: string}) into one shared type node, and set-unions their `description` type attributes. In JSONSchemaRenderer.definitionForObject, that merged type-level description was applied to an inlined property schema before the correct, per-property description was considered, so unrelated descriptions got concatenated onto properties that share a structural shape with other, unrelated properties elsewhere in the schema. Fix: always prefer the property-specific description (descriptionForClassProperty) over the type-level one when rendering an object's properties, falling back to the type-level description only when no property-specific one exists. Adds a schema-to-schema regression fixture (schema-schema) that asserts on the description text of generated JSON Schema output, which the existing JSONSchemaFixture/JSONSchemaJSONFixture classes cannot express. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
…3056) Co-Authored-By: Claude <noreply@anthropic.com>
cJSON's generated deserializer does not validate absent required properties (the same known limitation that already skips required.schema and intersection.schema for this language), so the newly-added description-unification.1.fail.no-defaults.json negative sample round-trips instead of being rejected, failing the schema-cjson fixture. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
…der (#undefined) Merging master into agent/fix-issue-2801 surfaced a pre-existing master failure unrelated to this PR's own diff: PR #2357 added cjson-enum-default.test.ts asserting an alphabetically-ordered enum (SUBSCRIPTION_CONFIG, HEARTBEAT, STATE), but PR #1289 landed afterward and intentionally stopped ConvenienceRenderer.forEachEnumCase from alphabetizing enum cases, so cJSON now emits them in declaration order (SUBSCRIPTION_STATE, CONFIG, HEARTBEAT). Nobody updated the stale test expectation, so it has been failing on master HEAD itself since #1289 merged. Update the expected enum layout to match the correct, intended declaration-order behavior while keeping the actual behavior under test (the invalid/absent value 0 never collides with a real enumerator, which start at 1) unchanged. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
…on (#undefined) Merging master into agent/fix-issue-2801 surfaced another pre-existing-vs-new conflict: this PR's own JSONSchemaToSchemaFixture (added in #2801) is named "schema-schema", and master's PR #2557 independently added `new JSONSchemaFixture(languages.JSONSchemaLanguage)`, whose default name (`schema-${language.name}`) computes to the SAME "schema-schema" string. Running FIXTURE=schema-schema (as the CI matrix job does) ran both fixtures under one name, and the description-unification regression assertion added by this PR failed intermittently when interleaved with the unrelated fixture's many samples, even though the same quicktype call in isolation produces the correct description. Rename this PR's regression fixture to "schema-description-unification" (unique, verified against all other registered fixture names) and update the CI matrix in .github/workflows/test-pr.yaml to match. No production source or other fixtures touched. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
Elixir's generated deserializer does not enforce required struct keys at runtime (missing required properties are just set to nil), the same already-documented limitation that skips required.schema, intersection.schema, and strict-optional.schema for this language. The newly-added description-unification.1.fail.no-defaults.json negative sample round-trips instead of being rejected, failing the schema-elixir fixture. Skip it for the same reason, following the identical fix already applied for cJSON. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
…#undefined) Haskell's test driver encodes decode results as Maybe, so a failed decode prints "null" and exits 0 -- expected-failure samples for missing required properties cannot be detected. This is the same documented limitation that already skips required.schema and required-non-properties.schema for this language. The newly-added description-unification.1.fail.no-defaults.json negative sample is exactly this class of missing-required-property case, so fix it proactively (Haskell's job was cancelled by fail-fast before reaching this schema in the last run, but the root cause is identical to required.schema). Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
…e-76a-74 # Conflicts: # test/fixtures.ts # test/languages.ts
Generated-output differences36 files differ — 1 modified, 35 new, 0 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
Converting a Kubernetes CRD's OpenAPI v3 schema through quicktype's
schema-to-schema mode (
--lang schema --src-lang schema) produced adescriptionfordefinitions.Allow.properties.labelsthat was a garbledconcatenation of eleven unrelated description strings from all over the
input schema, instead of the single correct description that actually
belongs to that property (
"The labels to apply to the policy").Root cause
quicktype's type unifier merges structurally identical types across the
whole schema graph — e.g. every plain
{additionalProperties: string}"map of string" property anywhere in the schema (regardless of which
parent object it lives under) gets unified into a single shared
Typenode, since they're structurally indistinguishable.
DescriptionTypeAttributeKind.combine()inpackages/quicktype-core/src/attributes/Description.tsset-unions thedescriptiontype attribute from every occurrence when such types getunified, so that one shared map type ends up carrying descriptions from
every unrelated property that happens to have the same map-of-string
shape.
JSONSchemaRenderer.definitionForObject()inpackages/quicktype-core/src/language/JSONSchema/JSONSchemaRenderer.tsbuilt each property's schema via
schemaForType(p.type). For a type withno
$ref(i.e. it gets inlined directly),schemaForTypealready stampsthe merged type-level description onto the inlined schema. The renderer
then only applied the correct, property-specific description
(
descriptionForClassProperty, which is not merged across unrelatedproperties) when
prop.descriptionwas stillundefined— so the wrong,merged description always won whenever it was present.
Fix
Always prefer the property-specific description over the (possibly
merged) type-level one when rendering an object's properties, falling
back to the type-level description only when no property-specific
description exists. This is a small, localized change in
JSONSchemaRenderer.definitionForObject.Test coverage
Added a
description-unification.schemainput plus a dedicatedschema-schemafixture (JSONSchemaToSchemaFixtureintest/fixtures.ts) that runs quicktype schema-to-schema and asserts onthe
descriptiontext of the generated output for two unrelatedproperties that share the same map-of-string shape. The existing
JSONSchemaFixture/JSONSchemaJSONFixtureclasses only validate/compilegenerated code and don't inspect JSON Schema
descriptiontext, so thisrequired new (minimal) fixture plumbing rather than reusing an existing
one. Also wired the new
schema-schemafixture into.github/workflows/test-pr.yaml.Verification (local)
renderer fix and re-ran
QUICKTEST=true FIXTURE=schema-schema script/test; it failed with the merged description) and passes withthe fix restored.
definitions.Allow.properties.labels.descriptionnow correctly reads"The labels to apply to the policy"instead of the 11newline-joined descriptions.
npm run buildpasses.npx vitest run test/unit— 176 tests passed, no regressions.QUICKTEST=true FIXTURE=schema-typescript script/test— all 72 schemafixtures pass, confirming the new schema input compiles cleanly across
the existing per-language schema fixtures too.
runtime.
Fixes #2801
🤖 Generated with Claude Code