[Java][client] make it possible to send explicit nulls for nullable fields

[bkabrda]

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.

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

Description of the PR

This is my attempt to fix #3435. Basically it makes sure that explicit null values can be sent for nullable fields:

• For a required nullable field, the null is always sent when that's the value.
• For an optional (not required) nullable field, I introduced a simple new Optional class with OptionalFilter that make sure that the null is sent if and only if it was set explicitly. AFAICS, the API of any of the generated classes didn't change in a backwards-incompatible way.

Discussion points:

• I added the Optional since java.util.Optional was added in JDK 8 and the templates still have support for previous Java versions. If desired, I can replace this with a different (third party) implementation pretty easily.
• I didn't run the ./bin/... generators yet to keep the PR clean and reviewable. I'm planning to run it later.
• I only added this for jackson, as I didn't figure out an easy way to do this for gson. I could try working on a different PR that would do this for gson, but that would take me a lot more time.

[cbornet]

Please see my comment in #3435 . Wouldn't JsonNullable be a good fit for this ?

[bkabrda]

@cbornet I added a commit that removes the custom Optional implementation that I did and instead uses JsonNullable. Thanks for the suggestion, it works like a charm.

Some more things I still need to do:

• I made sure that elements that map to null in maps with nullable items are preserved, but I didn't really check what the behavior is for arrays, so I need to see how that works.
  • [Edit] - I verified that nulls in lists are preserved at all times, which is what I consider to be the right behaviour.
• If/when this change seems fine, I'll have to update JSON.mustache and pom.xml of all the other library templates that use jackson to make sure they include JsonNullable relevant code.

[cbornet]

Is there a need for a vendor extension ? I think it can be wrapped with JsonNullable as soon as the field is "nullable".

[bkabrda]

This was my line of thinking with the vendor extension:

I needed to have a bunch of "if and else" conditions in the pojo template to determine if the context have jackson, !required and nullable set at the same time. I could theoretically drop the !required as condition (even though I like having the simplicity of going with JsonInclude.Include.ALWAYS for required fields), but I can't drop the jackson as condition.
If I didn't have this as one variable in the mustache rendering context, it'd be hard doing the "else" part. Additionally, since jackson is Java specific, I decided to use a "vendor" variable for this, as the other uses of vendor extensions in other codegens seemed to be similar to what I'm doing here.

I'm of course ok with improving this if you have a better idea.

[bkabrda]

@cbornet hmm, so I noticed that this isn't really working. The "explicit nulls" (IOW JsonNullable that is defined and set to null) are not serialized as null. Any idea on what I'm doing wrong? I can't seem to make it work.

[bkabrda]

Right, so it seems that this works when I use the jackson annotations on the {{getter}}_JsonNullable instead of the attribute itself. I'm guessing that this may be caused by the internal implementation of jackson and how it works with private attributes and their getters/setters. I'm going to push an additional commit that fixes this.

[bkabrda]

Fixed and seems to work fine. This is now pretty much ready from my side, except I'll still have to edit pom.xml/JSON.java templates in other generators using jackson and also actually regenerate all affected samples.

[bkabrda]

As requested, I'm sharing a simple example of what a generated pojo would look like. There are some extraneous whitespaces that I need to get rid of in my changes, but everything seems to work. (Note that none of the samples really illustrate this good and I can't share my real work yet, so this is a made up example).

The biggest changes are on optional-nullable fields and maps that contain null values, so I'm sharing an example with these two.

[edit] note that one more notable change is not providing null value as default value to reference objects, which doesn't play good with this approach. This is achieved by the change in AbstractJavaCodegen: https://github.com/OpenAPITools/openapi-generator/pull/3474/files#diff-f7863d157cb04293ab52ff741f420c0cR805

public class MySchema {
  // mymap has additionalProperties that are nullable
  private Map<String, Long> mymap = new HashMap<String, Long>();

  // end is a nullable, not required Long
  private JsonNullable<Long> end = JsonNullable.undefined();

  public MonitorOptions mymap(Map<String, Long> mymap) {
    
    this.mymap = mymap;
    return this;
  }

