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

[kotlin-multiplatform-client] New, separate and refreshed #7353

Open
wants to merge 75 commits into
base: master
Choose a base branch
from

Conversation

cromefire
Copy link

@cromefire cromefire commented Sep 4, 2020

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.

@cromefire
Copy link
Author

@cromefire cromefire mentioned this pull request Sep 4, 2020
6 tasks
@cromefire
Copy link
Author

cromefire commented Sep 4, 2020

Some things to do (list may be expanded):

  • Fix up codegen
  • Subproject mode (no gradle wrapper and settings)
  • Interop for the individual, non-kotlin languages via Deferred<T>
  • Better ktor integration (remove unneeded request and response helpers and get more "native")
    • Native request infra
    • Add authentication
      • To the individual clients
      • To the general client
  • Lazy for individual clients in container client
  • More fitting initialization
  • Shared client instance
  • More configuration options (maybe for gradle, kotlin and ktor version)
  • Add an option to have open api classes (maybe by default?)
  • Allow configuration of more names:
    • The name of the full client (default: ApiClient)
    • The async suffix (default: Async)
  • Fix additionalProperties
  • Fix anyOf
  • Add tests

@cromefire
Copy link
Author

I added the cli options, so if someone wants to have a look, if they're correctly implemented

@cromefire
Copy link
Author

I did not forget this, but I'll need ~5-6 Weeks until I have time to work on this again

@chris-hatton
Copy link

Hi @cromefire . I came on a journey seeking OpenAPI / Multiplatform generation and, after looking at performing the changes myself, ended up here. This is clearly already great progress; is there any way I can help to complete it?

@cromefire
Copy link
Author

cromefire commented Nov 26, 2020

I don't really think so I guess, there's not much left and I already have concrete ideas for it. I should be able to get it done in the next weeks though.

@fullkomnun
Copy link

Hey @cromefire I have been looking into your great work and tried to use it.
I encountered some issues around Enum classes nested in data classes,
will issue a PR on your fork so you can take a look at it.

What are your ideas for platform specific interop?

@cromefire
Copy link
Author

What are your ideas for platform specific interop?

Provide stuff like Futures for java or promises for JS, so you could also use the same client from a java/JS codebase instead of needing a kotlin shim or something like that. (For example if you publish a client for general use)

@chris-hatton
Copy link

I have a burning use case for this. @fullkomnun did you find the current
fork state to be usable?

@fullkomnun
Copy link

fullkomnun commented Dec 30, 2020

@chris-hatton Sorry for the late response. I also have a use-case that requires this support for the latest
Kotlin MP eco-system (with gradle Kotlin DSL, more recent releases of kotlinx.serialization, kotlinx.coroutines, ktor etc).

I have been testing this PR, mainly focusing on JVM/NodeJS as targets and it works great except for a few issues i have fixed and submitted for review here.
I have not used the new 'kotlinx.datetime' yet though as my target API does not benefit from it...

I performed one more significant change which I will soon submit as a PR which allows clients to provide a custom HttpClient to ApiClient. It is useful for using common Ktor features such as logging, timeouts and so on.
@cromefire would love to get your feedback =]

@cromefire
Copy link
Author

cromefire commented Dec 30, 2020

It should be already possible to use a custom HttpClient though.
Would also be great (if easily doable) could break it up into multiple PRs (I haven't looked at it yet, but it sounded like it's a lot of changes) so we can look at it on a feature by feature basis. (I've missed your PRs)

@Ingwersaft
Copy link

Ingwersaft commented Feb 5, 2021

Another problem I'm facing while using the subproject generation:

configOptions.set(
        mapOf(
            "dateLibrary" to "kotlinx",
            "async" to "false",
            "iosEnabled" to "true",
            "jsEnabled" to "true",
            "jsBrowser" to "true",
            "subproject" to "true",
            "packageName" to "com.liftric.portal.client"
        )
    )

Gradle errors with:

* What went wrong:
Could not create an instance of type org.jetbrains.kotlin.gradle.targets.js.subtargets.KotlinBrowserJs.
> Failed to apply plugin class 'org.gradle.language.base.plugins.LifecycleBasePlugin'.
   > Cannot add task 'build' as a task with that name already exists.

I found some slightly related youtrack issues with the workaround being to declare the plugins in the root project (with apply false), but I'm already doing it. Your generated subproject doesn't contain any version, so should be correct as far as I can tell, but maybe you have an Idea how to fix this?

With jsEnabled and jsBrowser both on false the error disappears.

EDIT: Nevermind, I had some noop build task defined in the root project. Removing this task seems to work... :)

@Ingwersaft
Copy link

Another thing which would be awesome: Enable typescript definitions generation for the kotlin js IR compiler.

The js target would need to be executable

js(BOTH) {
        browser {
            binaries.executable()
        }
}

and all files containing toplevel stuff would need the JsExport treatment (otherwise no definitions will be generated):

@file:OptIn(ExperimentalJsExport::class)
@file:JsExport

import kotlin.js.ExperimentalJsExport
import kotlin.js.JsExport
[...]

@cromefire
Copy link
Author

I found some slightly related youtrack issues with the workaround being to declare the plugins in the root project (with apply false), but I'm already doing it. Your generated subproject doesn't contain any version, so should be correct as far as I can tell, but maybe you have an Idea how to fix this?

