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

Update ktor an kotlin dependencies #30

Merged
merged 7 commits into from
Jun 20, 2022

Conversation

vadims-grusas
Copy link
Contributor

Updates are based on #27

ssoper and others added 5 commits November 11, 2021 21:30
Updated ktor to latest version.

Fixed exception raised in kotlin usage sample:
Exception in thread "main" java.lang.IllegalArgumentException: Engine doesn't support WebSocketCapability.
@vadims-grusas
Copy link
Contributor Author

This update makes it possible to use this library together with Spring Boot. Tested with spring 2.5.5

@vadims-grusas
Copy link
Contributor Author

@mmoghaddam385 Hi, requesting a review, please :)

@mmoghaddam385 mmoghaddam385 self-requested a review January 31, 2022 18:22
Copy link
Contributor

@mmoghaddam385 mmoghaddam385 left a comment

Choose a reason for hiding this comment

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

This is fantastic thank you very much for putting the time in to do this! I pulled the branch and made sure the samples still work so that's good, I would like to hear your thoughts on whether or not these version bumps introduce a breaking change or not.
Unfortunately I don't have the time right now to spend deep-dive into the changelogs between the kotlin and ktor versions, but as far as I know they should be using semantic versioning so theoretically there shouldn't be any breaking changes introduced. For that reason I'm fairly confident in releasing these changes under a minor version bump and not a major version bump, but let me know if I'm wrong on that.

Also if you'd be willing we'd absolutely love if you could add some sample code for using this library with spring boot (not in this PR necessarily).

@vadims-grusas
Copy link
Contributor Author

There could be breaking changes.
The example raised one case
DefaultJvmHttpClientProvider(engine = CIO.create()) seems does not work for websockets anymore. Which you have mentioned here #27 (review)
I haven`t looked for a solution for that case as default provider works fine and CIO marked as experimental.

And as far as example will not work anymore, there is a high chance that someone`s code will break.

@vadims-grusas
Copy link
Contributor Author

vadims-grusas commented Feb 1, 2022

Also if you'd be willing we'd absolutely love if you could add some sample code for using this library with spring boot (not in this PR necessarily).

I am not sure if it's needed anymore :) Java example fits fine for spring. I guess people asked for that example as spring app was failing to start with pretty advanced exception, making it harder to understand what`s wrong and how to fix that.

@mmoghaddam385
Copy link
Contributor

mmoghaddam385 commented Feb 2, 2022

Also if you'd be willing we'd absolutely love if you could add some sample code for using this library with spring boot (not in this PR necessarily).

I am not sure if it's needed anymore :) Java example fits fine for spring. I guess people asked for that example as spring app was failing to start with pretty advanced exception, making it harder to understand what`s wrong and how to fix that.

Even better! Btw out of curiosity, do you use gradle or maven (or something else) when using this library with spring boot?

@mmoghaddam385
Copy link
Contributor

There could be breaking changes. The example raised one case DefaultJvmHttpClientProvider(engine = CIO.create()) seems does not work for websockets anymore. Which you have mentioned here #27 (review) I haven`t looked for a solution for that case as default provider works fine and CIO marked as experimental.

And as far as example will not work anymore, there is a high chance that someone`s code will break.

Ah good point. I'll release this as a major version update.

@vadims-grusas
Copy link
Contributor Author

vadims-grusas commented Feb 2, 2022

Even better! Btw out of curiosity, do you use gradle or maven (or something else) when using this library with spring boot?

I am using gradle, there shouldn't be problems with maven as well. The actual problem was that spring has found kotlinx.serialization.json.Json class in the class path and enabled the kotlin codec support, which cannot be disabled "all automated", and then via reflection it has initialized kotlin codec support that was expecting there newer version of Json class. That`s why it failed during the context initialization.

@sethjones348
Copy link

@vadims-grusas is there a workaround for this in the meantime?

@vadims-grusas
Copy link
Contributor Author

@vadims-grusas is there a workaround for this in the meantime?

You may use my fork using jitpack, I go with that approach, while this is not merged.
If you use gradle:
Add maven { url "https://jitpack.io" } to repositories
And implementation "com.github.M8fintech:client-jvm:101e2a630b" as dependency

