Support kotlin coroutines #1706

wplong11 · 2022-07-31T16:47:49Z

Resolves: #1565

!! This is working PoC

Inspired by PlaytikaOSS/feign-reactive#486

TODO

Separate Kotlin support module
Enhance test case
Refactoring
Clean up pom.xml

velo · 2022-07-31T19:37:00Z

This looks really nice. It makes total sense to make this available on the AsyncFeign only, really good plan!

snyk is asking to "Upgrade org.jetbrains.kotlin:kotlin-stdlib to version 1.6.0 or higher"

Sounds like a solid plan, once you are ready to move to a new module, fee free to ping me for any assistance needed

wplong11 · 2022-08-01T14:27:18Z

@velo I pushed some changes

Refactor some code
- Use FutureKt.await instead of KotlinExtensions.awaitResponse
- Renamed KotlinExtensions to MethodKt
Update the Kotlin version
- If the Feign library is using the latest version, and the Kotlin version of the project using the Feign library is lower, is it safe? I'm going to do some research on this, but I'm asking to check if you have any knowledge to share with me.

wplong11 · 2022-08-01T14:27:40Z

@velo I'm looking into other libraries to isolate the Kotlin support module so that the core module doesn't reference the Kotlin library, but Spring is using Kotlin in the core module. How important do you think the necessity is with regard to module separation?

I came up with a good idea. Looking at the Spring code a little more, it seems that I am handling it smarter than I thought. I'll look into it a bit more and update.

kdavisk6 · 2022-08-01T21:26:08Z

I’m so happy someone picked this up. @velo could I have a day or two to review this as well?

velo · 2022-08-01T21:39:48Z

I’m so happy someone picked this up. @velo could I have a day or two to review this as well?

You absolutely can sir!

But, until this is moved to a module, I wouldn't be comfortable to merge, as core needs to be lean and have no dependencies.

kdavisk6 · 2022-08-01T21:40:32Z

I’m so happy someone picked this up. @velo could I have a day or two to review this as well?

You absolutely can sir!

But, until this is moved to a module, I wouldn't be comfortable to merge, as core needs to be lean and have no dependencies.

Exactly why I wanted to review. Like minds

wplong11 · 2022-08-02T00:37:20Z

@velo

But, until this is moved to a module, I wouldn't be comfortable to merge, as core needs to be lean and have no dependencies.

I'm considering splitting modules using Optional Dependencies. What do you think about this approach? After a brief investigation, it seems that Spring Core also supports Kotlin using Optional Dependencies. I'm going to try implementing it today using optional dependencies, and investigate further if there are other approaches. (I live in the KST time zone (UTC+9), and I can write code after work.)

Optional Dependencies

Optional dependencies are used when it's not possible (for whatever reason) to split a project into sub-modules. The idea is that some of the dependencies are only used for certain features in the project and will not be needed if that feature isn't used. Ideally, such a feature would be split into a sub-module that depends on the core functionality project. This new subproject would have only non-optional dependencies, since you'd need them all if you decided to use the subproject's functionality.

However, since the project cannot be split up (again, for whatever reason), these dependencies are declared optional. If a user wants to use functionality related to an optional dependency, they have to redeclare that optional dependency in their own project. This is not the clearest way to handle this situation, but both optional dependencies and dependency exclusions are stop-gap solutions.

velo · 2022-08-02T02:37:55Z

I don't think optional is going to cut. But tell you what, get your changes to a place where you are happy with, and I can jump in and help refactoring the core to make a module possible

wplong11 · 2022-08-02T16:45:07Z

@velo I pushed some changes to share idea.

Because the optional dependency is used, Kotlin support related libraries will not be referenced in a Java-only project. 407ab47
Assert.java / ClasUtils.java / KotlinDetector.java is from spring core project -> I will refactor when the implementation approach is agreed upon.
As an additional idea, I thought of separating the code related to Kotlin support into a separate module and referencing that module as an Optional Dependency from the core module. 883f036

velo · 2022-08-02T20:56:26Z

core/src/main/java/feign/kotlinSupport/ClassUtils.java

