[BUG][Java/spring] useOptional:true is applied only if openApiNullable:true

[slobodator]

Bug Report Checklist

• Have you provided a full/minimal spec to reproduce the issue?
• Have you validated the input using an OpenAPI validator (example)?
• Have you tested with the latest master to confirm the issue still exists?
• Have you searched for related issues/PRs?
• What's the actual output vs expected output?
• [Optional] Sponsorship to speed up the bug fix or feature request (example)

OpenAPI declaration file content or url

openapi: 3.0.0
components:
  schemas:
    Item:
      type: object
      properties:
        foo:
          type: string

Description

Assume we have a very minimal specification (see above) and use Java Spring Boot generator.

With useOptional:true everything is fine i.e.

public class Item {
  private Optional<String> foo = Optional.empty();

  public Item foo(String foo) {
    this.foo = Optional.ofNullable(foo);
    return this;
  }

  @JsonProperty("foo")
  public Optional<String> getFoo() {
    return foo;
  }

  public void setFoo(Optional<String> foo) {
    this.foo = foo;
  }
}

... but Optional wrappers are applied only if openApiNullable:true.
Luckily, it is a default behavior. Still, if I explicitly turn it off openApiNullable:false then useOptional:true doesn't have any effect

public class Item {
  private String foo;

  public Item foo(String foo) {
    this.foo = foo;
    return this;
  }

  @JsonProperty("foo")
  public String getFoo() {
    return foo;
  }

  public void setFoo(String foo) {
    this.foo = foo;
  }
}

I guess it is a bug. The output should be the very same as above.

Or am I missing an idea behind it? Is it written somewhere in documentation that useOptional makes sense only together with openApiNullable?

@MelleD as you seem to be an expert of using Optional, what are your thoughts of it?
Should I provide a PR to fix?

openapi-generator version

7.10.0

[slobodator]

@wing328 If you allow me to tag you, your opinion is highly appreciated as well

[MelleD]

I see no reason why both options are related to each other. IMHO Optional is also valid without JsonNullable.

Should I provide a PR to fix?

Yes would be great

[slobodator]

Yes would be great

TL; DR. Ok, I will try to provide something.

Before I start, let's confirm that we are at the same page.

Assume there is a spec

openapi: 3.0.0
components:
  schemas:
    Item:
      type: object
      properties:
        foo:
          type: string
        bar:
          type: string
          nullable: true

Both foo and bar are not required. bar is nullable to emphasize that

{
  "foo": "someValue"
  // "bar" is not mentioned
}

... and

{
  "foo": "someValue"
  "bar": null
}

... have different meanings.

Now we have two config parameters openApiNullable and useOptional and their 4 combinations.

1. openApiNullable:false and useOptional:false

It generates

  private @Nullable String foo;
  private @Nullable String bar = null;

Explanations:

• @Nullable is nothing more than a kind warning for IDEs/static analyzers. Don't pay much attention on that, please
• not sure if it makes sense to assign that null explicitly, not a big deal anyhow
• there is no way to differentiate nullable bar

2. openApiNullable:true and useOptional:false. These are default settings.

It generates

  private @Nullable String foo;
  private JsonNullable<String> bar = JsonNullable.<String>undefined();

Explanations:

• JsonNullable wraps that nullable bar and handles if it is missed or assigned to null
• foo is a regular property. It might be null, .getFoo().anyMethod() might throw a NPE, the only minor safety net is @Nullable warning

3. openApiNullable:true and useOptional:true. I would say they are recommended and convenient settings.

It generates

  private Optional<String> foo = Optional.empty();
  private JsonNullable<String> bar = JsonNullable.<String>undefined();

Explanations:

• both vars are guarded by associated wrappers. So far so good
• @Nullable is not used as it is not needed at all

4. openApiNullable:false and useOptional:true.

Currently it generates

  private String foo;
  private String bar = null;

Explanations:

• this is not good as Optional is not actually used

I am going to change it to

  private Optional<String> foo = Optional.empty();
  private Optional<String> bar = Optional.empty();

Explanations:

