[REQ][JAVA] Add JsonIgnoreProperties to model

[Fjolnir-Dvorak]

Work in progress. I only did the Pojo class so far. Enums and other specialities have to be checked, too.

PR checklist

• Read the contribution guidelines.
• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
• Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Added the annotation

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
@JsonIgnoreProperties(ignoreUnknown = true)

to every generated Pojo. This feature can be turned off via the flag disableAdditionalFieldsAnnotation.
This makes the generated code more independend from the attached default json parser.

Fixed Issues:

• fixes [REQ] [Java] Add JsonIgnoreProperties to model #3438

Technical Commitee for Java:

@bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04)

[Fjolnir-Dvorak]

I ran bin/java-pestore-all.sh and nothing happened. How do I repair the circleci test?

[macjohnny]

after running bin/java-pestore-all.sh, commit the changed files in samples/

[jmini]

The description of the flag is not really clear. Instead of

Disables the @JsonIgnoreProperties(ignoreUnknown = true) annotation

I would prefer something as:

Do not add the @JsonIgnoreProperties(ignoreUnknown = true) annotation on the generated model classes (jackson only)

---

I am also not sure about the flag name disableAdditionalFieldsAnnotation.

• disable: this is not really a "enable/disable" case
• AdditionalFields: the annotation is called @JsonIgnoreProperties

To not generate the @Generated annotation, there is an hideGenerationTimestamp flag.

---

Otherwise this feature looks interesting… I am wondering if true is the correct default value.

[jmini]

See also #3438 (comment) on the issue.

For the different jackson configuration, we need to decide (or to offer a way to configure) if we do it with annotations or with proper mapper configuration.

Instead of this disableAdditionalFieldsAnnotation option, we could solve more clearly with an option jacksonIgnoreProperties with values:

• annotation
• mapper
• annotation_and_mapper (makes less sense in my eyes)
• none

At the end, I think that both use-cases are interesting (annotation or mapper) and I do not think that it can be said that one approach is 100% better than the other

---

And we do this consistently for the different Jackson configuration option that we have.

