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

Fix Retrofit template with Jackson for Kotlin #14239

Merged

Conversation

yonatankarp
Copy link
Contributor

@yonatankarp yonatankarp commented Dec 10, 2022

To fix #8147

This commit amends the ApiClient template for Retrofit. It fixes a bug where the Retrofit client uses Jackson as the serialization library.

Until now, the ApiClient could not be compiled when using the Jackson library (default in SpringBoot applications), as the constructor of the class had the following parameter:

private val serializerBuilder: Builder = Serializer.Builder,

You can see it more clearly here:

image

After the fix the outcome of this line would be:

private val serializerBuilder: ObjectMapper = Serializer.jacksonObjectMapper,

The configuration that was used for the test is as follows:

config = mapOf(
    "dateLibrary" to "java8",
    "interfaceOnly" to "true",
    "implicitHeaders" to "true",
    "hideGenerationTimestamp" to "true",
    "useTags" to "true",
    "documentationProvider" to "none",
    "serializationLibrary" to "jackson",
    "useCoroutines" to "true",
    "library" to "jvm-retrofit2"
)

Commits:

  1. The bug fix itself
  2. Adding code sample for CI comparison
  3. Update existing samples with the new change.

All commits would be squashed into a single commit when this PR is approved, and are separated only for easier review process.


The change done in the commit is to ensure that in the case of Jackson, the right property of the Serializer class is assigned