+ * @author Sam Brannen
+ * @since 1.1
+ */
+public abstract class ClassUtils {


This class (and others on the PR) look like copy and paste from somewhere.... please use the original library

#1706 (comment)

Assert.java / ClasUtils.java / KotlinDetector.java is from spring core project -> I will refactor when the implementation approach is agreed upon.

If the approach is agreed upon, Assert.java / ClasUtils.java used in KotlinDetector and KotlinDetector implementation will be refactored. These classes are from Spring Core. If I use the original library, I must use the Spring Core library.

How about reviewing where the feign code branches off using KotlinDetector for Kotlin support? I wrote the code only to demonstrate the approach.

Assert.java -> Removed
ClasUtils.java / KotlinDetector.java -> Refactored

velo · 2022-08-03T00:54:40Z

Hi @wplong11 , so, as is, we can't merge, as we (Myself and @kdavisk6 ) won't approve any changes that introduce dependencies to feign core.... optional=>true is still a dependency, and that is not going to fly. Core needs ZERO dependencies.

My plan is to refactor feign-core to allow for the new bits you created to be attached to core like we do with everything else.
Like, feign supports "jackson" but there is no jackson dependency on core. Could you imagine the caos it would bring to everybody if overnight feign-core included jackson, json, http commons, apache http5 and more and more

So, once you get your changes to a point that you are happy with, ping me and I will help you out.

wplong11 · 2022-08-03T01:38:54Z

@velo Looking at the final result, feign-core depends on feign-kotlin as an optional dependency. I think it's a bit better than relying on kotlin-related dependencies directly, but as you said, I'd also like to have zero dependencies if possible. But I have no idea how to implement it. Do you have any ideas?

Considerations

People using feign only depend on feign-kotlin and do not require additional code.

velo · 2022-08-03T01:59:58Z

Do you have any ideas?

Not yet. But, I will get something done

People using feign only depend on feign-kotlin and do not require additional code.

Not really, it will bring in kotlin and whatever else it depends on. As is, you can't use feign if kotlin library is not included.

wplong11 · 2022-08-03T03:01:30Z

@velo Looking at the Github Example project, it seems that feign-core is used with zero dependency. Am I misunderstanding something?

velo · 2022-08-03T03:10:02Z

either that dependency tree wrong, or a runtime exception will happen when performing an async operation

wplong11 · 2022-08-03T15:54:41Z

@velo

either that dependency tree wrong, or a runtime exception will happen when performing an async operation

I checked that Github Example in Feign project runs correctly without kotlin related modules.

According to Maven document,

Project-A -> Project-B
The diagram above says that Project-A depends on Project-B. When A declares B as an optional dependency in its POM, this relationship remains unchanged. It's just like a normal build where Project-B will be added in Project-A's classpath.

Project-X -> Project-A
When another project (Project-X) declares Project-A as a dependency in its POM, the optional nature of the dependency takes effect. Project-B is not included in the classpath of Project-X. You need to declare it directly in the POM of Project X for B to be included in X's classpath.

Kotlin related modules is not included in the classpath of Github Example. Is there a difference between not being included on the classpath and excluded from the dependency tree?

+) Even though Spring Core uses kotlin related modules as optional dependency, It seems that all java only projects also run correctly without any kotlin related dependency

velo · 2022-08-08T22:59:32Z

@wplong11 I moved kotlin to a module, but couldn't get the tests working.... could you take a look, I must admit I don't know where to start

velo · 2022-08-08T23:01:33Z

kotlin/src/test/kotlin/feign/kotlin/CoroutineFeignTest.kt

