Using Optional.ofNullable() at the fluent setters to prevent NPE

[slobodator]

Motivation:

When using useOptional:true it is worth to generate fluent setters that could accept null values properly.
Thus, they should be wrapped with Optional.ofNullable() not Optional.of(), i.e. having

public class Payload {
  private Optional<String> name = Optional.empty();
  ...

its fluent builder method should look like

  public Payload name(String name) {
    this.name = Optional.ofNullable(name); // NPE safe
    return this;
  }

instead of

  public Payload name(String name) {
    this.name = Optional.of(name); // might throw an NPE if name is null
    return this;
  }

Issues:

• fixing [BUG][Java/spring] Incorrect Handling of Nullable Fields in OpenAPI Generator #17538

Modifications:

• pojo.mustache improved

• tests adjusted

• 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 || exit
  ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
  ./bin/utils/export_docs_generators.sh || exit
  

• File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)

• 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 @welshm @MelleD @atextor @manedev79 @javisst @borsch @banlevente @Zomzog @martin-mfg Please, have a look

• the 1st commit is an actual fix
• the 2nd commit is the samples update

[MelleD]

Here are some points. It‘s not really a bug it‘s intended the behavior. I would never use a null value to set an optional to empty.

I would prefer a that you can configure what you would like to use: Optional#ofNullable or Optional#of.

Another point is maybe you should check this PR that you have maybe use some Nullable annotation as well.
https://github.com/OpenAPITools/openapi-generator/pull/20345/files/f9593124f42c004b350735ee451131537103dfea..d3a2343df21d1c803cc46ca215cf73dc5015d622
EDIT: Ah I saw it's your PR nice :D... So maybe also add the @nullable annotation here should make sense, right?

@wing328 can you remove the bug label please

[slobodator]

It‘s not really a bug it‘s intended the behavior

Could you elaborate on that, please? I still can't get the idea

I would never use a null value to set an optional to empty

You mean you never write

new Payload()
  .name(null);

...? Indeed, no one does that.

Still, let me explain the issue, please.

Assume, I am writing an endpoint that accepts a body with non-required parameter (I changed Category -> Payload and id -> name at my initial message to make it more natural).
To prevent NPEs at the server side I am wrapping it with Optional.
Then the getter

  public Optional<String> getName() {
    return name;
  }

... gives me a clear indication to handle it properly.
So far so good.

Now let's look at this from the client perspective.
Assume, they have an inputStr which comes from somewhere and is going to call my endpoint. So, they write

var payload = new Payload()
  .name(inputStr);

restClient.post(url, payload);

If the inputStr is null they get theNPE even before the call!

"F*ck!" say @Kavan72 and @Moribund7 and @mvdwalle and @jwilmoth-ehs.
It is 2025 but I still need to wrap that f*cking nulls with ugly

var payload = new Payload();

if (inputStr != null) {
  payload.name(inputStr);
}

restClient.post(url, payload);

Someone may say that they prefer a builder for the fluent object building but

var payload = Payload.builder()
  .name(inputStr)
  .build();

... won't help as the builder .name() method calls the very same .name() method from the base class.
It will throw the NPE as well.

Or do you want us to use a regular setter i.e.

var payload = new Payload();
payload.setName(Optional.of(inputStr));

...?

"F*cking f*ck again!" says the team as it still throws the very same NPE!

I would prefer a that you can configure what you would like to use: Optional#ofNullable or Optional#of.

So, the only correct way is

var payload = new Payload();
payload.setName(Optional.ofNullable(inputStr));

... right?

So, I guess that the fluent builder method in its current form

public Payload name(String name) {
    this.name = Optional.of(name);
    return this;
}

.. is totally inconvenient/useless for Optional users.

In what scenarios do you use it?

Just in case.

At the required arguments constructor you wrote Optional.ofNullable which is correct.
At the all arguments constructor @wing328 did the very same.
So, why are you against using the safe form at the fluent setter method?

P.S. @Nullable annotation doesn't not intersect with this issue at all. @Nullable one is for those who don't use Optional for some reasons.

