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

Flux type response should be corresponding to List #2199

Merged
merged 7 commits into from
Oct 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 36 additions & 0 deletions core/src/main/java/feign/Types.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import java.util.Arrays;
import java.util.NoSuchElementException;

import static feign.Util.checkState;

/**
* Static methods for working with types.
*
Expand Down Expand Up @@ -325,6 +327,40 @@ public static Type resolveReturnType(Type baseType, Type overridingType) {
return baseType;
}

/**
* Resolves the last type parameter of the parameterized {@code supertype}, based on the {@code
* genericContext}, into its upper bounds.
* <p/>
* Implementation copied from {@code
* retrofit.RestMethodInfo}.
*
* @param genericContext Ex. {@link java.lang.reflect.Field#getGenericType()}
* @param supertype Ex. {@code Decoder.class}
* @return in the example above, the type parameter of {@code Decoder}.
* @throws IllegalStateException if {@code supertype} cannot be resolved into a parameterized type
* using {@code context}.
*/
public static Type resolveLastTypeParameter(Type genericContext, Class<?> supertype)
throws IllegalStateException {
Type resolvedSuperType =
Types.getSupertype(genericContext, Types.getRawType(genericContext), supertype);
checkState(resolvedSuperType instanceof ParameterizedType,
"could not resolve %s into a parameterized type %s",
genericContext, supertype);
Type[] types = ParameterizedType.class.cast(resolvedSuperType).getActualTypeArguments();
for (int i = 0; i < types.length; i++) {
Type type = types[i];
if (type instanceof WildcardType) {
types[i] = ((WildcardType) type).getUpperBounds()[0];
}
}
return types[types.length - 1];
}

public static ParameterizedType parameterize(Class<?> rawClass, Type... typeArguments) {
return new ParameterizedTypeImpl(rawClass.getEnclosingClass(), rawClass, typeArguments);
}

