Feature/decoder interceptor to response interceptor #2116
Conversation
…same manner that RequestInterceptor does) rather than intercepting the decoding process. Signed-off-by: Iain Henderson <Iain.henderson@mac.com>
…esponse-interceptor
|
Sorry for the delay, I will undo the rename and throw together a test |
…terceptor # Conflicts: # core/src/main/java/feign/ResponseHandler.java
…Interceptor and include unit tests for redirection interception, error interception, and void decoding in FeignTest.
|
Tests have been added and README updated |
…decoder-interceptor-to-response-interceptor
…decoder-interceptor-to-response-interceptor
WalkthroughThis pull request introduces significant changes to the Feign library, primarily focusing on enhancing response handling. A new 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.
Files selected for processing (6)
- README.md (1 hunks)
- core/src/main/java/feign/InvocationContext.java (2 hunks)
- core/src/main/java/feign/RedirectionInterceptor.java (1 hunks)
- core/src/main/java/feign/ResponseHandler.java (4 hunks)
- core/src/main/java/feign/ResponseInterceptor.java (1 hunks)
- core/src/test/java/feign/FeignTest.java (7 hunks)
Files skipped from review due to trivial changes (1)
- README.md
Review comments generated (11)
- Actionable : 11
- LGTM (Skipped): 1
- Noise Suppression (Skipped): 6
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Tag the bot with
@coderabbitaiin any review comment to ask it questions. The bot will not respond unless explicitly tagged. - A review comment is a comment on a diff or a file and not a comment on the PR itself.
- You can use the mouse to select multiple lines of code to provide exact context to the bot.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
|
Um... why was this closed? |
My bad, misclicked |
…decoder-interceptor-to-response-interceptor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files selected for processing (2)
- core/src/main/java/feign/RedirectionInterceptor.java (1 hunks)
- core/src/test/java/feign/FeignTest.java (7 hunks)
Additional comments (Suppressed): 5
core/src/main/java/feign/RedirectionInterceptor.java (2)
29-52: The
aroundDecodemethod is handling redirection responses by checking the status code and "Location" header. However, it's not considering the possibility of multiple redirections. If there are multiple "Location" headers, only the first one is used. This might lead to incorrect behavior if the first location redirects to another location. Consider handling this scenario.46-51: As per the previous conversation chain, you've mentioned that the response must remain open in case it will be further processed. However, it's important to ensure that the response is closed at some point to avoid resource leaks. If the response isn't being closed here, make sure it's being closed elsewhere after it's no longer needed.
core/src/test/java/feign/FeignTest.java (3)
1011-1019: The test
decodeVoid()is verifying that thedecode()method of themockDecoderis called once whenapi.body(emptyList())is invoked. However, there is no assertion to check the return value ofapi.body(emptyList()). As per the conversation chain, it's mentioned that there is no value to return. If this is the case and the function is void, then this comment can be disregarded.1022-1051: These tests
redirectionInterceptorString(),redirectionInterceptorCollection(),responseInterceptor400Error(), andresponseInterceptor500Error()are well written and cover different scenarios for the newRedirectionInterceptorandErrorInterceptorclasses. They ensure that the interceptors correctly handle redirection responses and error responses respectively.1326-1339: The
ErrorInterceptorclass implements theResponseInterceptorinterface and overrides thearoundDecode()method. This method checks if the response status is 300 or above, and if the return type isString, it reads the body of the response and returns it as a string. Otherwise, it proceeds with the invocation context. This implementation seems correct and handles different scenarios properly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (2)
- core/src/main/java/feign/RedirectionInterceptor.java (1 hunks)
- core/src/test/java/feign/FeignTest.java (11 hunks)
Additional comments (Suppressed): 4
core/src/main/java/feign/RedirectionInterceptor.java (1)
- 46-51: As per the previous comment chain, the response should remain open if it will be further processed. However, it's important to ensure that the response is closed at some point to avoid resource leaks. Please verify that the response is being closed elsewhere in the code after it has been fully processed.
core/src/test/java/feign/FeignTest.java (3)
952-953: There seems to be a typo in the method name in the old hunk (
queryMapEndcoder). It has been corrected in the new hunk (queryMapEncoder). Please ensure that all calls to this method throughout the codebase have been updated to match the new method name.1317-1320: These methods are returning the instance of the current object using
this. This is a common practice when designing classes with a fluent interface or builders. The methods modify the state of the object and then return the same object, allowing the operations to be chained together.1327-1340: This class
ErrorInterceptorimplements theResponseInterceptorinterface and overrides thearoundDecodemethod. If the response status is greater than or equal to 300, it checks if the return type isString. If so, it reads the body of the response into a string and returns it. Otherwise, it proceeds with the invocation context. This interceptor can be useful for handling error responses where the body contains a message or additional details about the error.
…decoder-interceptor-to-response-interceptor # Conflicts: # core/src/main/java/feign/InvocationContext.java # core/src/main/java/feign/ResponseHandler.java # core/src/main/java/feign/ResponseInterceptor.java
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (8)
- core/pom.xml (1 hunks)
- core/src/main/java/feign/RedirectionInterceptor.java (1 hunks)
- core/src/main/java/feign/Request.java (2 hunks)
- core/src/main/java/feign/ResponseHandler.java (3 hunks)
- core/src/main/java/feign/ResponseInterceptor.java (3 hunks)
- core/src/main/java/feign/ResponseMapper.java (1 hunks)
- core/src/test/java/feign/DefaultContractTest.java (1 hunks)
- core/src/test/java/feign/FeignTest.java (11 hunks)
Files skipped from review due to trivial changes (4)
- core/pom.xml
- core/src/main/java/feign/Request.java
- core/src/main/java/feign/ResponseMapper.java
- core/src/test/java/feign/DefaultContractTest.java
Additional comments (Suppressed): 16
core/src/main/java/feign/RedirectionInterceptor.java (1)
- 50-51: Closing the response stream manually might not be necessary if you're using try-with-resources or similar resource management. If you're not, consider adding error handling around this operation as it can throw an IOException.
core/src/main/java/feign/ResponseInterceptor.java (3)
37-37: The
interceptmethod now throws a genericExceptioninstead of anIOException. This change could potentially break existing implementations of theResponseInterceptorinterface that are only handlingIOException. Ensure all implementations of this interface have been updated to handle the more generalException.54-64: The default implementation of the
Chaininterface has been simplified. It no longer handles decoding or exceptions explicitly, which might affect error handling and reporting in your application. Make sure that these responsibilities are properly handled elsewhere in your code.72-74: This part of the code remains unchanged and looks good as it is.
core/src/main/java/feign/ResponseHandler.java (3)
- 60-75: The new implementation of the
handleResponsemethod has removed the checks for whether the response should be decoded based on the status code and return type. This could potentially lead to issues if the response is not suitable for decoding. Please ensure that this change does not introduce any unexpected behavior.- if (returnType == Response.class) { - return disconnectResponseBodyIfNeeded(response); - } - - final boolean shouldDecodeResponseBody = (response.status() >= 200 && response.status() < 300) - || (response.status() == 404 && dismiss404 && !isVoidType(returnType)); - - if (!shouldDecodeResponseBody) { - throw decodeError(configKey, response); - } - - return decode(response, returnType);
30-32: The constant
MAX_RESPONSE_BUFFER_SIZEhas been removed from the class. If this constant was used elsewhere in the codebase, it might cause compilation errors or unintended behavior. Please verify that this removal doesn't affect other parts of the code.84-87: The methods
disconnectResponseBodyIfNeeded,decode,decodeError, andisVoidTypehave been removed from the class. These methods provided functionality for handling the response body and decoding the response. Ensure that their removal doesn't impact the functionality of the class and that their logic has been incorporated elsewhere as necessary.core/src/test/java/feign/FeignTest.java (9)
62-62: The
mockDecoderis declared and initialized correctly. The previous comment chain indicates a misunderstanding, as themockDecoderis indeed initialized with a mock object.902-902: There was a typo in the method name
queryMapEndcoderwhich has been corrected toqueryMapEncoder. This change should be verified across the codebase to ensure that all calls to this method have been updated.918-918: There was a typo in the method name
queryMapEndcoderwhich has been corrected toqueryMapEncoder. This change should be verified across the codebase to ensure that all calls to this method have been updated.937-937: There was a typo in the method name
queryMapEndcoderwhich has been corrected toqueryMapEncoder. This change should be verified across the codebase to ensure that all calls to this method have been updated.952-952: There was a typo in the method name
queryMapEndcoderwhich has been corrected toqueryMapEncoder. This change should be verified across the codebase to ensure that all calls to this method have been updated.1011-1020: This test case verifies that the
decode()method of themockDecoderis called once whenapi.body(emptyList())is invoked. However, there is no assertion to check the return value ofapi.body(emptyList()). As per the conversation chain, it's clarified that there is no value to return, so the test performs as expected.1287-1289: A new method
responseInterceptor(ResponseInterceptor responseInterceptor)has been added to theTestInterfaceBuilderclass. Ensure that this method is used correctly throughout the codebase.1312-1314: A new method
decodeVoid()has been added to theTestInterfaceBuilderclass. Ensure that this method is used correctly throughout the codebase.1327-1340: A new class
ErrorInterceptorimplementingResponseInterceptorinterface has been added. It overrides theinterceptmethod to handle responses with status codes 300 and above. If the return type isString, it reads the body of the response and returns it. Otherwise, it proceeds to the next interceptor in the chain. This implementation seems correct and efficient.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- core/src/test/java/feign/FeignTest.java (11 hunks)
Additional comments (Suppressed): 8
core/src/test/java/feign/FeignTest.java (8)
901-905: Good job on fixing the typo from
queryMapEndcodertoqueryMapEncoder. This will prevent potential runtime errors due to calling an undefined method.917-921: Good job on fixing the typo from
queryMapEndcodertoqueryMapEncoder. This will prevent potential runtime errors due to calling an undefined method.936-940: Good job on fixing the typo from
queryMapEndcodertoqueryMapEncoder. This will prevent potential runtime errors due to calling an undefined method.949-953: Good job on fixing the typo from
queryMapEndcodertoqueryMapEncoder. This will prevent potential runtime errors due to calling an undefined method.1011-1022: Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
1337-1345: The newly added methods
responseInterceptorandresponseInterceptorsin theTestInterfaceBuilderclass are good additions for setting response interceptors. They improve the flexibility and configurability of the test interface builder.1367-1375: The newly added method
decodeVoidin theTestInterfaceBuilderclass is a good addition for setting the decode void behavior. It improves the flexibility and configurability of the test interface builder.1382-1396: The new
ErrorInterceptorclass provides a way to handle HTTP responses with status codes 300 and above. It's a good practice to handle such responses separately as they often indicate client or server errors. However, ensure that this new interceptor is properly integrated into the Feign client setup.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Files selected for processing (6)
- core/pom.xml (1 hunks)
- core/src/main/java/feign/Logger.java (1 hunks)
- core/src/main/java/feign/Request.java (1 hunks)
- core/src/main/java/feign/template/Template.java (1 hunks)
- core/src/test/java/feign/FeignTest.java (11 hunks)
- core/src/test/java/feign/MultipleLoggerTest.java (3 hunks)
Files skipped from review due to trivial changes (4)
- core/pom.xml
- core/src/main/java/feign/Logger.java
- core/src/main/java/feign/Request.java
- core/src/main/java/feign/template/Template.java
Additional comments (Suppressed): 8
core/src/test/java/feign/FeignTest.java (8)
902-902: Good job on fixing the typo in the method name from
queryMapEndcodertoqueryMapEncoder.918-918: Good job on fixing the typo in the method name from
queryMapEndcodertoqueryMapEncoder.937-937: Good job on fixing the typo in the method name from
queryMapEndcodertoqueryMapEncoder.952-952: Good job on fixing the typo in the method name from
queryMapEndcodertoqueryMapEncoder.1011-1020: This test case verifies that the
decode()method of themockDecoderis called once whenapi.body(emptyList())is invoked. However, there is no assertion to check the return value ofapi.body(emptyList()). Consider adding an assertion to verify the expected return value.Object result = api.body(emptyList()); verify(mockDecoder, times(1)).decode(ArgumentMatchers.any(), eq(void.class)); assertEquals("Expected result", result);
1337-1345: These methods are added to allow setting a single
ResponseInterceptoror multipleResponseInterceptorsin theTestInterfaceBuilder. This is a good practice as it allows for flexibility and extensibility in testing different scenarios with different interceptors.1367-1370: The
decodeVoid()method has been added to theTestInterfaceBuilderclass. This method sets thedecodeVoidflag in the Feign builder, which controls whether void methods should be decoded. This is a useful addition for testing scenarios where the response body is not important and can be ignored.1382-1395: The
ErrorInterceptorclass is a customResponseInterceptorimplementation that extracts the response body as a string if the status code is 300 or above. This is a good practice for handling error responses in a custom way.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Files selected for processing (2)
- core/src/main/java/feign/Request.java (2 hunks)
- core/src/test/java/feign/FeignTest.java (10 hunks)
Files skipped from review due to trivial changes (1)
- core/src/main/java/feign/Request.java
Additional comments (Suppressed): 4
core/src/test/java/feign/FeignTest.java (4)
18-29: The import statements have been rearranged and some new ones have been added. Ensure that all the newly imported classes are used in the code and that there are no unused imports.
35-55: The import statements have been rearranged and some new ones have been added. Ensure that all the newly imported classes are used in the code and that there are no unused imports.
1336-1344: New methods
responseInterceptorandresponseInterceptorshave been added to theTestInterfaceBuilderclass. Ensure that these methods are used correctly in the test cases and that they function as expected.1381-1395: A new inner class
ErrorInterceptorhas been added. This class implements theResponseInterceptorinterface and overrides theinterceptmethod. Ensure that this class is used correctly in the test cases and that it functions as expected.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (2)
- core/src/main/java/feign/RedirectionInterceptor.java (1 hunks)
- core/src/main/java/feign/ResponseInterceptor.java (3 hunks)
Additional comments (Suppressed): 3
core/src/main/java/feign/ResponseInterceptor.java (3)
33-33: The method signature has changed from
Object intercept(Context context, Chain chain) throws IOException;toObject intercept(InvocationContext invocationContext, Chain chain) throws Exception;. Ensure that all calls to this function throughout the codebase have been updated to match the new signature and that they handle the more generalExceptioninstead of justIOException.59-59: The method signature has changed from
Object next(Context context) throws IOException;toObject next(InvocationContext context) throws Exception;. Ensure that all calls to this function throughout the codebase have been updated to match the new signature and that they handle the more generalExceptioninstead of justIOException.51-51: The default implementation of the
Chaininterface has been simplified. This could potentially lead to different behavior if the previous implementation was relied upon. Verify that this change does not introduce any unexpected behavior.
I utilize Feign to implement REST clients and have a need to extract the location header from some 300 series HTTP responses.
So far, we have found four ways to implement this behavior:
Responseobject and extract the header information. This is the current method I am using, but it short circuits the retry logic contained in Feign for 429 and 503 HTTP responses. I have re-implemented this logic, but that is less than ideal.RedirectionDecoder. This would be effective but seems to have limited usefulness.ResponseInterceptoris called. Currently theResponseInterceptoris wrapped around the decode operations. This means that theResponseInterceptoris not called if the HTTP response is not in the 200 series (or a 404). It is also not called if the return type isResponse. Functionally, theResponseInterceptoris a decode interceptor, rather than a response interceptor. This solution is more flexible than aRedirectiondecoderwould be and should enable many workflows.This pull request updates when
ResponseInterceptoris called to intercept responses before they are processed (much asRequestInterceptordoes).Because this change requires changing the signature of the functional method of
ResponseInterceptor, that signature has been updated to match the parallel method inRequestInterceptor(method name changed as well as the exception thrown).Summary by CodeRabbit
RedirectionInterceptorclass to handle redirection responses, extracting the "Location" header and returning it as a result.InvocationContextclass with new constructor parameters and methods for improved error handling and response decoding.ResponseHandlerclass to incorporate logic from removed methods intohandleResponsemethod.ResponseInterceptorinterface to useInvocationContextparameter instead ofContext.FeignTestfor decoding void responses, handling redirection and error responses.