Skip to content

UPDATE ktor-client to 3.0.0 and fix HttpBasicAuth.kt to support it #19886

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

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

Conversation

ninovanhooff
Copy link

@ninovanhooff ninovanhooff commented Oct 16, 2024

For ktor 3.0.0 compatibility, an import must be updated in HttpBasicAuth.kt

I basically did a project wide find-and-replace for the offending import

Fixes issue #19447

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/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    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*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

fix-issue-19447-invalid-ktor-internalapi-import
fix-issue-19447-invalid-ktor-internalapi-import
@ninovanhooff ninovanhooff force-pushed the fix-issue-19447-invalid-ktor-internalapi-import branch from 565fc91 to f252ad0 Compare October 16, 2024 10:09
@ninovanhooff
Copy link
Author

ninovanhooff commented Oct 16, 2024

Force push reason: changing commit message

@e5l please advise:
with this change ktor client is updated to 3.0.0. And it also fixes the samples. Client Users of openapi-generators might need to update ktor to 3.0.0

  • The migration guide only mentions steps for server, not client. https://ktor.io/docs/migrating-3.html Is this considered a breaking change and should I target branch 8.x.0? Note that I did not update the server version in modules/openapi-generator/src/main/resources/kotlin-server/libraries/ktor/build.gradle.kts.mustache
  • When running ./bin/generate-samples.sh ./bin/configs/*.yaml my ktor version change is being reverted to 2.3.6. But I don't see relevant occurrences of 2.3.6 anymore in the project. Can you explain why?
    edit: found it in the .mustanche file. fixed.

@ninovanhooff ninovanhooff changed the title FIX import for Ktor's InternalApi import UPDATE ktor to 3.0.0 Oct 16, 2024
fix-issue-19447-invalid-ktor-internalapi-import
@ninovanhooff ninovanhooff marked this pull request as ready for review October 16, 2024 10:47
@ninovanhooff ninovanhooff changed the title UPDATE ktor to 3.0.0 UPDATE ktor to 3.0.0 and fix HttpBasicAuth.kt to support it Oct 16, 2024
@wing328
Copy link
Member

wing328 commented Oct 16, 2024

thanks for the PR

cc
@dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11) @yutaka0m (2020/03) @stefankoppier (2022/06) @e5l (2024/10)

@wing328
Copy link
Member

wing328 commented Oct 16, 2024

The migration guide only mentions steps for server, not client. https://ktor.io/docs/migrating-3.html Is this considered a breaking change and should I target branch 8.x.0?

My take is to go ahead with this change and if users need to use ktor 2.x for whatever reasons, we may create an option for fallback if needed (or they can use customize templates with a different set of versions for dependencies).

Copy link
Contributor

@e5l e5l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ninovanhooff, thank you for the PR!
LGTM

It would be nice if you adjusted ktor to ktor-client in the PR title for the change log :)

@e5l
Copy link
Contributor

e5l commented Oct 16, 2024

@wing328,
the client has no broken changes in this release (except the shared ktor-io module)

@wing328
Copy link
Member

wing328 commented Oct 16, 2024

@ninovanhooff can you please review the CI failures when you've time?

@wing328 wing328 added this to the 7.10.0 milestone Oct 16, 2024
@ninovanhooff ninovanhooff changed the title UPDATE ktor to 3.0.0 and fix HttpBasicAuth.kt to support it UPDATE ktor-client to 3.0.0 and fix HttpBasicAuth.kt to support it Oct 16, 2024
fix-issue-19447-invalid-ktor-internalapi-import
fix-issue-19447-invalid-ktor-internalapi-import
@ninovanhooff
Copy link
Author

ninovanhooff commented Oct 16, 2024

I can't seem to find the last ones. Whatever other version number I find that is left, I assume is related to ktor-server rather than client. Help appreciated, @wing328 @e5l

@e5l
Copy link
Contributor

e5l commented Oct 16, 2024

Your current Kotlin version is 1.9.20, while kotlinx.serialization core runtime 1.7.3 requires at least Kotlin 2.0.0-RC1. Please update your Kotlin compiler and IDE plugin.

To use 1.7.3 kotlinx.serialization you need to bump Kotlin to 2.0.0

@eirikvaa
Copy link

I'd love to help get Ktor v3 into the Kotlin clients. Regarding Kotlin 2.0.0, do you prefer to do this in a separate PR?

@ninovanhooff
Copy link
Author

@eirikvaa thanks for reaching out and offering support.

Tbh, my team decided to not use this project due to lack of oneof (aka sealed class in kotlin) support. So my motivation to finish this up has dropped significantly.
In addition, kotlin 2 migration is a bit more work (not necessarily a lot) than just replacing some version numbers.

If you would like to, please branch / fork from my work and open a new PR, then I will close this.

@eirikvaa
Copy link

@eirikvaa thanks for reaching out and offering support.

Tbh, my team decided to not use this project due to lack of oneof (aka sealed class in kotlin) support. So my motivation to finish this up has dropped significantly. In addition, kotlin 2 migration is a bit more work (not necessarily a lot) than just replacing some version numbers.

If you would like to, please branch / fork from my work and open a new PR, then I will close this.

You can keep it open for now 👍

@wing328
Copy link
Member

wing328 commented Oct 22, 2024

Tbh, my team decided to not use this project due to lack of oneof (aka sealed class in kotlin) support. So my motivation to finish this up has dropped significantly.

Did you try the option generateOneOfAnyOfWrappers ?

https://openapi-generator.tech/docs/generators/kotlin/

@ninovanhooff
Copy link
Author

Tbh, my team decided to not use this project due to lack of oneof (aka sealed class in kotlin) support. So my motivation to finish this up has dropped significantly.

Did you try the option generateOneOfAnyOfWrappers ?

https://openapi-generator.tech/docs/generators/kotlin/

Yes, that did not produce compilable code. I think it was designed to produce Gson models and even then it did not compile or clash with the kotlin-serialization models generated by the remainder of the kotlin + jvm-ktor and kotlin + Multiplatform config. Very disappointing

@wing328
Copy link
Member

wing328 commented Oct 22, 2024

  library: jvm-retrofit2
  serializationLibrary: gson

that's what we've tested so far

ref: https://github.com/OpenAPITools/openapi-generator/blob/master/bin/configs/kotlin-model-prefix-type-mapping.yaml

you can give it a try to see if the output meets your requirements

and we will see how we can plot the implementation to other serialization libraries

@wing328 wing328 removed this from the 7.10.0 milestone Nov 18, 2024
@wing328 wing328 added this to the 7.11.0 milestone Nov 18, 2024
@wing328 wing328 modified the milestones: 7.11.0, 7.12.0 Jan 20, 2025
@wing328 wing328 modified the milestones: 7.12.0, 7.13.0 Feb 28, 2025
@wing328 wing328 modified the milestones: 7.13.0, 7.14.0 Apr 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants