openapi-generator

openapi-generator copied to clipboard

Bug Report Checklist

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

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.

@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.

@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.

@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.

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.

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).

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

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.

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

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?

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.

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?

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.

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?

Is there an important distinction between required = false and nullable = true?

As context, I'm coming to this bug because of the impact on Kubernetes clients. For instance, this field is optional in the schema of a Pod, but with the earlier change clients are now initializing the field to an empty container which results in the transmission of {} rather than leaving the field unset. Unfortunately, this changes the meaning of the field. There are likely many more such cases.

I'm very happy if we are able to find a solution that resolves this problem while also not breaking @jorgerod's use case; however, I suspect it won't be reasonable to get all fields that are using required to align nullable.

Hi @rjeberhard @mosesonline @martin-mfg @wing328

The behaviour to be followed does not have to be the one that each one creates or is convenient, but the one that the OpenApi3 specification says. And the specification is very clear: by default nullable is false (see here and here )

A true value adds "null" to the allowed type specified by the type keyword, only if type is explicitly defined within the same Schema Object. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use of null as a value. A false value leaves the specified or default type unmodified. The default value is false.

If by default nullable is false, to ensure that the specification is met the collections must be initialized.

On the other hand, in Openapi Generator there is the containerDefaultToNull property. Right now that property defines all collections as null. What I would do in a new PR (I can do it myself) is that if the collection is required it is initialized and otherwise it is null. With this behavior, we could have the same as in versions previous to 7.5.0.

I add this matrix to make it clearer:

# ...
components:
  schemas:
    User:
      type: object
      properties:
        phones:
          type: array
          items:
            type: string
# ...

nullable	required	containerDefaultToNull	Result
false	false	false	private List<String> phones= new ArrayList<>();
true	false	false	private List<String> phones;
not defined	false	false	private List<String> phones= new ArrayList<>();
false	true	false	private List<String> phones= new ArrayList<>();
true	true	false	private List<String> phones;
not defined	true	false	private List<String> businessPhones = new ArrayList<>();

containerDefaultToNull	required	Result	Comments
true	false	private List<String> phones
true	true	private List<String> phones= new ArrayList<>();	TODO

I think @jorgerod explained the alternatives.

Facts:

1. The change at https://github.com/OpenAPITools/openapi-generator/issues/15891 was a breaking change in a minor version.
2. The aforementioned change https://github.com/OpenAPITools/openapi-generator/issues/15891 is the right thing to do.
3. Reverting that change means introducing ANOTHER breaking change in a minor.
4. Reverting that change is NOT the right thing to do regarding specification of nullability.

Interpretation:

Making the change at (1) in a MINOR version seemed wrong. I agree. It broke some of our clients but it's an opportunity to better define our API specs.

