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

Add support for multiple ResponseInterceptors #1829

Merged
merged 12 commits into from
Aug 29, 2023
2 changes: 1 addition & 1 deletion core/src/main/java/feign/AsyncFeign.java
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ public AsyncFeign<C> internalBuild() {
decoder,
errorDecoder,
dismiss404,
closeAfterDecode, decodeVoid, responseInterceptor),
closeAfterDecode, decodeVoid, executionChain()),
AsyncResponseHandler.class,
capabilities);

Expand Down
12 changes: 5 additions & 7 deletions core/src/main/java/feign/AsyncResponseHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,11 @@
class AsyncResponseHandler {
private final ResponseHandler responseHandler;

AsyncResponseHandler(Level logLevel, Logger logger, Decoder decoder,
ErrorDecoder errorDecoder, boolean dismiss404, boolean closeAfterDecode, boolean decodeVoid,
ResponseInterceptor responseInterceptor) {
this.responseHandler = new ResponseHandler(
logLevel, logger, decoder,
errorDecoder, dismiss404, closeAfterDecode, decodeVoid,
responseInterceptor);
AsyncResponseHandler(Level logLevel, Logger logger, Decoder decoder, ErrorDecoder errorDecoder,
boolean dismiss404, boolean closeAfterDecode, boolean decodeVoid,
ResponseInterceptor.Chain executionChain) {
this.responseHandler = new ResponseHandler(logLevel, logger, decoder, errorDecoder, dismiss404,
closeAfterDecode, decodeVoid, executionChain);
}

public CompletableFuture<Object> handleResponse(String configKey,
Expand Down
30 changes: 28 additions & 2 deletions core/src/main/java/feign/BaseBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public abstract class BaseBuilder<B extends BaseBuilder<B, T>, T> implements Clo

protected final List<RequestInterceptor> requestInterceptors =
new ArrayList<>();
protected ResponseInterceptor responseInterceptor = ResponseInterceptor.DEFAULT;
protected final List<ResponseInterceptor> responseInterceptors = new ArrayList<>();
protected Logger.Level logLevel = Logger.Level.NONE;
protected Contract contract = new Contract.Default();
protected Retryer retryer = new Retryer.Default();
Expand Down Expand Up @@ -203,11 +203,23 @@ public B requestInterceptors(Iterable<RequestInterceptor> requestInterceptors) {
return thisB;
}

/**
* Sets the full set of request interceptors for the builder, overwriting any previous
* interceptors.
*/
public B responseInterceptors(Iterable<ResponseInterceptor> responseInterceptors) {
this.responseInterceptors.clear();
for (ResponseInterceptor responseInterceptor : responseInterceptors) {
this.responseInterceptors.add(responseInterceptor);
}
Comment on lines +210 to +214
Copy link

Choose a reason for hiding this comment

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

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.

return thisB;
}

/**
* Adds a single response interceptor to the builder.
*/
public B responseInterceptor(ResponseInterceptor responseInterceptor) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about add Deprecated annotation? Without looking at the implementation it seems difficult to infer the behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

this.responseInterceptor = responseInterceptor;
this.responseInterceptors.add(responseInterceptor);
return thisB;
}

Expand Down Expand Up @@ -288,4 +300,18 @@ public final T build() {
}

protected abstract T internalBuild();

protected ResponseInterceptor.Chain executionChain() {
Copy link
Member

Choose a reason for hiding this comment

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

can we consider a more descriptive name? How about interceptors or responseInterceptorChain?

ResponseInterceptor.Chain endOfChain =
ResponseInterceptor.Chain.DEFAULT;
ResponseInterceptor.Chain executionChain = this.responseInterceptors.stream()
.reduce(ResponseInterceptor::andThen)
.map(interceptor -> interceptor.apply(endOfChain))
.orElse(endOfChain);

return (ResponseInterceptor.Chain) Capability.enrich(executionChain,
ResponseInterceptor.Chain.class, capabilities);
}


}
4 changes: 4 additions & 0 deletions core/src/main/java/feign/Capability.java
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ default ResponseInterceptor enrich(ResponseInterceptor responseInterceptor) {
return responseInterceptor;
}

default ResponseInterceptor.Chain enrich(ResponseInterceptor.Chain chain) {
return chain;
}

default Logger enrich(Logger logger) {
return logger;
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/feign/Feign.java
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ public <T> T target(Target<T> target) {
public Feign internalBuild() {
final ResponseHandler responseHandler =
new ResponseHandler(logLevel, logger, decoder, errorDecoder,
dismiss404, closeAfterDecode, decodeVoid, responseInterceptor);
dismiss404, closeAfterDecode, decodeVoid, executionChain());
MethodHandler.Factory<Object> methodHandlerFactory =
new SynchronousMethodHandler.Factory(client, retryer, requestInterceptors,
responseHandler, logger, logLevel, propagationPolicy,
Expand Down
58 changes: 0 additions & 58 deletions core/src/main/java/feign/InvocationContext.java

This file was deleted.

18 changes: 9 additions & 9 deletions core/src/main/java/feign/ResponseHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@
*/
package feign;

import static feign.FeignException.errorReading;
import static feign.Util.ensureClosed;
import feign.Logger.Level;
import feign.codec.Decoder;
import feign.codec.ErrorDecoder;
import java.io.IOException;
import java.lang.reflect.Type;
import static feign.FeignException.errorReading;
import static feign.Util.ensureClosed;

/**
* The response handler that is used to provide synchronous support on top of standard response
Expand All @@ -39,20 +39,20 @@ public class ResponseHandler {

private final boolean decodeVoid;

private final ResponseInterceptor responseInterceptor;
private final ResponseInterceptor.Chain executionChain;

public ResponseHandler(Level logLevel, Logger logger, Decoder decoder,
ErrorDecoder errorDecoder, boolean dismiss404, boolean closeAfterDecode, boolean decodeVoid,
ResponseInterceptor responseInterceptor) {
public ResponseHandler(Level logLevel, Logger logger, Decoder decoder, ErrorDecoder errorDecoder,
boolean dismiss404, boolean closeAfterDecode, boolean decodeVoid,
ResponseInterceptor.Chain executionChain) {
super();
this.logLevel = logLevel;
this.logger = logger;
this.decoder = decoder;
this.errorDecoder = errorDecoder;
this.dismiss404 = dismiss404;
this.closeAfterDecode = closeAfterDecode;
this.responseInterceptor = responseInterceptor;
this.decodeVoid = decodeVoid;
this.executionChain = executionChain;
}

public Object handleResponse(String configKey,
Expand Down Expand Up @@ -122,8 +122,8 @@ private Object decode(Response response, Type type) throws IOException {
}

try {
final Object result = responseInterceptor.aroundDecode(
new InvocationContext(decoder, type, response));
final Object result = executionChain.next(
new ResponseInterceptor.Context(decoder, type, response));
if (closeAfterDecode) {
ensureClosed(response.body());
}
Expand Down
93 changes: 84 additions & 9 deletions core/src/main/java/feign/ResponseInterceptor.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,101 @@
*/
package feign;

import static feign.FeignException.errorReading;
import feign.codec.DecodeException;
import feign.codec.Decoder;
import java.io.IOException;
import java.lang.reflect.Type;
import java.util.function.Function;

/**
* Zero or One {@code ResponseInterceptor} may be configured for purposes such as verify or modify
* headers of response, verify the business status of decoded object. Once interceptors are applied,
* {@link ResponseInterceptor#aroundDecode(Response, Function)} is called around decode method
* called
* Interceptor for purposes such as verify or modify headers of response, verify the business status
* of decoded object. Once interceptors are applied,
* {@link ResponseInterceptor#intercept(Response, Function)} is called around decode method called
*/
public interface ResponseInterceptor {

ResponseInterceptor DEFAULT = InvocationContext::proceed;

/**
* Called for response around decode, must either manually invoke
* {@link InvocationContext#proceed} or manually create a new response object
* Called for response around decode, must either manually invoke {@link Chain#next(Context)} or
* manually create a new response object
*
* @param invocationContext information surrounding the response been decoded
* @return decoded response
*/
Object aroundDecode(InvocationContext invocationContext) throws IOException;
Object intercept(Context context, Chain chain) throws IOException;

/**
* Return a new {@link ResponseInterceptor} that invokes the current interceptor first and then
* the one that is passed in.
*
* @param nextInterceptor the interceptor to delegate to after the current
* @return a new interceptor that chains the two
*/
default ResponseInterceptor andThen(ResponseInterceptor nextInterceptor) {
Copy link
Member

@kdavisk6 kdavisk6 Nov 21, 2022

Choose a reason for hiding this comment

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

This feels an awful lot like Function.andThen or Consumer.andThen. Do you think there's a way we could use those instead?

return (ic, chain) -> intercept(ic,
nextContext -> nextInterceptor.intercept(nextContext, chain));
}

/**
* Contract for delegation to the rest of the chain.
*/
public interface Chain {
Chain DEFAULT = ic -> {
try {
return ic.decoder().decode(ic.response(), ic.returnType());
} catch (final FeignException e) {
throw e;
} catch (final RuntimeException e) {
throw new DecodeException(ic.response().status(), e.getMessage(),
ic.response().request(), e);
} catch (IOException e) {
throw errorReading(ic.response().request(), ic.response(), e);
}
};

/**
* Delegate to the rest of the chain to execute the request.
*
* @param context the request to execute the {@link Chain} .
* @return the response
*/
Object next(Context context) throws IOException;
}

/**
* Apply this interceptor to the given {@code Chain} resulting in an intercepted chain.
*
* @param chain the chain to add interception around
* @return a new chain instance
*/
default Chain apply(Chain chain) {
return request -> intercept(request, chain);
}

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;
}

}
Comment on lines +87 to +111
Copy link

Choose a reason for hiding this comment

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

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.


}
6 changes: 4 additions & 2 deletions core/src/test/java/feign/BaseBuilderTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public class BaseBuilderTest {
public void checkEnrichTouchesAllAsyncBuilderFields()
throws IllegalArgumentException, IllegalAccessException {
test(AsyncFeign.builder().requestInterceptor(template -> {
}), 14);
}).responseInterceptor((ic, c) -> c.next(ic)), 14);
Comment on lines 30 to +31
Copy link

Choose a reason for hiding this comment

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

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);

}

