Repository navigation
Conversation
…erties The isJsonNull guard in the okhttp-gson CustomTypeAdapterFactory (pojo.mustache) was merged to master via OpenAPITools#24573 without referencing OpenAPITools#24393 and without regression tests, which left the issue open. Fixes OpenAPITools#24393 - generator-level test JavaOkHttpGsonNullAdditionalPropertiesTest: generates an additionalProperties:true model with the java/okhttp-gson generator, compiles the generated CustomTypeAdapterFactory against the real Gson runtime (same fixture approach as JavaOkHttpGsonOneOfPrimitiveArrayTest) and asserts a serialize -> deserialize -> serialize round trip with a null undeclared-property value, plus unchanged behavior for string/array/object entry values - sample test JSONTest.testAdditionalPropertiesNullValue with the petstore AdditionalPropertiesClass repro from the issue No template changes and no sample regeneration required: okhttp-gson samples already carry the guard from OpenAPITools#24573, and a targeted regen of bin/configs/java-okhttp-gson.yaml produces no diff.
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/client/petstore/java/okhttp-gson/src/test/java/org/openapitools/client/JSONTest.java">
<violation number="1" location="samples/client/petstore/java/okhttp-gson/src/test/java/org/openapitools/client/JSONTest.java:788">
P3: `deserialized` is populated from unrelated hard-coded JSON, so this does not exercise the claimed serialize→deserialize→serialize path. Parse the serialized output instead, and initialize `map_property` on `model` if that field must remain asserted.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| assertFalse(model.toJson().contains("unknownField")); | ||
|
|
||
| // deserialize: JSON null undeclared properties are stored as null-valued entries | ||
| AdditionalPropertiesClass deserialized = AdditionalPropertiesClass.fromJson("{\"map_property\":{\"key\":\"value\"},\"unknownField\":null}"); |
There was a problem hiding this comment.
P3: deserialized is populated from unrelated hard-coded JSON, so this does not exercise the claimed serialize→deserialize→serialize path. Parse the serialized output instead, and initialize map_property on model if that field must remain asserted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/petstore/java/okhttp-gson/src/test/java/org/openapitools/client/JSONTest.java, line 788:
<comment>`deserialized` is populated from unrelated hard-coded JSON, so this does not exercise the claimed serialize→deserialize→serialize path. Parse the serialized output instead, and initialize `map_property` on `model` if that field must remain asserted.</comment>
<file context>
@@ -765,6 +765,39 @@ public void testAdditionalArrayProperties() throws IOException {
+ assertFalse(model.toJson().contains("unknownField"));
+
+ // deserialize: JSON null undeclared properties are stored as null-valued entries
+ AdditionalPropertiesClass deserialized = AdditionalPropertiesClass.fromJson("{\"map_property\":{\"key\":\"value\"},\"unknownField\":null}");
+ assertEquals(deserialized.getMapProperty(), Collections.singletonMap("key", "value"));
+ assertTrue(deserialized.getAdditionalProperties().containsKey("unknownField"));
</file context>
There was a problem hiding this comment.
Fixed in 349bca4: the generator-side test (JavaOkHttpGsonNullAdditionalPropertiesTest) now runs the full serialize -> deserialize -> serialize round-trip through the client's configured Gson. It generates the client (model + supporting JSON factory), compiles the generated sources against the real Gson runtime (gson-fire, okio and jsr305 added as test-scoped dependencies of the generator module) and takes the Gson from the generated JSON.getGson() instead of a hand-built builder over a model stub. A model instance with a null-valued additionalProperties entry is serialized, the resulting JSON is fed back into fromJson, the null entry is asserted to survive the round trip, and re-serialization is asserted byte-stable; the existing no-throw-on-serialize assertion is kept. The generator-side test and the sample JSONTest (34 tests) both pass locally.
…t Gson Rework JavaOkHttpGsonNullAdditionalPropertiesTest so the claimed serialize -> deserialize -> serialize round trip is actually exercised: the test generates the client (model + supporting JSON factory), compiles the generated sources against the real Gson runtime (gson-fire, okio and jsr305 added as test-scoped dependencies of the generator module) and takes the Gson from the generated JSON.getGson() instead of a hand-built GsonBuilder over a model stub. The model instance with a null undeclared-property value is serialized, the resulting JSON is deserialized back, the null entry is asserted to survive the round trip and re-serialization is asserted byte-stable. The existing no-throw-on-serialize assertion (the ApiClient's Gson omits named nulls) is kept.
…ope provider Declaring com.google.code.findbugs:jsr305 directly at test scope made Maven mediate the transitive compile-scope jsr305 (via guava) down to test scope, so javax.annotation.Nullable disappeared from the main compile classpath and AbstractPythonCodegen, AbstractRustCodegen and K6ClientCodegen failed to compile on JDK 11/17 CI jobs. The generated okhttp-gson sources still resolve javax.annotation.Nullable from the transitive compile-scope artifact, which also covers the test classpath. Verified locally: modules/openapi-generator main compile + the new JavaOkHttpGsonNullAdditionalPropertiesTest pass. Signed-off-by: Inf1mum <vadimlazuk5@gmail.com>
|
The Fixed in 1ebfe98 by removing the direct jsr305 declaration (the generated okhttp-gson sources still resolve |
…piler can read okio 3.4.0 (added in 349bca4 as a compile-time runtime dep for the generated okhttp-gson JSON factory) also landed on the module-wide test classpath, which KotlinTestUtils feeds into kotlin-compiler-embeddable 1.6.21 when compiling generated Kotlin. okio 3.4.0 carries Kotlin metadata 1.8.0 and broke KotlinTestUtilsTest, KotlinMicroprofileClientCodegenTest and KotlinSpringServerCodegenTest ('binary version of its metadata is 1.8.0, expected version is 1.6.0'). okio 3.2.0 carries metadata 1.6.0 and keeps the generated-client compile path working. Signed-off-by: Inf1mum <vadimlazuk5@gmail.com>
Fixes #24393
Context
#24393 reports that the okhttp-gson
CustomTypeAdapterFactoryinpojo.mustachethrowsjava.lang.IllegalStateException: Not a JSON Object: nullwhen a model withadditionalProperties: trueserializes an undeclared property whose value is
null(e.g. an unknown field deserialized from aserver response that was JSON null):
gson.toJsonTree(null)returnsJsonNull.INSTANCEand theelsebranch calledgetAsJsonObject()on it.The template guard proposed in the issue is already on master: it was merged in #24573 (based on
#24395), which added the
jsonElement.isJsonNull()check writingJsonNull.INSTANCEplus thecom.google.gson.JsonNullimport, and regenerated the samples. That PR did not reference #24393and did not add tests, which left the issue open without regression coverage.
This PR closes that gap with tests only, so the issue can be closed on merge. No template or
generated-sample changes are needed.
Changes (tests only)
modules/openapi-generator/src/test/resources/3_0/java/okhttp-gson-null-additional-properties.yaml:minimal spec with an
additionalProperties: truemodel.modules/openapi-generator/src/test/java/org/openapitools/codegen/java/JavaOkHttpGsonNullAdditionalPropertiesTest.java:generates that model with the
java/okhttp-gsongenerator, compiles the generatedCustomTypeAdapterFactoryagainst the real Gson runtime (same fixture approach asJavaOkHttpGsonOneOfPrimitiveArrayTest), and asserts:Not a JSON Object: null, both at JsonElement level and in full text output;additionalProperties;samples/client/petstore/java/okhttp-gson/src/test/java/org/openapitools/client/JSONTest.java:new
testAdditionalPropertiesNullValuereproducing the issue's exact repro with the generatedpetstore
AdditionalPropertiesClass(putAdditionalProperty("unknownField", null)+toJson()).Regenerated samples
None required. This PR contains no template changes; the okhttp-gson samples already carry the
JsonNullguard since #24573, and re-running./bin/generate-samples.sh bin/configs/java-okhttp-gson.yamlon this branch produces no diff in
samples/client/petstore/java/okhttp-gson(verified).Verification
./mvnw -pl modules/openapi-generator -am test -Dtest=JavaOkHttpGsonNullAdditionalPropertiesTestpasses.mvn test -Dtest=JSONTestinsamples/client/petstore/java/okhttp-gsonpasses (34 tests, including the new one).PR checklist
./mvnw package -DskipTests,and the targeted regen
./bin/generate-samples.sh bin/configs/java-okhttp-gson.yamlproducesno diff on this branch (see "Regenerated samples" above). All changed files are committed.
(@mentions intentionally omitted)
Summary by cubic
Adds regression tests for the already-merged fix to #24393, where the okhttp-gson
CustomTypeAdapterFactorythrewIllegalStateException: Not a JSON Object: nullwhen serializing a null undeclared-property value. The fix shipped in #24573 without test coverage; this tests-only PR closes that gap so the issue can be closed. No template changes or sample regeneration needed.additionalProperties: truemodel plus the client's JSON factory, compiles both against the real Gson runtime, and verifies a serialize → deserialize → serialize round trip with a null value through the generatedJSON.getGson(), plus unchanged behavior for string/array/object values. Addsgson-fireandokio(pinned to 3.2.0 so its Kotlin metadata stays readable by the module's test compiler) as test-scoped dependencies;jsr305stays transitive fromguavaso the main compile classpath is unaffected.AdditionalPropertiesClass.JSONTest.javato match the modified file.Written for commit 7bf3c79. Summary will update on new commits.