Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Kotlin][Client] Enum toString handling #5327

Merged

Conversation

AlexanderEggers
Copy link
Contributor

I have noticed that in certain cases, the enum usage for path parameter and url query parameter is incorrectly implemented.

In the case I have an enum with MY_ENUM("MyEnum") and I am using the enum as a path parameter. The path would look like that "myPath/.../MY_ENUM/...". This request would always fail. That is due to the different enum value than the server would expect.

To fix this issue, I suggest we add a custom toString to every enum class so that we will not use the enum var name but the value. That way the path would look like this "myPath/.../MyEnum/...".

Changes made with this PR:

  • Added custom toString to the enum class script.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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

This toString avoids using the enum var name and uses the enum's value instead. This will fix cases when enum var name and value are quite different.
@4brunu
Copy link
Contributor

4brunu commented Feb 16, 2020

Thanks for your contribution 👍
Can you please run ./mvnw package && sh bin/kotlin-client-all.sh and commit the changes in the pet projects?

Copy link
Member

@jimschubert jimschubert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great idea for strings. You'd get a compile error if the dataType for the enum is not a string, though. To fix this, you'd need to check the isString option and to a toString() for all other types.

Co-Authored-By: Jim Schubert <[email protected]>
@AlexanderEggers
Copy link
Contributor Author

@4brunu I already run this but it caused no changes to the sample project. I guess the sample project are not using any enums?

@jimschubert
Copy link
Member

I guess the sample project are not using any enums?

Looks like there are only two enums in the test yaml for Kotlin: a query string and a model property. We might want to regenerate at some point later with a yaml that tests more use cases.

The Shippable error was a failure to build Elixir client due to Shippable being flaky. I've kicked off a rebuild. If it fails again, we shouldn't let this block merging.

Copy link
Member

@jimschubert jimschubert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jimschubert jimschubert added this to the 4.3.0 milestone Feb 18, 2020
@wing328
Copy link
Member

wing328 commented Feb 18, 2020

We might want to regenerate at some point later with a yaml that tests more use cases.

We should switch to the fake petstore spec which contains a lot more tests for enum. Last time I tried to switch but got compilation errors. I'll test again later to see if these PRs for the Kotlin generators fix the issue.

@jimschubert jimschubert merged commit a7f2791 into OpenAPITools:master Feb 19, 2020
@AlexanderEggers AlexanderEggers deleted the feature/enum-to-string branch February 19, 2020 03:06
MikailBag pushed a commit to MikailBag/openapi-generator that referenced this pull request Mar 23, 2020
* Added toString to enum_class script

This toString avoids using the enum var name and uses the enum's value instead. This will fix cases when enum var name and value are quite different.

* Updated enum template

Co-Authored-By: Jim Schubert <[email protected]>

Co-authored-by: Jim Schubert <[email protected]>
@wing328
Copy link
Member

wing328 commented Mar 27, 2020

@Mordag thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952

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.

5 participants