But once accepted this breaking change in the OpenAPI generator release and released, I think reverting it (remember that it's the right thing to do) it's completely nonsense. What is done, it's done.

Conclusion: The only error was to accept a breaking change in a (Semver) MINOR version, but the change was correct/needed. So introducing again a bug as well a new breaking change sounds spectacularly wrong.

Change my mind

@jorgerod and @juliojgd Thank you for clarifying the context and take care of this so patiently.

Hi everyone, this topic was also discussed in #18080. As a result, a new option SET_CONTAINER_TO_NULLABLE was introduced in #18128.

Here's a comparison of the different behaviours:

	7.5.0	7.4.0	7.5.0, containerDefaultToNull=true	7.5.0, SET_CONTAINER_TO_NULLABLE=array|set|map
not required, not nullable	✅	❌	❌	✅
not required, nullable	❌	❌	❌	❌
required, not nullable	✅	✅	❌	✅
required, nullable	❌	❌	❌	❌
not required, nullable not specified	✅	❌	❌	❌
required, nullable not specified	✅	✅	❌	❌

✅ = container is initialized ❌ = container is not initialized (i.e. container is null)

^{OpenAPI Generator 7.6.0 has the same behaviour as 7.5.0. Several versions before 7.4.0 have the same behaviour as 7.4.0. containerDefaultToNull is a generator specific setting of several generators. It's documented for example here. SET_CONTAINER_TO_NULLABLE is documented here. For the comparison I used the java generator without any additional settings. The comparison is based on this input spec.}

---

I think most people who face problems with the switch from 7.4.0 to 7.5.0 are affected only by the change for the case "not required, nullable not specified". If that's the case, you can use the new SET_CONTAINER_TO_NULLABLE option. In maven, it works like this:

    [...]
    <generatorName>java</generatorName>
    <openapiNormalizer>SET_CONTAINER_TO_NULLABLE=array|set|map</openapiNormalizer>
    [...]

But if you need to restore the 7.4.0 behaviour for all rows in the table, there is currently no simple option to do this. I think it would make sense to introduce an optional setting to fully restore the 7.4.0 behaviour, and it would be great if someone could create a PR for this. (I would not advise to change the behaviour of the existing option containerDefaultToNull, because changing this would again breaks someone's use case.)

Do you have any additional thoughts, @wing328?

Chiming in here, the change to initialize collections to an empty collection broke multiple tests for one of our projects.

Admittedly, some specs are woefully terrible at my company, but my team has no control over another's teams' specification. So for some, we disable schema validation to generate the models/POJOs to consume their API from our Spring Boot applications.

For our use case specifically, we have the code generation in a separate dedicated project which is published as a Java library internally. The project has a plethora of integration tests with WireMock to mock the actual backend for the specifications. We upgraded the Gradle plugin from 7.3.0 to 7.7.0 and observed the failures. After some investigation, I came across various issues and finally landing on this one.

Configuring containerDefaultToNull to true resolves some of the issues, but I think one could argue that initializing all of the collections to an empty collection could result is degradation to JVM applications since memory consumption goes up. As always, benchmark don't guess.

@ciscoo the discussion here is about correctness, not resource consumption. Anyway an empty collection should not consume so many memory resources.

:wave: Chiming in here as another consumer who was broken by this change. We had an optional property with schema {type: array, minItems: 1}, so version 7.5.0 of the generator led to valid requests (that omit the property) failing validation.

The change might be a correct reading of the OAS spec (not sure, although it seems wrong to me), but... in the future could we ensure that changes like that go into new major versions of the generator? Breaking changes without a major version are unpredictable and confusing to consume.

@tomdeering-wf You are right, IMHO OpenAPI generator project has a history of introducing breaking changes in minor versions. This is wrong and needs to be improved. Breaking changes should be introduced in major versions only.

But once made the wrongdoing (breaking change in a minor version), considering that the change is a fix, reverting it would do more harm than good, in my opinion.

I think @jorgerod explained the alternatives.

Facts:

1. The change at [BUG][JAVA] Since 6.5.0 Generated Lists no longer default to empty List but null #15891 was a breaking change in a minor version.
2. The aforementioned change [BUG][JAVA] Since 6.5.0 Generated Lists no longer default to empty List but null #15891 is the right thing to do.
3. Reverting that change means introducing ANOTHER breaking change in a minor.
4. Reverting that change is NOT the right thing to do regarding specification of nullability.

Interpretation:

Making the change at (1) in a MINOR version seemed wrong. I agree. It broke some of our clients but it's an opportunity to better define our API specs.

But once accepted this breaking change in the OpenAPI generator release and released, I think reverting it (remember that it's the right thing to do) it's completely nonsense. What is done, it's done.

Conclusion: The only error was to accept a breaking change in a (Semver) MINOR version, but the change was correct/needed. So introducing again a bug as well a new breaking change sounds spectacularly wrong.

Change my mind

I disagree with this interpretation and point 2 in particular.

I think several things are being mixed up here. In my understanding both required and nullable are concerned with what values are allowed in the object field on the wire, not directly with what happens in the implementation.

• required means the field has to be present, saying nothing about allowed values
• nullable means beside the specified type of the field value, an explicit null value is also allowed, if the value is actually present.

So these are two completely separate conditions. nullable: false still allows the field to not be present at all. From what I can tell the decision was made for the java generators, to not distinguish between a missing field and a field containing null. This is reasonable, I think. It also means though, that with nullable: false and required: false the field might be missing on the wire and the field on the object would have to contain the value null after deserialization. This use-case is currently broken.

Most importantly, a patch-semantic of "update all fields that are set, while skipping missing fields", can now unknowingly delete all that data that should have been left alone. This is a very dangerous breaking change! Luckily in our case it was caught in a test early enough.

Another problem with the current solution, is that the current intepretation of a missing field is not stable. As discussed above, required: false allows not sending the field at all. Currently with nullable: false this would lead to an empty array to be created. Now imagine, that you would extend the semantics of the field with a new value null, by setting nullable: true. Then suddenly the missing field would be interpreted as null as well instead of as an empty array.

So I think the behaviour should be reverted to how it was in 7.4 asap. I also agree that these kind of changes should usually not be done in a minor version. However due to the danger of data-loss I think fixing the problem asap in any release is more important right now.

It seems there is no consensus about the correct behaviour. The meaning of containerDefaultToNull or SET_CONTAINER_TO_NULLABLE is not clear because it depends on other attributes (required, optional...) and combination of properties.

Instead of containerDefaultToNull=true/false we could specify exactly when the default is null. containerDefaultToNull=<expression> takes precedence to other properties. containerDefaultToNull=true/false is deprecated but kept for backward compatibility. Same for SET_CONTAINER_TO_NULLABLE

I just can't find a good syntax for the expression used in containteDefaultToNull=<expression>. It needs to be not ambiguous and easy to parse.

The maxtrix with 6 rows can be used to specify exactly the generated code. Using the row number (from 1 to 6):

To replicate the 7.5.0 behaviour: containerDefaultToNull=24 (2 meaning second row, i.e. not required, nullable and 4 meaning 4th row i.e. not required nullable not specified. or if we want to specify the containerType: containerDefaultToNull=array:24,set:24,map:24 or containerDefaultToNull=array|set|map=24 To replicate the 7.4.0 behaviour: containerDefaultToNull=1245

This is really precise but very obscure.

So a better syntax is needed. What do think of this one?

Using exclamation mark for not, question mark for not specified, pipe for or. It would give: for 7.5.0: containerDefaultToNull=!required&nullable | required&nullable for 7.4.0: containerDefaultToNull=!required&!nullable | !required&!nullable | required&nullable | !required&?nullable

To specify the container type: containerDefaultToNull=array:!required&nullable | required&nullable , set|map:!required&nullable | required&nullable

Shortcuts for the expression could be added: containerDefaultToNull=7.4.0 // revert to the 7.4.0 behaviour. containerDefaultToNull=openapi3specification // combination of @jorgerod containerDefaultToNull=all // equivalent to containerDefaultToNull=true containerDefaultToNull=array|set|map // equivalent of SET_CONTAINER_TO_NULLABLE=array|set|map

Implementation wise: when the expression is parsed, it is really easy to set the default using a decision table.

It seems there is no consensus about the correct behaviour. The meaning of containerDefaultToNull or SET_CONTAINER_TO_NULLABLE is not clear because it depends on other attributes (required, optional...) and combination of properties.

Instead of containerDefaultToNull=true/false we could specify exactly when the default is null. containerDefaultToNull=<expression> takes precedence to other properties. containerDefaultToNull=true/false is deprecated but kept for backward compatibility. Same for SET_CONTAINER_TO_NULLABLE

I just can't find a good syntax for the expression used in containteDefaultToNull=<expression>. It needs to be not ambiguous and easy to parse.

The maxtrix with 6 rows can be used to specify exactly the generated code. Using the row number (from 1 to 6):

To replicate the 7.5.0 behaviour: containerDefaultToNull=24 (2 meaning second row, i.e. not required, nullable and 4 meaning 4th row i.e. not required nullable not specified. or if we want to specify the containerType: containerDefaultToNull=array:24,set:24,map:24 or containerDefaultToNull=array|set|map=24 To replicate the 7.4.0 behaviour: containerDefaultToNull=1245

This is really precise but very obscure.

So a better syntax is needed. What do think of this one?

Using exclamation mark for not, question mark for not specified, pipe for or. It would give: for 7.5.0: containerDefaultToNull=!required&nullable | required&nullable for 7.4.0: containerDefaultToNull=!required&!nullable | !required&!nullable | required&nullable | !required&?nullable

To specify the container type: containerDefaultToNull=array:!required&nullable | required&nullable , set|map:!required&nullable | required&nullable

Shortcuts for the expression could be added: containerDefaultToNull=7.4.0 // revert to the 7.4.0 behaviour. containerDefaultToNull=openapi3specification // combination of @jorgerod containerDefaultToNull=all // equivalent to containerDefaultToNull=true containerDefaultToNull=array|set|map // equivalent of SET_CONTAINER_TO_NULLABLE=array|set|map

Implementation wise: when the expression is parsed, it is really easy to set the default using a decision table.

Hi @wing328 @mosesonline @bodograumann @martin-mfg @juliojgd @jpfinne

I think this is a good option. Right now there is a solution that is not complete and is quite controversial.

What would seem important to me is that this change comes after a full check and approval by the openapi-generator maintainers. Also, ideally, this is a behaviour that should apply to all languages.

@jorgerod does my PR fit your needs?

@jorgerod does my PR fit your needs?

I think this is a very important change. It should be endorsed by a project maintainer (I just contribute from time to time and give my opinion).

@wing328 @bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @karismann @Zomzog @lwlee2608 @martin-mfg

IMHO, it looks good.

Hi @jpfinne, I like your proposal! Also, thanks for preparing a PR already.

I am wondering if anyone needs different initialization behaviours for array vs set vs map? If so, please let us know here. Otherwise I'd propose to not make this distinction.

Since in the proposed changes the option is still named containerDefaultToNull, it should not be deprecated. But the old valid values - "true" and "false" - should still work the same as before.

Since containerDefaultToNull is available only in the Java based generators, I think SET_CONTAINER_TO_NULLABLE should not be deprecated. So other generators can still use that option.

I think your proposal is an excelent compromise. @jorgerod , @jpfinne . Thanks for moving the issue ahead. In our use-case there is no need to differentiate between different container types, @martin-mfg . Hopefully we can get the PR merged soon.

Hi @martin-mfg, I noticed you received one approval for the PR addressing this issue. It’s likely to be merged soon? If there’s anything I can assist with, I’d be happy to help.