Flux type response should be corresponding to List #2199
Conversation
WalkthroughThis pull request introduces significant enhancements to the Feign library, adding support for reactive programming with Reactor and RxJava. It includes new decoders for handling Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (4)
- core/src/main/java/feign/Types.java (2 hunks)
- core/src/main/java/feign/Util.java (1 hunks)
- reactive/README.md (2 hunks)
- reactive/src/main/java/feign/reactive/ReactiveDecoder.java (1 hunks)
Files skipped from review due to trivial changes (1)
- core/src/main/java/feign/Util.java
Additional comments (Suppressed): 4
reactive/README.md (1)
- 37-38: The
ReactorFeign.builder()now includes adecodermethod call. Ensure that theReactiveDecoderclass is properly implemented and that it correctly handlesMonoandFluxtypes.core/src/main/java/feign/Types.java (3)
327-358: The new method
resolveLastTypeParameteris introduced to resolve the last type parameter of a parameterized supertype. It throws anIllegalStateExceptionif the supertype cannot be resolved into a parameterized type using the provided context. This method is copied fromretrofit.RestMethodInfo. Ensure that the copied method is compatible with the current codebase and that it doesn't introduce any licensing issues.360-362: The new method
parameterizeis introduced to create a newParameterizedTypeImplinstance. It takes a raw class and an array of type arguments as parameters. This method improves the modularity of the code by providing a way to createParameterizedTypeImplinstances.364-366: The
ParameterizedTypeImplclass is now made package-private (default access modifier in Java). This change restricts the visibility of the class to the package in which it is defined. Ensure that this change doesn't break any code outside the package that might be using this class.
| @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); |
There was a problem hiding this comment.
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.
| ### 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. |
There was a problem hiding this comment.
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.
Sure. I'll add. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (9)
- reactive/README.md (3 hunks)
- reactive/src/main/java/feign/reactive/ReactiveDelegatingContract.java (1 hunks)
- reactive/src/main/java/feign/reactive/ReactorDecoder.java (1 hunks)
- reactive/src/main/java/feign/reactive/ReactorInvocationHandler.java (1 hunks)
- reactive/src/main/java/feign/reactive/RxJavaDecoder.java (1 hunks)
- reactive/src/test/java/feign/reactive/ReactiveFeignIntegrationTest.java (12 hunks)
- reactive/src/test/java/feign/reactive/examples/ConsoleLogger.java (1 hunks)
- reactive/src/test/java/feign/reactive/examples/ReactorGitHubExample.java (1 hunks)
- reactive/src/test/java/feign/reactive/examples/RxJavaGitHubExample.java (1 hunks)
Files skipped from review due to trivial changes (1)
- reactive/src/test/java/feign/reactive/examples/ConsoleLogger.java
Additional comments (Suppressed): 20
reactive/src/main/java/feign/reactive/ReactorDecoder.java (1)
- 27-50: The
ReactorDecoderclass is implementing theDecoderinterface and overriding thedecodemethod to handleMonoandFluxtypes. However, forFluxtype, it seems to be decoding the response into aList. This might not be the expected behavior asFluxis a stream of 0 to N items, and decoding it into aListmight cause memory issues for large data streams. Consider returning aFluxinstead of aList.- Type listType = Types.parameterize(List.class, lastType); - return delegate.decode(response, listType); + return Flux.fromIterable((List<?>) delegate.decode(response, listType));reactive/src/test/java/feign/reactive/examples/RxJavaGitHubExample.java (1)
- 31-36: The
RxJavaDecoderis being used withJacksonDecoderwhich is a good practice for handling JSON responses. However, ensure that theJacksonDecoderis compatible with the data being returned from the API calls. If the API returns a different data format, you may need to use a different decoder.reactive/src/test/java/feign/reactive/examples/ReactorGitHubExample.java (5)
33-37: The
ReactorFeign.builder()is correctly configured with the newReactorDecoderandJacksonDecoder. The logger and log level are also set appropriately. The target URL is correctly set to "https://api.github.com".39-45: The
contributorsFluxmethod is used to fetch a list of contributors using aFluxtype. ThecollectList()method is used to convert theFluxto aList, and theblock()method is used to subscribe to theFluxand block until it completes. The contributors are then printed to the console. This is a correct usage of theFluxtype.47-52: The
contributorsMonomethod is used to fetch a list of contributors using aMonotype. Theblock()method is used to subscribe to theMonoand block until it completes. The contributors are then printed to the console. This is a correct usage of theMonotype.56-61: The
GitHubinterface is correctly defined with two methods:contributorsFluxandcontributorsMono. Both methods use the@RequestLineannotation to define the HTTP request, and the@Paramannotation to define the path parameters. ThecontributorsFluxmethod returns aFlux<Contributor>, and thecontributorsMonomethod returns aMono<List<Contributor>>.63-74: The
Contributorclass is correctly defined with two fields:loginandcontributions. ThesetLoginandsetContributionsmethods are used to set the values of these fields.reactive/src/main/java/feign/reactive/ReactiveDelegatingContract.java (1)
- 55-59: The new hunk has replaced the direct assignment of the first type argument to the method's return type with a call to the
type()method. This change could potentially alter the behavior of the code if thetype()method does not return the first type argument as expected. Please verify that thetype()method is implemented correctly and that this change does not introduce any unexpected behavior.reactive/src/main/java/feign/reactive/ReactorInvocationHandler.java (1)
- 36-42: The new hunk introduces a
flatMapIterableoperation on theFluxtype. This operation transforms the elements of the Flux into Iterables and then flattens these Iterables into a new Flux. This change implies that the method now expects the elements of the Flux to be Iterables. Ensure that this change is compatible with the expected return types of the methods that will be invoked by this handler. If the methods are not returning Iterables, this could lead to aClassCastException.- return Flux.from(invocation).flatMapIterable(e -> (Iterable) e).subscribeOn(scheduler); + return Flux.from(invocation).subscribeOn(scheduler);reactive/src/test/java/feign/reactive/ReactiveFeignIntegrationTest.java (6)
86-97: The
ReactorDecoderis being used instead of theJacksonDecoder. This change is in line with the PR's goal of handlingFluxtype responses in Feign clients. Ensure that theReactorDecoderis compatible with all the places where this function is used.105-126: The test
testReactorTargetFullhas been updated to include tests forservice.usersFlux()andservice.usersMono(). This is a good practice as it ensures that the new methods are tested.134-139: The
RxJavaDecoderis being used instead of theJacksonDecoder. This change is in line with the PR's goal of handlingFluxtype responses in Feign clients. Ensure that theRxJavaDecoderis compatible with all the places where this function is used.315-320: In all these instances, the
ReactorDecoderorRxJavaDecoderis being used instead of theDecoder.Default(). This change is in line with the PR's goal of handlingFluxtype responses in Feign clients. Ensure that these decoders are compatible with all the places where these functions are used.329-338: The
TestReactorServiceinterface has been updated to includeusersFlux()andusersMono(). This is a good practice as it ensures that the new methods are included in the interface.346-354: The
TestReactiveXServiceinterface has been updated to includeusers(). This is a good practice as it ensures that the new method is included in the interface.reactive/src/main/java/feign/reactive/RxJavaDecoder.java (1)
- 25-43: The
RxJavaDecoderclass is well implemented. It delegates the decoding to anotherDecoderinstance, which allows for flexibility in choosing the decoding strategy. Thedecodemethod checks if the response type is assignable fromFlowable, and if so, it resolves the last type parameter of theFlowableand decodes the response using that type. This is a good approach to handleFlowabletypes in the response. However, it would be beneficial to handle the case where thedelegate.decodemethod might throw an exception. Wrapping the decode calls in a try-catch block and handling the exceptions appropriately would make the code more robust.@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); try { return delegate.decode(response, lastType); } catch (IOException | FeignException e) { // handle exception } } try { return delegate.decode(response, type); } catch (IOException | FeignException e) { // handle exception } }reactive/README.md (4)
14-33: The
contributorsmethod has been split into two methods:contributorsFluxandcontributorsMono, returning aFluxand aMonorespectively. This change provides more flexibility in handling the response, allowing for both a stream of contributors and a single emission of a list of contributors. However, ensure that all calls to the originalcontributorsmethod throughout the codebase have been updated to use one of the new methods.36-46: The
ExampleReactorclass has been updated to use the newReactorDecoderin theReactorFeign.builder()call. This change allows for reactive decoding of the response. ThecontributorsFluxandcontributorsMonomethods are used to retrieve contributors, demonstrating the use of bothFluxandMonoresponse types. Theblock()method is used to subscribe to theFluxandMonoand block until they emit their items, collecting them into a list. This is a good use of the reactive capabilities provided by the Reactor library.64-69: The
ExampleRxJava2class has been updated to use the newRxJavaDecoderin theRxJavaFeign.builder()call. This change allows for reactive decoding of the response using RxJava. However, thecontributorsmethod has not been updated to match the changes in theGitHubReactorinterface. Please verify if this is intentional or if theGitHubReactiveXinterface should also be updated.90-94: The section on
IterableandCollectionsresponses has been updated to remove the information about manually convertingMonoorIterableresponse types intoFluxusing thefromIterablemethod. This is consistent with the changes in theGitHubReactorinterface, which now provides aFluxresponse type directly. However, the statement "this limits the use ofFluxas a response type" may no longer be accurate, as thecontributorsFluxmethod does return aFlux. Please verify if this statement should be updated or removed.
| @RequestLine("GET /repos/{owner}/{repo}/contributors") | ||
| Flowable<List<Contributor>> contributors(@Param("owner") String owner, @Param("repo") String repo); | ||
| } |
There was a problem hiding this comment.
The contributors method is returning a Flowable<List<Contributor>>. This means that it will emit multiple lists of contributors. If the API only returns a single list of contributors, consider changing the return type to Single<List<Contributor>> to better reflect the expected response.
| static class Contributor { | ||
|
|
||
| private String login; | ||
| private int contributions; | ||
|
|
||
| void setLogin(String login) { | ||
| this.login = login; | ||
| } | ||
|
|
||
| void setContributions(int contributions) { | ||
| this.contributions = contributions; | ||
| } | ||
| } |
There was a problem hiding this comment.
The Contributor class is using package-private visibility for its fields and methods. This is generally not recommended as it can lead to issues with encapsulation. Consider changing the visibility to private and providing public getter methods to access the fields.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Files selected for processing (9)
- reactive/README.md (3 hunks)
- reactive/src/main/java/feign/reactive/ReactiveDelegatingContract.java (1 hunks)
- reactive/src/main/java/feign/reactive/ReactorDecoder.java (1 hunks)
- reactive/src/main/java/feign/reactive/ReactorInvocationHandler.java (1 hunks)
- reactive/src/main/java/feign/reactive/RxJavaDecoder.java (1 hunks)
- reactive/src/test/java/feign/reactive/ReactiveFeignIntegrationTest.java (12 hunks)
- reactive/src/test/java/feign/reactive/examples/ConsoleLogger.java (1 hunks)
- reactive/src/test/java/feign/reactive/examples/ReactorGitHubExample.java (1 hunks)
- reactive/src/test/java/feign/reactive/examples/RxJavaGitHubExample.java (1 hunks)
Additional comments (Suppressed): 16
reactive/src/main/java/feign/reactive/ReactiveDelegatingContract.java (1)
- 55-59: The new hunk has removed the explicit setting of the return type to the first actual type (
actualTypes[0]). Ensure that thetypevariable being used inmetadata.returnType(type);is correctly set and represents the intended return type.reactive/src/main/java/feign/reactive/RxJavaDecoder.java (2)
33-42: The
decodemethod has been overridden to handle decoding based on the type of the response. If the type isFlowable, it resolves the last type parameter and delegates the decoding to the underlying decoder. This is a good approach to handle different types of responses in a reactive context. However, it's important to ensure that the underlying decoder can handle the resolved type correctly.29-31: The constructor takes a
Decoderas a parameter and assigns it to thedelegatefield. This allows theRxJavaDecoderto delegate the actual decoding to anotherDecoder, which can be any implementation of theDecoderinterface. This is a good example of the Delegation pattern and promotes code reusability and flexibility.reactive/src/test/java/feign/reactive/examples/ReactorGitHubExample.java (1)
- 32-53: The use of
.block()in a reactive stream is generally discouraged as it defeats the purpose of using a non-blocking reactive stream. It would be better to use.subscribe()or.doOnNext()to handle the results of theFluxandMonostreams. However, this might be acceptable in a simple main method for demonstration purposes. Please verify if this is the intended usage.reactive/src/test/java/feign/reactive/examples/RxJavaGitHubExample.java (2)
31-36: The
RxJavaDecoderis being used here, which is a good choice for handling reactive types. However, it's important to ensure that the rest of the codebase is compatible with this change, especially if it was previously using a non-reactive decoder.48-50: The
contributorsmethod returns aFlowable<List<Contributor>>. This is a good use of RxJava'sFlowablefor handling potentially large lists of data. However, ensure that the API endpoint/repos/{owner}/{repo}/contributorssupports streaming responses, asFlowableis designed to handle backpressure in streaming scenarios.reactive/README.md (4)
14-33: The
contributorsmethod in theGitHubReactorinterface has been split into two methods:contributorsFluxandcontributorsMono. This change allows for differentiation betweenFluxandMonoresponses. Ensure that all calls to these methods throughout the codebase have been updated to match the new method names. Also, theContributorclass no longer has a constructor and instead uses getter and setter methods for theloginfield. Make sure this change doesn't affect the instantiation ofContributorobjects in the codebase.35-47: The
ExampleReactorclass now uses aReactorDecoderwith aJacksonDecoderfor decoding responses. This change provides reactive decoding capabilities. ThecontributorsFluxandcontributorsMonomethods are used to retrieve contributors, and the responses are collected into lists usingcollectList(). This is a more efficient way of collecting responses compared to the previous method of usingcollect(Collectors.toList()).62-69: The
ExampleRxJava2class now uses aRxJavaDecoderwith aJacksonDecoderfor decoding responses. This change provides reactive decoding capabilities for RxJava. Ensure that theRxJavaDecoderworks as expected and doesn't introduce any issues.90-94: The section on
IterableandCollectionsresponses has been updated to remove the mention of manual conversion fromMonoorIterabletoFluxusing thefromIterablemethod. This is because the newReactorDecoderhandles the conversion automatically. Ensure that this change doesn't introduce any issues with handlingFluxresponses.reactive/src/test/java/feign/reactive/ReactiveFeignIntegrationTest.java (6)
86-97: The
ReactorDecoderis being used instead of theJacksonDecoder. This change is likely due to the need to handleFluxtype responses in Feign clients. Ensure that theReactorDecoderis compatible with the rest of the codebase and that it correctly handles all expected response types.105-126: The test method
testReactorTargetFullhas been updated to include tests forFluxandMonoresponses. This is a good addition as it ensures that the new functionality is being properly tested.134-139: The
RxJavaDecoderis being used instead of theJacksonDecoder. This change is likely due to the need to handleFluxtype responses in Feign clients. Ensure that theRxJavaDecoderis compatible with the rest of the codebase and that it correctly handles all expected response types.315-320: In each of these instances, the
ReactorDecoderorRxJavaDecoderis being used instead of theDecoder.Default(). This change is likely due to the need to handleFluxtype responses in Feign clients. Ensure that these decoders are compatible with the rest of the codebase and that they correctly handle all expected response types.329-338: The
TestReactorServiceinterface has been updated to include methods for handlingFluxandMonoresponses. This is a good addition as it allows for testing of the new functionality.346-354: The
TestReactiveXServiceinterface has been updated to include a method for handlingFlowable<List<User>>responses. This is a good addition as it allows for testing of the new functionality.
| import feign.Logger; | ||
|
|
||
| public class ConsoleLogger extends Logger { | ||
| @Override | ||
| protected void log(String configKey, String format, Object... args) { | ||
| System.out.println(String.format(methodTag(configKey) + format, args)); | ||
| } |
There was a problem hiding this comment.
The ConsoleLogger class extends Logger and overrides the log method to print logs to the console. This is a simple and straightforward implementation. However, consider using a proper logging framework like Log4J or SLF4J for more robust logging capabilities, including log levels, log file rotation, and asynchronous logging.
- System.out.println(String.format(methodTag(configKey) + format, args));
+ // Assuming a logger instance is available
+ logger.info(String.format(methodTag(configKey) + format, args));| 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); | ||
| } |
There was a problem hiding this comment.
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));| 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); | ||
| } |
There was a problem hiding this comment.
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);| interface GitHub { | ||
| @RequestLine("GET /repos/{owner}/{repo}/contributors") | ||
| 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); | ||
| } |
There was a problem hiding this comment.
The GitHub interface has two methods contributorsFlux and contributorsMono that seem to be doing the same thing but returning different types (Flux<Contributor> and Mono<List<Contributor>>). This might be confusing for users of the interface. Consider consolidating these methods into one or clearly documenting the difference in their behaviors.
| static class Contributor { | ||
|
|
||
| private String login; | ||
| private int contributions; | ||
|
|
||
| void setLogin(String login) { | ||
| this.login = login; | ||
| } | ||
|
|
||
| void setContributions(int contributions) { | ||
| this.contributions = contributions; | ||
| } |
There was a problem hiding this comment.
The Contributor class is using manual setters for its fields. Consider using Lombok's @Data or @Setter annotations to generate these methods automatically, which would make the code cleaner and more maintainable.
| System.out.println("Let's fetch and print a list of the contributors to this library."); | ||
| List<Contributor> contributorsFromFlux = github.contributors("OpenFeign", "feign") | ||
| .blockingLast(); | ||
| for (Contributor contributor : contributorsFromFlux) { | ||
| System.out.println(contributor.login + " (" + contributor.contributions + ")"); | ||
| } |
There was a problem hiding this comment.
The blockingLast() method is being used to convert the Flowable to a List. This is a blocking operation and may not be ideal in a reactive context. Consider using non-blocking operations to fully leverage the benefits of reactive programming.
| private String login; | ||
| private int contributions; | ||
|
|
||
| void setLogin(String login) { | ||
| this.login = login; | ||
| } | ||
|
|
||
| void setContributions(int contributions) { | ||
| this.contributions = contributions; | ||
| } |
There was a problem hiding this comment.
The Contributor class is mutable, as it provides setters for its fields. This could potentially lead to issues in a multi-threaded environment. Consider making this class immutable if it's safe to do so.
* Refactoring: Move resolveLastTypeParameter from Util to Types * Add ReactiveDecoder * Update code as per suggestions * Update code as per suggestions * Refactoring * Add tests
* Refactoring: Move resolveLastTypeParameter from Util to Types * Add ReactiveDecoder * Update code as per suggestions * Update code as per suggestions * Refactoring * Add tests
Fixes: #1869
Summary by CodeRabbit
ReactiveDecoder,ReactorDecoder, andRxJavaDecoderclasses to support reactive decoding capabilities for Feign clients, enhancing the ability to handleMonoandFluxtypes from the Reactor library andFlowabletype from RxJava.resolveLastTypeParametermethod fromUtilclass toTypesclass.README.mdto reflect changes in theGitHubReactorinterface and usage examples for Reactor and RxJava2.ReactiveFeignIntegrationTestto cover new features and changes, providing better test coverage.ConsoleLoggerfor improved logging during testing.ReactorGitHubExampleandRxJavaGitHubExampleclasses to demonstrate the usage of Feign with Reactor and RxJava respectively.