  public MonitorOptions putMymapItem(String key, Long mymapItem) {
    if (this.mymap == null) {
      this.mymap = new HashMap<String, Long>();
    }
    this.mymap.put(key, mymapItem);
    return this;
  }


   /**
   * Get mymap
   * @return mymap
  **/
  @javax.annotation.Nullable
  @ApiModelProperty(value = "")

  @JsonProperty("mymap")
  @JsonInclude(content = JsonInclude.Include.ALWAYS, value = JsonInclude.Include.USE_DEFAULTS)


  public Map<String, Long> getMymap() {
    return mymap;
  }

  public void setMymap(Map<String, Long> mymap) {
    this.mymap = mymap;
  }

  public Downtime end(Long end) {
    this.end = JsonNullable.of(end);
    
    return this;
  }

  @JsonProperty("end")
  @JsonInclude(value = JsonInclude.Include.USE_DEFAULTS)

  public JsonNullable<Long> getEnd_JsonNullable() {
    return end;
  }

  public void setEnd_JsonUndefined() {
    this.end = JsonNullable.undefined();
  }

   /**
   * Get end
   * @return end
  **/
  @javax.annotation.Nullable
  @ApiModelProperty(example = "1412793983", value = "")

  public Long getEnd() {
        return end.orElse(null);
  }

  public void setEnd(Long end) {
    this.end = JsonNullable.of(end);
  }
}

[bkabrda]

I pushed some additional commits:

• one to remove some of the extraneous workspace
• one to add the JsonNullable integration to the rest of library templates that use jackson
• one that actually regenerates all the samples

[bkabrda]

