[Kotlin Multiplatform] Update and fixes for Kotlin 1.5.10 and Kotlinx.serialization 1.2.1 #9755

837 · 2021-06-13T15:24:43Z

Update Kotlin Multiplatform with Kotlin 1.5.10, Ktor 1.6.0 and Kotlinx.serialization 1.2.1
remove @serializable from interfaces
Use CharArray.concatToString() instead of 'String(CharArray): String' as it is an error now
Fix, RequestConfig needs a type to infer type variable T

Fixes from @DevSrSouzaern #9232
Seemed not to be updated anymore.

Fix private keyword not being handle as a reservedWords
Fix enum serialization generation
Migrate gradle build to Kotlin DSL

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, 5.1.x, 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
@jimschubert , @dr4ke616, @karismann, @Zomzog, @andrewemery, @4brunu, @yutaka0m

@serializable

…x.serialization 1.2.1 * remove @serializable from interfaces * Using 'String(CharArray): String' is an error. Use CharArray.concatToString() instead * Fix, RequestConfig needs a type to infer type variable T Fixes from @DevSrSouzaern * Fix `private` keyword not being handle as a reservedWords * Fix enum serialization generation * Migrate gradle build to Kotlin DSL

837 · 2021-06-13T20:27:40Z

Someone an idea why CircleCi fails?

4brunu

Hi @837 since I don't have much experience with kotlin multiplatform I left some comments to try to understand the changes.

4brunu · 2021-06-15T11:47:44Z

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

-        httpClientEngine: HttpClientEngine? = null,
-        jsonConfiguration: JsonConfiguration = JsonConfiguration.Default
-    ) : this(baseUrl, httpClientEngine, KotlinxSerializer(Json(jsonConfiguration)))
+    jsonSerializer: Json


@837 could you please explain why the change from KotlinxSerializer to Json?

The creation of the KotlinxSerializer is now lazily done in the ApiClient::class.
The Json is used to pass down settings as they might vary depending on the api:

Json { isLenient = true ignoreUnknownKeys = true }

Now when instantiating some Api class, it's needed to pass jsonSerializer. It wasn't needed before this PR (jsonConfiguration: JsonConfiguration = JsonConfiguration.Default in the constructor that was removed), so I think that we should remain backward-compatible here and some kind of a sane default Json configuration would be good. Best if this default configuration is accessible for the client, so that they can customize it by building upon the default one.

I agree that e.g. ignoreUnknownKeys = true should be in this default. Otherwise if the service being called starts including some new properties, JSON deserialization will fail. No experience/opinion on isLenient.

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

...rc/main/resources/kotlin-client/libraries/multiplatform/infrastructure/ApiClient.kt.mustache

4brunu · 2021-06-22T17:35:04Z

I'm not a kotlin multiplaform users, but I don't see nothing wrong here.
@wing328 what do you think of this PR?

krzema12

Thanks for opening this PR! I started one myself (#9827) but it's so similar to yours that I'm happy to help you polish this one :)

As a smoke test, I cloned your repo and branch, and generated a client for my company's internal service. For an example call, it works good!

Apart from the comments I placed inline, let me repeat concerns that I have for my PR:

are there any end-to-end tests that actually try to call the Petstore service with the generated module? Ideally we'd like to test various edge cases in an automated and repeatable way
how to ensure there are no regressions in other Kotlin templates that reuse the shared pieces of template?
is master the right target branch for this change, given it's formally backward-incompatible? The policy is confusing to me

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

docs/generators/kotlin.md

modules/openapi-generator/src/main/resources/kotlin-client/data_class.mustache

krzema12 · 2021-06-23T09:01:10Z

...rc/main/resources/kotlin-client/libraries/multiplatform/infrastructure/ApiClient.kt.mustache

    }