@wing328 Please, don't remove the bug label :-D I hope I could convince @MelleD that Optional.ofNullable is better.

[wing328]

At the all arguments constructor @wing328 did the very same.

To be clear I didn't write that. I was updating that (written by someone) to fix something if I remember correctly

[slobodator]

To be clear I didn't write that. I was updating that (written by someone) to fix something if I remember correctly

Bag my pardon.
It seemed to be @jpfinne 's one

[mvdwalle]

The fluent API in combination with the Optional.of() is very messy. I never understood why Oracle added the of() method anyway as it still forces you to do null checks everywhere. The case here is that mapping from for example a domain object, a result from the database or other sources where certain fields might be null gives a lot of headaches.

@MelleD can you explain why it is intended? I do understand not calling the setter IF you already know it is null, but with the fluent API and mapping from sources that I mentioned before you cannot always be sure without added all the null checks. I really hope you reconsider this as (in my opinion) a simple change from Optional.of() to Optional.ofNullable() has enormous benefits without any serious impact.

@MelleD You also mention that you do not consider this a bug. Is it not a bug if users of the library need to jump through hoops just to make it work?

[jpfinne]

To be clear I didn't write that. I was updating that (written by someone) to fix something if I remember correctly

Bag my pardon. It seemed to be @jpfinne 's one

I've written the allArgConstructor and the builder.

The handling of optional for DTOs is very messy.
First, it uses the configOptions useOptional. It is documented as "Use Optional container for optional parameters". It was used to have optional in the generated api. Reusing the same option for generated model is IMHO a mistake.

Second, the implementation is very bad. It would be much cleaner to just change the getters:

public Optional<Dog> getDog() {
  return Optional.ofNullable(dog);
}

Instead optional fields are used.

private Optional<Dog> dog  = Optional.empty();

Third the fluent setters can throw NullPointerException. The main design goals of Optional are:

• having 2 states: present or not present
• avoid NullPointerException
• make life easier for developpers because they don't need to worry about nullity.

If we want 3 states, we should use org.openapitools.jackson.nullable.JsonNullable

@slobodator your fix is the safest and easiest

In the mid term we should review the implementation of Optional for the domain.

[MelleD]

Second, the implementation is very bad. It would be much cleaner to just change the getters:

No member variable are fine and good. There is no reason to change it. Why it should be cleaner? Thats just personal taste.
I would like to see directly into the member variable, which fields are optional and which not.

Also I like the pattern that you have two constructors.
One for all args with Optional and one without Optional args just with all mandatory fields.

First, it uses the configOptions useOptional. It is documented as "Use Optional container for optional parameters". It was used to have optional in the generated api. Reusing the same option for generated model is IMHO a mistake.

Yes documentation looks out to date. Also there are some request to separate it with 2 config parameter e.g. useOptionalParameter and useOptionalModel.

Could you elaborate on that, please? I still can't get the idea

Look into the PR it was discussed what we should use. So it was a decision what we should use. If it was an intentional and clear decision, it can't be a bug. But does it matter whether it's a bug or not?

You mean you never write

new Payload()
.name(null);
...? Indeed, no one does that.

A lot of people would like to do that ;)

Assume, they have an inputStr which comes from somewhere and is going to call my endpoint. So, they write

There was a lot discussion.
I don't feel like having to discuss it again and again. For me the change is fine if it can be adjusted or if it configurable.Then everyone can decide for themselves what they want to use. What bothers you about it?

If my/our code gets a null in your place, we have a bug somewhere and would like to detect it early.
Without knowing the overall context of the application, it is pointless to discuss right or wrong.
In my case, a null at this point is a bug in the code because this cannot and should NOT happen . We use mapper frameworks for this handling.

It is also always tricky to share client and server. The change could be helpful for the client, but it could be dangerous for the server, as in our case.

It is 2025 but I still need to wrap that f*cking nulls with ugly

Thats a good question why do you have a inputString which is not directly an Optional ;), but yes thats different between the Optionals camp. In camp Optional everywhere your inputString is directly an Optional.
Therefore I'am missing more that the fluent api cannot set a Optional directly like