• JsonNullable is not used as it was turned off explicitly
• so, there is no way to differentiate nullable bar unfortunately. Theoretically, Optional could handle three states (null, empty and value) but it is a very bad practice causing unexpected NPEs. Noone should be forced to check Optional against null. So, there are just two states for the bar.
• NPEs are guarded by Optional
• @Nullable is not used

So, the algorithm is

• if useOptional:true
• wrap all non-required properties with Optional
• but if openApiNullable:true and they are marked as nullable then JsonNullable has a priority

Any thoughts or concerns?

[MelleD]

I think point 4 is not correct if you explicitly set to nullable true it should be not an Optional. Also Jackson will not handle that because it will generate a Optional#empty instead of null.
Not sure but if nullable true generate the same code, do you think this is not misleading?

IMHO if you look into the semantic in json is absent and set to null semantically the same meaning the value is not there. What is in a POST or GET the difference between both states?
Excpetion is a tri state scenario for patch.

[slobodator]

Frankly speaking, I am not a big fan of using nullable as handling 3 states is harder and error prone. Still, it's nice to have this feature. I couldn't think out of my head a good scenario for GET but it definitely could make sense for PATCH as you mentioned. Also, it could be used for POST the same way

POST /items

{
  "foo" : "123"
  // "bar" is not mentioned, meaning it should be set to its default value "xyz"
}

POST /items

{
  "foo" : "123",
  "bar": null // this time I really want to set it to "null" explicitly
}

As far as I see the only valid to support 3 states is JsonNullable.

So, how are we going to handle the nullable bar if openApiNullable is turned off explicitly (but useOptional:true)?

• say a warning that nullable won't be taken into consideration?
• throw an error that the spec could not be processed, and the file could not be generated?
• use JsonNullable anyhow?
• wrap it with Optional at least? -- my suggestion
• not wrap it with Optional AKA put just a property? -- your suggestion if I got you right. What is the benefit of it?
• your option?

[MelleD]

No matter how you're going to do it. Someone will complain ;).

First of all, JsonNullable is made for a merge patch (
I think the name actually says it). According to the spec you just need a tristate (null, absent, value). So far this was the only point in my application where I needed it. It is also explicitly described here in the README:
https://github.com/OpenAPITools/jackson-databind-nullable

Of course, you might find other use cases, although I don't know whether your example with Jackson would simply work. I think with absent/null Jackson uses the default value, but never tested. You don't have to do everything just because it's possible ;)

not wrap it with Optional AKA put just a property? -- your suggestion if I got you right. What is the benefit of it?

Since I don't know how people use it, everyone can decide for themselves whether an optional or a nullable field comes out.
You just give the control to the API designer and he/she can decide (for whatever reason he/she wants a nullable field).

[slobodator]

No matter how you're going to do it. Someone will complain ;).

That's a holy truth, still I wouldn't like to spend time on the PR that will be rejected. So, I would prefer to get your sign off first.

In order to keep the discussion focused, let's move forward with small "yes" questions as Socrates recommended.

I got the purpose of JsonNullable.

There are no disagreements at the points 1-3, right?

Then we're discussing just point 4 openApiNullable:false and useOptional:true.

First, you implemented it that

the template was only adapted when using JsonNullable (openApiNullable). The code remains the same for all other templates.

then you seemed to agree that

IMHO Optional is also valid without JsonNullable

For the spec

openapi: 3.0.0
components:
  schemas:
    Item:
      type: object
      properties:
        # foo is not mentioned at the required 
        foo:
          type: string
          # nullable is not set, assuming it is false by default

it will generate

  private String foo;

It is ok or not?

I am going to change it to

private Optional<String> foo = Optional.empty();

Are you fine with that?
If not, what would you suggest? To leave it as is? To change for something different?

[MelleD]

There are no disagreements at the points 1-3, right?

I thought this is current state, right? There will be no change?

then you seemed to agree that

Yes, why not?
I don't see any contradiction. I just started with the feature only if openApiNullable is true. It was a starting point, but if someone wants to use Optional without JsonNullable, that's perfectly valid. The feature request has not yet been implemented. So far the demand for it hasn't been that high :) -- till now

