Skip to content

[BUG][Kotlin Client] API using case other than camelCase not generated properly #2391

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

Conversation

lemoinem
Copy link
Contributor

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh, ./bin/security/{LANG}-petstore.sh and ./bin/openapi3/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language. (/cc @jimschubert @dr4ke616)

Description of the PR

Generating from a json file, if an API uses anything other than camelCase, the property names are not generated with the proper Json name. The generated code does not serialize/deserialize properly.

Adding a @Json annotation on the properties fixes it.

@jimschubert
Copy link
Member

Thanks for the PR. I think we had similar code before but it was removed. I may be remembering another generator, so I'll need to do some investigation. If memory serves, the issue was that it removes the possibility of xml support.

I'll try to do some digging tonight or tomorrow, unless someone else on the core team remembers the scenario.

@wing328 wing328 added this to the 4.0.0 milestone Mar 14, 2019
@lemoinem lemoinem force-pushed the kotlin-client/json-annotation-for-data-class-constructor branch from b963b1e to 91c7bb6 Compare March 14, 2019 13:20
@lemoinem lemoinem force-pushed the kotlin-client/json-annotation-for-data-class-constructor branch from 91c7bb6 to 5a47c55 Compare March 14, 2019 13:29
@lemoinem
Copy link
Contributor Author

Please note that the Kotlin client code, only support JSON serialization and deserialization (see https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/kotlin-client/infrastructure/ApiClient.kt.mustache#L33-L68)

In the case where we do choose to implement XML support (which is out of scope for this PR), I'm still having a hard time understanding how adding a Moshi (which is only a JSON serializer) annotation could prevent XML Support.
If the interaction actually exists, I'd prefer we find a way around it once we've settled on a concrete XML serializer. requiring APIs to use camelCase is a pretty strong requirement IMO. Especially without documentation, warnings, error or exception thrown. It took me quite a while to realize this was the issue.

I noticed the circleci tests failed because of the missing imports for the annotation. I've fixed this in a new commit plus the new line issue.

@jimschubert
Copy link
Member

@lemoinem I searched through history in this project and swagger-codegen, and I couldn't find the code I was remembering. It must have been an issue from another generator that I was remembering. You're correct that Moshi is JSON only, which I should've realized immediately that what I was thinking about couldn't have been related to this PR!

This is good to merge once the builds pass.

@lemoinem
Copy link
Contributor Author

Thanks! Glad we agree!

If you can confirm which output you prefer (@Json on its own line or together with the var), I'll change that and fix the CircleCI check ASAP.

@jimschubert
Copy link
Member

@lemoinem Sorry for taking a bit to respond. I personally prefer the oneliners, but in this case I would stick with the official Kotlin style guide: https://kotlinlang.org/docs/reference/coding-conventions.html#modifiers. Basically it says only single annotations without parameters should go on the same line, all others go above.

My point previously was mainly about the additional newlines, though. In generated code, people often consider large "unconventional" spacing to be an indicator that something didn't get generated as expected. It can sometimes be a pain with mustache and newlines at the ends of files, so no worries if the whitespace remains.

@lemoinem lemoinem force-pushed the kotlin-client/json-annotation-for-data-class-constructor branch from 5a47c55 to 2b31621 Compare March 21, 2019 17:49
@lemoinem
Copy link
Contributor Author

I've fixed the templates to match the style guide for annotation (for all templates, just so it's consistent).
I've also updated the code samples generated from the templates. This should fix the build.

@jimschubert
Copy link
Member

Shippable failure was unrelated to this PR. I've retried the build.

@jimschubert jimschubert self-assigned this Mar 25, 2019
@wing328 wing328 merged commit 81c57ee into OpenAPITools:master Mar 27, 2019
@lemoinem lemoinem deleted the kotlin-client/json-annotation-for-data-class-constructor branch March 27, 2019 12:19
@lemoinem
Copy link
Contributor Author

Great, thanks!

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.

3 participants