Optional<String> inputString...
new Payload().name(inputString);

That is what Iam missing

[slobodator]

Look into the PR it was discussed what we should use. So it was a decision what we should use.

I followed all links from this comment and found this discussion, but still can't the reason.

@welshm suggested the same at the very beginning.

I don't feel like having to discuss it again and again.

Sorry, still trying to convivence you.

Let me try to use this way, please.

I am writing a client call to some endpoint.

Without knowing the overall context of the application

My bad, I didn't mention the spec meaning it is obvious.

openapi: 3.0.0
components:
  schemas:
    Payload:
      type: object
      # name is not mentioned at required
      properties:
        name:
          type: string

The codegen settings are openApiNullable:true (by default) and useOptional:true.

We use mapper frameworks for this handling.

That definitely makes sense for converting entity -> response but I am building a request. I don't need any framework for that.

Also I like the pattern that you have two constructors.
One for all args with Optional and one without Optional args just with all mandatory fields.

I do like it too but let's assume generateConstructorWithAllArgs wasn't turned on for some reason. Or developers prefer other styles.

So, I am writing either

var payload = new Payload()
  .name(inputStr);

... or

var payload = Payload.builder()
  .name(inputStr)
  .build();

... if I like the builder more.

I am a client; I don't even look at the generated code.
If inputStr is null it throws a NPE.
"It's weird" I am thinking and double check the spec above. The name is not required as far as I see.

If my/our code gets a null in your place, we have a bug somewhere and would like to detect it early.

What did I wrong at the code above?
The inputStr could be null, what is wrong with it?

"How should I build that payload?" the team is asking.

Therefore I'am missing more that the fluent api cannot set a Optional directly like

Optional<String> inputString...
new Payload().name(inputString);

That's awesome that you mentioned it.
Indeed, if I saw the signature name(Optional<String>) it would be a clear indication for me that I need to wrap it with Optional in a proper way

what you would like to use: Optional#ofNullable or Optional#of

But currently it is name(String) and Optional#of is within it. It is a generated code, as a user I can't change it.

So, I see two options.

1. To change the signature of the fluent builder method to accept Optional<T> as the Optional-everywhere camp suggests. It would be the clearest way probably, but it is a breaking change.
2. To apply that minor fix as the current generated implementation

public Payload name(String name) {
  this.name = Optional.of(name);
  return this;
}

... is useless actually.

but it could be dangerous for the server, as in our case.

What is your concern? How does it affect the server?

[MelleD]

So, I see two options.

Yes you are totally right. Personally I would avoid null (because thats the reason why we would like to use Optional) so on my client
The code looks like this without fluent api

Optional<String> inputString...
Payload payload = new Payload();
payload.setName(inputString);

Perfect would be for Optional this

Optional<String> inputString...
new Payload().name(inputString);
new Payload().name("I know the value");

public class Payload {

	private Optional<String> name;

	public Payload name(Optional<String> name) {
		this.name = name;
		return this;
	}

	public Payload name(String name) {
		this.name = Optional.of(name);
		return this;
	}

	public static void main(String[] args) {
		final Optional<String> inputString = Optional.ofNullable(null);
		final Payload payload = new Payload().name(inputString);

		new Payload().name("I know the value");
	}

}

[slobodator]

@MelleD After your message above I guess I got your way of thinking.

You seem to follow the set of rules:

• rule 1. Any variable SHOULD be assignment to some value. If it is null for some reason it is a bug at the moment of assignment. No exceptions and no further discussion regardless of circumstances
• rule 2. It is always safe to invoke variable.method(). If a NPE was thrown, see rule 1
• rule 3. If any method has a signature like (T t) it means that parameter t CAN NOT be null, see rule 1
  • so, within the method it is safe to call t.anyMethod() or wrap it with Optional.of(t) or whatever you want
  • if there is a NPE occurred within the method see rule 1
• rule 4. If any method parameter is optional that it SHOULD be always explicitly wrapped with Optional i.e. (Optional<T> t)
  • rule 3 and rule 4 could be obviously combined together i.e. the signature(A a, Optional<B> b) is clear that a is mandatory and b is not
  • rule 5. Eventually there should be no NPEs at all

