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

OpenFeign doesn't work with kotlin suspend method #1565

Closed
XhstormR opened this issue Jan 8, 2022 · 16 comments · Fixed by #1706
Closed

OpenFeign doesn't work with kotlin suspend method #1565

XhstormR opened this issue Jan 8, 2022 · 16 comments · Fixed by #1706

Comments

@XhstormR
Copy link

XhstormR commented Jan 8, 2022

I have an API interface where the methods are kotlin suspend methods:

@FeignClient(path = "/api/masker", contextId = "MaskerApi", name = Const.SERVICE, configuration = [FeignConfig::class])
interface MaskerApi {

    @PostMapping("/mask")
    suspend fun mask(
        @Valid @RequestBody maskRequest: MaskRequest,
    ): RestResponse<List<CharSequence>>

    @PostMapping("/matches")
    suspend fun matches(
        @Valid @RequestBody maskRequest: MaskRequest,
    ): RestResponse<List<Boolean>>
}

But when I run the program, the program throws an exception:

Caused by: java.lang.IllegalStateException: Method has too many Body parameters: public abstract java.lang.Object io.github.xhstormr.masker.web.api.masker.MaskerApi.matches(io.github.xhstormr.masker.model.masker.MaskRequest,kotlin.coroutines.Continuation)
Warnings:
- 
	at feign.Util.checkState(Util.java:121) ~[feign-core-11.7.jar:na]
	at feign.Contract$BaseContract.parseAndValidateMetadata(Contract.java:142) ~[feign-core-11.7.jar:na]
	at org.springframework.cloud.openfeign.support.SpringMvcContract.parseAndValidateMetadata(SpringMvcContract.java:193) ~[spring-cloud-openfeign-core-3.1.0.jar:3.1.0]
	at feign.Contract$BaseContract.parseAndValidateMetadata(Contract.java:65) ~[feign-core-11.7.jar:na]
	at feign.ReflectiveFeign$ParseHandlersByName.apply(ReflectiveFeign.java:151) ~[feign-core-11.7.jar:na]
	at feign.ReflectiveFeign.newInstance(ReflectiveFeign.java:49) ~[feign-core-11.7.jar:na]
	at feign.Feign$Builder.target(Feign.java:268) ~[feign-core-11.7.jar:na]
	at org.springframework.cloud.openfeign.DefaultTargeter.target(DefaultTargeter.java:30) ~[spring-cloud-openfeign-core-3.1.0.jar:3.1.0]
	at org.springframework.cloud.openfeign.FeignClientFactoryBean.loadBalance(FeignClientFactoryBean.java:373) ~[spring-cloud-openfeign-core-3.1.0.jar:3.1.0]
	at org.springframework.cloud.openfeign.FeignClientFactoryBean.getTarget(FeignClientFactoryBean.java:421) ~[spring-cloud-openfeign-core-3.1.0.jar:3.1.0]
	at org.springframework.cloud.openfeign.FeignClientFactoryBean.getObject(FeignClientFactoryBean.java:396) ~[spring-cloud-openfeign-core-3.1.0.jar:3.1.0]
	at org.springframework.cloud.openfeign.FeignClientsRegistrar.lambda$registerFeignClient$0(FeignClientsRegistrar.java:235) ~[spring-cloud-openfeign-core-3.1.0.jar:3.1.0]
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.obtainFromSupplier(AbstractAutowireCapableBeanFactory.java:1249) ~[spring-beans-5.3.14.jar:5.3.14]
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1191) ~[spring-beans-5.3.14.jar:5.3.14]
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:582) ~[spring-beans-5.3.14.jar:5.3.14]
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:542) ~[spring-beans-5.3.14.jar:5.3.14]
	... 28 common frames omitted

This kotlin.coroutines.Continuation parameter is generated by the kotlin compiler at compile time, so I can't modify this parameter, how can I tell FeignClient to ignore this Continuation parameter?

@velo
Copy link
Member

velo commented Jan 9, 2022

The alternatives I see are:

  • we would need a feign-kotling module that would know how to handle this; or
  • create an abstract method without the suspension and a default method that includes suspension.

If the second alternative works, could you please contribute with an example project (like GitHub and Wikipedia examples)?

If doesn't, I would be keen to help on a kotlin module. I just don't know where to get started

@XhstormR
Copy link
Author

XhstormR commented Jan 9, 2022

I tried the second method, but I have a Controller class that inherits this interface and it doesn't work.

I finally customized a class that inherits SpringMvcContract:

class FeignIgnoreParameterContract(
    annotatedParameterProcessors: List<AnnotatedParameterProcessor>,
    conversionService: ConversionService,
    decodeSlash: Boolean
) : SpringMvcContract(
    annotatedParameterProcessors,
    conversionService,
    decodeSlash,
) {

    override fun processAnnotationsOnParameter(
        data: MethodMetadata,
        annotations: Array<out Annotation>,
        paramIndex: Int
    ): Boolean {
        if (data.method().parameterTypes[paramIndex] == Continuation::class.java) {
            data.ignoreParamater(paramIndex)
        }
        return super.processAnnotationsOnParameter(data, annotations, paramIndex)
    }
}