(Sorry for so many force-pushes, I'm just trying to squash all the bugs/typos... I've already figured out that I can run the tests for the regenerated libraries individually on my laptop, so I'll try to fix everything before the next force push which will hopefully also be the last one.)

[jmini]

Sorry for so many force-pushes

No worry.

If you prefer, you can also push multiple mini-commits to your branch. At the end all of them will be squashed into one single commit on the master branch.

[bkabrda]

So finally all tests are passing. Can I get another round of review? Thanks!

CC @cbornet

[jimschubert]

I think JsonNullable might be confusing if someone uses withXml for an API with mixed JSON and XML endpoints. Would a nullable property in such a scenario serialize correctly for the xml endpoint?

I kinda wish the JsonNullable object didn't include the transport protocol in the name. It seems it would be nice to call it a Nullable.

[bkabrda]

I think JsonNullable might be confusing if someone uses withXml for an API with mixed JSON and XML endpoints. Would a nullable property in such a scenario serialize correctly for the xml endpoint?

So I agree with you on the naming part, but there's not much I can do about that unless I wanted to radically change the jackson-databind-nullable library.

I have to admit I didn't try to use this with XML endpoints. I was assuming that jackson would automagically do the correct thing, but I do agree that it's something to test for sure.

I kinda wish the JsonNullable object didn't include the transport protocol in the name. It seems it would be nice to call it a Nullable.

Yeah, I guess that would be better from this perspective, but I honestly consider that to be out of scope of this PR, as it's already complex as it is.

[jmini]

Is this feature always included, or is it something the user can opt-in/opt-out?

I see that some code in the template is guarded with {{#vendorExtensions.isJacksonOptionalNullable}}..{{/vendorExtensions.isJacksonOptionalNullable}}

But not all of them.

[bkabrda]

This feature is always included.

The isJacksonOptionalNullable is basically the only reasonable way of expressing the following condition in the mustache template: The serialization library is jackson AND this field is optional AND this field is nullable (to be precise, I also need to be able to express negation of this condition, which would be the hard part in mustache). In the places that this is not used, one (or more) of the clauses in this condition are false or not required and thus this is not necessary everywhere.

Hope this makes it clearer. I'd be happy to add some comments if that would improve the situation.

[bkabrda]

Just to make the above statement clearer: this feature is always included for Java clients that use jackson as their parsing library - with the exception of jersey1, which doesn't work with new enough version of jackson (which is required for JsonNullable to work).

[jimschubert]

I'm wondering if it might address everyone's comments in the PR to change the vendor extension to a generator option useJsonNullable. This way, you can set the default according to the logic you've already applied, but it makes the functionally both discoverable and overridable from a user's perspective.

[bkabrda]

@jimschubert yeah, I think that sounds fair. I can do that. FTR, what do you think about making it default tu true to turn this feature on by default? From my POV it fixes current deficiencies (as opposed to bringing a completely new feature).

[jimschubert]

@bkabrda I think defaulting to true is fine. In minor releases, we allow breaking changes with fallback. I think without a user facing option, this would break all existing Java clients meaning a code change in a minor release. The option (as I understand the changes) allows a user to fallback to similar code as before.

[cbornet]

As always I'm reluctant to add yet another option. If we think that's the best way to handle nullables then we should enforce it.

[bkabrda]

Ok folks, I really appreciate all the feedback that I received, so thanks for that. As for adding/not adding the new option, I guess that's your decision and I will implement whatever way you want. Let me try to sum up the pros of both ways:

Pros of having a new option:

• People who want to preserve exactly the same (admittedly broken) behavior that all the Java clients using Jackson had up until now will be able to do so.

Pros of not having a new option:

• Less complexity with not introducing a new option (which would likely be completely removed in a future version anyway).
• Fixing the broken behavior for everyone all the time.

Personally I would prefer to not have a new option, since the existing methods of the generated POJOs stay stable, so there is no breaking change to their API - there are only additions. The only thing left to verify is what the behavior is for XML clients, I'll try to do that today (but no promises, this PR has unfortunately already eaten too much of my time and I have to attend to other things as well).

[bkabrda]

@jimschubert @cbornet alternatively, if it seems that there's no agreement on the form in which the feature is accepted, would it make sense to:

• Separate this PR into 2 smaller PRs:
  • A PR that would only do the Java code changes, IOW it would fill in the mustache context with the variables that are necessary to make this work - this change wouldn't hurt anyone, as it wouldn't change anything on the template level
  • A PR that would do the rest of the changes (including adding the actual imports in the Java code)
• We could then keep discussing the second PR until everyone is happy with it

I'd love to do at least that, since that would make it possible for me to not have to build my own openapi-generator - I would only have to use my custom templates, which is much easier to do for my usecase.

Would that make sense to you? I don't mean to bother you too much, but I really need to make this work in a reasonable way for a project I'm working on. Thanks for considering and thanks for keeping the discussion constructive 👍

[bkabrda]

So after some testing, it seems that JsonNullable doesn't serialize correctly to XML right now. This makes me think that we should adopt the approach with 2 PRs that I noted above and implement XML serialization to JsonNullable before the second PR can be merged. I'll open an RFE for JsonNullable in the meanwhile. Does this sound good to everyone?

[bkabrda]

Ok, so the tests are passing now and I feel like I've addressed all the comments that were raised. Please let me know if this is ok to be merged or if there's something more that I need to improve on.

[wing328]

@bkabrda I wonder if you can help resolve the merge conflicts when you've time. (I've merged @jmini PR before yours)

I'll put other Java PR on hold before merging this one.

[bkabrda]

Rebased and pushed - let's wait to see if the tests pass. I actually found out one minor bug while testing this PR and fixed it - this made me realize that there are actually no tests for OpenAPI 3 Java clients. I did as much testing as I could, but this is pretty complex and someone could break it with their PRs. When this is merged, I'd love to write tests, but can't guarantee 100 % that I'll have time to do so, so I'd appreciate if someone could help with this (testing various combinations of nullable/required/readOnly/container fields and how they behave during serialization would be great).

[wing328]

I actually found out one minor bug while testing this PR and fixed it

@bkabrda can you please elaborate a bit more on the bug you fixed?

[bkabrda]

@wing328 it was related to #3615 - previously, I was assuming in add{{AListField}}Item and put{{AMapField}}Item that the attributes were never null, which got changed by that PR to being initially null. I discovered that during my local testing, but it would be really awesome to have some tests for at least one jackson-based Java client for OpenAPI 3 for various combination of field attributes as I noted in my comment above.

[bkabrda]

(Oh, I just realized I might have misunderstood your question. I didn't find a bug that was already in current code of openapi-generator, I found a bug that was result of combining PR #3615 with this PR when I rebased. That's why I said we should have tests for openapi 3 for Java clients.)

[wing328]

That's why I said we should have tests for openapi 3 for Java clients.)

Totally agreed. I added new some tests earlier this week: #3640

Please feel free to add more later with a separate PR.

[wing328]

Tested lcoally and the code compiles without issues

[jmini]

That's why I said we should have tests for openapi 3 for Java clients.

I am going to present something based on Mock-Server in #689, feel free to follow this issue and/or to add your comment.

---

I am sorry I could not test this before it was merged, so I am reviewing the impact of this PR now.

I observe @JsonProperty being moved from the field to the getters.
=> is that intended?

I observe @JsonInclude(value = JsonInclude.Include.USE_DEFAULTS) on the getter.
=> What is the impact?

@bkabrda I hope you can elaborate on those points

[bkabrda]

I observe @JsonProperty being moved from the field to the getters.

Yes, It's intended. It was required for the JsonNullable attribute (I couldn't get this to work with JsonProperty on the JsonNullable attribute itself for some reason, but I couldn't figure out why). It is fully backwards compatible AFAICS.

I observe @JsonInclude(value = JsonInclude.Include.USE_DEFAULTS) on the getter.

Yeah, I used this to make sure we have explicit statement on what the behavior is on inclusion of all fields to make sure the behavior is stable. I probably could have assumed that USE_DEFAULTS [1] is always the default if nothing is specified, but I thought this was safer. There should be no backwards incompatible effect of this. (But if you don't want it there, it can be removed safely, I think.)

[1] https://fasterxml.github.io/jackson-annotations/javadoc/2.9/com/fasterxml/jackson/annotation/JsonInclude.Include.html#USE_DEFAULTS

[jmini]

@JsonProperty being moved from the field to the getters.

I did some tests with:

• an object containing a boolean property (I tried with the is and with get prefix)
• an object containing an _ such that the field name and the serialized string in the annotation are not the same.

Both worked for me.
=> It also makes #3573 less important, because it fixes it as side effect.

---

About using all the Json annotations in the model or not, I am not sure.
I am not a big fan of it, especially because in this case, it is not changing anything, so it could be removed.

This Jackson-annotation topic goes also behind this PR:
There is a PR #3441 to add @JsonIgnoreProperties on the class. Somehow redundant with the Mapper Configuration we provide, but I can understand that some users prefers the annotation on the model. This way they can use any mapper instance (without having to change the config). If you have an opinion, fell free to comment.

[jmini]

I only added this for jackson, as I didn't figure out an easy way to do this for gson. I could try working on a different PR that would do this for gson, but that would take me a lot more time.

The problem is that jackson-databind-nullable is made for Jackson.
For gson, something similar to JsonNullable could be used, but the rest (serializer / deserializer / ...) must be written the JSON way.

[bkabrda]

The problem is that jackson-databind-nullable is made for Jackson.
For gson, something similar to JsonNullable could be used, but the rest (serializer / deserializer / ...) must be written the JSON way.

Yeah. I'll admit I'm not familiar with gson too much (and Java in general TBH), so making this PR work just for jackson was complex enough for me that I didn't want to go into it. The way I understand it, it might be possible to make JsonNullable compatible with gson - I think that would be preferrable to using a different nullable implementation for each serialization library.

@jmini if you see any specific followup things to fix, please let me know, I'll be happy to look into them. Thanks!

[wing328]

@bkabrda thanks for the PR, which has been included in the v4.1.1 release: https://twitter.com/oas_generator/status/1165944867391860737

[jmini]

The new dependency was not added to the generated build.gradle. See PR #3793 to fix it.

[RockyMM]

Hello all,

From my perspective, as a user of useXml option, this PR produces a regression I described in #4014. Can anyone take a look?

Thanks!