For example for "AutoDetect" (described in #3573) we could have:

jacksonAutoDetect

• annotation
• mapper
• annotation_and_mapper
• none

---

What do you think?

[Fjolnir-Dvorak]

I like fine control in my models. If we explicitly declare every object as ignoreUnknown it is easy to change for the developer to specialize this case by removing this annotation, perhaps by adding a x-parameter like "x-forbid-additional-fields" which will in turn remove the annotation to let the client handle that as an bad response instead of silently throwing the error away.

Adding it as a default does not destroy anything so I do not see any harm in that. Per default it duplicates the ignore additional fields setting which I would consider as defensive. Removing the setting from the default mapper could lead to some unexpected behaviours if some people are reusing the json mapper for other purposes which is why I left that as it is.

About the option to select which variant you want to have: I understand that but would consider it as over-complicating? As an end user I am happy about every setting with which I do not need to bother.
I would also prefer boolean on-off switches instead of complex combi-switches. Boolean on-off switches are not positional-sensitive and do not permutate with the available options if another option is added to the set.

I will accept your description and naming suggestion/changes in my next commit.

[jmini]

As an end user I am happy about every setting with which I do not need to bother.

Let's say we go with one boolean switch jacksonHideIgnorePropertiesAnnotation or hideIgnorePropertiesAnnotation to just control the presence/absence of @JsonIgnoreProperties in the generated code.

---

Case 1:

If set to false (your suggested new default), the generated code ends up with Jackson configured twice:

• once in the mapper
• once as annotation on top of the model.

My understanding is that the setting on the mapper is no longer relevant, because the annotation wins.

---

Case 2:

The end user of OpenAPI-Generator would like to be notified when unknown properties are present (example scenario: the team develops a back-end and uses OpenAPI Generator to generate a client and is writing integration tests using the generated client), it goes through the docs and finds about the hideIgnorePropertiesAnnotation flag sets it to true and try again.

Result: same result because they need to also modify the generated mapper.

Not user friendly in my opinion

---

The advantage I see with an option to select the variant: with a single setting in OpenAPI Generator, you tell what you would like to have: not ignoring unknown properties, ignoring them at mapper level (current implementation), ignoring them at model level with annotation, or both (which could become the new default - to not introduce a breaking change in case users are using the mapper as you have mentioned).

---

The complexity comes from the number of options (not following a clear semantic about what they do). With this PR you are adding one.

If we want less complexity, let just not add the optinon and always generate the @JsonIgnoreProperties.

But in this case we remove the flexibilty for people that wants to be notified about unexpected properties in JSON. Right now they just need to exchange a setting in the object-mapper (manageable), if we always generate @JsonIgnoreProperties they can no longer have the desired behavior.

[Fjolnir-Dvorak]

I have another Idea, trying to merge both strategies. We could remove the flag to turn of the annotations and add a new flag which is feature oriented called "ignoreUnknownProperties" which defaults to true. This switch controls both, the object mapper and all annotations.
I would like to remove the option in the object mapper since the model is now defined explicit to ignore those properties, but that could lead to breaking changes in already existing programs.
That would remove the complexity but also would give the user the option to not ignore properties.

Since generated code is not edited by hand this hopefully should not lead to any problems. If the developer wants to deactivate the ignore feature he/she has to write his/her own mapper at the moment to be able to regenerate the code and update the API.
If generated code is changed manually and is added to the .openapiignore the generator is no longer able to guarantee any compatibility. I do not see the reason yet why we should support this case.

If the variant selection is preferred I would like to change that to an array of "features", that we do not have options like:

• annotation
• mapper
• annotation_and_mapper (makes less sense in my eyes)
• none

but instead a concatenated list of features which could lead to following options:

configureJsonParser

• model (annotation)
• mapper
• model,mapper
• mapper,model
• none

I would also only give the option where to configure all json parser configurations, if they should be done in each model directly or in the mapper. Each single option what can be configured or not would be configured with feature-flags like described above.
The other question is: What would make the most sense to use as default in this case?

[jmini]

I have another Idea, trying to merge both strategies.

I appreciate the effort you do.

a concatenated list of features which could lead to following options:

configureJsonParser

• model (annotation)
• mapper
• model,mapper
• mapper,model
• none

I like this approach.

Question 1) is there a difference between model,mapper and mapper,model ?

Question 2) About the name: configureJsonParser:
Jackson is not only for JSON but also for XML
If we think about gson which is the other json-serializer/deserializer library supported by the generator, there is no object mapper in this case, and the annotations on the model are different.

So this mapper vs model is really a Jackson thing.

Question 3) Last question: is this new option unique for all configuration option of Jackson (for all feature/annotation) or will there be one per feature:

• @JsonAutoDetect / MapperFeature.AUTO_DETECT_*
• @JsonPropertyOrder
• ...

---

The other question is: What would make the most sense to use as default in this case

For the next a patch version (3rd digit of the version), change the generated code as few as possible: I would let it to mapper (which is the current behavior).

For the next minor version (2d digit, e.g. 4.2.x) there we can change the default to be model (instead of mapper) as you have suggested.

[cbornet]

My opinion is that this kind of configuration should be done globally in the ObjectMapper and not be enforced in the POJO. I think it's already possible to configure the ObjectMapper so I don't think there is anything to do.
Also I don't think we should add yet another flag. We already have too many of them and it's confusing for the end user.

[jmini]

@cbornet: thank you a lot for your feedback.

My opinion is that this kind of configuration should be done globally in the ObjectMapper and not be enforced in the POJO.

I respect this opinion, but this is really opinionated.

---

I see @Fjolnir-Dvorak points with having it in the POJO with jackson-annotations.
This is why I think we need a clear concept for all options that are influencing how Jackson is used in the generated project (Object-Mapper vs Annotation on POJO). It is about naming of those flag and having always the same structure.

---

We can not close all PRs coming in with the idea to add the jackson-annotations as won't fix "please do it at Mapper level".

This PR is the illustration that the mapper approach does not work for everybody:
DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES is already false in the generated projects. This has the same effect than having @JsonIgnoreProperties on POJO, but annotation with POJO works better for @Fjolnir-Dvorak.

