Add Java Optional for POJOs in JavaSpring templates

[MelleD]

This PR add Optionals for the Spring generator. There are many opinions and discussions on this topic.
See: #14765 (comment)

For this reason, the template was only adapted when using JsonNullable (openApiNullable). The code remains the same for all other templates.
Additionally, useOptional must be used in the code generator.

Then the following will be converted

nullable: true --> JsonNullable<T> foo
required and nullable: false --> <T> foo
none required and nullable: false--> Optional<T> foo

In my view, nullable: true and required make no sense. If you use optionals, this state is not needed. For reasons of consistency, a JsonNullable was used in this case as before.

A JsonNullable is intended for a JSON merge patch (see
https://github.com/OpenAPITools/jackson-databind-nullable). As the name suggests, it is mainly needed for the PATCH operation.
However, the templates support more and the solution was more generic. As before, you can create a JsonNullable for all operations as soon as the attribute nullable: is true.

For fans of Optional everywhere (https://medium.com/97-things/using-optional-nowhere-somewhere-or-everywhere-b1eb5645eab5), there is now finally a way to use Java Optional for POJOs like in the API templates.

This PR is breaking.

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
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
  @cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09)
  @wing328

[welshm]

Just some minor comments - I think this is great.

I do think nullable and required can both be true but I don't think it needs to change anything here.

Looking forward to seeing this in a release!

[welshm]

Can this be simplified? Looks like it resolves to nullableDataTypeBeanValidation in several cases.

[MelleD]

I found no other way. Also with the default value etc. different cases

[MelleD]

Also don't like that I need now 2 different files with and without bean validation, but this is the issue that Optional need the bean validation between <>

[welshm]

Should this be Optional.ofNullable()? Or is the decision to explicitly disallow setting null?

[MelleD]

Yes that's a good point.
I would say it should not null, because you should not use id(null), because avoiding null values, but maybe a matter of taste.

[welshm]

Can/should there be a clearId() method then?

Otherwise, the only way to set this to null or remove the value is to do...

category.setId(Optional.empty())

Maybe we don't need it 🤷 Seems like an edge case anyway

[MelleD]

I'll probably ask 4 people and get 5 opinions :D

[MelleD]

I never used a clearXYZ method to handle or clear Optionals, but why not. I would wait for more feedback and then the community maybe can decide which option is better.

Personally i like more set directly Optional.empty() because then you see you make something special.

But it the same issue with JsonNullable you cannot clear/undefined it with the fluent pattern. You need the setter

[welshm]

Yeah I think setId(Optional.empty() makes it very clear what's happening - that's a good point.

[welshm]

Can/should there be a clearId() method then?

Otherwise, the only way to set this to null or remove the value is to do...

category.setId(Optional.empty())

Maybe we don't need it 🤷 Seems like an edge case anyway

[MelleD]

@welshm thanks for the quick and fast feedback :).

[MelleD]

@wing328 what do you think here?

[MelleD]

@wing328 new week new try :)

[wing328]

let me try to review tomorrow or later this week

[wing328]

In my view, nullable: true and required make no sense. If you use optionals, this state is not needed. For reasons of consistency, a JsonNullable was used in this case as before.

thanks for considering backward-compatibility in your PR

In my view, nullable: true and required make no sense

it's not a common use case based on my experience and good that you still have it covered in your PR.

[wing328]

Thanks @welshm for the review 🙏

Thanks @MelleD for the change, tests and detailed explanation.

Let's go with what you've so far.

[MelleD]

Cool thanks for the feedback

[adam-geipel]

Hi! Coincidentally, I was trying to figure out how to do this myself in my project and came across this PR. Any idea when a new release with this change will be made generally available?

[MelleD]

Hi! Coincidentally, I was trying to figure out how to do this myself in my project and came across this PR. Any idea when a new release with this change will be made generally available?

If you would try upfront you can copy the three or four files and put in your template dir

[Ironlink]

I disagree with this change.

First, the code for building a response becomes more verbose.

Before:

record Entity(String first, String second, String third) {}
public ResponseEntity<Entity> foo(EntityRequest request) {
	Entity e = repo.get(request.getId());
	return ok(new EntityResponse()
		.first(e.first())
		.second(e.second())
		.third(e.third()));
}

After:

