Skip to content

[kotlin-spring] fix #9902 use coroutine Flow for arrays in delegate when reactive=true#11695

Merged
wing328 merged 2 commits intoOpenAPITools:masterfrom
giveadamakick:#9902-use-flow-for-arrays-in-kotlin-delegate
Feb 25, 2022
Merged

[kotlin-spring] fix #9902 use coroutine Flow for arrays in delegate when reactive=true#11695
wing328 merged 2 commits intoOpenAPITools:masterfrom
giveadamakick:#9902-use-flow-for-arrays-in-kotlin-delegate

Conversation

@giveadamakick
Copy link
Copy Markdown
Contributor

@giveadamakick giveadamakick commented Feb 23, 2022 •
edited
Loading

This PR fixes the problem described in issue #9902, which I'll summarize here:

  • For the kotlin-spring generator, with delegatePattern and reactive set to true, the generated API file should use a Kotlin coroutine-type Flow when dealing with an OpenAPI array data type.
  • For a requestBody parameter, it does this for a generated API class, but not for a generated Delegate class. In generated Delegate classes, an array type that is the request body is generated as a kotlin.collections.List instead of a Flow.
  • This is not only incorrect, it also means the generated code does not compile, as the API class is attempting to pass a Flow to a Delegate that is expecting a kotlin.collections.List.

To fix this, I have made the following changes:

  • I have updated the relevant Delegate Moustache template to use a Flow when the requestBody parameter is an array.
  • I've also added some unit tests to cover this case.

How to validate

  1. Using this OpenAPI spec, and using an openapi-generator-maven-plugin config with delegatePattern and reactive set to true (see this example I used), run a mvn install without the changes in this PR. You'll see that the generated code does not compile.
  2. Repeat the above using the generator including the changes in this PR. The code now compiles, and the delegate has the correct Flow type for the requestBody parameter.

After running both ./bin/generate-samples.sh and ./bin/utils/export_docs_generators.sh, no extra files were generated, so nothing extra to include in this PR.

Mentioning Kotlin technical committee members: @jimschubert @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m

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.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@giveadamakick giveadamakick mentioned this pull request Feb 23, 2022
Closed
@blawlor
Copy link
Copy Markdown

blawlor commented Feb 23, 2022

Looking forward to seeing this get merged - really need it! :-)

@giveadamakick
Copy link
Copy Markdown
Contributor Author

giveadamakick commented Feb 23, 2022 •
edited
Loading

The "node2" CircleCI tests failed. Looking at the reason for the failure, it seems to be something to do with the GoGinServer project, which my PR does not touch:

/home/circleci/.go_workspace/src/golang.org/x/net/http2/transport.go:417:45: undefined: os.ErrDeadlineExceeded
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (go-get) on project GoGinServer: Command execution failed. Process exited with an error: 2 (Exit value: 2) -> [Help 1]

Could this be due to build flakiness?

@wing328
Copy link
Copy Markdown
Member

wing328 commented Feb 25, 2022

@giveadamakick thanks for the PR. Please ignore the CircleCI node 2 failure for the time being.

cc @jimschubert (2017/09) ❤️, @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11) @yutaka0m (2020/03)

@wing328 wing328 added this to the 6.0.0 milestone Feb 25, 2022
@wing328
Copy link
Copy Markdown
Member

wing328 commented Feb 25, 2022

I tested with petstore.yaml but got the following errors:

[ERROR] Failed to execute goal org.jetbrains.kotlin:kotlin-maven-plugin:1.3.30:compile (compile) on project openapi-spring: Compilation failure: Compilation failure: 
[ERROR] /private/tmp/kotlin-spring2/src/main/kotlin/org/openapitools/api/PetApi.kt:[70,47] Type mismatch: inferred type is List<String> but Flow<String> was expected
[ERROR] /private/tmp/kotlin-spring2/src/main/kotlin/org/openapitools/api/PetApi.kt:[81,45] Type mismatch: inferred type is List<String> but Flow<String> was expected
[ERROR] -> [Help 1]

Can you please take a look when you've time?

command to test:

java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g kotlin-spring -i modules/openapi-generator/src/test/resources/3_0/petstore.yaml -o /tmp/kotlin-spring2/ --additional-properties delegatePattern=true,reactive=true

@giveadamakick
Copy link
Copy Markdown
Contributor Author

giveadamakick commented Feb 25, 2022 •
edited
Loading

@wing328 that's strange, I can confirm that I've been able to recreate the problem you describe locally. For what it's worth, the errors you describe also happen when using master. Those errors are what this PR is meant to actually fix.

It looks like my changes are not being picked up when using openapi-generator-cli.jar. I did verify that the changes are being picked up when using the Maven plugin. I'll investigate further. Let me know if you have any idea why this would be the case in the meantime.

@wing328 update on the above: the problem that occurs on master with petstore.yaml is the existing problem that this PR is trying to fix. The problem reported by you is similar but different, in that the delegate class takes a Flow parameter, but the API class is passing a kotlin.collections.List.

@giveadamakick
Copy link
Copy Markdown
Contributor Author

giveadamakick commented Feb 25, 2022

@wing328 ok, I've worked out the cause of the compilation error you reported using petstore.yaml.

The sample petstore API contains endpoints that take an array as a value for a query parameter. In the API, these values are modeled as a Kotlin List, not as a Flow, and so were being passed to the delegate as a List. However, the change I had made meant the delegate treats all array values as a Flow.

Initially, I tried to change the types of the array query parameters to also be a Flow, but Spring doesn't support this type mapping. So in the end, the best solution is to update the delegate template to only treat a parameter as a Flow when it's both an array, and a bodyParam.

I've made this change in the latest commit, just pushed.

@wing328
Copy link
Copy Markdown
Member

wing328 commented Feb 25, 2022

@giveadamakick thanks for the prompt fix and the detailed explanation. Tested again and the result is good.

Will add some tests to CI to cover this.

Have a nice weekend.

@wing328 wing328 merged commit 8b74053 into OpenAPITools:master Feb 25, 2022
@giveadamakick giveadamakick deleted the #9902-use-flow-for-arrays-in-kotlin-delegate branch March 1, 2022 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Issue: Bug Server: Kotlin

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@giveadamakick @blawlor @wing328