Skip to content

Conversation

@sburnicki
Copy link
Contributor

@sburnicki sburnicki commented Dec 22, 2020

These commits add support for two things:

I added test cases adopted from the java spring codegen, which had the same issues (see linked issues and PRs in #8080) and tested the changes locally.

Manually test changes

Tests for model object params

Generate code based on one of the existing examples before the PR:

java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g kotlin-spring \
  -i modules/openapi-generator/src/test/resources/3_0/objectQueryParam.yaml \
  -o /tmp/openapi/8080/ \
  --skip-validate-spec

and make /tmp/openapi/8080/src/main/kotlin/org/openapitools/api/PonyApi.kt return the following:

return ResponseEntity(pageQuery, HttpStatus.OK)

also modify the return type and test to compile. After running mvn spring-boot:run, you can query via curl:

curl -X GET \
  'http://localhost:8080/pony?offset=1&limit=2' \
  -H 'Content-Type: application/json' \
  -H 'cache-control: no-cache'

which won't return anything, as pageQuery is null.

After this PR, run the same steps again. Executing the curl command will correctly return {"offset":1,"limit":2}, since
spring was able to parse the parameters as a valid pageQuery object.

Steps for date-time params

Same procedure as above:
Generate code for src/test/resources/3_0/issue_2053.yaml:

java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g kotlin-spring \             
  -i modules/openapi-generator/src/test/resources/3_0/issue_2053.yaml \
  -o /tmp/openapi/datetime/ \
  --skip-validate-spec

Test with curl:

curl -vX GET \
  'http://localhost:8080/elephants?startDate=2020-12-22' \               
  -H 'Content-Type: application/json' \
  -H 'cache-control: no-cache

curl -vX GET \
  'http://localhost:8080/zebras?startDateTime=2020-12-22T08:00:12.345Z' \
  -H 'Content-Type: application/json' \
  -H 'cache-control: no-cache'

Both requests fail with HTTP 400, since Spring cannot parse the parameters.

After the PR, repeat the same steps, both requests will return HTTP 501 (not implemented) and not fail anymore.

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.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. 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.1.x, 6.0.x
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Please review:
@jimschubert (2017/09), @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11) @yutaka0m (2020/03)

@auto-labeler
Copy link

auto-labeler bot commented Dec 22, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@sburnicki sburnicki changed the title [kotlin-spring] Support query params with model objects (#8080) [kotlin-spring] Support model objects and date-time query params Dec 22, 2020
@sburnicki
Copy link
Contributor Author

I added another commit that adds date/date-time support to this PR, as opening a separate PR would immediately result in a merge conflict, since both commits modify the one-line file modules/openapi-generator/src/main/resources/kotlin-spring/queryParams.mustache

@sburnicki sburnicki force-pushed the feature/8080-kotlin-spring-support-objects-inquery-param branch from 4f13591 to f3ff42b Compare December 23, 2020 09:13
@sburnicki sburnicki force-pushed the feature/8080-kotlin-spring-support-objects-inquery-param branch from f3ff42b to d41922b Compare January 5, 2021 13:47
@sburnicki
Copy link
Contributor Author

I re-pushed the commits since circleci failed to download dependencies last time.
Can in the meantime somebody give feedback on the contents of the PR?

@wing328
Copy link
Member

wing328 commented Jan 5, 2021

I'll try to test it out later this week. Thanks for the PR.

@wing328 wing328 added Enhancement: New generator Schema: Ktorm Generation of Ktorm schemas and removed Server: Kotlin labels Jan 5, 2021
@wing328 wing328 added this to the 5.0.1 milestone Jan 5, 2021
@sburnicki sburnicki force-pushed the feature/8080-kotlin-spring-support-objects-inquery-param branch from d41922b to da2a75d Compare January 5, 2021 14:32
@sburnicki
Copy link
Contributor Author

Thanks. The labels do not make sense in my opinion, as it neither adds a new generator, nor is it related to ktorm?

@wing328
Copy link
Member

wing328 commented Jan 6, 2021

Fixed the label.

@sburnicki
Copy link
Contributor Author

Any news on this?

@wing328 wing328 modified the milestones: 5.0.1, 5.1.0 Feb 8, 2021
@wing328 wing328 modified the milestones: 5.1.0, 5.1.1 Mar 19, 2021
@wing328
Copy link
Member

wing328 commented Mar 30, 2021

@sburnicki sorry we're busy with v5.1.0 release (which was published recently).

Can you please resolve the merge conflicts and PM me via Slack when this PR is ready for another review? Thanks.

ref: https://join.slack.com/t/openapi-generator/shared_invite/enQtNzAyNDMyOTU0OTE1LTY5ZDBiNDI5NzI5ZjQ1Y2E5OWVjMjZkYzY1ZGM2MWQ4YWFjMzcyNDY5MGI4NjQxNDBiMTlmZTc5NjY2ZTQ5MGM

@MaksimMyshkin
Copy link

When this fix will be merged? We had to use java version of spring generator to get around of this bug(( Using kotlin generator is preferable as produced dto's has nullable/notnullable types. Maybe you need some help?

@sburnicki
Copy link
Contributor Author

I also used workarounds since this wasn't merged and then forgot about this PR. I didn't notice the update from wing328 to be honest.

I'm quite busy at the moment and have no idea when I will be able to pick up the work.
@MaksimMyshkin If you have some time, feel free to resolve the merge conflicts to get this merged. Tests should be in place.

@MaksimMyshkin
Copy link

How can I fix conflict in foreign fork? Only option that I see is to make new fork, move all changes there and crate new PR)

@sburnicki
Copy link
Contributor Author

I gave you access to my fork, it's not used for anything else anyway.

…in-spring-support-objects-inquery-param

# Conflicts:
#	modules/openapi-generator/src/main/resources/kotlin-spring/queryParams.mustache
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/kotlin/spring/KotlinSpringServerCodegenTest.java
@MaksimMyshkin
Copy link

Done 😉

@sburnicki
Copy link
Contributor Author

@wing328 This seems to be ready then :)

@wing328
Copy link
Member

wing328 commented Jun 3, 2021

I'll try to review and test tomorrow. Thanks again for the PR.

@zipper2110
Copy link

@wing328 how is it going then? The build doesn't seem to work for reasons not related to the PR itself. Would be happy to see this merged.

@wing328 wing328 added this to the 5.2.1 milestone Jul 29, 2021
@wing328
Copy link
Member

wing328 commented Jul 29, 2021

Tested locally and didn't spot issues related to this change. Found an issue with the auto-generated test file thought. Will file a PR to fix it separately.

Thanks again for the PR and sorry for the delay in reviewing.

@wing328 wing328 merged commit d18ee54 into OpenAPITools:master Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][KOTLIN-SPRING] When using models in query params, the RequestParam should not be generated

4 participants