record Entity(String first, String second, String third) {}
public ResponseEntity<Entity> foo(EntityRequest request) {
	Entity e = repo.get(request.getId());
	EntityResponse response = new EntityResponse();
	// Option 1
	if (e.first() != null) {
		response.first(e.first());
	}
	// Option 2
	Optional.ofNullable(e.second()).ifPresent(response::second);
	// Option 3
	response.setThird(Optional.ofNullable(e.third());
	return ok(response);
}

At the core of this is the fact that the fluent setters do not accept Optional<T>. If they had done so, this code could have been new EntityResponse().first(Optional.ofNullable(e.first())), which of course would look just like in the "Before" snippet if the entity getters returned Optional. In fact, I argue that the fluent setters are useless in their current state, as they cannot be used fluently even with a getter that returns Optional:

record Entity(Optional<String> first, Optional<String> second) {}
new EntityResponse()
	.first(e.first()) // Does not compile
	.second(e.second().orElse(null)) // Throws NPE

Second, the code in the "Before" snippet still compiles, but has changed to throw an exception. If I did not have test cases that caught this, I might have shipped this upgrade to production and only then discovered these NullPointerExceptions. That this is not listed as a breaking change is very surprising to me.

[thomasrepnik]

I noticed during the update to 7.2.0 that we have a huge number of compilation errors and then came across this PR here.

This PR is a massive breaking change

It forces the contract provider update his contract accordingly, otherwise you won't be able to update to 7.2.0 (without a massive refactoring)

Sadly it does not have its own config toggle for it, but instead simply reuses the existing useOptional

In the documentation of useOptional the description is as follows:
Use Optional container for optional parameters
-> It is explicitly written that it only affects parameters (not POJOs and attributes as implemented now).

Now there are different variants, none of which is satisfactory:

• stay with 7.1.0 until all contract providers have adapted their contract (probably waiting for years)
• update to 7.2.0 ,stick with the <useOptional>true<useOptional> and deal with a massive refactoring
• update to 7.2.0, set useOptional to false and refactor all controllers, as no more optionals have been generated in the API.

UPDATE: We just figured out to get the old behaviour back after setting

<openApiNullable>false</openApiNullable>

To anyone who has the same Problem, hope this helps.

[MelleD]

First of all, you should read the description and the ticket in order to understand the change.

This PR is breaking.

Then a few comments on this

stay with 7.1.0 until all contract providers have adapted their contract (probably waiting for years)

That makes absolutely no sense. No contract has been broken by the change. The REST Api + Json structure looks exactly the same.
The only thing that breaks for the customer may be a client (e.g. Feign) and if the DTO is reused, but it is not an API breaking change. This is a normal, major change that can always happen, and that should be clear to the customer.

By the way, I hope that you have released your old clients etc., because of course they continue to work because the API is not broken ;)

We just figured out to get the old behaviour back after setting

Really you should read the description first.
"For this reason, the template was only adapted when using JsonNullable (openApiNullable)"

I disagree with this change.

I don't know what you're trying to achieve, your code doesn't look like you should use Optionals or JsonNullables. Then just turn it off.
Additional you could provide that the fluent API accepts null, but personally I don't like your code snippet for this mapping cases, I would use a right mapping framework (like Mapstruct) which have the power to handle the cases.
Or provide a constructor instead of fluent api could also do the job.

[Ironlink]

While it is indeed stated in this PR that this is a breaking change, this PR is not mentioned at all in the release notes. This PR also does not show up if you click the "2 breaking changes (with fallback)" link from the release notes. If this had been in the release notes, I would have found it before starting upgrades.

Additional you could provide that the fluent API accepts null, but personally I don't like your code snippet for this mapping cases, I would use a right mapping framework (like Mapstruct) which have the power to handle the cases.

I'm not advocating for the fluent setter to accept null, but a simple overload of the fluent setter such that it accepts an Optional like the setFoo method does. I vastly prefer solutions with plain code over reflective solutions. Especially so for what should be simple logic, like in this case where I could just add my own call to ofNullable if that's what I need.

[MelleD]

While it is indeed stated in this PR that this is a breaking change, this PR is not mentioned at all in the release notes. This PR also does not show up if you click the "2 breaking changes (with fallback)" link from the release notes. If this had been in the release notes, I would have found it before starting upgrades.

Yes, it was probably mislabeled. But after the first generation you probably noticed it straight away.

I'm not advocating for the fluent setter to accept null, but a simple overload of the fluent setter such that it accepts an Optional like the setFoo method does

It is exactly the same as JsonNullable also implements the fluent API.
The null value as noted in the PR comments is debatable.