validation of this change was by running the generator and ensuring that code the generated code passed compilation.

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 (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.

Copy link
Contributor Author

@yonatankarp yonatankarp left a comment

Choose a reason for hiding this comment

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

@jimschubert could you please have a look? Noticed this bug at work yesterday. We've fixed it locally by altering the templates, but I think that it's better to fix it on the generator level.

@yonatankarp yonatankarp force-pushed the fix_retrofit_generator_with_jackson branch 2 times, most recently from 5dc216b to 11edd12 Compare December 10, 2022 09:44
@yonatankarp yonatankarp changed the title Fix Retrofit template with Jackson for Kotlin Fix Retrofit template with Jackson for Kotlin (Fix #8147) Dec 10, 2022
@yonatankarp yonatankarp force-pushed the fix_retrofit_generator_with_jackson branch from 11edd12 to da0eabf Compare March 20, 2023 15:54
Copy link
Contributor Author

@yonatankarp yonatankarp left a comment

Choose a reason for hiding this comment

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

@jimschubert after rebasing again on master all broken CI's have passed. can you please review this small fix? 🙏

@wing328
Copy link
Member

wing328 commented Aug 6, 2023

@yonatankarp thanks for the PR. Can you please PM me via Slack to discuss this further?

https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g

@wing328 wing328 added this to the 7.0.0 milestone Aug 10, 2023
@yonatankarp yonatankarp force-pushed the fix_retrofit_generator_with_jackson branch 2 times, most recently from dc64035 to d7a1728 Compare August 10, 2023 18:19
@yonatankarp yonatankarp force-pushed the fix_retrofit_generator_with_jackson branch 3 times, most recently from 7933f59 to 8719b8b Compare August 10, 2023 18:58
@@ -0,0 +1,9 @@
generatorName: kotlin
outputDir: samples/client/petstore/kotlin-jvm-jackson
inputSpec: modules/openapi-generator/src/test/resources/2_0/petstore.yaml
Copy link
Member

Choose a reason for hiding this comment

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

I did a test locally but got the following errors:

➜  kotlin-jvm-jackson git:(fix_retrofit_generator_with_jackson) ✗ gradle build
Starting a Gradle Daemon (subsequent builds will be faster)

> Task :compileKotlin
e: /Users/williamcheng/gitclone/yonatankarp_fix_retrofit_generator_with_jackson/samples/client/petstore/kotlin-jvm-jackson/src/main/kotlin/org/openapitools/client/infrastructure/ApiClient.kt: (60, 28): Unresolved reference: Builder
e: /Users/williamcheng/gitclone/yonatankarp_fix_retrofit_generator_with_jackson/samples/client/petstore/kotlin-jvm-jackson/src/main/kotlin/org/openapitools/client/infrastructure/ApiClient.kt: (60, 49): Unresolved reference: Builder
e: /Users/williamcheng/gitclone/yonatankarp_fix_retrofit_generator_with_jackson/samples/client/petstore/kotlin-jvm-jackson/src/main/kotlin/org/openapitools/client/infrastructure/ApiClient.kt: (75, 28): Unresolved reference: Builder
e: /Users/williamcheng/gitclone/yonatankarp_fix_retrofit_generator_with_jackson/samples/client/petstore/kotlin-jvm-jackson/src/main/kotlin/org/openapitools/client/infrastructure/ApiClient.kt: (75, 49): Unresolved reference: Builder

> Task :compileKotlin FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':compileKotlin'.
> A failure occurred while executing org.jetbrains.kotlin.compilerRunner.GradleCompilerRunnerWithWorkers$GradleKotlinCompilerWorkAction
   > Compilation error. See log for more details

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
> Get more help at https://help.gradle.org.

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

Also update use 3.0 spec instead: modules/openapi-generator/src/test/resources/3_0/petstore.yaml

Copy link
Member

Choose a reason for hiding this comment

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

Please commit the new sample files as well so that github workflow will trigger the build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was indeed a but with my fix - my bad! 🤦‍♂️
I have fixed it and updated the sample app and the spec version however seems that GitHub Actions isn't picking the changes yet, and CircleCI still failing after I have rebased on master

@yonatankarp yonatankarp force-pushed the fix_retrofit_generator_with_jackson branch 2 times, most recently from 1d4038a to c5a64f7 Compare August 12, 2023 07:04
@wing328
Copy link
Member

wing328 commented Aug 12, 2023

please ignore the circleci failure for the time being.

can you please update the samples?

ref: https://github.com/OpenAPITools/openapi-generator/actions/runs/5840007126/job/15839427531?pr=14239

@wing328 wing328 changed the title Fix Retrofit template with Jackson for Kotlin (Fix #8147) Fix Retrofit template with Jackson for Kotlin Aug 12, 2023
@yonatankarp yonatankarp force-pushed the fix_retrofit_generator_with_jackson branch from c5a64f7 to 07768a7 Compare August 13, 2023 09:31
Copy link
Contributor Author

@yonatankarp yonatankarp left a comment

Choose a reason for hiding this comment

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

@wing328 I have rebased the branch on master again (seems that previously it was wrongly done as the generator version was 6.3.0-SNAPSHOT on my branch) and also executed the command:

$ bin/generate-samples.sh bin/configs/kotlin-*

Everything should (hopefully) be up-to-date now, and waiting for approval for GitHub to run the pipeline.

Moreover, the PR was split into 3 different commits to ensure easier review process 😄

@yonatankarp yonatankarp force-pushed the fix_retrofit_generator_with_jackson branch from 07768a7 to f5a1278 Compare August 13, 2023 13:54
@wing328
Copy link
Member

wing328 commented Aug 13, 2023

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@wing328
Copy link
Member

wing328 commented Aug 13, 2023

https://github.com/OpenAPITools/openapi-generator/actions/runs/5847743068/job/15854622468?pr=14239 failure can be ignored for the time being.

This commit amends the `ApiClient` template for Retrofit. It fixes a bug where the Retrofit client uses Jackson as the serialization library.

Until now, the `ApiClient` could not be compiled when using the Jackson library (default in SpringBoot applications), as the constructor of the class had the following parameter:

`private val serializerBuilder: Builder = Serializer.Builder,`

The change done in the commit, is to ensure that in case of Jackson, the right property of the `Serializer` class is assigned
This commit adds a sample code for the `kotlin-jvm-jackson` generated code to test the changes on CI
This commit only runs the command `bin/generate-samples.sh bin/configs/kotlin-*` to ensure that all code samples are up-to-date and CI can pass successfully.
@yonatankarp yonatankarp force-pushed the fix_retrofit_generator_with_jackson branch from f5a1278 to 0000b7d Compare August 13, 2023 16:07
Copy link
Contributor Author

@yonatankarp yonatankarp left a comment

Choose a reason for hiding this comment

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

Thanks a lot for noticing that!

I believe that now the commits are linked to your GitHub account.

@wing328 wing328 merged commit 83af019 into OpenAPITools:master Aug 14, 2023
@yonatankarp yonatankarp deleted the fix_retrofit_generator_with_jackson branch August 14, 2023 07:35
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] jvm-retrofit2 with Jackson issues
2 participants