[java] fix: okhttp-gson additionalProperties field makes every allOf child undeserializable

[wiebren]

With disallowAdditionalPropertiesIfNotPresent=false, additional_properties.mustache declares private Map<String, Object> additionalProperties; in every okhttp-gson model. An allOf child (Child extends Person) therefore declares the field twice, once itself and once inherited, and gson refuses the class as soon as anything asks for its adapter, before the model's own CustomTypeAdapterFactory can run:

java.lang.IllegalArgumentException: Class Child declares multiple JSON fields named 'additionalProperties'; conflict is caused by fields Child#additionalProperties and Person#additionalProperties

So every allOf child is undeserializable (and unserializable) under the flag. Reproduced on current master.

The fix

• JavaClientCodegen.postProcessAllModels (okhttp-gson only) tags each model whose parentModel chain carries the bag with x-inherits-additional-properties.
• Tagged models declare neither the field nor its getters; they override putAdditionalProperty only to keep the covariant return type.
• Their equals, hashCode and toString leave the bag to super.
• The field is declared once, on the topmost ancestor that has one, so a multi-level chain (Root <- Middle <- Leaf) works too.
• Flat models are byte-identical to master.

Tests

• JavaClientCodegenTest#testAdditionalPropertiesFieldIsDeclaredOncePerHierarchyForGson (3_0/allOf_extension_parent.yaml) and #testAdditionalPropertiesFieldIsDeclaredOnceAcrossMultiLevelAllOfForGson (new 3_0/java/okhttp-gson-additional-properties-allof-chain.yaml) assert that one class per hierarchy declares the bag and that descendants override putAdditionalProperty.
• JSONTest#testAdditionalPropertiesRoundTripOnAllOfChild in the okhttp-gson petstore sample round-trips a Dog with an undeclared property; it throws on master.

Known gaps

• A JSON property literally named additionalProperties is bound by gson's reflective delegate on every okhttp-gson model (it is stored twice, and a primitive or array value throws). This is pre-existing on master for flat models, allOf children now behave the same way, and it deserves a separate fix.

PR checklist

• Read the contribution guidelines.
• Built the project and regenerated every okhttp-gson sample. Only the allOf descendants change (Cat, Dog, ParentPet).
• Technical committee: @bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @karismann @Zomzog @lwlee2608 @martin-mfg

Merge note: some intermediate commit messages describe superseded iterations (transient, a clear(), a delegate copy), so please squash with the PR title and this description rather than the commit list.

---

Generated with Claude Code

[cubic-dev-ai]

1 issue 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/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>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

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>

[wiebren]

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.

[wiebren]

cubic's P1 was a real hole, now fixed: a model with children gets no CustomTypeAdapterFactory of its own ({{^hasChildren}} in pojo.mustache), so making its field transient dropped the bag - reflection was its only carrier. The transient modifier is now gated exactly the way the factory is: parents with children keep the field visible (their reflective round-trip is unchanged - Animal, GrandparentAnimal and friends revert in the samples), and an allOf child's transient declaration shadows the inherited visible one, so the duplicate-JSON-field conflict still cannot arise. The test now pins both sides: Child transient, Person (a parent with children) not.

On the other remarks:

• serializableModel: correct that transient also opts the bag out of Java serialization. That is a real trade-off against the gson conflict, which makes every allOf child undeserializable outright; if maintainers prefer, the modifier could additionally be gated on {{^serializableModel}} at the cost of reintroducing the gson break for that combination. Happy to add it on request.
• raw new Gson() users: also true - and already true of everything else the generated JSON class configures (type adapters, date formats, enums). The generated JSON instance is the supported serialization path.
• the now-no-op obj.remove("additionalProperties"): kept deliberately - it is what makes the write path correct for the non-transient (hasChildren) parents this revision reintroduces, and harmless where the field is transient.
• test depth: the codegen test asserts generated source, in line with the suite's conventions; the behavioral claim (round-trip with bag populated, absent/extra properties unchanged) was verified end to end against a generated client with gson directly, as described in the PR body.

[cubic-dev-ai]

1 existing issue remains and no new issues found across 147 files (changes from recent commits).

<sub>Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 146 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

1 existing issue remains and no new issues found across 150 files (changes from recent commits).

<sub>Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 9 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

1 issue found across 150 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/pojo.mustache">

