[java] fix: okhttp-gson additionalProperties field makes every allOf child undeserializable - #24895
Conversation
…Of child
With disallowAdditionalPropertiesIfNotPresent=false every model declares
private Map<String, Object> additionalProperties - so an allOf child
declares the field twice, once itself and once inherited, and gson refuses
the class before the model's own TypeAdapterFactory can run:
IllegalArgumentException: Class Child declares multiple JSON fields
named 'additionalProperties'
Every allOf child model is undeserializable (and unserializable). Mark the
field transient: the custom TypeAdapterFactory already reads and writes the
bag explicitly around the reflective delegate - on write it even removes
the additionalProperties key the delegate emitted - so hiding it from
gson's reflection changes nothing for flat models and unbreaks the children.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
There was a problem hiding this comment.
4 issues found across 147 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="samples/client/petstore/java/okhttp-gson/src/main/java/org/openapitools/client/model/ArrayDefault.java">
<violation number="1" location="samples/client/petstore/java/okhttp-gson/src/main/java/org/openapitools/client/model/ArrayDefault.java:131">
P3: Now that additionalProperties is transient, the default Gson Excluder excludes it from thisAdapter.toJsonTree in the write() method for every model, so `obj.remove("additionalProperties")` is a guaranteed no-op. Consider deleting it (and leaving the loop that writes the bag entries).</violation>
</file>
<file name="samples/client/petstore/java/okhttp-gson-swagger1/src/main/java/org/openapitools/client/model/User.java">
<violation number="1" location="samples/client/petstore/java/okhttp-gson-swagger1/src/main/java/org/openapitools/client/model/User.java:268">
P2: Marking the additionalProperties field `transient` also removes it from Java serialization, not just Gson reflection. When `--serializable-model` is enabled, the generated model implements `java.io.Serializable` (AbstractJavaCodegen line 2215), and the `transient` modifier causes the additional-properties bag to be dropped on Java serialization. Previously the bag round-tripped through Java serialization; now it is silently lost. Prefer a Gson-specific exclusion (e.g. a custom Excluder or a `@JsonAdapter`-driven binding) rather than the Java `transient` modifier, which is shared with the JVM serialization mechanism.</violation>
</file>
<file name="samples/client/petstore/java/okhttp-gson/src/main/java/org/openapitools/client/model/Zebra.java">
<violation number="1" location="samples/client/petstore/java/okhttp-gson/src/main/java/org/openapitools/client/model/Zebra.java:167">
P1: Making additionalProperties transient silently drops the bag for inheritance base classes. The template emits `transient` on the additionalProperties field unconditionally, but the `CustomTypeAdapterFactory` that re-reads/writes the bag is only generated inside `{{^hasChildren}}` (pojo.mustache line 514). Base classes with children (Animal.java, GrandparentAnimal.java) therefore get a transient field with no adapter to restore it: Gson's default reflective adapter now excludes the field, so additionalProperties is no longer serialized and never repopulated on deserialization, whereas before the change it round-tripped as a nested "additionalProperties" object. Gate the transient modifier on the same condition that emits the CustomTypeAdapterFactory (e.g. `{{^hasChildren}}`), or generate a factory for these base classes, so leaf classes benefit from the fix while base classes keep working.</violation>
</file>
<file name="samples/client/petstore/java/okhttp-gson/src/main/java/org/openapitools/client/model/NumberOnly.java">
<violation number="1" location="samples/client/petstore/java/okhttp-gson/src/main/java/org/openapitools/client/model/NumberOnly.java:90">
P2: The added JavaClientCodegenTest#testAdditionalPropertiesFieldIsTransientForGson does not validate the behavior the PR claims it verifies. It only asserts that the generated Child/Person source contains the literal text "private transient Map<String, Object> additionalProperties;". It never constructs a Gson instance, never runs fromJson/toJson, and never checks the bag round-trips, despite the PR description saying it "Verifies gson.fromJson round-trips with the bag populated; extra/absent properties behave as before." As written, the test would pass even if the allOf deserialization were still broken, so the regression is not actually guarded. Add a real round-trip assertion (e.g., generate the models, then use JSON.getGson().fromJson on a Child with a header field and verify getAdditionalProperties()/serialization output), or correct the PR/test description.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * parent) from declaring two JSON fields of one name. | ||
| */ | ||
| private Map<String, Object> additionalProperties; | ||
| private transient Map<String, Object> additionalProperties; |
There was a problem hiding this comment.
P1: Making additionalProperties transient silently drops the bag for inheritance base classes. The template emits transient on the additionalProperties field unconditionally, but the CustomTypeAdapterFactory that re-reads/writes the bag is only generated inside {{^hasChildren}} (pojo.mustache line 514). Base classes with children (Animal.java, GrandparentAnimal.java) therefore get a transient field with no adapter to restore it: Gson's default reflective adapter now excludes the field, so additionalProperties is no longer serialized and never repopulated on deserialization, whereas before the change it round-tripped as a nested "additionalProperties" object. Gate the transient modifier on the same condition that emits the CustomTypeAdapterFactory (e.g. {{^hasChildren}}), or generate a factory for these base classes, so leaf classes benefit from the fix while base classes keep working.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/java/okhttp-gson/src/main/java/org/openapitools/client/model/Zebra.java, line 167:
<comment>Making additionalProperties transient silently drops the bag for inheritance base classes. The template emits `transient` on the additionalProperties field unconditionally, but the `CustomTypeAdapterFactory` that re-reads/writes the bag is only generated inside `{{^hasChildren}}` (pojo.mustache line 514). Base classes with children (Animal.java, GrandparentAnimal.java) therefore get a transient field with no adapter to restore it: Gson's default reflective adapter now excludes the field, so additionalProperties is no longer serialized and never repopulated on deserialization, whereas before the change it round-tripped as a nested "additionalProperties" object. Gate the transient modifier on the same condition that emits the CustomTypeAdapterFactory (e.g. `{{^hasChildren}}`), or generate a factory for these base classes, so leaf classes benefit from the fix while base classes keep working.</comment>
<file context>
@@ -159,8 +159,12 @@ public void setClassName(@javax.annotation.Nonnull String className) {
+ * parent) from declaring two JSON fields of one name.
*/
- private Map<String, Object> additionalProperties;
+ private transient Map<String, Object> additionalProperties;
/**
</file context>
There was a problem hiding this comment.
Fixed - this was a real hole. transient is now gated on {{^hasChildren}}, exactly like the CustomTypeAdapterFactory it depends on: a parent with children keeps the field bound for reflection (its bag round-trips as before), and the child excludes only its own copy, so exactly one field stays bound and no duplicate-name conflict arises. The test pins both sides.
| * parent) from declaring two JSON fields of one name. | ||
| */ | ||
| private Map<String, Object> additionalProperties; | ||
| private transient Map<String, Object> additionalProperties; |
There was a problem hiding this comment.
P2: Marking the additionalProperties field transient also removes it from Java serialization, not just Gson reflection. When --serializable-model is enabled, the generated model implements java.io.Serializable (AbstractJavaCodegen line 2215), and the transient modifier causes the additional-properties bag to be dropped on Java serialization. Previously the bag round-tripped through Java serialization; now it is silently lost. Prefer a Gson-specific exclusion (e.g. a custom Excluder or a @JsonAdapter-driven binding) rather than the Java transient modifier, which is shared with the JVM serialization mechanism.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/java/okhttp-gson-swagger1/src/main/java/org/openapitools/client/model/User.java, line 268:
<comment>Marking the additionalProperties field `transient` also removes it from Java serialization, not just Gson reflection. When `--serializable-model` is enabled, the generated model implements `java.io.Serializable` (AbstractJavaCodegen line 2215), and the `transient` modifier causes the additional-properties bag to be dropped on Java serialization. Previously the bag round-tripped through Java serialization; now it is silently lost. Prefer a Gson-specific exclusion (e.g. a custom Excluder or a `@JsonAdapter`-driven binding) rather than the Java `transient` modifier, which is shared with the JVM serialization mechanism.</comment>
<file context>
@@ -260,8 +260,12 @@ public void setUserStatus(@javax.annotation.Nullable Integer userStatus) {
+ * parent) from declaring two JSON fields of one name.
*/
- private Map<String, Object> additionalProperties;
+ private transient Map<String, Object> additionalProperties;
/**
</file context>
There was a problem hiding this comment.
Correct, and worth stating plainly: transient also opts the bag out of Java serialization when serializableModel is on. That is a genuine trade-off against the alternative, which is that every allOf child model is undeserializable by gson outright. If maintainers prefer the other side of it for that combination, the modifier can additionally be gated on {{^serializableModel}} - happy to push that on request.
| * parent) from declaring two JSON fields of one name. | ||
| */ | ||
| private Map<String, Object> additionalProperties; | ||
| private transient Map<String, Object> additionalProperties; |
There was a problem hiding this comment.
P2: The added JavaClientCodegenTest#testAdditionalPropertiesFieldIsTransientForGson does not validate the behavior the PR claims it verifies. It only asserts that the generated Child/Person source contains the literal text "private transient Map<String, Object> additionalProperties;". It never constructs a Gson instance, never runs fromJson/toJson, and never checks the bag round-trips, despite the PR description saying it "Verifies gson.fromJson round-trips with the bag populated; extra/absent properties behave as before." As written, the test would pass even if the allOf deserialization were still broken, so the regression is not actually guarded. Add a real round-trip assertion (e.g., generate the models, then use JSON.getGson().fromJson on a Child with a header field and verify getAdditionalProperties()/serialization output), or correct the PR/test description.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/java/okhttp-gson/src/main/java/org/openapitools/client/model/NumberOnly.java, line 90:
<comment>The added JavaClientCodegenTest#testAdditionalPropertiesFieldIsTransientForGson does not validate the behavior the PR claims it verifies. It only asserts that the generated Child/Person source contains the literal text "private transient Map<String, Object> additionalProperties;". It never constructs a Gson instance, never runs fromJson/toJson, and never checks the bag round-trips, despite the PR description saying it "Verifies gson.fromJson round-trips with the bag populated; extra/absent properties behave as before." As written, the test would pass even if the allOf deserialization were still broken, so the regression is not actually guarded. Add a real round-trip assertion (e.g., generate the models, then use JSON.getGson().fromJson on a Child with a header field and verify getAdditionalProperties()/serialization output), or correct the PR/test description.</comment>
<file context>
@@ -82,8 +82,12 @@ public void setJustNumber(@javax.annotation.Nullable BigDecimal justNumber) {
+ * parent) from declaring two JSON fields of one name.
*/
- private Map<String, Object> additionalProperties;
+ private transient Map<String, Object> additionalProperties;
/**
</file context>
There was a problem hiding this comment.
Fair. The codegen test asserts generated source, which is the convention in JavaClientCodegenTest and what makes it cheap to run in this suite. The behavioural claim - gson.fromJson of an allOf child round-trips with the bag populated, absent and extra properties unchanged - was verified end to end against a generated client with a real Gson instance, as described in the PR body; that verification does not have a natural home in this test class, but I am glad to add a runtime test under the java sample suites if you would like it pinned in CI.
| * parent) from declaring two JSON fields of one name. | ||
| */ | ||
| private Map<String, Object> additionalProperties; | ||
| private transient Map<String, Object> additionalProperties; |
There was a problem hiding this comment.
P3: Now that additionalProperties is transient, the default Gson Excluder excludes it from thisAdapter.toJsonTree in the write() method for every model, so obj.remove("additionalProperties") is a guaranteed no-op. Consider deleting it (and leaving the loop that writes the bag entries).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/java/okhttp-gson/src/main/java/org/openapitools/client/model/ArrayDefault.java, line 131:
<comment>Now that additionalProperties is transient, the default Gson Excluder excludes it from thisAdapter.toJsonTree in the write() method for every model, so `obj.remove("additionalProperties")` is a guaranteed no-op. Consider deleting it (and leaving the loop that writes the bag entries).</comment>
<file context>
@@ -123,8 +123,12 @@ public void setWithoutDefault(@javax.annotation.Nullable List<String> withoutDef
+ * parent) from declaring two JSON fields of one name.
*/
- private Map<String, Object> additionalProperties;
+ private transient Map<String, Object> additionalProperties;
/**
</file context>
There was a problem hiding this comment.
Kept deliberately. With the {{^hasChildren}} gate now in place, the obj.remove("additionalProperties") is no longer a guaranteed no-op: a parent with children keeps its field bound for reflection, so the delegate does emit the key for those models and the removal is what keeps it out of the output. For models where the field is transient it is simply harmless.
…heir own
Review caught that models with children get no CustomTypeAdapterFactory
({{^hasChildren}} in pojo.mustache), so a transient field on such a parent
silently dropped its bag - reflection was its only carrier. The transient
modifier is now gated the same way the factory is: parents with children
keep the field visible (their reflective round-trip is unchanged), and an
allOf child's transient declaration shadows it, so the duplicate-field
conflict still cannot arise.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
|
cubic's P1 was a real hole, now fixed: a model with children gets no On the other remarks:
|
There was a problem hiding this comment.
2 existing issues remain and no new issues found across 147 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Re-trigger cubic
Gson does not use Java field-hiding semantics: it collects the declared fields of each class in the hierarchy and refuses two bound to one JSON name, so the child's transient declaration excludes its own copy rather than hiding the parent's. Same behaviour, correct description - the comment ships into every generated model. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
There was a problem hiding this comment.
1 issue found across 146 files (changes from recent commits).
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/resources/Java/libraries/okhttp-gson/additional_properties.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/Java/libraries/okhttp-gson/additional_properties.mustache:12">
P1: With a three-level `allOf` inheritance chain, the leaf still encounters duplicate `additionalProperties` fields: every ancestor with children remains non-transient, while only the leaf is hidden. Hide all but one field per inheritance hierarchy and add a multi-level regression test.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| * declared fields of every class in the hierarchy and rejects two bound to one JSON | ||
| * name, which is what an allOf child - declaring the field itself and inheriting it - | ||
| * used to hit. A parent with children has no factory of its own, so it keeps the field | ||
| * bound for reflection; the child excludes only its own copy, leaving exactly one. |
There was a problem hiding this comment.
P1: With a three-level allOf inheritance chain, the leaf still encounters duplicate additionalProperties fields: every ancestor with children remains non-transient, while only the leaf is hidden. Hide all but one field per inheritance hierarchy and add a multi-level regression test.
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/resources/Java/libraries/okhttp-gson/additional_properties.mustache, line 12:
<comment>With a three-level `allOf` inheritance chain, the leaf still encounters duplicate `additionalProperties` fields: every ancestor with children remains non-transient, while only the leaf is hidden. Hide all but one field per inheritance hierarchy and add a multi-level regression test.</comment>
<file context>
@@ -5,10 +5,11 @@
+ * declared fields of every class in the hierarchy and rejects two bound to one JSON
+ * name, which is what an allOf child - declaring the field itself and inheriting it -
+ * used to hit. A parent with children has no factory of its own, so it keeps the field
+ * bound for reflection; the child excludes only its own copy, leaving exactly one.
*/
private {{^hasChildren}}transient {{/hasChildren}}Map<String, Object> additionalProperties;
</file context>
With
disallowAdditionalPropertiesIfNotPresent=false,additional_properties.mustachedeclares a private field in every okhttp-gson model:
An allOf child (
Child extends Person) therefore declares the field twice — once itself,once inherited — and gson refuses the class the moment any code asks for its adapter,
before the model's own
CustomTypeAdapterFactorycan run:The conflict surfaces on
gson.fromJson(element, Child.class)and equally onserialization: the custom adapter's
getDelegateAdapterchains to the reflective factory,which walks both fields. Every allOf child model is undeserializable under the flag.
Clients largely get away with it because allOf hierarchies typically live in error bodies,
which are rarely deserialized into their models; found while running a generated client
whose discriminated error responses are deserialized, against a production registry API
on v7.15.0.
The fix
Mark the field
transient— one word. The generatedCustomTypeAdapterFactoryalreadyreads and writes the bag explicitly around the reflective delegate (on write it even
removes the
additionalPropertieskey the delegate emitted), so hiding the field fromgson's reflection changes nothing for flat models and unbreaks every allOf child. Verified
end to end against the generated client:
gson.fromJsonof an allOf child round-tripswith the bag populated, and absent/extra properties behave exactly as before.
Tests
JavaClientCodegenTest#testAdditionalPropertiesFieldIsTransientForGsongenerates theexisting
3_0/allOf_extension_parent.yamlfixture (Child extends Person) with the flagand asserts both classes declare the field
transient. Fails without the template change(verified by stashing only the template).
PR checklist
./bin/generate-samples.sh ./bin/configs/java-*.yaml).145 sample models change, uniformly: the one-word
transientplus the field's javadocnote explaining it.
Generated with Claude Code
Summary by cubic
Marks the okhttp-gson
additionalPropertiesfield astransienton models without children, so gson no longer refuses to deserialize allOf child models that declare the same inherited field.disallowAdditionalPropertiesIfNotPresent=false, an allOf child previously declaredadditionalPropertiestwice (its own and inherited), causing gson to throwIllegalArgumentExceptionbefore the model'sCustomTypeAdapterFactorycould run.JavaClientCodegenTestthat verifies the child declares the field astransientand the parent does not.Written for commit 2b160d6. Summary will update on new commits.