Skip to content

Split OpenAPI in module needed for user projects and module needed for code-generation #916

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

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

koperagen
Copy link
Collaborator

@koperagen koperagen commented Oct 9, 2024

Rght now openapi module has a lot of dependencies that are only used for parsing. I might be wrong, but i decided to try and split it into two modules. One used by gradle plugin and in kotlin notebooks for generating code. And openapi-runtime that user will need to add to be able to read (convert) some open api json responses.
Correct me if i'm wrong somewhere. I tried this module organization in test project and it compiles and run well. Do we need to move something else from openapi to openapi-runtime?

If we merge this PR, we likely should be able to greatly simplify https://kotlin.github.io/dataframe/gettingstartedgradleadvanced.html#general-configuration and reduce total size of dependencies of "org.jetbrains.kotlinx:dataframe" artifact without sacrificing any functionality

Make sure to run publish for this module, publishMavenLocal won't work somehow (yet)

@Jolanrensen
Copy link
Collaborator

First of all I'd think about naming. The thing the user should depend on if they want to have OpenAPI support should be named "dataframe-openapi". Otherwise we'd deviate from our standard and only cause confusion. If you rename the other to "dataframe-openapi-schema-generation" or something that would be better I think.

I can see how it might save total dependency size at runtime, but I don't really see how it simplifies the configuration process for the user. Nothing essentially changes for them right?

I think you covered everything for the module to be used at runtime. It is possible now to call OpenApi.readCodeForGeneration() and readOpenApiAsString() from the user side, but if these are put in the schema-generation module that would probably be clear enough I suppose :).

Might be worth it to also look into JDBC, since it also has a schema generation part and a runtime part

@koperagen
Copy link
Collaborator Author

First of all I'd think about naming. The thing the user should depend on if they want to have OpenAPI support should be named "dataframe-openapi". Otherwise we'd deviate from our standard and only cause confusion. If you rename the other to "dataframe-openapi-schema-generation" or something that would be better I think.

Agree

I can see how it might save total dependency size at runtime, but I don't really see how it simplifies the configuration process for the user. Nothing essentially changes for them right?

I decided to experiment with it when i saw configuration for Android here: https://kotlin.github.io/dataframe/gettingstartedgradleadvanced.html#general-configuration. Now that i look closer, maybe it won't change after all :c Still, since we suggest to use implementation("org.jetbrains.kotlinx:dataframe:0.14.1") by default, here's comparison:
fat jar for openapi (runtime) is 11.1 mb. fat jar for openapi codegeneration is 27.4 mb

@Jolanrensen
Copy link
Collaborator

Jolanrensen commented Oct 11, 2024

I was thinking some more... When would a user actually use just the runtime module and not the data-schema generator module? In your version it just contains a helper function necessary for executing generated openAPI dataschema code. So the only scenario is: The user already generated some data schema's using the openapi data-schema generator and tries to work with them in a project that has no gradle plugin? That doesn't seem very common to me. We could just as well put this helper function inside the :core module or the generated code.

Additionally, Let's say the user has a json file which they want to generate data schema's from but they keep getting recognized erroneously as OpenAPI schemas (or more realistically, a user wants to get rid of the stupid error messages that mean nothing). Removing the dataframe-openapi dependency does not solve this as dataframe-gradle-plugin depends on the OpenAPI data schema generation anyway...

This does point at our gradle plugin not being configurable in terms of dependencies actually. It would be nice if we could:

plugins {
    // id("org.jetbrains.kotlinx.dataframe")
    id("org.jetbrains.kotlinx.dataframe-core")
}

dependencies {
    // implementation("org.jetbrains.kotlinx.dataframe")

    // let's say we just need jdbc and csv and not openapi etc.

    dataframe("org.jetbrains.kotlinx.dataframe-jdbc")
    dataframe("org.jetbrains.kotlinx.dataframe-csv")

    // maybe these could be controlled by dataframe() too
    implementation("org.jetbrains.kotlinx.dataframe-core")
    implementation("org.jetbrains.kotlinx.dataframe-jdbc")
    implementation("org.jetbrains.kotlinx.dataframe-csv")
}

@koperagen koperagen force-pushed the openapi-runtime branch 2 times, most recently from 56fa394 to 02923d7 Compare October 15, 2024 16:15
@koperagen
Copy link
Collaborator Author

koperagen commented Oct 15, 2024

Ok, i updated PR. Please have a look at gettingStartedAdvanced.
dataframe-openapi -> dataframe-openapi-generator
(new) dataframe-openapi
We're on the same page here?
I tested it in notebooks

image

In gradle project both KSP and gradle codegen methods were tested
Project only depends on
implementation("org.jetbrains.kotlinx:dataframe:0.15.0-dev")
fatjar 49.3 mb -> ±40 mb

@Jolanrensen
@zaleslaw

@koperagen koperagen self-assigned this Oct 15, 2024
@Jolanrensen Jolanrensen mentioned this pull request Oct 16, 2024
…eded only for code-generation to project runtime classpath
@koperagen koperagen changed the title Openapi runtime module Split OpenAPI in module needed for user projects and module needed for code-generation Oct 16, 2024
@koperagen koperagen added the enhancement New feature or request label Oct 16, 2024
@koperagen koperagen added this to the 0.15.0 milestone Oct 16, 2024
Copy link
Contributor

Generated sources will be updated after merging this PR.
Please inspect the changes in here.

@koperagen koperagen merged commit 40c0113 into master Oct 16, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants