Upgrade multiplatform libraries #7149

cromefire · 2020-08-06T19:19:14Z

Fixes #5346

Upgraded the kotlin version
Changed the gradle files to kotlin
Upgraded ktor (and fixed the code, because there have been many changes)
Removed explicit coroutines and kotlinx.serialization dependency
Fixed some codestyle

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
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

modules/openapi-generator/src/main/resources/kotlin-client/libraries/multiplatform/api.mustache

samples/client/petstore/kotlin-gson/src/main/kotlin/org/openapitools/client/models/Category.kt

cromefire · 2020-08-07T00:02:20Z

...generator/src/main/resources/kotlin-client/libraries/multiplatform/build.gradle.kts.mustache

+    ios {
+        binaries {
+            framework {
+                freeCompilerArgs = listOf("-Xobjc-generics")
+            }
+        }
+    }


I can't test IOS

So I basically need someone with a mac who can write a quick unittest for pet store or so and then run the test

cromefire · 2020-08-07T00:04:24Z

Technical committee:
@jimschubert, @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinClientCodegen.java

…ved some minor issues

cromefire · 2020-08-08T15:28:41Z

...api-generator/src/main/resources/kotlin-client/libraries/multiplatform/ApiClient.kt.mustache

+) {
+    {{#apiInfo}}
+    {{#apis}}
+    val {{{classVarName}}} = {{{classFilename}}}(baseUrl, httpClientEngine, jsonConfiguration)


This may generate the following (invalid) code:

val package = PackageApi(baseUrl, httpClientEngine, jsonConfiguration)

package is a reserved word and should be escaped. Did you perform a test that lead to the invalid example you provided? If so, could you share? I'd like to debug through to understand why package wasn't escaped.

I can get you an openapi def that reproduces it, but it's rather large

I can put together a minimal doc. I just figured I'd test the same as you if it was handy, but not a big deal if it's cumbersome.

This is the thing:
https://gitlab.com/-/snippets/2007909 (github sadly errors when I try to upload it)

But it's huge

The thing that's causing it is probably the

"tags": [ "Package" ]

Where you able to find the issue that causes it's or should I try crafting a smaller file?

Did a "hacky" solution, for now I just escape everthing. It works fine but it's not quite a best practice

wing328 · 2020-08-13T02:31:18Z

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

andrewemery · 2020-08-15T23:50:14Z

LGTM

cromefire · 2020-08-15T23:56:02Z

My last review comment (the package thing) should probably be fixed before merging though

jimschubert · 2020-08-25T02:43:03Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinClientCodegen.java

        importMapping.put("OctetByteArray", packageName + ".infrastructure.OctetByteArray");

+        // bundled client file
+        supportingFiles.add(new SupportingFile("ApiClient.kt.mustache", baseFolder, "ApiClient.kt"));


Note that renaming ApiClient.kt.mustache to ApiClientBase.kt.mustache is going to cause a lot of problems for anyone who has customized ApiClient.kt.mustache.

Can we find a naming strategy which retains ApiClient.kt.mustache without breaking existing extensions?

If not, we need to mark this PR as a breaking change and provide some guidelines in the PR for how one would migrate from using multiplatform before 5.0 to use it after this PR is merged.

It's breaking pretty much, because it used experimental / alpha state libraries (MPP itself is still experimental) and they had a lot of breaking changes themselves, so that not really something you can avoid. I tried to do my best and migrate it to stable versions so there's no deprecated, opt-in or otherwise experimental stuff anymore but it might be incompatible here and there.

I'll see what I can do to add a little migration guide or so

jimschubert · 2020-08-25T02:44:17Z

I left a few comments as inline comments rather than a "review", since they began as replies to existing comments.

cromefire · 2020-08-25T10:46:06Z

I hope I got everything

jimschubert · 2020-08-27T01:33:04Z

I hope I got everything

@cromefire Did you mean you'd hoped that you had touched each comment? I don't see any additional commits, so I'm confused about whether you were going to push additional work.

cromefire · 2020-08-27T07:28:55Z

Yeah I meant comments because you said you put them there as inline comments and I only found 2

cromefire · 2020-08-29T23:38:25Z

Upgraded to Kotlin 1.4, Ktor 1.4 and kotlinx.serialization 1.4. That way even more experimental stuff has been replaced with more stable APIs. (So there won't be as many (hopefully none) breaking changes in the future)

cromefire · 2020-08-29T23:54:30Z

Due to large code and structural differences between kotlin-(okhttp|retrofit) and kotlin-multiplatform it might be wise to split it to a separate generator (like done for many other languages)

# Conflicts: # samples/client/petstore/kotlin-multiplatform/gradlew.bat

cromefire · 2020-08-30T13:23:04Z

CI (travis & circle) seems to have some issues with keyserver.ubuntu.com timing out:

gpg: requesting key E084DAB9 from hkp server keyserver.ubuntu.com

Also Shippable is trying to find a pom file in the kotlin-multiplatform sample which of course does not exist because it uses gradle

cromefire · 2020-09-01T16:42:48Z

Do you want me to do the split here or do you want me to create a 2nd PR for it after this one is merged? If it's fine doing it in this PR is easier and faster for me (for you it might increase the number of lines changed but decrease complexity due to less entangled code)

cromefire · 2020-09-01T16:44:23Z

What I can also do if you want to keep compatibility is I can create a new generator (I've already seen that there are some -next ones) from this PR and keep the old so it can be gracefully deprecated

jimschubert · 2020-09-02T01:07:55Z

I originally thought it might be better to have multiplatform separately for the same reasons… maybe now's the time to do that with the major version coming up.

Any thoughts? @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m @OpenAPITools/generator-core-team

cromefire · 2020-09-02T08:05:02Z

Also with the new version is there a timeframe when it has to land or if it's implemented as a "new" one can it land after the version is already released?

4brunu · 2020-09-02T08:17:57Z

I also think it's a good idea to split this into a new generator, since it has more differences that similarities with the jvm kotlin clients.

cromefire · 2020-09-02T17:32:44Z

Based one your feedback (it doesn't seem like anyone wants to oppose splitting this into a new one, although it's also only been 16h) I've decided that I'll probably close this PR on the weekend (if nobody has anything against it until then) and propose a new one, which adds a new client that uses the code from this PR. I'll still be watching feedback you give me here and incorporate it into the new PR.

Following that:

What do I need to do to add a new client (other than creating the generator class for it)?
Is there anything special I need to know about (command line) options? (e.g. special naming, common options vs. additional options, etc.)

jimschubert · 2020-09-03T01:38:25Z

@cromefire I'm glad you asked. We have documentation about how to create a new generator here: https://openapi-generator.tech/docs/new-generator/.

While I was finding the link for you, I realized that it hasn't been updated with the new configuration-based approach that we've recently started following (rather than the sample script, it would be a sample config under bin/config).

jimschubert · 2020-09-03T02:17:13Z

@cromefire I've updated the documentation via #7334. By the time you read this, it'll probably be live on the site and the new.sh script in master is now updated to output a yaml config under ./bin/configs rather than a .sh and .bat script under bin.

cromefire · 2020-09-04T22:21:58Z

Replaced by #7353

cromefire added 2 commits August 6, 2020 21:11

Upgraded the version of kotlin and ktor for multiplatform

4f3593e

Fixed a lot of issues

5e4eff6

cromefire commented Aug 6, 2020

View reviewed changes

modules/openapi-generator/src/main/resources/kotlin-client/libraries/multiplatform/api.mustache Outdated Show resolved Hide resolved

cromefire commented Aug 6, 2020

View reviewed changes

modules/openapi-generator/src/main/resources/kotlin-client/libraries/multiplatform/api.mustache Outdated Show resolved Hide resolved

cromefire commented Aug 7, 2020

View reviewed changes

samples/client/petstore/kotlin-gson/src/main/kotlin/org/openapitools/client/models/Category.kt Outdated Show resolved Hide resolved

cromefire commented Aug 7, 2020

View reviewed changes

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/KotlinClientCodegen.java Show resolved Hide resolved

wing328 added Client: Kotlin Enhancement: Compatibility labels Aug 8, 2020

wing328 added this to the 5.0.0 milestone Aug 8, 2020

cromefire added 2 commits August 8, 2020 17:23

Resolved issues with queries, introduced bundled api client and resol…

49e6502

…ved some minor issues

Updated samples

50cf3a1

cromefire commented Aug 8, 2020

View reviewed changes

cromefire marked this pull request as ready for review August 11, 2020 15:23

Revert changes on request parameterization

041924a

jimschubert reviewed Aug 25, 2020

View reviewed changes

Upgraded to Kotlin 1.4 and fixed minor issues

b2c1548

Updated sample

aababfb

cromefire added 2 commits August 30, 2020 01:59

Merge branch 'master' into ktor-upgrade

ab79e7e

# Conflicts: # samples/client/petstore/kotlin-multiplatform/gradlew.bat

Updated kotlin docs

b8441e2

cromefire closed this Sep 4, 2020

Uh oh!

Upgrade multiplatform libraries #7149

Upgrade multiplatform libraries #7149

Uh oh!

Conversation

cromefire commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cromefire commented Aug 7, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wing328 commented Aug 13, 2020

Uh oh!

andrewemery commented Aug 15, 2020

Uh oh!

cromefire commented Aug 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimschubert commented Aug 25, 2020

Uh oh!

cromefire commented Aug 25, 2020

Uh oh!

jimschubert commented Aug 27, 2020

Uh oh!

cromefire commented Aug 27, 2020

Uh oh!

cromefire commented Aug 29, 2020

Uh oh!

cromefire commented Aug 29, 2020

Uh oh!

cromefire commented Aug 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cromefire commented Sep 1, 2020

Uh oh!

cromefire commented Sep 1, 2020

Uh oh!

jimschubert commented Sep 2, 2020

Uh oh!

cromefire commented Sep 2, 2020

Uh oh!

4brunu commented Sep 2, 2020

Uh oh!

cromefire commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimschubert commented Sep 3, 2020

Uh oh!

jimschubert commented Sep 3, 2020

Uh oh!

cromefire commented Sep 4, 2020

cromefire commented Aug 6, 2020 •

edited

Loading

cromefire commented Aug 30, 2020 •

edited

Loading

cromefire commented Sep 2, 2020 •

edited

Loading