@beginner137
Copy link

Hey just wanna say thanks, tried your fork, and now my spring boot app works!

@zhemaituk
Copy link
Contributor

@mmoghaddam385 what's remaining to merge/release this PR?
The PR also addresses usage of some @UnstableDefault, which were removed in newer versions of ktor, and would allow bumping up ktor version to 2.0.1 addressing several security vulnerabilities as well ( e.g. https://advisory.checkmarx.net/advisory/vulnerability/CVE-2022-29930/ )

@mmoghaddam385
Copy link
Contributor

@mmoghaddam385 what's remaining to merge/release this PR? The PR also addresses usage of some @UnstableDefault, which were removed in newer versions of ktor, and would allow bumping up ktor version to 2.0.1 addressing several security vulnerabilities as well ( e.g. https://advisory.checkmarx.net/advisory/vulnerability/CVE-2022-29930/ )

Apologies for the inaction here. Before merging this I would like the issue breaking the sample code to be addressed.

The example raised one case DefaultJvmHttpClientProvider(engine = CIO.create()) seems does not work for websockets anymore. Which you have mentioned here #27 (review) I haven`t looked for a solution for that case as default provider works fine and CIO marked as experimental.

I would be fine with the solution being to use a different engine in the example since the CIO engine is experimental. The Kotlin example uses the CIO engine just to illustrate how you might provide your own engine, it doesn't have to be that one in particular.

Unfortunately I still don't have time to work on this first hand, but once the sample are working I'll be happy to merge and release these changes as a major version update.

@vadims-grusas
Copy link
Contributor Author

vadims-grusas commented Jun 15, 2022

@mmoghaddam385 what's remaining to merge/release this PR? The PR also addresses usage of some @UnstableDefault, which were removed in newer versions of ktor, and would allow bumping up ktor version to 2.0.1 addressing several security vulnerabilities as well ( e.g. https://advisory.checkmarx.net/advisory/vulnerability/CVE-2022-29930/ )

Apologies for the inaction here. Before merging this I would like the issue breaking the sample code to be addressed.

The example raised one case DefaultJvmHttpClientProvider(engine = CIO.create()) seems does not work for websockets anymore. Which you have mentioned here #27 (review) I haven`t looked for a solution for that case as default provider works fine and CIO marked as experimental.

I would be fine with the solution being to use a different engine in the example since the CIO engine is experimental. The Kotlin example uses the CIO engine just to illustrate how you might provide your own engine, it doesn't have to be that one in particular.

Unfortunately I still don't have time to work on this first hand, but once the sample are working I'll be happy to merge and release these changes as a major version update.

@mmoghaddam385 Hi, sample is working, do you wan't to add an example of how to provide custom http client?

@mmoghaddam385
Copy link
Contributor

@mmoghaddam385 what's remaining to merge/release this PR? The PR also addresses usage of some @UnstableDefault, which were removed in newer versions of ktor, and would allow bumping up ktor version to 2.0.1 addressing several security vulnerabilities as well ( e.g. https://advisory.checkmarx.net/advisory/vulnerability/CVE-2022-29930/ )

Apologies for the inaction here. Before merging this I would like the issue breaking the sample code to be addressed.

The example raised one case DefaultJvmHttpClientProvider(engine = CIO.create()) seems does not work for websockets anymore. Which you have mentioned here #27 (review) I haven`t looked for a solution for that case as default provider works fine and CIO marked as experimental.

I would be fine with the solution being to use a different engine in the example since the CIO engine is experimental. The Kotlin example uses the CIO engine just to illustrate how you might provide your own engine, it doesn't have to be that one in particular.
Unfortunately I still don't have time to work on this first hand, but once the sample are working I'll be happy to merge and release these changes as a major version update.

@mmoghaddam385 Hi, sample is working, do you wan't to add an example of how to provide custom http client?

Ah you're right, sorry I completely missed that you had already done this. Don't worry about showing an example of a custom http client. I'll merge and release this under v3.0.0

Thanks for you patience and for doing all of this in the first place!

@mmoghaddam385 mmoghaddam385 merged commit 3224374 into polygon-io:master Jun 20, 2022
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.

None yet

6 participants