Don't use visible = true in Java model classes

[delenius]

This causes errors when deserializing polymorphic classes.

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 and ./bin/security/{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\.
• Filed the PR against the correct branch: master, 3.1.x, 4.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

When deserializing polymorphic classes using a discriminator property in the Java client, I get these errors from Jackson:

javax.ws.rs.ProcessingException: com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field <fieldname> not marked as ignorable

This is because we have @JsonTypeInfo(..., visible = true) in the model classes. This PR removed the visible = true part, which fixes the problem.

Tagging Java tech committee:
@bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01)

[jmini]

Can you please provide some more information about this "polymorphic classes" case?
A small OpenAPI Specification example would be great.

My guess is that this visible = true is there for a reason (I am not saying this PR is wrong). I want to ensure that changing it does not break other stuff.
Can you also explain what the visible property in the @JsonTypeInfo is doing? (pointing to a documentation is also fine).

Related to this topic, for 3.2.0 we have changed the way discriminator are handled => #536

[jeff9finger]

visible = true is used to allow the discriminator to be visible in the json. It is generally needed when a discriminator is specified. As @jmini requested, please provide an example of an Open API Spec that exhibits the problem.

The following issues from the swagger-codegen project may provide more details on this issue - these deal with the @JsonTypeInfo and may not be specific to "visible", but I think they might be helpful here.

• [Java] Fix SubClass annotations missing from the base class swagger-api/swagger-codegen#4085
• [java] discriminator not defined when inside an allOf: inline object swagger-api/swagger-codegen#4226
• Add a vendor extension to specify the discriminator value for a model definition swagger-api/swagger-codegen#4244

[delenius]

The official documentation is here.

Property that defines whether type identifier value will be passed as part of JSON stream to deserializer (true), or handled and removed by TypeDeserializer (false). Property has no effect on serialization.
Default value is false, meaning that Jackson handles and removes the type identifier from JSON content that is passed to JsonDeserializer.

My understanding is that the difference is in what "layer" the property is seen. In order for Jackson to correctly use the discriminator property, the TypeDeserializer, and not the JsonDeserializer needs to handle the discriminator property. I guess you set it to true if you want to "manually" handle the discriminator property.

[delenius]

@0v1se , do your changes to the 3.2 branch require visible = true? Have you tested this with the RESTEasy client? It just doesn't work currently.

[delenius]

Oh, I just discovered something. The reason it didn't work for me was because I had registered a custom ObjectMapper. The one in the generated client has this:

mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);

whereas mine didn't. I can just retrieve and modify the existing ObjectMapper and add my custom setting instead:

apiClient.getJSON().getContext(Object.class).disable(SerializationFeature.FAIL_ON_EMPTY_BEANS);

BTW, one could argue that this setting should be a default as well. I had to add it in order to support POST requests with an empty body.

I think the visible=true setting is still probably wrong and not needed, but I will let someone else decide what to do. It is not really a problem, other than that you lose error checking for actual unknown properties, other than the discriminator.

[wing328]

@delenius when you've time, can you please resolve the merge conflicts?

[jmini]

@wing328 we first need to find out if we want to remove visible = true or not... If I understood @delenius correctly he is no longer sure about this.

[wing328]

@delenius we'll close this PR for the time being as it has been opened for a while. Please file a new one if you still want to add these changes to the master branch.

[jeff9finger]

IIRC, if "x-discriminator-value" is specified, then "visible" is required in the '@JsonTypeInfo'. Otherwise it can be false.

To be clear, what I am suggesting is to set '@JsonTypeInfo(..., visible = true)' only when "x-discriminator-value" is non null. And set it to false (or not add it at all) otherwise.

It seems that this may provide a solution to this issue