spring: fix spring pageable used without any config by gonzalad · Pull Request #15113 · OpenAPITools/openapi-generator

gonzalad · 2023-04-02T22:35:28Z

Edit on 05/25/2023: filled PR template

Spring Pageable is imported whenever we have an API with a Pageable Json schema, even if x-spring-paginated is not set.

This was breaking existing open Api which where using a Pageable (non-Spring) component.

This commit imports Spring Pageable only if x-spring-paginated is set to true.

To achieve that, I need to wait until operation processing (fromOperation method) to alter the importMapping setting.

This fixes #13052

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/utils/export_docs_generators.sh
```
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*.
For Windows users, please run the script in Git BASH.
In case you are adding a new generator, run the following additional script :
```
./bin/utils/ensure-up-to-date
```
Commit all changed files.
File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.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.

Java Spring Technical Committee:

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

downloadpizza · 2023-04-03T13:02:57Z

Not part of the project, but LGTM, tests are also good. This should be merged since the current behaviour really only causes issues.

gonzalad · 2023-05-25T16:40:15Z

Hello, pinging the Java Spring Technical committee: @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)

Since I didn't used the PR template at first, hence this PR was perhaps lost.

Cheers,
Adrian

welshm

This will break anyone relying on the existing unintended behaviour.

We can either do this and announce it as a breaking change - or we can introduce a configuration flag.

I think what you've done is fine, just want to make sure we're being mindful that this is a breaking change.

modules/openapi-generator/src/test/resources/bugs/issue_13052.yaml

gonzalad · 2023-05-25T21:06:25Z

This will break anyone relying on the existing unintended behaviour.

We can either do this and announce it as a breaking change - or we can introduce a configuration flag.

I think what you've done is fine, just want to make sure we're being mindful that this is a breaking change.

I completely agree that this fix changes the current behaviour.
But I don't see how we can fix the issue properly without this breaking change:

we already have the x-spring-paginated configuration settings (https://openapi-generator.tech/docs/generators/spring/) which has a default value of false.

If we want no breaking change, we could:

change the default value of x-spring-paginated to true (don't know what would be the side effects).
we could add a global configuration setting to ignore x-spring-paginated (but it seems to me to be awkward to have 2 flags for the same thing)

Or just bite the bullet and advertise the breaking change (adding a note for anyone relying on the previous behaviour to set x-spring-paginated to true).

wdyt ?

welshm · 2023-05-25T21:08:09Z

This will break anyone relying on the existing unintended behaviour.
We can either do this and announce it as a breaking change - or we can introduce a configuration flag.
I think what you've done is fine, just want to make sure we're being mindful that this is a breaking change.

I completely agree that this fix changes the current behaviour. But I don't see how we can fix the issue properly without this breaking change:

we already have the x-spring-paginated configuration settings (https://openapi-generator.tech/docs/generators/spring/) which has a default value of false.

If we want no breaking change, we could:

change the default value of x-spring-paginated to true (don't know what would be the side effects).

we could add a global configuration setting to ignore x-spring-paginated (but it seems to me to be awkward to have 2 flags for the same thing)

Or just bite the bullet and advertise the breaking change (adding a note for anyone relying on the previous behaviour to set x-spring-paginated to true).

wdyt ?

I agree and I think we bite the bullet - I was just making sure we're all aware it's breaking :)

borsch · 2023-05-26T11:24:26Z

sorry, maybe I've missed something, but how this change can break existing clients?

welshm · 2023-05-26T13:14:46Z

sorry, maybe I've missed something, but how this change can break existing clients?

My understanding is that currently, if you include a Pageable in your spec, then it will import Spring Pageable even if that is not the intended behaviour

Spring Pageable is imported whenever we have an API with a Pageable Json schema, even if x-spring-paginated is not set.

This is enforcing the usage of x-spring-paginated to get that behaviour, instead of as a side effect. So if existing projects are relying on that side effect when generating, they will instead need to set x-spring-paginated: true in their configuration.

borsch · 2023-05-26T17:59:58Z

sorry, maybe I've missed something, but how this change can break existing clients?

My understanding is that currently, if you include a Pageable in your spec, then it will import Spring Pageable even if that is not the intended behaviour

Spring Pageable is imported whenever we have an API with a Pageable Json schema, even if x-spring-paginated is not set.

This is enforcing the usage of x-spring-paginated to get that behaviour, instead of as a side effect. So if existing projects are relying on that side effect when generating, they will instead need to set x-spring-paginated: true in their configuration.

I assume that users will face this issue only if they use some custom templates, because otherwise they shouldn't care much about unused import inside generate code

welshm · 2023-05-26T20:36:10Z

sorry, maybe I've missed something, but how this change can break existing clients?

My understanding is that currently, if you include a Pageable in your spec, then it will import Spring Pageable even if that is not the intended behaviour

Spring Pageable is imported whenever we have an API with a Pageable Json schema, even if x-spring-paginated is not set.

This is enforcing the usage of x-spring-paginated to get that behaviour, instead of as a side effect. So if existing projects are relying on that side effect when generating, they will instead need to set x-spring-paginated: true in their configuration.

I assume that users will face this issue only if they use some custom templates, because otherwise they shouldn't care much about unused import inside generate code

Fair point - likely to only impact those using custom templates

borsch · 2023-05-28T17:42:19Z

given above I'll merge this PR once build is green. Also I'll mark this change as "with breaking changes" since still there might be small percentage of users which might rely on previous behavior

@gonzalad please update PR & fix failed pipelines

gonzalad · 2023-05-29T20:06:38Z

Hi @borsch, the pipeline is failing on Verify git status step.

I do not reproduce it in my local laptop. I'm running:

./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
git status -s
=> no changes detected

Some things are quite strange:

it fails [1] because it finds that the sample a/samples/client/petstore/csharp-netcore/OpenAPIClientCore/src/Org.OpenAPITools/Org.OpenAPITools.csproj has changed. I never changed csharp code in this commit.
the initial PR ran find on CI. This issue appeared when I pushed the last (third) commit. I don't understand the relation between the third commit content and the error

Did something similar happened before ? Or am I looking at a wrong location ?

Thanks,
Adrian

[1] Failed CI error is available here: https://github.com/OpenAPITools/openapi-generator/actions/runs/5084497711/jobs/9136946749?pr=15113

welshm · 2023-05-29T20:37:52Z

@gonzalad I would try rebasing from master and re-pushing the branch. It could be that your branch is out of date which is causing the issues.

Spring Pageable is imported whenever we have an API with a Pageable Json schema, even if x-spring-paginated is not set. This commit imports Spring Pageable only if x-spring-paginated is set to true.

remove unrelated example

gonzalad · 2023-05-29T21:25:26Z

I just rebased and pushed, and CI looks good now

Thanks @welshm !

welshm approved these changes May 25, 2023

View reviewed changes

modules/openapi-generator/src/test/resources/bugs/issue_13052.yaml Outdated Show resolved Hide resolved

borsch added this to the 7.0.0 milestone May 28, 2023

borsch added Breaking change (without fallback) Server: Spring labels May 28, 2023

gonzalad added 3 commits May 29, 2023 22:57

spring: fix spring pageable used without any config

2ce86b8

Spring Pageable is imported whenever we have an API with a Pageable Json schema, even if x-spring-paginated is not set. This commit imports Spring Pageable only if x-spring-paginated is set to true.

spring: add unit test for pageable fix

0c96e1b

spring: fix spring pageable feedback from review

6177393

remove unrelated example

gonzalad force-pushed the bugfix/13052 branch from a07fab7 to 6177393 Compare May 29, 2023 20:57

borsch merged commit 8a0f374 into OpenAPITools:master May 30, 2023

Uh oh!

Conversation

gonzalad commented Apr 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

downloadpizza commented Apr 3, 2023

Uh oh!

gonzalad commented May 25, 2023

Uh oh!

welshm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gonzalad commented May 25, 2023

Uh oh!

welshm commented May 25, 2023

Uh oh!

borsch commented May 26, 2023

Uh oh!

welshm commented May 26, 2023

Uh oh!

borsch commented May 26, 2023

Uh oh!

welshm commented May 26, 2023

Uh oh!

borsch commented May 28, 2023

Uh oh!

gonzalad commented May 29, 2023

Uh oh!

welshm commented May 29, 2023

Uh oh!

gonzalad commented May 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gonzalad commented Apr 2, 2023 •

edited

Loading