@XhstormR
Copy link
Author

XhstormR commented Jan 12, 2022

When I make a successful call to make an http request, but it fails when converting the http call result json into an object. When I remove the suspend modifier in the method, try again and it will be successful.

java.lang.IllegalStateException: Failed to execute ApplicationRunner
	at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:761) ~[spring-boot-2.6.2.jar:2.6.2]
	at org.springframework.boot.SpringApplication.callRunners(SpringApplication.java:748) ~[spring-boot-2.6.2.jar:2.6.2]
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:309) ~[spring-boot-2.6.2.jar:2.6.2]
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1301) ~[spring-boot-2.6.2.jar:2.6.2]
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1290) ~[spring-boot-2.6.2.jar:2.6.2]
	at io.github.xhstormr.masker.ApplicationKt.main(Application.kt:54) ~[main/:na]
Caused by: java.lang.ClassCastException: class java.util.LinkedHashMap cannot be cast to class io.github.xhstormr.masker.model.response.RestResponse (java.util.LinkedHashMap is in module java.base of loader 'bootstrap'; io.github.xhstormr.masker.model.response.RestResponse is in unnamed module of loader 'app')
	at io.github.xhstormr.masker.web.api.masker.MaskerApi.maskSync(MaskerApi.kt:32) ~[main/:na]
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:710) ~[na:na]
	at feign.DefaultMethodHandler.invoke(DefaultMethodHandler.java:141) ~[feign-core-11.7.jar:na]
	at feign.ReflectiveFeign$FeignInvocationHandler.invoke(ReflectiveFeign.java:100) ~[feign-core-11.7.jar:na]
	at com.sun.proxy.$Proxy116.maskSync(Unknown Source) ~[na:na]
	at io.github.xhstormr.masker.Application.init$lambda-0(Application.kt:45) ~[main/:na]
	at org.springframework.boot.SpringApplication.callRunner(SpringApplication.java:758) ~[spring-boot-2.6.2.jar:2.6.2]
	... 5 common frames omitted

@tmdgusya
Copy link

tmdgusya commented May 29, 2022

@XhstormR @velo

Is that correct you want? i did using feign with coroutines using kotlin default library and do not any setting

if correct that below example code, i wish i write example project "feign-with-kotlin-coroutines"

https://github.com/tmdgusya/feign-with-coroutines/blob/master/src/main/kotlin/com/roach/feignwithkotlinexample/TodoController.kt

@XhstormR
Copy link
Author

XhstormR commented May 29, 2022

@tmdgusya Thanks for your example. I looked at the code and my thought is https://github.com/tmdgusya/feign-with-coroutines/blob/master/src/main/kotlin/com/roach/feignwithkotlinexample/JsonPlaceHolderClients.kt#L14 method should have suspend modifier, so that TodoController can directly implement JsonPlaceHolderClients.

@tmdgusya
Copy link

tmdgusya commented May 29, 2022

@XhstormR thanks, i think that it's clients api call executed with coroutine context but I may be wrong.
plz see below README.md

https://github.com/tmdgusya/feign-with-coroutines

@XhstormR
Copy link
Author

@tmdgusya I see what you mean. The problem with this issue is to make FeignClient annotated classes to resolve suspend method calls.

@tmdgusya
Copy link

tmdgusya commented May 29, 2022

@XhstormR
Thanks to your explanation.
I have a one question. What's difference suspend method in interface and my code's(using withContext)
I just wonder what the difference is.

@tmdgusya
Copy link

@XhstormR
i totally knew that.
it's bigger difference suspend method in feignClients interfcae and suspend method in own's implements code.

my example code is using coroutines but not suspend function!
thanks. i got new knowledge in conversation

@tmdgusya
Copy link

Maybe, i think only one way that solving this problem that is implementation to Continuation

@velo
Copy link
Member

velo commented May 30, 2022

@XhstormR @velo

Is that correct you want? i did using feign with coroutines using kotlin default library and do not any setting

if correct that below example code, i wish i write example project "feign-with-kotlin-coroutines"

https://github.com/tmdgusya/feign-with-coroutines/blob/master/src/main/kotlin/com/roach/feignwithkotlinexample/TodoController.kt

That would be almost it...

Could it be a non-springboot project and with a unit test for the expected behavior. ideally (not sure if that is possible or not) unit test written in java instead of kotlin.

Also, would be much easier for me to pick up if was a maven project =)

@ArtemenkoAndrii
Copy link

Hello guys,

Are there any updates on this, please?
I'm also the one who wants to use Feign with coroutines but doesn't see clearway how exactly.

@XhstormR
Copy link
Author

Currently I have no good solution.

@velo
Copy link
Member

velo commented Jun 21, 2022

@ArtemenkoAndrii @XhstormR