It looks like a Kotlin way with Java tools. With some compiler assistance, but the main rule 1 is on the developers.

I am not going to discuss the pros and cons of this approach, I will say just two sentences

• neither rule 1 nor rule 3 are MANDATORY in Java world. That is status quo
  • again, I am not going discuss its pros and cons and how to get from (A a, B b) without documentation if they are nullable or not.
• I can't change the habits of all my colleagues at once

Look into the PR it was discussed what we should use.

I finally got what you meant and read it till the very end.

So, a lot of people staring with @robinjhector are trying to tell you that

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

For you it is obvious and fine that

var payload = Payload.builder()
  .name(inputStr)
  .build();

... throws a NPE if rule 1 and rule 3 are violated. It is a punishment that inputStr got null somewhere.

But... it is NOT obvious for others.
A regular java developer assumes that it is safe to build a payload this way. Because they do it countless times with other frameworks.

(Just in case, I am not going to start discussion "constructors vs builders" even my personal opinion is here. Folks are using builders, it is a fact)

They check the spec, see that the name is not required and go away confused or raise the issue as @Kavan72 did.

There is no documentation of this behavior.
Probably, because for you rule 1 and rule 3 are obvious and mandatory and others are not aware of them at all.

And the "funny" fact is that you don't use that builder method yourself preferring regular setters!

You didn't add the builder method .name(Optional<String), broke the existing one for regular developers and are against fixing it defending your rules.

Even a guy from the Optional-everywhere camp with the current implementation should either

• use regular setters as you do
• or wrap it with weird and ugly

Optional<String> inputStr = ...;
var builder = Payload.builder();
inputStr.ifPresent(s -> builder.name(s));
var payload = builder.build();

... as there is no way to write it in the single call/fluent style.

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.

Is the only way to use a fork/customize the template or could we convince you somehow?

[jpfinne]

Thanks @slobodator for your efforts

I would do:

1. add 2 parameters to distinguish between api and model generation
   useOptionalApi and useOptionalModel
   if not defined, then useOptional gives true for backward compatibility
   make useOptional deprecated

2. add a fluent setter with Optional parameter

 public Payload name(Optional<String> name) {
    this.name = name;
    return this;
  }

3. use Optional.ofNullable

  public Payload name(String name) {
    this.name = Optional.ofNullable(name); // NPE safe
    return this;
  }

Clear for everyone, backward compatible...

[Chrimle]

What's the point of Optional at all, if it isn't used as intended?

• It should not be used as method parameters, as suggested here:

  public Payload name(Optional<String> name) {
    this.name = name;
    return this;
  }

  As it would allow the Optional itself to be null, which violates the API note.

• It should not be used as member fields, as suggested here:

  private Optional<String> name;

  As it would allow the state of the object to be unstable, and could cause NPEs when invoking getName(). This is also what the API note is hinting at...

  Optional is primarily intended for use as a method return type [...]

  Yes, even if getName() itself returns Optional<?>, invoking any method on that Optional will also result in NPE, e.g. object.getOptionalField().isPresent(). Regardless of where and when, as mentioned here, this causes a lot of unnecessary confusion and concern of null... which was the entire point of using Optionals in the first place.

If a field is truly optional, it should simply have 2 possible states - present or not - accessible by a getter-method. If it would require a 3rd state, to be able to differentiate between empty() and null, the suggestion made by @jpfinne is promising - assuming that it should be a supported use-case.

This is the part of the discussion which was difficult to follow across these PRs - is it redundant/contradictory configOptions, a matter of personal coding preferences, misaligned assumptions, or lack of documentation that led to the bug being introduced and many users encountering issues?

The fix that @slobodator is proposing would actually allow name to be optional - without introducing any violations of the language and/or well-established expectations of how Optional is intended to be used. 👍

[jpfinne]

Just a clarification: I don't want 3 states optional

So the setters should.not accept null values.