Yeah you can only set a version once in the whole project so this why I did it that way, schools probably document that.

@cromefire
Copy link
Author

Another thing which would be awesome: Enable typescript definitions generation for the kotlin js IR compiler.

I think we need some gradle extension script or so but I didn't get it to work yet.

@Nutriz
Copy link

Nutriz commented Feb 5, 2021

Hi, nice job. I have a problem with enums.

I tried to export the petstore sample and in models I have an unresolved reference for CommonEnumSerializer. I got hard time to find information about this class ! Seems to be renamed/removed since a long time ago and you don't need it anymore. Enums are working for you ? I used your last commit 39cafb6db6 2021-01-28 Merge branch 'master' into multiplatform-client

image

@cromefire
Copy link
Author

Are you sure it didn't cache anything or so while building? Because there isn't any reference to that in the code (anymore)

@cromefire
Copy link
Author

Maven seems really bad at caching so I think you do something like clean build

@Nutriz
Copy link

Nutriz commented Feb 5, 2021

Super thanks ! I will try again after a clean !

@Ingwersaft
Copy link

Ingwersaft commented Feb 8, 2021

Another thing which would be awesome: Enable typescript definitions generation for the kotlin js IR compiler.

I think we need some gradle extension script or so but I didn't get it to work yet.

I got it to work, but a lot of generated code issn't exportable to JS, so this would need some major changes to the generated client (with @file:JsExport):

Declaration of such kind (enum class) cant be exported to JS
Declaration of such kind (secondary constructor without @JsName) cant be exported to JS
Declaration of such kind (suspend function) cant be exported to JS

@cromefire
Copy link
Author

cromefire commented Feb 8, 2021

Well without enums and suspend functions you're probably not getting far, I've also tried some things there and settled that for a client to use from (directly) ts, you probably want to generate a "real" ja/ts client.

@darizhap
Copy link

darizhap commented Jun 2, 2021

Hi,
Do you have any plans to make use of Kotlin multiplatform date-time library? It would be great if the generator could recognize definitions like this:

properties:
  someInstantProperty:
    format: date-time
    type: string

and generate properties or parameters of type Instant.

@cromefire
Copy link
Author

cromefire commented Jun 2, 2021

It already does make use of it (but it sadly still has other problems I can't solve).

@darizhap
Copy link

darizhap commented Jun 2, 2021

What kind of problems? Need any help?

@cromefire
Copy link
Author

Actually yes, the one I remember I couldn't figure out happens if you specify additionalProperties (or something similar, I can't look it up right now). The old code sends it to extend HashMap which doesn't work, because it's final, so there needs to be some other mechanism that deals with the unknown properties there and put them into a map or something similar (probably some custom serializer?)

@xxfast
Copy link

xxfast commented Feb 5, 2022

Any update on this?

@cromefire
Copy link
Author

Nope, still those same problems and as I'm quite busy right now I haven't spent a lot of time on trying to find a solution for it.

@silvaignacio
Copy link

Some update on this @cromefire , as the Android Client is quite outdated and it would be interesting to have Multiplatform support with Kotlin.

@cromefire
Copy link
Author

cromefire commented Sep 21, 2023

As always, there wasn't a lot of time to spare and too else much to do, there's not a lot of issues with the PR in it's current state except additionalProperties and it probably needs a Kotlin and Ktor update or two...

Also thought of maybe rewriting it without openapi-generator and Kotlin poet or so for more native code generation and less issues with templating... (as openapi-generator doesn't provide too much I think except the built-in templating) Maybe they'd even accept this generator not using tempating, but a code generator, but I don't think so and it would also add a dependency to the generator in the end (one that's not needed by most) and I don't think there's a plugin mechanism.

If you have a good solution to the issue with additionalProperties we might be able to get this over the finish line. For me it seems like I'd have to do the serialization manually in that case, for the whole object...

Overall I really could use this myself and I'd love to have this but I just didn't have the time and motivation yet to finish it.

Maybe now that multiplatform is more stable it'd be a fairly good time, especially with things like compose multiplatform.

@wing328
Copy link
Member

wing328 commented Sep 22, 2023

If you have a good solution to the issue with additionalProperties we might be able to get this over the finish line

java, C# ,etc supports the additionalProperties. maybe we can these implementations as a starting point.

I think we can cherry-pick some of the enhancements in #7353 (comment) to start with so that we can merge these enhancements separately.

If anyone wants to help on this, please reply to let us know.

most importantly thank you @cromefire for the awesome contributions 👍

@cromefire
Copy link
Author

java, C# ,etc supports the additionalProperties. maybe we can these implementations as a starting point.

AFAIK they usually do that by inheriting from a list, which isn't doable in kotlinx.serialization last time I checked (3 years ago). In general it can of course be done, but it's a bit of work, especially since I have to do it in templating. I might have some time next week, I just have to remember to look at this again.

@kroegerama
Copy link
Contributor

Hi there, I have been following this branch for a long time, because I am also planning to write an OpenAPI generator for Kotlin multiplatform (or rather: rewrite my existing one to KMM, instead of Retrofit+Moshi).

My plan is to just use JsonElement for unknown schemas.

The value additionalProperties can have two different values:

  • true -> Use JsonElement. It is a "free-form" object
  • schema XXX
    • if no own properties: use Map<String, XXX>
    • else: Use JsonElement to be sure, everything will be parsed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.