-    private fun RequestConfig.updateForAuth(authNames: kotlin.collections.List<String>) {
+    private fun <T: Any?> RequestConfig<T>.updateForAuth(authNames: kotlin.collections.List<String>) {
        for (authName in authNames) {
            val auth = authentications?.get(authName) ?: throw Exception("Authentication undefined: $authName")
            auth.apply(query, headers)


A comment to line which mentions ktorio/ktor#851 and the below workaround (I cannot place a comment there 😞): how about removing this workaround? I see that this issue was resolved 2 years ago, so the newest version of Ktor almost for sure doesn't have this issue. On the other hand, to keep the number of changes minimal, I'm fine with doing it as a follow-up.

modules/openapi-generator/src/main/resources/kotlin-client/README.mustache

krzema12 · 2021-06-23T09:12:19Z

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

-        httpClientEngine: HttpClientEngine? = null,
-        jsonConfiguration: JsonConfiguration = JsonConfiguration.Default
-    ) : this(baseUrl, httpClientEngine, KotlinxSerializer(Json(jsonConfiguration)))
+    jsonSerializer: Json


Now when instantiating some Api class, it's needed to pass jsonSerializer. It wasn't needed before this PR (jsonConfiguration: JsonConfiguration = JsonConfiguration.Default in the constructor that was removed), so I think that we should remain backward-compatible here and some kind of a sane default Json configuration would be good. Best if this default configuration is accessible for the client, so that they can customize it by building upon the default one.

I agree that e.g. ignoreUnknownKeys = true should be in this default. Otherwise if the service being called starts including some new properties, JSON deserialization will fail. No experience/opinion on isLenient.

* sample update * revert non multiplatform kotlinx_serialization removal * default Json{} in ApiClient * revert accidental version push for jvm client * Match ktor and Kotlinx Serialization version in documentation and build.gradle.kts

...rc/main/resources/kotlin-client/libraries/multiplatform/infrastructure/ApiClient.kt.mustache

...tlin-multiplatform/src/commonMain/kotlin/org/openapitools/client/infrastructure/ApiClient.kt

krzema12

LGTM - I think we can merge as is because it works fine, and then apply any improvements like end-to-end tests or code cleanup as follow-ups.

4brunu · 2021-06-29T16:38:55Z

@837 could you please close and reopen this PR to restart CI? Thanks

4brunu · 2021-06-29T17:09:37Z

Sorry if I miscommunicated, but to restart the CI you could just close and reopen the same PR, there is no need to create a new one.
But this works too 🙂

837 · 2021-06-29T19:19:01Z

Oh didnt know that. Makes more sense to keep this one.

4brunu · 2021-06-30T13:05:53Z

Looks good to me 👍

wing328 · 2021-07-01T09:54:50Z

@837 can you please resolve the merge conflicts when you've time?

wing328 · 2021-07-03T15:33:36Z

Kotlin tests pass via #9883

This dependency was accidentally removed in PR OpenAPITools#9755 where we switched from `build.gradle` to `build.gradle.kts`.

This dependency was accidentally removed in PR #9755 where we switched from `build.gradle` to `build.gradle.kts`.

shadowsheep1 · 2021-09-23T12:36:10Z

I had to use a custom mustache template for HttpBasicAuth.kt.mustache to change it as:

package {{packageName}}.auth

import io.ktor.util.InternalAPI
import io.ktor.util.encodeBase64

/**
 * Custom Template
 */

class HttpBasicAuth : Authentication {
    var username: String? = null
    var password: String? = null

    @OptIn(InternalAPI::class)
    override fun apply(query: MutableMap<String, List<String>>, headers: MutableMap<String, String>) {
        if (username == null && password == null) return
        val str = (username ?: "") + ":" + (password ?: "")
        val auth = str.encodeBase64()
        headers["Authorization"] = "Basic $auth"
    }
}

Because with AS Artic Fox 2020.3.1 Patch 2

With this configuration

object Versions {
    const val iosTestTask = false
    const val foo = "8.6.8"
    const val kotlin = "1.5.31"

    // Gradle
    const val androidGradlePlugin = "7.0.2"
    const val gradle = "gradle-7.2-bin"

    // Android
    const val compileSdk = 30
    const val minSdk = 23
    const val targetSdk = compileSdk

    // You can also use this to specify versions for dependencies. Having consistent
    // versions between modules can avoid behavior conflicts.
    const val sqldelight = "1.5.1"
    const val settings = "0.8"
    const val serialization = "1.2.2"
    // https://kotlinlang.org/docs/mobile/concurrency-and-coroutines.html#multithreaded-coroutines
    const val coroutines = "1.5.1-native-mt"
    const val ktor = "1.6.3"
    const val openapigen = "5.2.1"
}

I had (using @InternalAPI annotation) this error.

e: Opt-in requirement marker annotation on override requires the same marker on base declaration

I'd to use @OptIn(InternalAPI::class) instead

4brunu · 2021-09-23T16:59:02Z

@shadowsheep1 could you please add a PR with those changes?

shadowsheep1 · 2021-09-24T06:44:52Z

@4brunu sure 👍

DevSrSouza and others added 2 commits June 13, 2021 17:16

update samples and documentation

67a9698

This was referenced Jun 14, 2021

[KOTLIN MULTIPLATFORM] Compilation errors related to Kotlinx.serialization #8862

Open

[REQ] kotlin multiplatform - support newer kotlin version #5346

Open

4brunu reviewed Jun 15, 2021

View reviewed changes

4brunu mentioned this pull request Jun 22, 2021

[#9805][Kotlin][multiplatform] Make the template work with Kotlin 1.5.x #9827

Closed

5 tasks

krzema12 suggested changes Jun 23, 2021

View reviewed changes

837 mentioned this pull request Jun 23, 2021

[BUG][kotlin-client][multiplatform] "NoSuchMethodError" in runtime when starting ktor server with Kotlin 1.5.10 #9805

Closed

6 tasks

PR Review changes

3243ead

* sample update * revert non multiplatform kotlinx_serialization removal * default Json{} in ApiClient * revert accidental version push for jvm client * Match ktor and Kotlinx Serialization version in documentation and build.gradle.kts

krzema12 suggested changes Jun 28, 2021

View reviewed changes

...rc/main/resources/kotlin-client/libraries/multiplatform/infrastructure/ApiClient.kt.mustache Outdated Show resolved Hide resolved

...tlin-multiplatform/src/commonMain/kotlin/org/openapitools/client/infrastructure/ApiClient.kt Outdated Show resolved Hide resolved

remove const from JSON_DEFAULT

d71aa07

krzema12 approved these changes Jun 29, 2021

View reviewed changes

837 closed this Jun 29, 2021

837 reopened this Jun 29, 2021

wing328 added Client: Kotlin Enhancement: Compatibility Issue: Bug labels Jul 1, 2021

wing328 added this to the 5.2.0 milestone Jul 1, 2021

837 added 2 commits July 1, 2021 14:51

Merge branch 'master' into refactor/update-kotlin-multiplatform

24b7d61

Update AbstractKotlinCodegen.java

2040de5

wing328 merged commit 17b6379 into OpenAPITools:master Jul 3, 2021

krzema12 added a commit to krzema12/openapi-generator that referenced this pull request Jul 28, 2021

[Kotlin][Multiplatform] Add missing JVM dependency

5e0c3b6

This dependency was accidentally removed in PR OpenAPITools#9755 where we switched from `build.gradle` to `build.gradle.kts`.

krzema12 mentioned this pull request Jul 28, 2021

[Kotlin][Multiplatform] Add missing JVM dependency #10051

Merged

5 tasks

wing328 pushed a commit that referenced this pull request Jul 29, 2021

[Kotlin][Multiplatform] Add missing JVM dependency (#10051)

b1a0098

This dependency was accidentally removed in PR #9755 where we switched from `build.gradle` to `build.gradle.kts`.

wing328 mentioned this pull request Apr 13, 2022

[Kotlin Multiplatform] Update and fixes #9232

Closed

5 tasks

kassim mentioned this pull request Jul 25, 2022

[BUG] [kotlin-client] kotlinx_serialization deserialization fails on unexpected/unknown keys #13013

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kotlin Multiplatform] Update and fixes for Kotlin 1.5.10 and Kotlinx.serialization 1.2.1 #9755

[Kotlin Multiplatform] Update and fixes for Kotlin 1.5.10 and Kotlinx.serialization 1.2.1 #9755

837 commented Jun 13, 2021

837 commented Jun 13, 2021

4brunu left a comment

4brunu Jun 15, 2021

837 Jun 17, 2021

krzema12 Jun 23, 2021

4brunu commented Jun 22, 2021

krzema12 left a comment

krzema12 Jun 23, 2021

krzema12 Jun 23, 2021

krzema12 left a comment

4brunu commented Jun 29, 2021

4brunu commented Jun 29, 2021

837 commented Jun 29, 2021

4brunu commented Jun 30, 2021

wing328 commented Jul 1, 2021

wing328 commented Jul 3, 2021

shadowsheep1 commented Sep 23, 2021 •

edited

Loading

4brunu commented Sep 23, 2021

shadowsheep1 commented Sep 24, 2021

[Kotlin Multiplatform] Update and fixes for Kotlin 1.5.10 and Kotlinx.serialization 1.2.1 #9755

[Kotlin Multiplatform] Update and fixes for Kotlin 1.5.10 and Kotlinx.serialization 1.2.1 #9755

Conversation

837 commented Jun 13, 2021

PR checklist

837 commented Jun 13, 2021

4brunu left a comment

Choose a reason for hiding this comment

4brunu Jun 15, 2021

Choose a reason for hiding this comment

837 Jun 17, 2021

Choose a reason for hiding this comment

krzema12 Jun 23, 2021

Choose a reason for hiding this comment

4brunu commented Jun 22, 2021

krzema12 left a comment

Choose a reason for hiding this comment

krzema12 Jun 23, 2021

Choose a reason for hiding this comment

krzema12 Jun 23, 2021

Choose a reason for hiding this comment

krzema12 left a comment

Choose a reason for hiding this comment

4brunu commented Jun 29, 2021

4brunu commented Jun 29, 2021

837 commented Jun 29, 2021

4brunu commented Jun 30, 2021

wing328 commented Jul 1, 2021

wing328 commented Jul 3, 2021

shadowsheep1 commented Sep 23, 2021 • edited Loading

4brunu commented Sep 23, 2021

shadowsheep1 commented Sep 24, 2021

shadowsheep1 commented Sep 23, 2021 •

edited

Loading