`public Payload name(@NotNull Optional<String> name) {
  this.name = name;
  return this;
}

public void setName(@NotNull Optional<String> name) {
  this.name = name;
}

[MelleD]

And again an endless discussion begins. If someone don't want to use an Optional, they don't have to. You can also have raw arguments and handle the null value.
Optionally you can use both as members and parameters (There are lots of reasons and lots of recommendations) , which is totally valid and even recommended.
There are people who don't want nulls in the code, which is also a good practice. Most of the time it just takes a little time and experience to understand it.

For this reason, the rule is very simple and is default for spring projects.
Every parameter is @NotNull by DEFAULT.
All nullable methods should be declared with @nullable.

And again you can introduce Optional.ofNullable, but please make it configurable so that everyone can decide for themselves what they want. What's the problem with making this configurable?

Config a)

public Payload name(@Nullable String name) {
  this.name = Optional.ofNullable(name);
  return this;
}

Config b)

public Payload name(String name) {
  this.name = Optional.of(name);
  return this;
}

Additional feature and independent is to allow in the builder pattern also Optional directly.
BY DEFAULT EVERY Optional SHOULD NEVER BE NULL! Thats is bad practice in java and I saw never where this is recommended.

public Payload name(Optional<String> name) {
  this.name = name;
  return this;
}

You didn't add the builder method .name(Optional<String), broke the existing one for regular developers and are against fixing it defending your rules.

Strange to break something what was no there before! I have the feeling people don't want to use Optional and don't see that they can just turn it off and the code is the same like before my PR. There is no obligation to use optionals.
People somehow feel obligated to use it, maybe we should set the defaults in the code generators differently.
The only bug maybe is that you can't turn it off in the DTO/Pojos if you want it only in the API.
But it didn't occur to me why you want it in the API but not in the Dtos

---

And again we can discuss for pages, but why don't you want to make it configurable, then everyone can set it as they want and everyone is happy.
Reminds me of the discussion about which default value collections should have. Make it adjustable and everyone is responsible for themselves.

[slobodator]

For this reason, the rule is very simple and is default for spring projects.
Every parameter is @NotNull by DEFAULT.
All nullable methods should be declared @Nullable.

This is definitely a good idea, and I encourage everyone to do the same!
Unfortunately, this is a rule neither in Spring (they love nulls) not at Java world.

Or did I miss something, and you kindly point me to Spring documentation now?

I guess, the root cause of our discussion is that the builder method is not annotated.

You guess that .name(String name) is actually .name(@NotNull name) according to rule 3 at your world.
But everyone around understands it as .name(@Nullable name) and wonder why it throws a NPE instead of accepting that non-required name.

Strange to break something what was no there before!

Folks used

var payload = Payload.builder()
  .name(inputStr)
  .build();

... before introducing useOptional and wondered why it stopped working.

@robinjhector told you that.

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.

What's the problem with making this configurable?

There will be extra efforts for

• implementing it
• adding a feature-toggle
• discussing its reasonable default
• documenting everything

I was not going to do such much, just to fix 2 lines at this PR.

Before starting it, I really need to get who will ever use that config B (current behavior) and for what purpose.

public Payload name(String name) {
  this.name = Optional.of(name);
  return this;
}

... should be at the form

/**
* @param name -- is mandatory!
* if you want to pass nullable parameter here -- check it against null first!
*
* @throws a NPE if name is null
*/
public Payload name(String name) {
  if (name == null) {
     throw new RuntimeException("name is null but it should be never null, see rule 1");
  }
  this.name = Optional.of(name);
  return this;
}

Who needs this?

How this small change from Optional#of to Optional#ofNullable will affect even your code?

How strict is rule 3 at your world?

rule 3. If any method has a signature like (T t) it means that parameter t CAN NOT be null, see rule 1

• the method MAY check if t is not null
• or the method SHOULD check that t is not null?

And a kind reminder -- it is your rule, not Spring one or Java world one.
(Just a side note -- I like strictness, but it seems to be easier to switch to Kotlin rather than force regular developers change their habits. And anyhow we should respect status quo)

May I tag the tech committee again?
@cachescrubber @welshm @MelleD @atextor @manedev79 @javisst @borsch @banlevente @Zomzog @martin-mfg

[robinjhector]

And a kind reminder -- it is your rule, not Spring one or Java world one.

This is the key here. The fact that you're introducing your personal code preference, in a non-backwards compatbile runtime-breaking fashion.

[MelleD]

And a kind reminder -- it is your rule, not Spring one or Java world one.

This is the key here. The fact that you're introducing your personal code preference, in a non-backwards compatbile runtime-breaking fashion.

It was a new feature and you can turn it off. Everyone just ignores that. I don't understand!
I've said it 100 times:
useOptional:false and you can be happy. What is the issue with useOptional = false?
You can use the same code like before!

The fact that you're introducing your personal code preference
Really funny and you would like to decide what is wrong or right? And that's not a personal preference, but what then?

What's the problem with making this configurable?
There will be extra efforts for

What yes for every configuration is extra effort same for collection nullables. Create a config option here:
https://openapi-generator.tech/docs/generators/spring/

Thats it

Before starting it, I really need to get who will ever use that config B (current behavior) and for what purpose.

I will use it (e.g in test or or or) with the extra method to set Optional and no you have not to throw a extra exception.

And a kind reminder -- it is your rule, not Spring one or Java world one.

Of course it is a recommendation and there are recommendations in the java world one example
https://medium.com/square-corner-blog/non-null-is-the-default-58ffc0bb9111
I'm honored that you think I'm setup such a cool thing.

or
https://www.baeldung.com/spring-null-safety-annotations

Where is it described and what sources do you have? So far I only see personal opinions and you act as if you speak for all Java developers.
https://www.quora.com/What-are-the-best-practices-for-managing-null-value-effectively-in-Java
spring-projects/spring-framework#20099
https://dzone.com/articles/when-to-use-jsr-305-for-nullability-in-java
"Everything is non-null by default, unless explicitly annotated as nullable."

And there are also other projects with the same discussion:
immutables/immutables#781

What is the solution? Ahhh a configuration that everybody can decide what is right or wrong for the project
@Value.Style(optionalAcceptNullable=true)

And friendly note: You are discussing here as if you are all right and it is a rule how you want to use it. There are also many other opinions or do you think all happy users without problems are checking in here?
In the end there is the possibility to turn it off or to create a custom mustache file.
We could also extract the Optional.of vs Optional.ofNullable into one single file and everybody can override the default.
The point is that no one has to decide what is right or wrong because there is no answer. It just depends!

https://nipafx.dev/inside-java-newscast-19/

For this reason it should be easily configurable.

[MelleD]

Some also use optionals for hidden if checks. Just because it works doesn't mean it's good.

String s = Optional.ofNullable(value).map(Objects::toString).orElse("not found");

Just like toogle and boolean method parameters, but there is a recommendation. However, not everyone does it.

[Chrimle]

The Config B, is a prime example of when not to use Optionals:

public Payload name(String name) {
  this.name = Optional.of(name);
  return this;
}

Because it does not allow name to be null. Optional is not intended to replace null-checks, it is to convey that a value is optional - which in Config B, is not the case. The current implementation is equivalent to:

public Payload name(String name) {
  this.name = Objects.requireNonNull(name)
  return this;
}

Since name is not nullable, the Config-Option useOptional should be set to false in the use case that @MelleD is describing.

For use-cases where name is nullable, @slobodator's fix is the correct one.

[MelleD]

The Config B, is a prime example of when not to use Optionals:

public Payload name(String name) {
  this.name = Optional.of(name);
  return this;
}

Because it does not allow name to be null. Optional is not intended to replace null-checks, it is to convey that a value is optional - which in Config B, is not the case. The current implementation is equivalent to:

public Payload name(String name) {
  this.name = Objects.requireNonNull(name)
  return this;
}

Since name is not nullable, the Config-Option useOptional should be set to false in the use case that @MelleD is describing.

For use-cases where name is nullable, @slobodator's fix is the correct one.

It’s your personal opinion and your personal code preference

[MelleD]

I think everything has been said. I'm opting out. I have provided enough sources and evidence. Mind you, I'm the only one so far, but I'm accused of it being my personal opinion. First of all provide your own sources and evidence beforehand you blame others.You can also turn off the feature and use the code like before.

@wing328 your decision. You have to go forward.

I stick with it - there is no wrong or right here and there are 100 different approaches.
Actually the solution is simple if you look at other projects. Introduce parameters/config option and make it configurable. Done and everyone has their way.

[robinjhector]

It’s your personal opinion and your personal code preference

It's really not. It's just incorrect use of Optional alltogether. Please step down from your high horse.

[Chrimle]

I think everything has been said. I'm opting out. I have provided enough sources and evidence. Mind you, I'm the only one so far, but I'm accused of it being my personal opinion. First of all provide your own sources and evidence beforehand you blame others

I can only speak for myself, but code preferences - which often comes from experience - is often very difficult to convey, phrase and be coherent in a medium such as a Pull Request thread.
I think you all are doing well at arguing your points ❤️
But as we all try to convince each other, this thread is becoming more like that other thread, starting to grow in size and increase in tension.

I would suggest moving the discussion elsewhere - or await input from the tech committee?

---

My view on Optional, to not bloat this thread any further.

[slobodator]

It was a new feature, and you can turn it off. Everyone just ignores that. I don't understand!

I could explain at least my motivation. It is a nice feature, and I would like to use it.
Let me elaborate on this.

I work for a company with a lot of teams and countless number of microservices.
Every team has its own view of coding styles.
I am a kind of support for everyone.

Not all of teams are even using OpenApi Generator, some of them still prefer Swagger.
But I believe that the spec first approach is more beneficial as it allows frontenders start their work in parallel.

Even I have my own rules of code, I can not dictate them to other teams. So, I am reviewing generation trying to improve it to bring values for colleagues and community.

• for ones that are not aware of useOptional or don't want to use it, I've added @Nullable annotation. It is not a safe net but at least some warning. It has been already merged, thanks
• we agreed that even if openApiNullable is off for some weird reason (maybe this one, hope it is outdated), it still makes sense to useOptional. I am going to cover this "gap"
• despite of various thoughts and camps of Optional I am pretty sure that your useOptional implementation is practically totally fine. Somehow, I assumed that this Optional#of is minor typo. I guessed I fix it with 2 lines PR and will suggest our teams to start using Optional. I didn't expect such a hot discussion ;-)
• well, if there are different Optional camps I would suggest this enhancement for #2

P.S. Thanks for @NonNullFields from https://www.baeldung.com/spring-null-safety-annotations. Somehow, I wasn't aware of it but will use it of course. Still, I keep thinking that the fluent builder method is either implicitly or explicitly annotated with @Nullable

[martin-mfg]

Hi, tech committee member here.

I propose the following course of action:

1. We should change Optional.of to Optional.ofNullable in the fluent methods.
2. Separately, anyone can introduce a new configuration which generates Optional.of instead of Optional.ofNullable, and potentially makes more such changes in other places too.

---

Whether null arguments should be accepted in the fluent methods is a matter of opinion. @MelleD cited sources supporting his position, and the amount of unhappy comments here shows that there are many people of the opposing view as well.

That the introduction of Optional.of was a breaking change without fallback in a minor version is certainly undesirable. So fixing that should have priority.

Turning off useOptional might be an option for some users, but since this option also influences API parameters, that's not a solution for everyone. Whether it makes sense to use Optional in the API and not in the POJOs is again a matter of opinion.

Also, OpenAPI Generator (with library=spring and useOptional=true) is currently not designed with the fundamental idea that everything is non-null by default. And especially not with the idea that every public method must fail on any null argument. There might be value in such behaviour for some users though, which is why I propose a separate configuration for it.

---

@MelleD, as another tech committee member, do you agree with my proposal above? (If not, I guess @wing328 will have to decide.)

[MelleD]

as another tech committee member, do you agree with my proposal above?

I have already written my preferred solution.

I would introduce a new parameter/config (e.g. optionalAcceptNullable) and everyone can decide for themselves.
Default optionalAcceptNullable = true can generate Optional.ofNullable.