search

OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
20.48k stars 6.25k forks source link

[BUG][JAVA] optional collection fields are initialized since 7.5.0 #18735

Open mosesonline opened 1 month ago

mosesonline commented 1 month ago

Bug Report Checklist

Hello,

first: thank you for the great tool.

Description

Our classes look not the same anymore since generator version 7.5.0. All the collections are initialized. I could use the 'containerDefaultToNull' property, but than all collections are null, also the required. Before I change our code I would like to know if this is an expected behavior?

Regards

openapi-generator version

7.5.0+

OpenAPI declaration file content or url

https://github.com/OpenAPITools/openapi-generator/commit/f360c697e6d7932587262ba994824fcbcf741d86#diff-4b50afe9f2863efdbde293ab01bf3775b7e99a32d330260569cb2c63db4d7f59

Generation Details

As the test SpringCodegenTest#testCollectionTypesWithDefaults_issue_collection requires I expect the nullable fields to be null and the required fields to be initialised, but both are.

Steps to reproduce

Run the test in the fork and branch https://github.com/mosesonline/openapi-generator/tree/required-collection-issue. You can see the behavior before in the other branch https://github.com/mosesonline/openapi-generator/tree/required-collection-issue-on74 I implemented the same test there and it is green.

Related issues/PRs

Maybe the new behavior was introduced with https://github.com/OpenAPITools/openapi-generator/pull/18104?

PR: https://github.com/OpenAPITools/openapi-generator/pull/18738

Suggest a fix

If it's an issue and not expected behavior, I can work on a fix.

jpfinne commented 1 month ago

@mosesonline The changes was done in #15891 to "fix" #18080 (23 comments!) So you ask to revert the behaviour. IMHO you're right.

The use of !required was intially introduced by @wing328 in #14961 to fix #14875 and #14833.

Maybe the main issue is with the handling of containerDefaultToNull that could have a wrong implementation for required collections. As you say: "I could use the 'containerDefaultToNull' property, but than all collections are null, also the required" See #14310

I have difficulties finding the "correct" implementation for required and nullable yes/no (not to mention openapiNullable and useOptional for the spring generator). Look for example at the crazy discussion in #14765 (47 comments!) Finally it was implemented and merged in #17202. That produces unmaintanable mustache templates, multiple bugs and regressions (for example there are NullPointerExceptions when using optional in spring!)

It seems people agree to disagree. To end the debate we could define a matrix defining the behaviour of nullable/required in the contracts + configOptions in the generator. And potentially adding new configOptions to satisfy everyone's opinion.

rjeberhard commented 2 weeks ago

@mosesonline, what's the status of this fix? I saw that you created a PR, but that it had at least one failing suite. Is there a way that I or my team can help? This same issue is causing a downstream problem in the Java client. Thanks.

jpfinne commented 2 weeks ago

@mosesonline I understand your frustation. I could work on that issue. But currently I have the feeling that several PR are not reviewed nor merged. It's quite discouraging.

rjeberhard commented 2 weeks ago

Thanks @jpfinne. I see that the project has a huge number of open PR's. Is there a way that I can help shepherd this one or encourage it to get attention? At least for the Java client, it is making current releases of the client unusable because the client generates empty content "{}" for fields that should be unset.

wing328 commented 1 week ago

I could work on that issue. But currently I have the feeling that several PR are not reviewed nor merged. It's quite discouraging.

@jpfinne sorry to hear that. if you can keep the PR small, definitely it will be much easier to review.

i know contributors like to have one (big) PRs with several enhancements/bug fixes to make their life easier but a PR changing several hundred files (including samples update) is just not easy to review/get it merged.

@rjeberhard if you can help review other PRs and ideally test these locally, that would be great (you can mark the PR as "approved" to draw our attentions).

mosesonline commented 1 week ago

Sorry, for being silent. I would be happy for any help to fix theis issue.

jorgerod commented 7 hours ago

Hello @wing328 @mosesonline @jpfinne

This is again a breaking change. We cannot absorb these changes in minor versions.

Also, in my opinion it seems wrong. By default, nullable is false, so if you don't set that property, collections must be initialised.

mosesonline commented 5 hours ago

The initial change to initialize the collections was also a breaking change. It broke our code.

jorgerod commented 4 hours ago

The initial change to initialize the collections was also a breaking change. It broke our code.

The same thing happened to me...

But the solution I think is to add to the API nullable: true (remember that by default nullable is false).

What do you think?

mosesonline commented 3 hours ago

Yes, often I could modify the code or modify the api. That's what you have to do in case of breaking change. But in our case the API definition is not in our hands and we would like to prevent to modificate the definition on every new release.

I understand why you decided to change the behavior. I don't agree to do the change in 7.x. But if you don't want to change it back it's ok. I can imagine that most people don't care or changed the implementation already.

jorgerod commented 3 hours ago

Yes, often I could modify the code or modify the api. That's what you have to do in case of breaking change. But in our case the API definition is not in our hands and we would like to prevent to modificate the definition on every new release.

I understand why you decided to change the behavior. I don't agree to do the change in 7.x. But if you don't want to change it back it's ok. I can imagine that most people don't care or changed the implementation already.

To clarify, I have not changed the behaviour. I in version 7.5.0 found that behaviour change and I have adapted to it (I have hundreds of apis...) because I think it is correct that if the nullable field is not defined, the collection must be initialized.

Also, correct me if I'm wrong, but for collections not to be initialized, there is the property containerDefaultToNull. Isn't that enough?

mosesonline commented 3 hours ago

Before version 7.5.0 the collections are not initialized and with they are. So, the behavior, or the result, is an other. I admit, I don't know when the old behavior was introduced, but it changed from 7.4 to 7.5. Even if it fixes to a more standard compliant behavior it's changing it.

Unfortunately the containerDefaultToNull sets all containers to null also that containers that where initialized before 7.5.0. I have not found any configuration in the generator that doesn't force me to change code and/or api.

And yes, for me it's only one API I cannot change myself not hundreds or thousands...

If the consensus is to leave it like it behaves now. I'm totally fine.

jorgerod commented 2 hours ago

Yeah, I totally understand.

Maybe the PR should be focused on the fact that if the property containerDefaultToNull is true but the collection is as required, in that case the collection should be initialized.

What do you think?