[Java] Add a new additional property to configure Jackson's `failOnUnknownProperties` #19271

fistons · 2024-07-30T21:45:39Z

This MR should fix #10306

It adds a new additional and non mandatory property to change the generated Jackson's ObjectMapper's FAIL_ON_UNKNOWN_PROPERTIES's value

If this property is not set, we keep the default value, i.e. true

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh ./bin/configs/*.yaml
./bin/utils/export_docs_generators.sh
```
(For Windows users, please run the script in Git BASH)
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

…knownProperties`

wing328 · 2024-08-04T09:54:13Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java

@@ -322,6 +325,7 @@ public AbstractJavaCodegen() {
        cliOptions.add(CliOption.newString(ADDITIONAL_MODEL_TYPE_ANNOTATIONS, "Additional annotations for model type(class level annotations). List separated by semicolon(;) or new line (Linux or Windows)"));
        cliOptions.add(CliOption.newString(ADDITIONAL_ONE_OF_TYPE_ANNOTATIONS, "Additional annotations for oneOf interfaces(class level annotations). List separated by semicolon(;) or new line (Linux or Windows)"));
        cliOptions.add(CliOption.newBoolean(OPENAPI_NULLABLE, "Enable OpenAPI Jackson Nullable library", this.openApiNullable));
+        cliOptions.add(CliOption.newBoolean(FAIL_ON_UNKNOWN_PROPERTIES, "Fail Jackson de-serialization on unknown properties", this.failOnUnknownProperties));


shall we add this option to java client codegen java class only as only java client needs this option?

Indeed, that's a good idea, I will make the change

martin-mfg · 2024-08-05T15:08:01Z

modules/openapi-generator/src/main/resources/Java/libraries/retrofit2/JSON_jackson.mustache

@@ -30,7 +30,7 @@ public class JSON implements ContextResolver<ObjectMapper> {
    mapper = JsonMapper.builder()
            .serializationInclusion(JsonInclude.Include.NON_NULL)
            .configure(MapperFeature.ALLOW_COERCION_OF_SCALARS, false)
-            .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, true)
+            .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, {{failOnUnknownProperties}})


Hi, could you please make this change for other libraries besides retrofit2 as well?

Hi, yes, I can do this, the only issue is that some libraries are configured to fail on unknown properties, some aren't.

I see that apache-httpclient, feign, google-api-client, restassured, restclient, resteasy, vertx and webclient currently have DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES set to false, whereas jersey2, jersey3, native and retrofit2 have DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES set to true

Is there an elegant way to tell JavaClientCodegen to default the failOnUnknownProperties variable to true or false, depending of the chosen library?

Good point.

My preferred approach would be to use this opportunity to unify the behaviour of the different libraries. Since the majority uses false as default, let's make false the default value of failOnUnknownProperties for all libraries.
This would also make this PR a "breaking change with fallback", i.e. it should become part of OpenAPI Generator 7.8.0, not of 7.7.1. But I think wing328 will take care of this aspect.

Thank you!

Some tests in samples/openapi3/client/petstore/java/jersey2-java8/ are failing now. Apparently due to the custom deserialization logic in this sample, which relies on FAIL_ON_UNKNOWN_PROPERTIES being true. Can you please fix this by simply adding in bin/configs/java-jersey2-8.yaml the following line somewhere under additionalProperties:?

failOnUnknownProperties: true

Fixed! Thanks for the hint, I had trouble finding where it came from

…ofit2

Add the `failOnUnknownProperties: true` additional properties to generate the expected mapper

ashirman · 2024-08-09T12:51:57Z

modules/openapi-generator/src/main/resources/Java/libraries/jersey3/JSON.mustache

@@ -30,7 +30,7 @@ public class JSON implements ContextResolver<ObjectMapper> {
    mapper = JsonMapper.builder()
            .serializationInclusion(JsonInclude.Include.NON_NULL)
            .configure(MapperFeature.ALLOW_COERCION_OF_SCALARS, false)
-            .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, true)
+            .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, {{failOnUnknownProperties}})


are there plans to have something similar for Kotlin generators too? I got exactly the same issue using Kotlin + okhttp3 client and seems like there we use the same properties and in Java's generators.

I can probably take a look once this PR is merged. Is there a issue already?

thanks a lot. I am not aware of formal issue yet but please let me know if I should open one.

Yes, that would be great!

here it is #19408 . Thanks!

Bennett-Lynch · 2024-08-28T14:41:59Z

Can we entertain the possibility of defaulting FAIL_ON_UNKNOWN_PROPERTIES to false? There is no sane client/server configuration where this should be true.

fistons · 2024-08-28T14:58:09Z

Can we entertain the possibility of defaulting FAIL_ON_UNKNOWN_PROPERTIES to false? There is no sane client/server configuration where this should be true.

That would be the new default with this MR

Bennett-Lynch · 2024-08-28T15:17:55Z

Can we entertain the possibility of defaulting FAIL_ON_UNKNOWN_PROPERTIES to false? There is no sane client/server configuration where this should be true.

That would be the new default with this MR

Amazing, thank you. I thought we were just adding a new config property. :)

wing328 · 2024-08-29T05:25:33Z

thanks for the PR, which has been merged

[Java] Add a new additional property to configure Jackson's `failOnUn…

6f8f318

…knownProperties`

wing328 reviewed Aug 4, 2024

View reviewed changes

Move FAIL_ON_UNKNOWN_PROPERTIES property to JavaClientCodegen

1cf8590

martin-mfg reviewed Aug 5, 2024

View reviewed changes

fistons and others added 3 commits August 6, 2024 00:29

Template FAIL_ON_UNKNOWN_PROPERTIES for other libraries beside retr…

38a15fb

…ofit2

Default failOnUnknownProperties to false for all Java Client libraries

a20b4bf

Fix integration tests

36f76f2

Add the `failOnUnknownProperties: true` additional properties to generate the expected mapper

fistons force-pushed the fail-on-unknown-properties-variable branch from 93c9b1a to 36f76f2 Compare August 7, 2024 11:16

martin-mfg approved these changes Aug 8, 2024

View reviewed changes

ashirman reviewed Aug 9, 2024

View reviewed changes

ashirman mentioned this pull request Aug 21, 2024

[REQ] Kotlin clients add a new additional property to change the generated Jackson's ObjectMapper's FAIL_ON_UNKNOWN_PROPERTIES's value #19408

Closed

wing328 added Breaking change (with fallback) Client: Java Enhancement: Feature labels Aug 29, 2024

wing328 added this to the 7.9.0 milestone Aug 29, 2024

wing328 merged commit dfc381c into OpenAPITools:master Aug 29, 2024
75 checks passed

simonhir mentioned this pull request Oct 31, 2024

chore(deps): update dependency org.openapitools:openapi-generator-maven-plugin to v7.9.0 it-at-m/refarch#220

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Java] Add a new additional property to configure Jackson's `failOnUnknownProperties` #19271

[Java] Add a new additional property to configure Jackson's `failOnUnknownProperties` #19271

fistons commented Jul 30, 2024 •

edited

Loading

wing328 Aug 4, 2024

fistons Aug 4, 2024

martin-mfg Aug 5, 2024

fistons Aug 5, 2024

martin-mfg Aug 6, 2024

fistons Aug 6, 2024

martin-mfg Aug 7, 2024

fistons Aug 7, 2024

ashirman Aug 9, 2024

fistons Aug 9, 2024

ashirman Aug 13, 2024

fistons Aug 14, 2024

ashirman Aug 21, 2024

Bennett-Lynch commented Aug 28, 2024

fistons commented Aug 28, 2024

Bennett-Lynch commented Aug 28, 2024

wing328 commented Aug 29, 2024

[Java] Add a new additional property to configure Jackson's failOnUnknownProperties #19271

[Java] Add a new additional property to configure Jackson's failOnUnknownProperties #19271

Conversation

fistons commented Jul 30, 2024 • edited Loading

PR checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bennett-Lynch commented Aug 28, 2024

fistons commented Aug 28, 2024

Bennett-Lynch commented Aug 28, 2024

wing328 commented Aug 29, 2024

[Java] Add a new additional property to configure Jackson's `failOnUnknownProperties` #19271

[Java] Add a new additional property to configure Jackson's `failOnUnknownProperties` #19271

fistons commented Jul 30, 2024 •

edited

Loading