fix: keep allOf-inherited "any type" maps from degrading to free-form object - #24547
fix: keep allOf-inherited "any type" maps from degrading to free-form object#24547donald wants to merge 2 commits into
Conversation
… object
A property whose additionalProperties schema declares no type is an "any
type" map. When such a property is inherited through allOf, the generated
value type silently changed from "any" to "free-form object", so the same
property produced two different data types in the same run:
Base.style: { [key: string]: any; } // declared here
Child.style: { [key: string]: object; } // inherited via allOf
The allOf composition branch in DefaultCodegen.fromModel calls
mergeProperties(), which deep-copies every inherited property schema via
ModelUtils.cloneSchema(). cloneSchema delegates to AnnotationsUtils.clone,
a JSON round-trip, and swagger-core's deserializer materializes a
sub-schema that has no type as an ObjectSchema (type: object):
Schema inner = new Schema().description("can be any type");
Schema outer = new ObjectSchema().additionalProperties(inner);
// before: class = Schema, type = null, isFreeFormObject = false
// after: class = ObjectSchema, type = object, isFreeFormObject = true
getPrimitiveType() then matches the isFreeFormObject branch and returns
"object", never reaching the isAnyType branch that returns "AnyType".
cloneSchema already guards a custom top-level type across the round-trip
but does not cover sub-schemas. This extends the same idea: after cloning,
walk the schema tree and clear the type wherever the original did not
declare one (additionalProperties, items, properties, allOf/anyOf/oneOf,
not). Schemas that genuinely declare `type: object` are untouched.
Tests: three ModelUtilsTest cases (typeless additionalProperties, nested
typeless schemas, and a declared `type: object` that must stay an object)
plus an end-to-end TypeScriptFetchClientCodegenTest that asserts inherited
properties keep the same types as the ones declared on the parent.
Regenerating all 787 sample configs produces no diff.
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java:2533">
P2: The `visited` identity set is keyed on the *original* schema objects, but the clone it protects against is produced by `AnnotationsUtils.clone`, which round-trips the schema through JSON. A JSON round-trip does not preserve shared object references, so a single original Schema instance that is reused at multiple places in the tree (e.g. the same free-form/additionalProperties schema referenced from two properties, or the same typeless items schema in two arrays) becomes several *distinct* clone objects. On first visit the original is added to `visited` and its first clone is fixed up, but every later occurrence of that same original is skipped, so those clones keep `type: object` and the PR's stated goal (a typeless sub-schema must stay typeless/`any`) is silently not met for the second location. Note that because the clone is built by JSON (de)serialization, the original graph cannot actually contain cycles (serialization would fail), so the visited set provides no cycle protection — it only causes these false skips. Consider dropping the visited set (relying on the null/instanceof guards) or keying it on the (original, clone) pair that is actually being fixed.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * @param visited identity set of already visited original schemas, guards against cycles | ||
| */ | ||
| private static void restoreTypelessSubSchemas(Schema original, Schema clone, Set<Schema> visited) { | ||
| if (original == null || clone == null || !visited.add(original)) { |
There was a problem hiding this comment.
P2: The visited identity set is keyed on the original schema objects, but the clone it protects against is produced by AnnotationsUtils.clone, which round-trips the schema through JSON. A JSON round-trip does not preserve shared object references, so a single original Schema instance that is reused at multiple places in the tree (e.g. the same free-form/additionalProperties schema referenced from two properties, or the same typeless items schema in two arrays) becomes several distinct clone objects. On first visit the original is added to visited and its first clone is fixed up, but every later occurrence of that same original is skipped, so those clones keep type: object and the PR's stated goal (a typeless sub-schema must stay typeless/any) is silently not met for the second location. Note that because the clone is built by JSON (de)serialization, the original graph cannot actually contain cycles (serialization would fail), so the visited set provides no cycle protection — it only causes these false skips. Consider dropping the visited set (relying on the null/instanceof guards) or keying it on the (original, clone) pair that is actually being fixed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java, line 2533:
<comment>The `visited` identity set is keyed on the *original* schema objects, but the clone it protects against is produced by `AnnotationsUtils.clone`, which round-trips the schema through JSON. A JSON round-trip does not preserve shared object references, so a single original Schema instance that is reused at multiple places in the tree (e.g. the same free-form/additionalProperties schema referenced from two properties, or the same typeless items schema in two arrays) becomes several *distinct* clone objects. On first visit the original is added to `visited` and its first clone is fixed up, but every later occurrence of that same original is skipped, so those clones keep `type: object` and the PR's stated goal (a typeless sub-schema must stay typeless/`any`) is silently not met for the second location. Note that because the clone is built by JSON (de)serialization, the original graph cannot actually contain cycles (serialization would fail), so the visited set provides no cycle protection — it only causes these false skips. Consider dropping the visited set (relying on the null/instanceof guards) or keying it on the (original, clone) pair that is actually being fixed.</comment>
<file context>
@@ -2505,10 +2505,70 @@ public static Schema cloneSchema(Schema schema, boolean openapi31) {
+ * @param visited identity set of already visited original schemas, guards against cycles
+ */
+ private static void restoreTypelessSubSchemas(Schema original, Schema clone, Set<Schema> visited) {
+ if (original == null || clone == null || !visited.add(original)) {
+ return;
+ }
</file context>
Addresses review feedback on the visited set in restoreTypelessSubSchemas. The parser reuses a single Schema instance across several places in a spec (the InlineModelResolver tests document this for external types), while the JSON round-trip inside AnnotationsUtils.clone gives each of those places its own clone object. Keying the visited set on the original meant only the first occurrence got fixed up; every later one silently kept type: object, so the fix did not hold for the second location. Keying on the clone fixes every occurrence. The set is not needed for cycle protection either -- the clone is produced by JSON (de)serialization, so the original graph cannot contain cycles in the first place -- but keying on the clone keeps a cheap guard without the false skips. New test testCloneKeepsEveryOccurrenceOfAReusedTypelessSchemaTypeless reproduces the reported scenario; on the previous version it fails with "second kept type: object expected [null] but found [object]". Regenerating all 787 sample configs still produces no diff.
|
Good catch — fixed in 7416148. You are right on both points. The parser does reuse a single I keyed the set on the clone rather than dropping it: each clone node is distinct, so every occurrence now gets fixed, and a cheap guard remains. Added Full test suite and all 787 sample configs re-run — still no diff. |
Fixes #24546
What
A property whose
additionalPropertiesschema declares notypeis an "any type" map. When such aproperty is inherited through
allOf, the generated value type silently changed from "any" to"free-form object" — so the same property produced two different data types in the same run:
Why
The
allOfcomposition branch inDefaultCodegen.fromModelcallsmergeProperties(), whichdeep-copies every inherited property schema via
ModelUtils.cloneSchema().cloneSchemadelegates toAnnotationsUtils.clone— a JSON round-trip — and swagger-core's deserializer materializes a sub-schemathat has no
typeas anObjectSchema(type: object):DefaultCodegen.getPrimitiveType()then matches theModelUtils.isFreeFormObject(...)branch and returns"object", never reaching theModelUtils.isAnyType(...)branch that returns"AnyType"(mapped toanyby the TypeScript generators).
This is generator-agnostic —
typescript-fetchis just where it is most visible.How
cloneSchema()already guards a custom top-level type across the round-trip, but does not coversub-schemas. This extends the same idea: after cloning, walk the schema tree and clear the type wherever
the original did not declare one (
additionalProperties,items,properties,allOf/anyOf/oneOf,not). An identity-based visited set guards against cycles.Schemas that genuinely declare
type: objectare untouched — there is an explicit test for that so the fixcannot over-reach.
Testing
ModelUtilsTest— 3 new cases: typelessadditionalProperties, nested typeless schemas(
properties/items), and a declaredtype: objectthat must stay an object.TypeScriptFetchClientCodegenTest.testAllOfInheritedFreeFormMapStaysAny— end-to-end, asserts theinherited properties on
Childcarry exactly the same types as the ones declared onBase.Verified that this test fails without the fix.
./bin/generate-samples.sh ./bin/configs/*.yaml) and./bin/utils/export_docs_generators.sh— no diff. The fix is inert on the entire existing samplecorpus; only the buggy case changes.
PR checklist
(Both scripts were run; neither produced any change, so there is nothing to commit beyond the fix and its tests.)
masterThe change is in the language-agnostic core (
ModelUtils), so it is not owned by one language committee.Pinging the TypeScript technical committee since that is where the symptom shows up:
@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @topce @akehir @petejohansonxo @amakhrov @davidgamero @mkusaka @joscha
CC also @wing328 as this touches shared codegen behaviour.
Summary by cubic
Fixes a bug where “any type” maps inherited via allOf were generated as free-form objects, causing parent and child models to disagree. Inherited properties now keep the correct
anyvalue type across generators (notablytypescript-fetch).ModelUtils.cloneSchema, after cloning, walk sub-schemas and cleartypewherever the original had none (additionalProperties,items,properties,allOf/anyOf/oneOf,not) with a visited set keyed on the clone to handle reused schemas; genuinetype: objectstays intact.typescript-fetch; no changes to existing samples.Written for commit 7416148. Summary will update on new commits.