If we do not support both Mapper and Annotations we will have always PRs like this one and we will end up with contradiction between POJO and mapper.

---

I also think that the set of jackson-features can not be decided by the generator (I plan to add @JsonPropertyOrder, see #3629. It make sense for my use case but maybe some user don't need or don't want this). Again I do not see how we can not have a flag for that. But all those jackson flags needs to work in the similar way, because otherwise this creates complexity.

We can come up with the defaults we consider as the best, but some people wants to change those and if we do not provide flags the only option is to maintain custom templates. I do this for our project, but believe me it is hard.

[jeff9finger]

I agree with @cbornet in general. However, in this case, it seems like a useful thing to allow - use of either annotations or ObjectMapper.

The default behavior should be to allow this behavior to be defined in the ObjectMapper as it is now. If different behavior is required, it seems reasonable to allow that customization. Is a parameter the only way to provide this option? (I think so, but just wondering if other options are available or have been considered)

[Fjolnir-Dvorak]

I have the problem that I have to work with a client where I cannot set or change the jackson parser. In order for the generated code to work I have to assure that it just works without any custom jackson parser. My final goal would be that every configuration is made in the model such that the model is independend from the used parser. That could be done via just removing the configurations from the mapper and adding it to the models on a major release or to add flags.
As far as I see it does not seem to be popular to make the generator usable via adding flags (the only configuration option) which would mean or lead to that the user is compelled to writing his/her own generator / adapt existing generators which would also lead to maintenance hell. Is the value more worth than the cost to use the generator, have your own fork, distribute it around your company and have to maintain it to keep it up to date?
In my opinion the greatest value of a generator is to be configurable. No one can assume that a generator is easy to configure if (and only if) you want to configure everything. I see no possible way to not add parameters but at the same time wanting to keep the generator usable. A generator is a complex problem hence the addition of parameters makes it more complex but at the same time more usable.

I would like to rethink this whole pull request since I implemented it with the thought of minimal inversive changes:

• What are the runtime costs of moving the configuration out of the mapper to annotations on each model? One possibility could be a longer runtime when parsing or writing json since jackson needs to use more reflections in total.
• Is it of any interest for a generated-mapper-user if the generated code uses configuration or annotation? Is there any pro/con to do it in a specific way?
• what would be the best long term solution?

Some Ideas how this feature can be implemented general:

• like mentioned above configuring where the configuration should be
  • via one flag for all features
  • via one flag per feature / config option
• just use both, configuration and annotation, all the time
• remove all the configuration from the mapper into the model
• Use mapper as default, but add a flag which removes the mapper and moves the configuration to annotations.
• Do nothing and force some users to use a fork to be able to monkey patch this issue and create tickets and pull requests to try to solve this problem once and for all.

[jmini]

There was a lot of discussion with @OpenAPITools/generator-core-team yesterday.

I try to summarize what I understood from the vision:

• have as few config flag as possible (to ease maintenance)
• for Jackson, change the behavior with the mapper and not with annotations on POJO
• mapping the Jackson complexity as options in the code generator is not desired

If implemented this way, this would mean that we have a no-go for this change.

---

In my opinion the greatest value of a generator is to be configurable. No one can assume that a generator is easy to configure if (and only if) you want to configure everything.

I tend to agree with this.
I think it is OK to have more flags, but they need to respect some semantic, some naming convention and have clear description.

If two flags are influencing something close in the generated code (e.g. both changes the way jackson is configured), they need to have close name and close behavior, so that the user would expect them and would look for them.

---

What are the runtime costs of moving the configuration out of the mapper to annotations on each model? One possibility could be a longer runtime when parsing or writing json since jackson needs to use more reflections in total.

The problem with annotation on POJO is not the runtime cost.
If you use the "annotation on POJO" approach, you loose the flexibility to configure Jackson the way you want (annotations on POJO wins over Mapper configuration).

I can give you my example: I am sharing the models and I reuse them across different use cases:

• In my JaxRS layer (server side) I want to ignore unknown properties sent by my users
• In my test client (used during integration tests) I want to be notified when something unexpected is detected (warn on unknown properties).
• In the SDK used by my to customers, I would tend to ignore unknown properties, but I let this up to them. It can be good for them if they log warning when the server is sending them more information than what their SDK is deserializing, it is a way to identify that their SDK is outdated.

If you add @JsonIgnoreProperties (as proposed with this change), my use-cases can not be fulfilled.

---

Is it of any interest for a generated-mapper-user if the generated code uses configuration or annotation?

I am not sure I get this right. The vision of @cbornet is that the mapper configuration are generated by OpenAPI-Generator, but if someone wants it differently he is free to change the generated code (add to add it to .openapi-generator-ignore file).

---

like mentioned above configuring where the configuration should be

• via one flag for all features
• via one flag per feature / config option

I would tend to have one flag per jackson feature, otherwise we loose the flexibility [this is my personal opinion].

NOTE: the member of the core-team that discussed this so far are more against new configuration-option.

---

just use both, configuration and annotation, all the time

This does not work for my use case presented above [this is my personal opinion].

---

remove all the configuration from the mapper into the model

This does not work for my use case presented above [this is my personal opinion].

---

Do nothing and force some users to use a fork to be able to monkey patch this issue and create tickets and pull requests to try to solve this problem once and for all.

I agree with you that this is the risk.
I think this is not in the interest of anybody to fragment our user base.

[cbornet]

The vision of @cbornet is that the mapper configuration are generated by OpenAPI-Generator, but if someone wants it differently he is free to change the generated code

No that's not what I think. Sorry if I was unclear and made you feel that. My point is that the generator generates a default mapper that can be overridden. So you can configure a per-SDK mapper however you want.

[cbornet]

And the root issue of @Fjolnir-Dvorak is that he shares the same mapper for all clients and even with other non http-libs (drivers, etc...) which is a design smell as it introduces coupling.

[Fjolnir-Dvorak]

That is correct. The problem I have currently is ugly and I am trying to fix that (other teams are involved. That does not make it easier) that (hopefully) I will be able to use the provided mapper in the future.

If we do not consider the design flaw I have to work with another neat feature would be that all model files would be speaking by themselfs and would be independent from any mapper that the user is not able to use those files in a way which was not intended (at least it is more difficult). I also like the idea of being picky on where to allow additional properties which will be ignored and where not (openapi-property x-jackson-forbid-unknown. Could even be extended to other generators [x-forbid-unknown]) by adding a custom openapi property which toggles the boolean switch from true to false for that specific model which can be useful if the client is using a not yet stable API which is still volatile to prevent malfunctioning. I do not know if this is possible if the mapper is the main configuration (Does an annotation overrides the behaviour of the mapper? Then it would be possible) ignoring for the moment if this feature is wanted or not.

Since defaulting to annotations is not possible (use case of jmini) I would let the mapper configuration be the default but would like to give the user the possibility to "bullet-proof" their model as far as possible by moving the configuration into the model.

A per feature flag option would be the most versatile but would also have the biggest overhead in the template files and could possibly add a lot of flags. Also it has to be implemented for every feature separately which does make it harder to add a new jackson configuration, since a new flag has to be added, new configuration options for the template etc. If we are using only one flag all those features could be implemented in one optional template block which does make it kind of trivial to keep this feature consistent. The annotations could be extracted in a annotation file which is inserted in every model. that would minimize the files that needed to be changed on a configuration edit to perhaps two template files (mapper and annotation).

[Fjolnir-Dvorak]

I have no problem with throwing this pull request away. I had to implement this as a temporary fix. But I do not think that I am the only one who had this issue. Also it looks like a good feature which could be added to enhance the generator.

[Fjolnir-Dvorak]

Which way should I implement it yet? Seems to be still in discussion / unclear if or how to implement this feature

[Fjolnir-Dvorak]

what is the current state? Is this still a feature/change worth of implementing or shall I close this pull request? @cbornet @jmini

[wing328]

What about using vendor extensions instead?

e.g. adding x-java-json-ignore-properties to the model to enable it.

[wangq8]

Is this resolved?