I also do not remember every detail from my PR. It's a year ago.

Are you fine with that?

yes

nullable: false --> Optional
nullable: true --> I would just leave it as it is

But again this is just my personal opinion (and in the end not my decision) and I will not use the feature (because I use optional and jsonnullable), so if you would like to go with Optional for both cases, go ahead. Also fine for me.
Maybe @welshm or the community have an opinion on that.

[welshm]

I also use both optional and json nullable so I don't have a strong opinion here.

I agree that the behaviour of not having optional parameters as Optional<> when they are not required (but not nullable) seems like a bug - your example above being foo.

If you wanted to preserve the current behaviour, you could add a third option.

useOptional: true
jsonNullable: false
nullableAsJavaOptional: true - new parameter, which makes nullable properties Optional<> but only applies when jsonNullable is set to false.

Then the developer can decide what they want (which should also solve your use case).

openapi: 3.0.0
components:
  schemas:
    Item:
      type: object
      properties:
        foo:
          type: string
        bar:
          type: string
          nullable: true

useOptional: true
jsonNullable: false
nullableAsJavaOptoinal: false

private Optional<String> foo = Optional.empty();  // New behaviour, _NOT_ backwards compatible. 
private @Nullable String bar = null;

useOptional: true
jsonNullable: false
nullableAsJavaOptoinal: true  // CHANGED

private Optional<String> foo = Optional.empty();
private Optional<String> bar = Optional.empty();

[MelleD]

@welshm the only question is what make the nullable true/false then in the spec? If the generated code is the same and also the json is the same. And there is no difference between nullable true/false then it looks redundant 😀

[welshm]

@welshm the only question is what make the nullable true/false then in the spec? If the generated code is the same and also the json is the same. And there is no difference between nullable true/false then it looks redundant 😀

I'm not sure I understand - nullable true false in the spec would be based on nullable: true?

The generated code is different depending on what options are set based on the discussion above.

I feel like I'm not understanding the point of what you're trying to tell me 😅

[wing328]

nullable true false in the spec would be based on nullable: true

Just want to share that in the openapi normalizer, there's a rule called SET_PRIMITIVE_TYPES_TO_NULLABLE

When set to string|integer|number|boolean (or just string) for example, it will set the type to nullable (nullable: true)

For example, one can enable this rule to get int? (nullable integer) instead of int (default) from the csharp client generator.

[slobodator]

I thought this is current state, right? There will be no change?

Correct. We're talking just about case 4 openApiNullable: false, useOptional:true

if someone wants to use Optional without JsonNullable, that's perfectly valid

I think the same

We seemed to come to conclusion that for the foo (nullable: false) it will be Optional

nullable: false --> Optional

The only thing to discuss is what to do with bar (nullable: true)

The options are:

1. to leave it as is AKA a raw value

nullable: true --> I would just leave it as it is

I don't see good reason for it. Why not wrap it with Optional? If it not required i.e. might be null,JsonNullable is not available as it was turned off by openApiNullable:false, so why not Optional if it is explicitly turned on?

2. wrap it with Optional

That's what I'm tending to do but we're still dicussing

if you would like to go with Optional for both cases, go ahead. Also fine for me.

3. make it configurable with nullableAsJavaOptional as @welshm suggests

Currently two feature toggles make 4 combinations, adding a new one will lead to handle 8 options. It will be a mess at the mustashe templates I am afraid. A developer should be aware of the new key, we need to discuss its reasonable default, cover all possible 8 cases with tests, update the documentation... to many efforts for me, sorry.

So, I am tending to wrap both cases with Optional.

Still, if you still strictly state that nullable:true should be left as is I don't mind as I don't use it much (i.e. PATCH, tristate and JsonNullable are rare guests at my code). That would be even less efforts for me.

P.S. If we don't come to agreement, does it make sense to amend documentation a bit that currently useOptional at models is applied only if openApiNullable is not turned off? I.e. to emphasize the current behaviour?

[MelleD]

How I said I don’t use it. Go for suggestion.
My only question mark what we as user is confusing what is the different between nullable:true and nullable:false when both ends in Optional