Hi, I'm willing and able to help, but I have a) ZERO kotlin know-how and b) almost no free time to dedicate to learn it c) I don't have a compelling reason today

If anyone really want to see this fixed I need the following:

  • A pull request to feign, with a new module feign-kotlin
  • This module must contain a unit test demonstrating the issue
  • Must be a maven project
  • NO springboot, spring, springlite, springish, no "helpful" framework
  • As much as possible, JAVA code, split testing into multiple classes if needed
  • CI must fail, as part of test demonstrating the problem
  • OPTIONAL Cherry on top would be a project that is tested to work with Eclipse =)

As most I appreciate the example provided by @XhstormR , that is too far off my comfort zone

@ArtemenkoAndrii
Copy link

Well, not sure if I will have enough time for this. I briefly checked the sources and to me, it doesn't look a simple task.
Effectively the method signature should expect to receive one extra parameter with kotlin.coroutines.Continuation type. The resumeWith(Result) should be called when a response is ready.

But I don't think that in itself will give any benefits except syntactic - the calling thread is still to be blocked.
As I understand all underlying implementations(okhttp etc) use their own pools so the returning of CompletableFuture will make more sense. The kotlinx-coroutines-jdk has embedded adapters to handle it.

@velo
Copy link
Member

velo commented Jun 22, 2022

Well, not sure if I will have enough time for this

Ok

wplong11 added a commit to wplong11/feign that referenced this issue Jul 31, 2022
Resolves: OpenFeign#1565

Inspired by PlaytikaOSS/feign-reactive#486

## TODO

- [ ] Separate Kotlin support module
- [ ] Enhance test case
- [ ] Refactoring
- [ ] Clean up pom.xml
wplong11 added a commit to wplong11/feign that referenced this issue Jul 31, 2022
Resolves: OpenFeign#1565

Inspired by PlaytikaOSS/feign-reactive#486

## TODO

- [ ] Separate Kotlin support module
- [ ] Enhance test case
- [ ] Refactoring
- [ ] Clean up pom.xml
wplong11 added a commit to wplong11/feign that referenced this issue Aug 1, 2022
Resolves: OpenFeign#1565

Inspired by PlaytikaOSS/feign-reactive#486

## TODO

- [ ] Separate Kotlin support module
- [ ] Enhance test case
- [ ] Refactoring
- [ ] Clean up pom.xml
wplong11 added a commit to wplong11/feign that referenced this issue Aug 1, 2022
Resolves: OpenFeign#1565

Inspired by PlaytikaOSS/feign-reactive#486

## TODO

- [ ] Separate Kotlin support module
- [ ] Enhance test case
- [ ] Refactoring
- [ ] Clean up pom.xml
wplong11 added a commit to wplong11/feign that referenced this issue Aug 1, 2022
Resolves: OpenFeign#1565

Inspired by PlaytikaOSS/feign-reactive#486

## TODO

- [ ] Separate Kotlin support module
- [ ] Enhance test case
- [ ] Refactoring
- [ ] Clean up pom.xml
wplong11 added a commit to wplong11/feign that referenced this issue Aug 2, 2022
Resolves: OpenFeign#1565

Inspired by PlaytikaOSS/feign-reactive#486

## TODO

- [ ] Separate Kotlin support module
- [ ] Enhance test case
- [ ] Refactoring
- [ ] Clean up pom.xml
wplong11 added a commit to wplong11/feign that referenced this issue Sep 3, 2022
Resolves: OpenFeign#1565

Inspired by PlaytikaOSS/feign-reactive#486

## TODO

- [ ] Separate Kotlin support module
- [ ] Enhance test case
- [ ] Refactoring
- [ ] Clean up pom.xml
wplong11 added a commit to wplong11/feign that referenced this issue Sep 8, 2022
Resolves: OpenFeign#1565

Inspired by PlaytikaOSS/feign-reactive#486

## TODO

- [ ] Separate Kotlin support module
- [ ] Enhance test case
- [ ] Refactoring
- [ ] Clean up pom.xml
velo added a commit that referenced this issue Sep 14, 2022
* Support kotlin coroutines

Resolves: #1565

Inspired by PlaytikaOSS/feign-reactive#486

## TODO

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

* Apply optional dependency to kotlin support related dependency

* Seperate Kotlin support module

* Remove unused code from ClassUtils.java

* Remove unused code from ClassUtils.java

* Refactor KotlinDetector

* Move ClassUtils location into KotlinDetector

* Move KotlinDetector location

* Format code

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

* Coroutine Feign using AyncFeign

* Coroutine Feign using AyncFeign

* Refactor suspending function  detect logic

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

* Cleanup CoroutineFeignTest test code format

* Fix suspend function contract parsing error when using http body

* Rename test names to be meaningful

* Add Github Example With Coroutine

- Copy of GithubExample

* Remove unnecessary dependency

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

Co-authored-by: Marvin Froeder <velo.br@gmail.com>
Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>
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 a pull request may close this issue.

4 participants