Skip to content

optimize ResponseInterceptor #1003

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

optimize ResponseInterceptor #1003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/modules/ROOT/pages/spring-cloud-openfeign.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,9 @@ spring:
requestInterceptors:
- com.example.FooRequestInterceptor
- com.example.BarRequestInterceptor
responseInterceptor: com.example.BazResponseInterceptor
responseInterceptors:
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change unfortunately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hello @spencergibb, Can compatibility be achieved? Keep responseInterceptor and add responseInterceptors

Copy link
Collaborator

Choose a reason for hiding this comment

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

@galaxy-sea It could be done, but seems rather messy in terms of the API. I think we will consider it for a major release if it's scheduled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OlgaMaciaszek , I agree with your opinion

- com.example.BazResponseInterceptor
- com.example.QuxResponseInterceptor
dismiss404: false
encoder: com.example.SimpleEncoder
decoder: com.example.SimpleDecoder
Expand Down
18 changes: 13 additions & 5 deletions ...nfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientFactoryBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,14 @@ protected void configureUsingConfiguration(FeignClientFactory context, Feign.Bui
AnnotationAwareOrderComparator.sort(interceptors);
builder.requestInterceptors(interceptors);
}
ResponseInterceptor responseInterceptor = getInheritedAwareOptional(context, ResponseInterceptor.class);
if (responseInterceptor != null) {
builder.responseInterceptor(responseInterceptor);
Map<String, ResponseInterceptor> responseInterceptors = getInheritedAwareInstances(context,
ResponseInterceptor.class);
if (responseInterceptors != null) {
List<ResponseInterceptor> interceptors = new ArrayList<>(responseInterceptors.values());
AnnotationAwareOrderComparator.sort(interceptors);
builder.responseInterceptors(interceptors);
}

QueryMapEncoder queryMapEncoder = getInheritedAwareOptional(context, QueryMapEncoder.class);
if (queryMapEncoder != null) {
builder.queryMapEncoder(queryMapEncoder);
Expand Down Expand Up @@ -287,8 +291,12 @@ protected void configureUsingProperties(FeignClientProperties.FeignClientConfigu
}
}

if (config.getResponseInterceptor() != null) {
builder.responseInterceptor(getOrInstantiate(config.getResponseInterceptor()));
if (config.getResponseInterceptors() != null && !config.getResponseInterceptors().isEmpty()) {
// this will add response interceptor to builder, not replace existing
for (Class<ResponseInterceptor> bean : config.getResponseInterceptors()) {
ResponseInterceptor interceptor = getOrInstantiate(bean);
builder.responseInterceptor(interceptor);
}
}

if (config.getDismiss404() != null) {
Expand Down
14 changes: 7 additions & 7 deletions ...enfeign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public static class FeignClientConfiguration {

private List<Class<RequestInterceptor>> requestInterceptors;

private Class<ResponseInterceptor> responseInterceptor;
private List<Class<ResponseInterceptor>> responseInterceptors;

private Map<String, Collection<String>> defaultRequestHeaders = new HashMap<>();

Expand Down Expand Up @@ -206,12 +206,12 @@ public void setRequestInterceptors(List<Class<RequestInterceptor>> requestInterc
this.requestInterceptors = requestInterceptors;
}

public Class<ResponseInterceptor> getResponseInterceptor() {
return responseInterceptor;
public List<Class<ResponseInterceptor>> getResponseInterceptors() {
return responseInterceptors;
}

public void setResponseInterceptor(Class<ResponseInterceptor> responseInterceptor) {
this.responseInterceptor = responseInterceptor;
public void setResponseInterceptors(List<Class<ResponseInterceptor>> responseInterceptors) {
this.responseInterceptors = responseInterceptors;
}

public Map<String, Collection<String>> getDefaultRequestHeaders() {
Expand Down Expand Up @@ -323,7 +323,7 @@ public boolean equals(Object o) {
&& Objects.equals(readTimeout, that.readTimeout) && Objects.equals(retryer, that.retryer)
&& Objects.equals(errorDecoder, that.errorDecoder)
&& Objects.equals(requestInterceptors, that.requestInterceptors)
&& Objects.equals(responseInterceptor, that.responseInterceptor)
&& Objects.equals(responseInterceptors, that.responseInterceptors)
&& Objects.equals(dismiss404, that.dismiss404) && Objects.equals(encoder, that.encoder)
&& Objects.equals(decoder, that.decoder) && Objects.equals(contract, that.contract)
&& Objects.equals(exceptionPropagationPolicy, that.exceptionPropagationPolicy)
Expand All @@ -338,7 +338,7 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
return Objects.hash(loggerLevel, connectTimeout, readTimeout, retryer, errorDecoder, requestInterceptors,
responseInterceptor, dismiss404, encoder, decoder, contract, exceptionPropagationPolicy,
responseInterceptors, dismiss404, encoder, decoder, contract, exceptionPropagationPolicy,
defaultQueryParameters, defaultRequestHeaders, capabilities, queryMapEncoder, micrometer,
followRedirects, url);
}
Expand Down
8 changes: 4 additions & 4 deletions ...core/src/test/java/org/springframework/cloud/openfeign/FeignClientConfigurationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ void shouldDefaultToValuesWhenFieldsNotSet() {
assertThat(config.getRetryer()).isNull();
assertThat(config.getErrorDecoder()).isNull();
assertThat(config.getRequestInterceptors()).isNull();
assertThat(config.getResponseInterceptor()).isNull();
assertThat(config.getResponseInterceptors()).isNull();
assertThat(config.getDefaultRequestHeaders()).isEmpty();
assertThat(config.getDefaultQueryParameters()).isEmpty();
assertThat(config.getDismiss404()).isNull();
Expand All @@ -84,8 +84,8 @@ void shouldReturnValuesWhenSet() {
config.setErrorDecoder(ErrorDecoder.class);
List<Class<RequestInterceptor>> requestInterceptors = Lists.list(RequestInterceptor.class);
config.setRequestInterceptors(requestInterceptors);
Class<ResponseInterceptor> responseInterceptor = ResponseInterceptor.class;
config.setResponseInterceptor(responseInterceptor);
List<Class<ResponseInterceptor>> responseInterceptors = Lists.list(ResponseInterceptor.class);
config.setResponseInterceptors(responseInterceptors);
Map<String, Collection<String>> defaultRequestHeaders = Maps.newHashMap("default", Collections.emptyList());
config.setDefaultRequestHeaders(defaultRequestHeaders);
Map<String, Collection<String>> defaultQueryParameters = Maps.newHashMap("default", Collections.emptyList());
Expand All @@ -107,7 +107,7 @@ void shouldReturnValuesWhenSet() {
assertThat(config.getRetryer()).isSameAs(Retryer.class);
assertThat(config.getErrorDecoder()).isSameAs(ErrorDecoder.class);
assertThat(config.getRequestInterceptors()).isSameAs(requestInterceptors);
assertThat(config.getResponseInterceptor()).isSameAs(responseInterceptor);
assertThat(config.getResponseInterceptors()).isSameAs(responseInterceptors);
assertThat(config.getDefaultRequestHeaders()).isSameAs(defaultRequestHeaders);
assertThat(config.getDefaultQueryParameters()).isSameAs(defaultQueryParameters);
assertThat(config.getDismiss404()).isTrue();
Expand Down