private void test(BaseBuilder<?, ?> builder, int expectedFieldsCount)
Expand All @@ -43,6 +43,8 @@ private void test(BaseBuilder<?, ?> builder, int expectedFieldsCount)
field.setAccessible(true);
Object mockedValue = field.get(enriched);
if (mockedValue instanceof List) {
assertThat((List) mockedValue).withFailMessage("Enriched list missing contents %s", field)
.isNotEmpty();
mockedValue = ((List<Object>) mockedValue).get(0);
}
assertTrue("Field was not enriched " + field, Mockito.mockingDetails(mockedValue)
Expand All @@ -56,7 +58,7 @@ private void test(BaseBuilder<?, ?> builder, int expectedFieldsCount)
public void checkEnrichTouchesAllBuilderFields()
throws IllegalArgumentException, IllegalAccessException {
test(Feign.builder().requestInterceptor(template -> {
}), 12);
}).responseInterceptor((ic, c) -> c.next(ic)), 12);
Comment on lines 60 to +61
Copy link

Choose a reason for hiding this comment

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

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);

}

}
2 changes: 1 addition & 1 deletion kotlin/src/main/java/feign/kotlin/CoroutineFeign.java
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public CoroutineFeign<C> internalBuild() {
.queryMapEncoder(queryMapEncoder)
.options(options)
.requestInterceptors(requestInterceptors)
.responseInterceptor(responseInterceptor)
.responseInterceptors(responseInterceptors)
.invocationHandlerFactory(invocationHandlerFactory)
.defaultContextSupplier((AsyncContextSupplier<Object>) defaultContextSupplier)
.methodInfoResolver(methodInfoResolver)
Expand Down