static final class ParameterizedTypeImpl implements ParameterizedType {

private final Type ownerType;
Expand Down
27 changes: 3 additions & 24 deletions core/src/main/java/feign/Util.java
Original file line number Diff line number Diff line change
Expand Up @@ -215,33 +215,12 @@ public static void ensureClosed(Closeable closeable) {
}

/**
* Resolves the last type parameter of the parameterized {@code supertype}, based on the {@code
* genericContext}, into its upper bounds.
* <p/>
* Implementation copied from {@code
* retrofit.RestMethodInfo}.
*
* @param genericContext Ex. {@link java.lang.reflect.Field#getGenericType()}
* @param supertype Ex. {@code Decoder.class}
* @return in the example above, the type parameter of {@code Decoder}.
* @throws IllegalStateException if {@code supertype} cannot be resolved into a parameterized type
* using {@code context}.
* Moved to {@code feign.Types.resolveLastTypeParameter}
*/
@Deprecated
public static Type resolveLastTypeParameter(Type genericContext, Class<?> supertype)
throws IllegalStateException {
Type resolvedSuperType =
Types.getSupertype(genericContext, Types.getRawType(genericContext), supertype);
checkState(resolvedSuperType instanceof ParameterizedType,
"could not resolve %s into a parameterized type %s",
genericContext, supertype);
Type[] types = ParameterizedType.class.cast(resolvedSuperType).getActualTypeArguments();
for (int i = 0; i < types.length; i++) {
Type type = types[i];
if (type instanceof WildcardType) {
types[i] = ((WildcardType) type).getUpperBounds()[0];
}
}
return types[types.length - 1];
return Types.resolveLastTypeParameter(genericContext, supertype);
}

/**
Expand Down
61 changes: 22 additions & 39 deletions reactive/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,34 @@ implementation to your classpath. Then configure Feign to use the reactive stre
public interface GitHubReactor {

@RequestLine("GET /repos/{owner}/{repo}/contributors")
Flux<Contributor> contributors(@Param("owner") String owner, @Param("repo") String repo);
Flux<Contributor> contributorsFlux(@Param("owner") String owner, @Param("repo") String repo);

@RequestLine("GET /repos/{owner}/{repo}/contributors")
Mono<List<Contributor>> contributorsMono(@Param("owner") String owner, @Param("repo") String repo);
Comment on lines 16 to +20
Copy link

Choose a reason for hiding this comment

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

The contributorsFlux and contributorsMono methods have the same @RequestLine annotation. This could lead to confusion and potential errors. Consider renaming these methods to better reflect their return types or the data they are fetching.


class Contributor {
String login;

public Contributor(String login) {
this.login = login;
}
String login;

public String getLogin() {
return login;
}

public void setLogin(String login) {
this.login = login;
}
}
}

public class ExampleReactor {
public static void main(String args[]) {
GitHubReactor gitHub = ReactorFeign.builder()
GitHubReactor gitHub = ReactorFeign.builder()
.decoder(new ReactorDecoder(new JacksonDecoder()))
.target(GitHubReactor.class, "https://api.github.com");

List<Contributor> contributors = gitHub.contributors("OpenFeign", "feign")
.collect(Collectors.toList())
List<GitHubReactor.Contributor> contributorsFromFlux = gitHub.contributorsFlux("OpenFeign", "feign")
.collectList()
.block();
List<GitHubReactor.Contributor> contributorsFromMono = gitHub.contributorsMono("OpenFeign", "feign")
.block();
}
}
Expand All @@ -52,7 +62,8 @@ public interface GitHubReactiveX {

public class ExampleRxJava2 {
public static void main(String args[]) {
GitHubReactiveX gitHub = RxJavaFeign.builder()
GitHubReactiveX gitHub = RxJavaFeign.builder()
.decoder(new RxJavaDecoder(new JacksonDecoder()))
.target(GitHub.class, "https://api.github.com");

List<Contributor> contributors = gitHub.contributors("OpenFeign", "feign")
Expand All @@ -79,33 +90,5 @@ the wrapped in the appropriate reactive wrappers.
### Iterable and Collections responses

Due to the Synchronous nature of Feign requests, methods that return `Iterable` types must specify the collection
in the `Publisher`. For `Reactor` types, this limits the use of `Flux` as a response type. If you
want to use `Flux`, you will need to manually convert the `Mono` or `Iterable` response types into
`Flux` using the `fromIterable` method.

in the `Publisher`. For `Reactor` types, this limits the use of `Flux` as a response type.
Comment on lines 90 to +93
Copy link

Choose a reason for hiding this comment

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

The removal of the example code that demonstrates how to manually convert Mono or Iterable response types into Flux using the fromIterable method could potentially confuse users. Consider adding a similar example or providing a link to documentation that explains this process.


```java
public interface GitHub {

@RequestLine("GET /repos/{owner}/{repo}/contributors")
Mono<List<Contributor>> contributors(@Param("owner") String owner, @Param("repo") String repo);

class Contributor {
String login;

public Contributor(String login) {
this.login = login;
}
}
}

public class ExampleApplication {
public static void main(String[] args) {
GitHub gitHub = ReactorFeign.builder()
.target(GitHub.class, "https://api.github.com");

Mono<List<Contributor>> contributors = gitHub.contributors("OpenFeign", "feign");
Flux<Contributor> contributorFlux = Flux.fromIterable(contributors.block());
}
}
```
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public List<MethodMetadata> parseAndValidateMetadata(Class<?> targetType) {
throw new IllegalArgumentException(
"Streams are not supported when using Reactive Wrappers");
}
metadata.returnType(actualTypes[0]);
metadata.returnType(type);
}
}

Expand Down
50 changes: 50 additions & 0 deletions reactive/src/main/java/feign/reactive/ReactorDecoder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright 2012-2023 The Feign Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/
package feign.reactive;

import feign.FeignException;
import feign.Response;
import feign.Types;
import feign.codec.Decoder;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;

import java.io.IOException;
import java.lang.reflect.Type;
import java.util.List;

public class ReactorDecoder implements Decoder {

private final Decoder delegate;

public ReactorDecoder(Decoder decoder) {
this.delegate = decoder;
}

@Override
public Object decode(Response response, Type type) throws IOException, FeignException {
Class<?> rawType = Types.getRawType(type);
if (rawType.isAssignableFrom(Mono.class)) {
Type lastType = Types.resolveLastTypeParameter(type, Mono.class);
return delegate.decode(response, lastType);
}
if (rawType.isAssignableFrom(Flux.class)) {
Type lastType = Types.resolveLastTypeParameter(type, Flux.class);
Type listType = Types.parameterize(List.class, lastType);
return delegate.decode(response, listType);
}

return delegate.decode(response, type);
}
Comment on lines +36 to +49
Copy link

Choose a reason for hiding this comment

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

The decode method has been updated to handle Mono and Flux types. It checks the raw type of the response and if it's Mono or Flux, it resolves the last type parameter and delegates the decoding to the underlying decoder. This is a good approach to handle reactive types in Feign clients. However, for Flux types, it's creating a List type to decode the response. This might not be the best approach as it could lead to memory issues for large responses. Consider using a streaming approach to handle Flux types.

-            Type listType = Types.parameterize(List.class, lastType);
-            return delegate.decode(response, listType);
+            return Flux.from(delegate.decode(response, lastType));

}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public class ReactorInvocationHandler extends ReactiveInvocationHandler {
protected Publisher invoke(Method method, MethodHandler methodHandler, Object[] arguments) {
Publisher<?> invocation = this.invokeMethod(methodHandler, arguments);
if (Flux.class.isAssignableFrom(method.getReturnType())) {
return Flux.from(invocation).subscribeOn(scheduler);
return Flux.from(invocation).flatMapIterable(e -> (Iterable) e).subscribeOn(scheduler);
} else if (Mono.class.isAssignableFrom(method.getReturnType())) {
return Mono.from(invocation).subscribeOn(scheduler);
}
Comment on lines 36 to 42
Copy link

Choose a reason for hiding this comment

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

The change from subscribeOn to flatMapIterable for Flux types is a significant one. This change ensures that the elements emitted by the Flux are processed sequentially on the scheduler. However, it also assumes that the emitted items are of type Iterable. This might not always be the case and could lead to a ClassCastException if the items are not Iterable. Consider adding a type check before casting to Iterable.

- return Flux.from(invocation).flatMapIterable(e -> (Iterable) e).subscribeOn(scheduler);
+ return Flux.from(invocation).flatMapIterable(e -> e instanceof Iterable ? (Iterable) e : Collections.singletonList(e)).subscribeOn(scheduler);

Expand Down
43 changes: 43 additions & 0 deletions reactive/src/main/java/feign/reactive/RxJavaDecoder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright 2012-2023 The Feign Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/
package feign.reactive;

import feign.FeignException;
import feign.Response;
import feign.Types;
import feign.codec.Decoder;
import io.reactivex.Flowable;

import java.io.IOException;
import java.lang.reflect.Type;

public class RxJavaDecoder implements Decoder {

private final Decoder delegate;

public RxJavaDecoder(Decoder decoder) {
this.delegate = decoder;
}

@Override
public Object decode(Response response, Type type) throws IOException, FeignException {
Class<?> rawType = Types.getRawType(type);
if (rawType.isAssignableFrom(Flowable.class)) {
Type lastType = Types.resolveLastTypeParameter(type, Flowable.class);
return delegate.decode(response, lastType);
}

return delegate.decode(response, type);
}
}