I don't use nulls (since I use a proper mapping framework and always recommend that), but it would be easy to implement. Although I still think that Optional and JsonNullable might not make sense in your case and you should turn it off.

I vastly prefer solutions with plain code over reflective solutions. Especially so for what should be simple logic, like in this case where I could just add my own call to ofNullable if that's what I need.

I have no idea what you're talking about. I don't see a suggestion for reflective code anywhere.

[robinjhector]

Yes, it was probably mislabeled. But after the first generation you probably noticed it straight away.

If you were only using the regular setters, yes. If you were using the fluent setters: THEN NO.

This is a scary change since it will result in runtime exceptions (because of your Optional.of usage instead of Optional.ofNullable) for fluent setters. They will still compile, but won't work

Thank god we had integration tests for this.

Here's a quick example of what's wrong:

spec:

components:
  schemas:
    ExampleObj:
      type: object
      properties:
        prop1Required:
          type: string
        prop2Optional:
          type: string
        prop3Nullable:
          type: string
          nullable: true
      required:
        - prop1Required

Generated code om 7.2.0:

[...]
  private String prop1Required;
  private Optional<String> prop2Optional = Optional.empty();
  private JsonNullable<String> prop3Nullable = JsonNullable.<String>undefined();

[...getters omitted in this example]

  public ExampleObj prop1Required(String prop1Required) {
    this.prop1Required = prop1Required;
    return this;
  }

  public void setProp1Required(String prop1Required) {
    this.prop1Required = prop1Required;
  }

  public ExampleObj prop2Optional(String prop2Optional) {
    this.prop2Optional = Optional.of(prop2Optional);
    return this;
  }

  public void setProp2Optional(Optional<String> prop2Optional) {
    this.prop2Optional = prop2Optional;
  }

  public ExampleObj prop3Nullable(String prop3Nullable) {
    this.prop3Nullable = JsonNullable.of(prop3Nullable);
    return this;
  }

  public void setProp3Nullable(JsonNullable<String> prop3Nullable) {
    this.prop3Nullable = prop3Nullable;
  }

While in 7.1.0 the fluent setter for prop2Optional looked like this:

  public ExampleObj prop2Optional(String prop2Optional) {
    this.prop2Optional = prop2Optional;
    return this;
  }

The difference is now we can't use our fluent setters for potentially null values! (Since you always assume passing non-null to them for some reason (by using Optional.of which throws)).
From 7.2.0 you have to guard each fluent setter call with an explicit null-check when building your API model objects.
And no, not everyone uses MapStruct, and you should not make that assumption.

Both versions compile fine, you'll just notice it runtime which is scary.

I would at least suggest adding a configOption (similar to the existing openApiNullable) to toggle off this new behaviour.

[MelleD]

From 7.2.0 you have to guard each fluent setter call with an explicit null-check when building your API model objects. From 7.2.0 you have to guard each fluent setter call with an explicit null-check when building your API model objects.

It depends on your project. In my project all values and parameter are none null. If it is null you have to declare it as Nullable (which is common sense in spring projects also for return values). If not the IDE gives a warning or error and do not compile.

So for me it is very scary to use null parameter to set a Optional to empty. I would not do that.

Also I see no difference to JsonNullable because you can also not set a JsonNullable to empty with the fluent pattern you have to use the setter.
If you use null to JsonNullable the isPresent is always true. So the JsonNullsble is never empty.

I don‘t see your point why you would like to use Optional when you handling with a lot of null values?

I would at least suggest adding a configOption (similar to the existing openApiNullable) to toggle off this new behaviour.

There are different possibilities, but hard to say because your use case is not clear what you try to achieve.

• Create a ticket and discuss with the community describe your use case
• Create PR and describe your use case, why you would like to use Optional.ofNullable or a new config
• Turn the Optional or JsonNullable values off, if you don’t need it. But would not prefer a new config because Optional and the JsonNullable concept make a lot of sense only together and therefore there are two configs to handle that.

[robinjhector]

It depends on your project. In my project all values and parameter are none null. If it is null you have to declare it as Nullable (which is common sense in spring projects also for return values).

Again, we're working with Java, so nullability is a thing. Annotating it as nullable only produces a warning, not a compile-time error.

So for me it is very scary to use null parameter to set a Optional to empty. I would not do that.

Also I see no difference to JsonNullable because you can also not set a JsonNullable to empty with the fluent pattern you have to use the setter. If you use null to JsonNullable the isPresent is always true. So the JsonNullsble is never empty.

