[BUG][Java][client] Erroneous support for nullable attributes

[bkabrda]

Bug Report Checklist

• Have you provided a full/minimal spec to reproduce the issue?
  • The included modules/openapi-generator/src/test/resources/3_0/petstore-with-fake-endpoints-models-for-testing.yaml can be used to reproduce this.
• Have you validated the input using an OpenAPI validator (example)?
• What's the version of OpenAPI Generator used?
  • All versions seem to be affected (I used 4.0.2)
• Have you search for related issues/PRs?
  • The same behaviour applied to Go, reported at [1] and fixed in go-experimental in [2].
• What's the actual output vs expected output?
  • The problem is that for attributes marked as nullable, null is never sent.
• [Optional] Bounty to sponsor the fix (example)

Description

The problem is very similar to what was described in [1] - if an attribute is marked as nullable in the OpenAPI spec, it's not actually possible to send a null value in a POST/PUT request for it (using okhttp/gson client, but AFAICS other clients would have these issues as well).

openapi-generator version

4.0.2, but the issue seems to always have been there.

OpenAPI declaration file content or url

modules/openapi-generator/src/test/resources/3_0/petstore-with-fake-endpoints-models-for-testing.yaml

Command line used for generation

openapi-generator -g java -i modules/openapi-generator/src/test/resources/3_0/petstore-with-fake-endpoints-models-for-testing.yaml -o /tmp/petstore/

Steps to reproduce

You just need to generate a Java client for OpenAPI spec that has any nullable attributes and see that it doesn't really send nulls for them in the request JSON objects.

Related issues/PRs

[1] #522
[2] #3371

Suggest a fix

My solution for Go from [2], which I believe is correct, considers 4 different states for every attribute:

1. not required, not nullable
2. required, not nullable
3. not required, nullable
4. required, nullable

All these 4 states need to be covered properly in order to be able to send the requests correctly according to the spec. Here's my thinking on how the 4 above scenarios should behave:

1. no change required, this already works (if attribute is null, then it's not serialized at all)
2. no change required, this already works (attribute can't be null, a non-null value is always sent)
3. (change) there has to be a flag for the attribute, which would carry the information whether or not a null value is explicit or not; if this flag is true and the attribute is null, it would get serialized as null; if this flag is false and the attribute is null, it wouldn't get serialized at all
4. (change) the attribute is serialized as null if it has null value

I propose that an {{name}}isExplicitNull attribute is added to the POJO class for every nullable attribute (defaultin to false as well as set{{name}}IsExplicitNull and is{{name}}ExplicitNull setter/getter. In addition to that, we'd need to add custom serialization logic that would respect these "explicit null flags" to properly serialize the request.

I'm also fairly certain that this can be done as a backwards compatible change, since it will only be adding things, not removing or changing the current API of the generated classes.

[auto-labeler]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[bkabrda]

Ok, so a potential way to go forward that I can see:

I will be detailing this out using jackson, since it seems to me that it would make it easier to implement than gson.

Scenario 4 (from the original issue report) is easy to solve, just mark the variable with @JsonInclude(JsonInclude.Include.ALWAYS).

Scenario 3 is a little bit complicated, but I think it could be solved like this:

• Create a simple equivalent of java.util.Optional - we can't use that java.util.Optional, as we want to support older Java versions (or maybe there's a library out there that implements this for older Java versions, I would have to see if that's the case).
• All fields that are not required and nullable would actually be Optional. This would be hidden behind the getters and setters, so the API of the generated client library wouldn't change.
• There would be an extra getter, e.g. getOptional{{name}}, which would return the Optional object. This would allow users to find out whether or not the API response contained explicit null or not.
• We would include custom logic in JSON.mustache that would know how to serialize Optional:
  • If Optional has never been set, it will be "empty" (== missing)
  • If Optional has been set to null (through calling the setter with null), the field would get serialized as null.
  • If Optional has been set to non-null value, then it will serialize as that value.

Deserialization should work out-of-the-box, since (IIUC) the setter is called if (and only if) the field is in the API response. Hence if the field is not in the API response, setter won't be called and the Optional will be "empty" (=> so even followup serialization of the same object will still work ok).

Does that make sense? I think this is easy enough that I could send a PR myself to implement it (for jackson).

[Fjolnir-Dvorak]

* Create a simple equivalent of `java.util.Optional` - we can't use that `java.util.Optional`, as we want to support older Java versions (or maybe there's a library out there that implements this for older Java versions, I would have to see if that's the case).

Phew, that would be (in my opinion) really ugly to create a new Optional. I would go as far as saying it is an 8+ feature only since older java versions are no longer officially supported I would not bother with making something ugly just to support some unsupported language versions.

The Idea of using a Monad to box the optional null sounds nice. The problem I see with this solution: Some people falsely assume that Optional adds some sort of null safety because an Optional can never be null *cough*. To make it clear to everyone I would only vote for that feature if all Optionals would be annotated with @Nullable. That's the problem programming in java, a language with a lot of promises, guesses and no Type-Safety (Nulltypes) :D

One big issue not using Optional is that Optional is a final class without an interface so all methods which are working with the Java-Implementation wouldn't work with the new implemented Optional.
On the other hand there is already another Optional in one of the dependencies (com.google.common.base.Optional) which could be used instead of the Java Optional?

[cbornet]

In the Spring codegen, we use a custom wrapper called JsonNullable which can have the three states: absent, present and null, present and non-null (where Optional can only have 2 states: empty or present).
I suggest to use it for the java clients also. See https://github.com/OpenAPITools/jackson-databind-nullable for documentation on JsonNullable

[bkabrda]

@cbornet looks nice! I'll try to swap it for the simple implementation that I did in my PR #3474, it looks like it could make it simpler.