+    }
+
+    @Test
+    fun shouldRun4(): Unit = runBlocking {


could we get meaningful names for tests?

Corrected. Please provide feedback on the naming convention.

wplong11 · 2022-08-15T13:53:13Z

Thanks for the additional work. I've been too busy lately. I'll be able to start working again in two weeks. I'll finish this PR soon.

velo · 2022-08-16T00:03:09Z

Awesome, and thanks for this massive contribution.

I have o my mind now, that I need to find a way to have simpler Feign#builder classes..

- Remove KotlinDetector.java - Add Method.isSuspend extension function

- Copy of GithubExample

https://github.com/OpenFeign/feign/pull/1706/files#r965389041

wplong11 · 2022-09-08T04:18:07Z

This Branch updated to latest master branch
```
$ git rebase master && git push --force
```
Add a single commit 78589d5

Revert the public access modifier changed in PR OpenFeign#1706

Revert the public access modifier changed in PR #1706 Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>

wplong11 force-pushed the 1565 branch from 865cfa1 to 80f960e Compare July 31, 2022 16:51

wplong11 force-pushed the 1565 branch 3 times, most recently from 6c88458 to fc89552 Compare August 1, 2022 14:12

wplong11 force-pushed the 1565 branch 2 times, most recently from 3e7499d to 407ab47 Compare August 2, 2022 16:33

wplong11 force-pushed the 1565 branch from b0ae76e to 883f036 Compare August 2, 2022 17:17

velo reviewed Aug 2, 2022

View reviewed changes

wplong11 force-pushed the 1565 branch 2 times, most recently from b228cff to 9bc6d2b Compare August 3, 2022 00:48

velo reviewed Aug 8, 2022

View reviewed changes

wplong11 force-pushed the 1565 branch from b0258e7 to ff496a0 Compare September 3, 2022 08:25

wplong11 and others added 17 commits September 8, 2022 13:13

Apply optional dependency to kotlin support related dependency

4d31e1d

Seperate Kotlin support module

593e0ad

Remove unused code from ClassUtils.java

c039f5c

Remove unused code from ClassUtils.java

93265fe

Refactor KotlinDetector

3cd2ae1

Move ClassUtils location into KotlinDetector

115c849

Move KotlinDetector location

202b0f8

Format code

887d8f2

First attempt to move kotlin work to it's own isolated module

69f017e

Coroutine Feign using AyncFeign

c48f1f5

Coroutine Feign using AyncFeign

e24c9af

Refactor suspending function detect logic

9e90725

- Remove KotlinDetector.java - Add Method.isSuspend extension function

Cleanup CoroutineFeignTest test code format

50162c7

Fix suspend function contract parsing error when using http body

6b65d79

Rename test names to be meaningful

a5fd11f

Add Github Example With Coroutine

b57140d

- Copy of GithubExample

Remove unnecessary dependency

78589d5

https://github.com/OpenFeign/feign/pull/1706/files#r965389041

wplong11 force-pushed the 1565 branch from 536ca68 to 78589d5 Compare September 8, 2022 04:16

wplong11 mentioned this pull request Sep 12, 2022

Non-blocking I/O support #361

Open

Merge branch 'master' into 1565

1e91c7a

velo approved these changes Sep 13, 2022

View reviewed changes

velo merged commit 39ed8ef into OpenFeign:master Sep 14, 2022

wplong11 deleted the 1565 branch September 14, 2022 08:16

aSemy mentioned this pull request Sep 29, 2022

OpenFeign doesn't work with kotlin suspend method spring-cloud/spring-cloud-openfeign#661

Open

wplong11 added a commit to wplong11/feign that referenced this pull request Oct 9, 2022

Change some access modifier

04dca10

Revert the public access modifier changed in PR OpenFeign#1706

wplong11 mentioned this pull request Oct 9, 2022

Change some access modifier #1784

Merged

wplong11 added a commit to wplong11/feign that referenced this pull request Oct 11, 2022

Change some access modifier

a394fc4

Revert the public access modifier changed in PR OpenFeign#1706

velo added a commit that referenced this pull request Oct 11, 2022

Change some access modifier (#1784)

f8c57da

Revert the public access modifier changed in PR #1706 Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>

wplong11 mentioned this pull request Dec 2, 2022

Support kotlin coroutines PlaytikaOSS/feign-reactive#486

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support kotlin coroutines #1706

Support kotlin coroutines #1706

wplong11 commented Jul 31, 2022 •

edited

velo commented Jul 31, 2022

wplong11 commented Aug 1, 2022 •

edited

wplong11 commented Aug 1, 2022 •

edited

kdavisk6 commented Aug 1, 2022 •

edited

velo commented Aug 1, 2022

kdavisk6 commented Aug 1, 2022

wplong11 commented Aug 2, 2022

velo commented Aug 2, 2022

wplong11 commented Aug 2, 2022 •

edited

velo Aug 2, 2022

wplong11 Aug 3, 2022

wplong11 Aug 3, 2022

velo commented Aug 3, 2022

wplong11 commented Aug 3, 2022 •

edited

velo commented Aug 3, 2022

wplong11 commented Aug 3, 2022

velo commented Aug 3, 2022

wplong11 commented Aug 3, 2022 •

edited

velo commented Aug 8, 2022

velo Aug 8, 2022

wplong11 Sep 3, 2022 •

edited

wplong11 commented Aug 15, 2022

velo commented Aug 16, 2022

wplong11 commented Sep 8, 2022

Support kotlin coroutines #1706

Support kotlin coroutines #1706

Conversation

wplong11 commented Jul 31, 2022 • edited

TODO

velo commented Jul 31, 2022

wplong11 commented Aug 1, 2022 • edited

wplong11 commented Aug 1, 2022 • edited

kdavisk6 commented Aug 1, 2022 • edited

velo commented Aug 1, 2022

kdavisk6 commented Aug 1, 2022

wplong11 commented Aug 2, 2022

velo commented Aug 2, 2022

wplong11 commented Aug 2, 2022 • edited

velo Aug 2, 2022

Choose a reason for hiding this comment

wplong11 Aug 3, 2022

Choose a reason for hiding this comment

wplong11 Aug 3, 2022

Choose a reason for hiding this comment

velo commented Aug 3, 2022

wplong11 commented Aug 3, 2022 • edited

velo commented Aug 3, 2022

wplong11 commented Aug 3, 2022

velo commented Aug 3, 2022

wplong11 commented Aug 3, 2022 • edited

velo commented Aug 8, 2022

velo Aug 8, 2022

Choose a reason for hiding this comment

wplong11 Sep 3, 2022 • edited

Choose a reason for hiding this comment

wplong11 commented Aug 15, 2022

velo commented Aug 16, 2022

wplong11 commented Sep 8, 2022

wplong11 commented Jul 31, 2022 •

edited

wplong11 commented Aug 1, 2022 •

edited

wplong11 commented Aug 1, 2022 •

edited

kdavisk6 commented Aug 1, 2022 •

edited

wplong11 commented Aug 2, 2022 •

edited

wplong11 commented Aug 3, 2022 •

edited

wplong11 commented Aug 3, 2022 •

edited

wplong11 Sep 3, 2022 •

edited