Add support for multiple ResponseInterceptors #1829
Conversation
| /** | ||
| * Adds a single response interceptor to the builder. | ||
| */ | ||
| public B responseInterceptor(ResponseInterceptor responseInterceptor) { |
There was a problem hiding this comment.
How about add Deprecated annotation? Without looking at the implementation it seems difficult to infer the behavior.
There was a problem hiding this comment.
But it's not deprecated, it just changed from you can have only one, to you can have as many as you want, like requestInterceptor
kdavisk6
left a comment
kdavisk6
left a comment
There was a problem hiding this comment.
Love the idea, feel like we may be re-inventing the wheel though. Any thoughts to using Function.andThen or Consumer.andThen instead?
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| protected ResponseInterceptor.Chain executionChain() { |
There was a problem hiding this comment.
can we consider a more descriptive name? How about interceptors or responseInterceptorChain?
| * @param nextInterceptor the interceptor to delegate to after the current | ||
| * @return a new interceptor that chains the two | ||
| */ | ||
| default ResponseInterceptor andThen(ResponseInterceptor nextInterceptor) { |
There was a problem hiding this comment.
This feels an awful lot like Function.andThen or Consumer.andThen. Do you think there's a way we could use those instead?
WalkthroughThe changes primarily revolve around the replacement of a single 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: 4
Files selected for processing (9)
- core/src/main/java/feign/AsyncFeign.java (1 hunks)
- core/src/main/java/feign/AsyncResponseHandler.java (1 hunks)
- core/src/main/java/feign/BaseBuilder.java (3 hunks)
- core/src/main/java/feign/Capability.java (1 hunks)
- core/src/main/java/feign/Feign.java (1 hunks)
- core/src/main/java/feign/ResponseHandler.java (3 hunks)
- core/src/main/java/feign/ResponseInterceptor.java (1 hunks)
- core/src/test/java/feign/BaseBuilderTest.java (3 hunks)
- kotlin/src/main/java/feign/kotlin/CoroutineFeign.java (1 hunks)
Additional comments (Suppressed): 15
core/src/main/java/feign/AsyncFeign.java (1)
- 200-203: The
executionChain()method is used instead of theresponseInterceptor. Ensure that all instances ofresponseInterceptorhave been replaced withexecutionChain()throughout the codebase and that theexecutionChain()method is correctly implemented to handle the chain of interceptors.core/src/main/java/feign/Feign.java (1)
- 205-211: The
executionChain()method is used instead of theresponseInterceptorin theResponseHandlerconstructor. Ensure that all instances ofResponseHandlerare updated to use this new constructor throughout the codebase.core/src/main/java/feign/AsyncResponseHandler.java (2)
30-34: The constructor of
AsyncResponseHandlerhas been updated to accept aResponseInterceptor.Chaininstead of a singleResponseInterceptor. Ensure that all instantiations ofAsyncResponseHandlerthroughout the codebase have been updated to match this new constructor signature.32-34: The
ResponseHandleris now initialized with aResponseInterceptor.Chaininstead of a singleResponseInterceptor. This change aligns with the PR's goal of supporting multipleResponseInterceptors.core/src/main/java/feign/Capability.java (1)
- 93-95: The new method
enrich(ResponseInterceptor.Chain chain)has been added to theCapabilityinterface. This method is designed to enrich aResponseInterceptor.Chain. By default, it returns the input chain as is, without performing any enrichment. If you want to provide custom enrichment logic forResponseInterceptor.Chain, you should override this method in your implementation of theCapabilityinterface.Please ensure that all implementations of the
Capabilityinterface throughout the codebase have been updated to handle this new method appropriately.core/src/main/java/feign/ResponseHandler.java (2)
42-55: The constructor of
ResponseHandlerhas been modified to replace the singleresponseInterceptorwith a chain of interceptors, represented byResponseInterceptor.Chain. Ensure that all instances ofResponseHandlerthroughout the codebase have been updated to match the new constructor signature.124-127: The
aroundDecode()method of theResponseInterceptorinterface has been replaced with thenext()method of theResponseInterceptor.Chaininterface. This change is consistent with the modifications in theResponseInterceptorinterface and its implementation inResponseHandler.kotlin/src/main/java/feign/kotlin/CoroutineFeign.java (1)
- 175-175: The change from
responseInterceptortoresponseInterceptorsis noted. Ensure that all instances where this builder method is called have been updated to pass a list of interceptors instead of a single interceptor.core/src/main/java/feign/BaseBuilder.java (3)
36-38: The change from a single
ResponseInterceptorto a list ofResponseInterceptors is a significant one. This allows for multiple interceptors to be used, which can provide more flexibility and control over the response handling process. However, this also means that all existing uses of theresponseInterceptorfield will need to be updated to work with the new list structure. Please ensure that these updates have been made throughout the codebase.221-223: This method adds a single interceptor to the list of response interceptors. It's a straightforward and expected change given the shift from a single interceptor to a list of interceptors.
304-314: This method creates an execution chain from the list of response interceptors. The use of the
reduceoperation with theandThenmethod effectively creates a chain of interceptors, where each interceptor is applied after the previous one. This is a good use of functional programming techniques to create a complex structure in a concise and readable way. However, please ensure that theandThenmethod in theResponseInterceptorinterface has been properly implemented to support this chaining behavior.core/src/main/java/feign/ResponseInterceptor.java (3)
37-37: Ensure that all calls to this function throughout the codebase have been updated to match the new signature. The method
intercept(Context context, Chain chain)has replaced the oldaroundDecode(InvocationContext invocationContext). This change in method signature could potentially break existing code if not properly updated.46-49: This is a good use of the decorator pattern to chain interceptors. It allows for flexibility and extensibility in handling responses.
54-66: The default implementation of the
Chaininterface handles exceptions well, ensuring that anyFeignExceptionorRuntimeExceptionthrown during decoding are appropriately caught and rethrown with more context. However, it's important to ensure that these exceptions are handled upstream where thenext(Context context)method is called.core/src/test/java/feign/BaseBuilderTest.java (1)
- 46-49: Good addition of the assertion to check if the list is not empty before accessing it. This prevents potential
IndexOutOfBoundsExceptionif the list is empty.
| public class Context { | ||
|
|
||
| private final Decoder decoder; | ||
| private final Type returnType; | ||
| private final Response response; | ||
|
|
||
| Context(Decoder decoder, Type returnType, Response response) { | ||
| this.decoder = decoder; | ||
| this.returnType = returnType; | ||
| this.response = response; | ||
| } | ||
|
|
||
| public Decoder decoder() { | ||
| return decoder; | ||
| } | ||
|
|
||
| public Type returnType() { | ||
| return returnType; | ||
| } | ||
|
|
||
| public Response response() { | ||
| return response; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
The Context class encapsulates the decoder, return type, and response. This is a good practice as it improves readability and maintainability by grouping related data together. However, the constructor for Context is package-private, which means it can only be instantiated within the same package. If this is intentional and aligns with your design, then it's fine. Otherwise, consider making the constructor public if instances of Context need to be created outside the package.
| test(AsyncFeign.builder().requestInterceptor(template -> { | ||
| }), 14); | ||
| }).responseInterceptor((ic, c) -> c.next(ic)), 14); |
There was a problem hiding this comment.
The responseInterceptor lambda function is empty and does not provide any functionality. If this is intentional, consider adding a comment to clarify its purpose.
- }).responseInterceptor((ic, c) -> c.next(ic)), 14);
+ }).responseInterceptor((ic, c) -> {
+ // Add response interceptor logic here
+ return c.next(ic);
+ }), 14);| test(Feign.builder().requestInterceptor(template -> { | ||
| }), 12); | ||
| }).responseInterceptor((ic, c) -> c.next(ic)), 12); |
There was a problem hiding this comment.
Similar to the previous comment, the responseInterceptor lambda function is empty. Consider adding a comment or implementing the necessary functionality.
- }).responseInterceptor((ic, c) -> c.next(ic)), 12);
+ }).responseInterceptor((ic, c) -> {
+ // Add response interceptor logic here
+ return c.next(ic);
+ }), 12);
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (3)
- core/src/main/java/feign/AsyncFeign.java (1 hunks)
- core/src/main/java/feign/BaseBuilder.java (3 hunks)
- core/src/main/java/feign/Feign.java (1 hunks)
Files skipped from review due to trivial changes (1)
- core/src/main/java/feign/Feign.java
Additional comments (Suppressed): 3
core/src/main/java/feign/AsyncFeign.java (1)
- 200-203: The
responseInterceptorChain()method is used here instead of the previousresponseInterceptor. Ensure that all instances ofresponseInterceptorhave been replaced withresponseInterceptorChain()throughout the codebase and that the new method is correctly implemented.core/src/main/java/feign/BaseBuilder.java (2)
36-38: The change from a single
ResponseInterceptorto a list ofResponseInterceptors is a significant one. This allows for multiple interceptors to be used, which can provide more flexibility and control over the response handling process. However, this also introduces additional complexity and potential issues. For example, the order in which the interceptors are added to the list can affect the behavior of the system. It's important to ensure that all existing and future uses of this class take this into account.304-314: This method creates a chain of response interceptors. The use of the
reduceoperation here is clever, as it effectively chains together all of the interceptors in the list. However, this code assumes that theandThenmethod ofResponseInterceptorcorrectly handles chaining, which should be verified. Additionally, the use ofCapability.enrichat the end suggests that capabilities can modify the chain of interceptors. This could potentially introduce unexpected behavior if not properly managed.
| public B responseInterceptors(Iterable<ResponseInterceptor> responseInterceptors) { | ||
| this.responseInterceptors.clear(); | ||
| for (ResponseInterceptor responseInterceptor : responseInterceptors) { | ||
| this.responseInterceptors.add(responseInterceptor); | ||
| } |
There was a problem hiding this comment.
This method clears the current list of response interceptors and replaces it with a new set. This could potentially remove interceptors that were previously added and are still needed. Consider providing a method to append to the list of interceptors instead of replacing them entirely.
* Add support for multiple ResponseInterceptors * Address PR comments --------- Co-authored-by: Marvin Froeder <marvin.froeder@police.govt.nz> Co-authored-by: Marvin Froeder <velobr@gmail.com>
* Add support for multiple ResponseInterceptors * Address PR comments --------- Co-authored-by: Marvin Froeder <marvin.froeder@police.govt.nz> Co-authored-by: Marvin Froeder <velobr@gmail.com>
3 participants
In general, I avoid breaking feign API contracts like the plague, but I couldn't keep the old contract and have multiple response interceptors.
Take a look if this change makes any sense, and lemme know if we can include this, probably on feign 13
FWIW, this is heavily inspired on how spring graphql does interceptors chaining
Summary by CodeRabbit
responseInterceptorwith a list ofresponseInterceptorsacross various classes, allowing for multiple interceptors.enrich(ResponseInterceptor.Chain chain)method inCapabilityinterface to enhance the functionality of response interceptor chains.BaseBuilderTestto accommodate the new response interceptor chaining feature.InvocationContextclass toResponseInterceptor.Contextfor better clarity and understanding of its role in the interception process.