Fix default empty collections when collection is optional

[MelleD]

If the collection is optional, an empty list/set is created so as not to have null values.
This behavior can be influenced by containerSetToNull

Fix issue #15891 (comment)

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) @martin-mfg (2023/08)

[wing328]

thanks for the PR.

from what I can tell, looks like we need another option, e.g. containerDefaultToEmptyContainer to fallback to 6.x behavior.

[MelleD]

@wing328

What exactly was the 6.x behavior?
Can you describe that?

And why was the 6.x behavior changed?

[wing328]

i think empty list was the default behaviour in 6.x

it was changed to allowed

• null for nullable container or optional container
• empty container for required container

with your change, JSON payload will be something like {"container":[]} instead of just {} by default, right?

[MelleD]

i think empty list was the default behaviour in 6.x

But now, that is the behaviour when containerSetToNull is false OR nullable: false is set

null for nullable container or optional container
empty container for required container

But now this is the case, if you set the optional collection to nullable: true, why is this not enough? Is there a reason or edge case?

with your change, JSON payload will be something like {"container":[]} instead of just {} by default, right?

It depends on your Jackson Object Mapper configuration:
if you use objectMapper.setSerializationInclusion( JsonInclude.Include.NON_EMPTY );
then it's {} empty fields are absent.
So it's up to the project how you would like to use it.

[wing328]

please see the comment in #15891 (comment)

no matter which way we set the default - null or empty list. some users may prefer the other way around instead.

that's why i think using an option to let users choose is the only way

[MelleD]

please see the comment in #15891 (comment)

Yes answer is set the the attribute to nullable:true and it works how expected. I mean thats the reason for this attribute, right?

no matter which way we set the default - null or empty list. some users may prefer the other way around instead.

From my point of view, it is currently possible to set any combination you want with the PR.

I don't see any advantage in using a new parameter just because users find it uncool to set nullable:true and would rather have their own parameter with a different name. That just makes things and documentation unnecessarily complicated.

If there is a use case that cannot currently be implemented, then I would understand it.

[wing328]

to be clear, I'm not against this fix at all. As you said, it covers pretty much all use cases which is good.

we can ask users to add nullable:true but as usual they may not like this approach (e.g. they are not the owner of the spec and do not want to maintain their own copy of the spec).

can you update the tests to fix https://github.com/OpenAPITools/openapi-generator/actions/runs/8245275952/job/22548958623?pr=18080?

I think later I will add a rule in normalizer (e.g. setContainerToNullable: array) to set array to nullable as some other generators (e.g. C# client) have similar use cases before.

[MelleD]

to be clear, I'm not against this fix at all. As you said, it covers pretty much all use cases which is good.

Yes 👍 , I would just to clarify the use case for a new option.

we can ask users to add nullable:true but as usual they may not like this approach (e.g. they are not the owner of the spec and do not want to maintain their own copy of the spec).

That was my question is there an "edge" case, but from the ticket, I don't see this.
So IMHO I would add a new option, when it is really necessary.

I just don't understand the difference between the new option and nullable:true.
Maybe I'm misunderstanding nullable:true, but that's exactly what it's intended for, right?

Even if the spec comes from someone else, the option is still clear. To me the option sounds like we would bypass the original open api semantics by the generator ;).

[wing328]

I don't see any advantage in using a new parameter just because users find it uncool to set nullable:true

Personally I don't want another option either as you may seen my similar replies before (some users are overloaded with too many options)

[wing328]

To me the option sounds like we would bypass the original open api semantics by the generator ;).

that's one of the directions we're going with the help of the openapi normalizer.

[MelleD]

@wing328 Can we make a new option (for whatever reason) in a different PR?

[welshm]

Would this also be resolved by specifying a default value (of an empty array) in the specification?

I can see the value of supporting this, but it does seem like the more proper solution would be to update the spec to provide a default value in the case.

[MelleD]

Would this also be resolved by specifying a default value (of an empty array) in the specification?

Good question. I think not, I changed nothing on this behaviour.
Do you mean default value is [] for a empty collection, right?

EDIT: Created a small test added

        tags:
          type: array
          default: []
          xml:
            name: tag
            wrapped: true
          items:
            $ref: '#/components/schemas/Tag'

codegen.setContainerDefaultToNull(true);

out come:

private List<@Valid TagDto> tags = new ArrayList<>();
private List<String> photoUrls = null;

So works, right?

[welshm]

Would this also be resolved by specifying a default value (of an empty array) in the specification?

Good question. I think not, I changed nothing on this behaviour. Do you mean default value is [] for a empty collection, right?

EDIT: Created a small test added

        tags:
          type: array
          default: []
          xml:
            name: tag
            wrapped: true
          items:
            $ref: '#/components/schemas/Tag'

codegen.setContainerDefaultToNull(true);

out come:

private List<@Valid TagDto> tags = new ArrayList<>();
private List<String> photoUrls = null;

So works, right?

Yeah, that is my thinking - by providing default: [] it will populate it when it's optional. Looks like it works.

I still think your change is valid for the scenario where the user can't change the spec, but seems like a workaround 🤷

[MelleD]

Yeah, that is my thinking - by providing default: [] it will populate it when it's optional. Looks like it works.

But looks for Set with uniqueItems is broken...

I still think your change is valid for the scenario where the user can't change the spec, but seems like a workaround 🤷

But why?

a)
The default behaviour is to instantiate a empty list, what for me is okay because this a normal java recommendation that you should not use null for Collections. And if you would like to have null you can use nullable:true

b)
If you don't like the default behaviour. You can change this default behaviour with containerDefaultToNull: true.
Then every Collections is set to null and you can change this with the defaultValue: []

So what is the workaround here?

And in the end you can decide with the Jackson Property how the json looks like.

[welshm]

Yeah, that is my thinking - by providing default: [] it will populate it when it's optional. Looks like it works.

But looks for Set with uniqueItems is broken...

I still think your change is valid for the scenario where the user can't change the spec, but seems like a workaround 🤷

But why?

a) The default behaviour is to instantiate a empty list, what for me is okay because this a normal java recommendation that you should not use null for Collections. And if you would like to have null you can use nullable:true

b) If you don't like the default behaviour. You can change this default behaviour with containerDefaultToNull: true. Then every Collections is set to null and you can change this with the defaultValue: []

So what is the workaround here?

And in the end you can decide with the Jackson Property how the json looks like.

Those are good points, I agree with you and think this doesn't need to be solved.

If it's broken for Set that might be a different issue - since I would imagine default value should work for Set as well

[MelleD]

If it's broken for Set that might be a different issue - since I would imagine default value should work for Set as well

Yes looks broken somehow for Set

@welshm What do you think can we merge this PR. And if need a new option and fix the Set issue we make a new PR?

[welshm-ideogram]

If it's broken for Set that might be a different issue - since I would imagine default value should work for Set as well

Yes looks broken somehow for Set

@welshm What do you think can we merge this PR. And if need a new option and fix the Set issue we make a new PR?

Sounds reasonable to me

[MelleD]

If it's broken for Set that might be a different issue - since I would imagine default value should work for Set as well

#18102

I think I found the issue

[wing328]

agreed with merging this one and file separate PRs to address other issues.

[wing328]

likely I will file a PR this weekend to create a rule in normalizer to set nullable in array to true.

[MelleD]

@wing328 and @welshm

This is the fix for the Set issue:
#18104

[wing328]

looks good. will merge after all tests pass.

thanks again for the PR.

[wing328]

FYI. Filed #18128 to add a new rule in normalizer to set container to nullable.