I can indeed? At least it's not going to throw an exception at me.

  public ExampleObj prop3Nullable(String prop3Nullable) {
    this.prop3Nullable = JsonNullable.of(prop3Nullable);
    return this;
  }

new ExampleObj()
  .prop3Nullable(null);

Is perfectly legit, since JsonNullable.of(null) works. You get an instance of JsonNullable that contains literal null. This is important since we like to distinguish between:

• abscence
• null
• value

Especially for deserializing JSON:

• {"prop3Nullable": undefined} or even {}
• {"prop3Nullable": null}
• {"prop3Nullable: "hello"}

respectively.

I don‘t see your point why you would like to use Optional when you handling with a lot of null values?

I don't! That's why I want a config option to turn off the wrapping of non-required values in java.util.Optional<>

[MelleD]

Again, we're working with Java, so nullability is a thing.r.

Again no, there are different ways and opinions to handle nulls. One is avoid nulls everywhere and use Optional
https://medium.com/97-things/using-optional-nowhere-somewhere-or-everywhere-b1eb5645eab5

Just because you don't see the benefit or can't imagine it doesn't mean it's not valid.

Annotating it as nullable only produces a warning, not a compile-time error.

It's up to your IDE setting

Is perfectly legit, since JsonNullable.of(null) works. You get an instance of JsonNullable that contains literal null. This is important since we like to distinguish between:

Again completely different thing and NOT the same thing. JsonNullable tri state:
absence/empty --> NOT possible to set via fluent method
null --> reset the attribute possible over fluent method
value --> set new value possible over fluent method

Optional two state:
absence/empty --> NOT possible to set via fluent method
value --> set new value possible over fluent method
null --> undefined not clear if this is a valid state for a fluent method. Discussable! Jackson handle this correct via serialization and deserialization. To make own mappings not sure why, because again I see no use case.
Personal opinion:
I see no use case to manipulate DTOs and when you could avoid the null case
For mapping between DTOs and Domain Object I would always use a propper framework which handles all the edge case.

I don't! That's why I want a config option to turn off the wrapping of non-required values in java.util.Optional<>

Then don't use it? https://openapi-generator.tech/docs/generators/spring/

useOptional --> false

[robinjhector]

Again, we're working with Java, so nullability is a thing.r.

Again no, there are different ways and opinions to handle nulls. One is avoid nulls everywhere and use Optional https://medium.com/97-things/using-optional-nowhere-somewhere-or-everywhere-b1eb5645eab5

Just because you don't see the benefit or can't imagine it doesn't mean it's not valid.

I'm not saying it's invalid, I'm just saying that you are opinionating this library and imposing your own preferred code style on this project. I don't care what your opinion (or some random blogger) is regarding this. If Mark Reinhold came and said "we're deprecating null", then it would be a totally different discussion.

Personal opinion: I see no use case to manipulate DTOs and when you could avoid the null case For mapping between DTOs and Domain Object I would always use a propper framework which handles all the edge case.

Again the world doesn't evolve around you.

Then don't use it? https://openapi-generator.tech/docs/generators/spring/

useOptional --> false

Thanks, I will

[MelleD]

or some random blogger

You should get more into Java if you call Parlog a random blogger.

I'm just saying that you are opinionating this library and imposing your own preferred code style on this project.

No, you're trying to push through your code style and don't understand the point of the PR.
JsonNullable and Optionals have special meanings. You don't have to share the opinion, but simply using it incorrectly doesn't make your opinion more correct.

I don't have merge rights in this project so the PR is through the normal community process and also a lot of discussions.
You have the same right and can do exactly the same thing (but you ignoring thing what you can do). Where is your PR to move this project forward? Contribution highly welcome. It is open source

Thanks, I will

Before you start blustering and assuming things about someone, you should probably just read the description. And the option is described directly.
If you don't like optional, that's your right, but then read the documentation on how to issue something like that.

... using JsonNullable (openApiNullable). ..., useOptional must be used in the code generator.

[pshem]

This PR has been the worst breaking change in openapi-generator we've ran into so far. The solution from @thomasrepnik #17202 (comment) works for versions 7.2, 7.3 and 7.4, but breaks in 7.5. Still working on way of achieving Optionals in required: false and avoiding optionals in required: true for newer versions of the plugin

UPDATE: We just figured out to get the old behaviour back after setting

<openApiNullable>false</openApiNullable>

To anyone who has the same Problem, hope this helps.