<violation number="1">
P1: This read path now delegates `jsonObj` directly to the reflective adapter, which re-exposes a literal `additionalProperties` key to the inherited bag. Because the bag is declared once on the topmost ancestor and is no longer `transient`, the child's `thisAdapter.fromJsonTree` binds a literal `additionalProperties` key straight into the inherited `Map<String, Object>` field: non-object values (number, array, string, boolean) make Gson throw during deserialization, and object values are double-stored (the delegate fills the map, then the unchanged loop below calls `putAdditionalProperty("additionalProperties", …)` again), so the bag and the round-trip output gain spurious keys. This contradicts the PR goal, which calls for dropping the key before delegating so round-trips preserve the key once; the added JSONTest only exercises plain undeclared keys like `"extra"`, so it does not cover this. Restore the `delegateObj` deep copy + `remove("additionalProperties")` before `thisAdapter.fromJsonTree` for models with `x-inherits-additional-properties`.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: This read path now delegates jsonObj directly to the reflective adapter, which re-exposes a literal additionalProperties key to the inherited bag. Because the bag is declared once on the topmost ancestor and is no longer transient, the child's thisAdapter.fromJsonTree binds a literal additionalProperties key straight into the inherited Map<String, Object> field: non-object values (number, array, string, boolean) make Gson throw during deserialization, and object values are double-stored (the delegate fills the map, then the unchanged loop below calls putAdditionalProperty("additionalProperties", …) again), so the bag and the round-trip output gain spurious keys. This contradicts the PR goal, which calls for dropping the key before delegating so round-trips preserve the key once; the added JSONTest only exercises plain undeclared keys like "extra", so it does not cover this. Restore the delegateObj deep copy + remove("additionalProperties") before thisAdapter.fromJsonTree for models with x-inherits-additional-properties.

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/pojo.mustache, line 567:

<comment>This read path now delegates `jsonObj` directly to the reflective adapter, which re-exposes a literal `additionalProperties` key to the inherited bag. Because the bag is declared once on the topmost ancestor and is no longer `transient`, the child's `thisAdapter.fromJsonTree` binds a literal `additionalProperties` key straight into the inherited `Map<String, Object>` field: non-object values (number, array, string, boolean) make Gson throw during deserialization, and object values are double-stored (the delegate fills the map, then the unchanged loop below calls `putAdditionalProperty("additionalProperties", …)` again), so the bag and the round-trip output gain spurious keys. This contradicts the PR goal, which calls for dropping the key before delegating so round-trips preserve the key once; the added JSONTest only exercises plain undeclared keys like `"extra"`, so it does not cover this. Restore the `delegateObj` deep copy + `remove("additionalProperties")` before `thisAdapter.fromJsonTree` for models with `x-inherits-additional-properties`.</comment>

<file context>
@@ -563,18 +563,8 @@ public class {{classname}} {{#parent}}extends {{{.}}} {{/parent}}{{#vendorExtens
-             {{/vendorExtensions.x-inherits-additional-properties}}
              // store additional fields in the deserialized instance
-             {{classname}} instance = thisAdapter.fromJsonTree({{#vendorExtensions.x-inherits-additional-properties}}delegateObj{{/vendorExtensions.x-inherits-additional-properties}}{{^vendorExtensions.x-inherits-additional-properties}}jsonObj{{/vendorExtensions.x-inherits-additional-properties}});
+             {{classname}} instance = thisAdapter.fromJsonTree(jsonObj);
              for (Map.Entry<String, JsonElement> entry : jsonObj.entrySet()) {
                if (!openapiFields.contains(entry.getKey())) {
</file context>

Suggested change

	}{{#hasVars}}
	{{#vendorExtensions.x-inherits-additional-properties}}
	// the inherited bag is visible to reflection, so hide a literal `additionalProperties`
	// key from the delegate (it would try to bind any JSON type into the Map); the loop
	// below still collects it from the raw JSON, like every other undeclared key
	JsonObject delegateObj = jsonObj;
	if (jsonObj.has("additionalProperties")) {
	delegateObj = jsonObj.deepCopy();
	delegateObj.remove("additionalProperties");
	}
	{{/vendorExtensions.x-inherits-additional-properties}}
	{{classname}} instance = thisAdapter.fromJsonTree({{#vendorExtensions.x-inherits-additional-properties}}delegateObj{{/vendorExtensions.x-inherits-additional-properties}}{{^vendorExtensions.x-inherits-additional-properties}}jsonObj{{/vendorExtensions.x-inherits-additional-properties}});

[wiebren]

This is the known gap the description lists: a literal additionalProperties JSON key holding a non-object value throws when gson's reflective delegate binds it to the bag field. It is not new to this PR. On master every model that declares the bag behaves the same way, because the field is visible to reflection there too; this PR keeps allOf children on that same behaviour instead of failing on the duplicate field. Fixing it for all models belongs in its own change.