-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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: Fix kotlinx_serialization #9576
Conversation
modules/openapi-generator/src/main/resources/kotlin-client/libraries/jvm-okhttp/api.mustache
Outdated
Show resolved
Hide resolved
val method: RequestMethod, | ||
val path: String, | ||
val headers: MutableMap<String, String> = mutableMapOf(), | ||
val query: MutableMap<String, List<String>> = mutableMapOf(), | ||
val body: kotlin.Any? = null | ||
val body: T? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this change has other hidden implications...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Others than providing stronger (or previously missing) type information, I don't think so. I generated code for several combinations of library
and serializationLibrary
but didn't run all of them. However, previously not a single combination worked reliably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clearer, I like this change, I think it makes sense, I was just thinking if this could have some side effects that I didn't think of.
Could you please remove the changes from #9358 on this PR? |
07e0969
to
a76c1ad
Compare
...r/src/main/resources/kotlin-client/libraries/jvm-okhttp/infrastructure/ApiClient.kt.mustache
Outdated
Show resolved
Hide resolved
a76c1ad
to
5d8d20e
Compare
Can you please run to try to fix CI?
Thanks |
5d8d20e
to
8752b6c
Compare
Done. Sorry, I forgot about it after the import change. My bad. |
@saschpe could you please fix the merge conflicts with the master branch please? |
8752b6c
to
766d6a9
Compare
Done. Not sure about the recent Travis-CI error though |
@saschpe thanks for the PR. When I ran
While the same command runs fine in the master. I wonder if you can take another look to see if you can reproduce the issue. |
@wing328 Sure, I'll take another look soon. |
And update to Kotlin 1.5.0 and kotlinx.serialization 1.2.1. Fix nested enum annotation '@serializable' instead of '@KSerializable' when 'kotlinx_serialization' is used. Fix missing JsonMediaType in ApiClient.kt (OpenAPITools#9242). Add 'kotlinx_serialization' serialization library to documentation. Use explicity type in RequestConfig to keep type information for JSON serialization. Resolves OpenAPITools#9242
766d6a9
to
a20eca0
Compare
Should be fixed now |
CI failures now are not related to this PR. |
No luck. Still got the same error:
Does it work for you locally? |
Maybe the gradle version also needs to be updated? |
I'm using 7.0.2:
Can you also test it locally to see if it works for you?
|
I tried in my machine using gradle wrapper and it works.
So it may be an issue with gradle version 7.X, but I think it's recommended to use the gradle wrapper. |
Yes, |
I can fix the compatibility with gradle 7.X in a different PR. |
If you look at the error message above. I simply ran |
Try to use the following command |
The problem is that you are using gradle 7.X to try to download the gradle wrapper, but the gradle 7.X looks at the |
👌 |
To add more onto this, the gradle wrapper is always copied together with the generated code, so there is no need to download it, you can just use it straight away. |
Previously there was an issue caught by the CI that we need to run |
Oh, I see, thanks for letting us know 👍 |
And update to Kotlin 1.5.0 and kotlinx.serialization 1.2.1. Fix nested
enum annotation '@serializable' instead of '@KSerializable' when
'kotlinx_serialization' is used. Fix missing JsonMediaType in
ApiClient.kt (#9242). Add 'kotlinx_serialization' serialization library
to documentation.
Resolves #9242
@jimschubert @rsinukov @wing328 @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m
Tested with
serializationLibrary
set togson
,kotlinx_serialization
and default (moshi
) as well aslibrary
set tomultiplatform
and default (jvm-okhttp4
).PR checklist
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.
master
,5.1.x
,6.0.x