This has been observed using spring-boot 3.2.1 with spring-web 6.1.2
When using RestClient.create(RestTemplate) and providing a RestTemplate instance that holds any HttpClientRequestInerceptors, RestClient copies the interceptor list and creates an own InterceptingClientHttpRequestFactory with the copied list.
But it also uses the InterceptingClientHttpRequestFactory obtained from RestTemplate.getRequestFactory() containing the (same) interceptors from the RestTemplate to create the requests.
Now when creating a request, a request chain containing two different instances of InterceptingClientHttpRequest each with an equal interceptor list are created and thus each interceptor is called twice during request execution.
There is a workaround, when you do not need to reuse the RestTemplate:
RestClient restClient = RestClient.create(restTemplate);
restTemplate.setInterceptors(List.of());
Complete code to reproduce:
@Test
void restclient_should_calls_interceptors_from_resttemplate_only_once() {
AtomicInteger calls = new AtomicInteger();
RestTemplate restTemplate = new RestTemplate();
restTemplate.setInterceptors(List.of((request, body, execution) -> {
calls.incrementAndGet();
return execution.execute(request, body);
}));
RestClient restClient = RestClient.create(restTemplate);
// Workaround when you do not need to reuse RestTemplate
// restTemplate.setInterceptors(List.of());
try {
restClient.get().uri("https://localhost:8080/foobar").retrieve();
} catch (ResourceAccessException e) {
// http-call fails but does not matter.
}
assertEquals(1, calls.get());
}
Comment From: sbrannen
Hi @tilm4nn,
Congratulations on reporting your first issue for the Spring Framework! π
Thanks for the detailed explanation and the test that reproduces the behavior.
We'll look into it.
Comment From: injae-kim
@Test
void restclient_should_calls_interceptors_from_resttemplate_only_once() {
AtomicInteger calls = new AtomicInteger();
RestTemplate restTemplate = new RestTemplate();
restTemplate.setInterceptors(List.of((request, body, execution) -> {
calls.incrementAndGet();
return execution.execute(request, body);
}));
RestClient restClient = RestClient.create(restTemplate);
try {
restClient.get().uri("https://localhost:8080/foobar").retrieve();
} catch (ResourceAccessException e) { }
Assertions.assertEquals(1, calls.get()); // β assertion failed, expected: <1> but was: <2>
// It means, interceptors are called twice
}
Thanks @tilm4nn for the detailed explanation! your reproduces works well with me too.
Root cause: RestTemplate.interceptors are used both on RestClient and RestTemplate twice.
https://github.com/spring-projects/spring-framework/blob/e1c8a84fec02373b79f9023880a8a8c3dd8ef593/spring-web/src/main/java/org/springframework/http/client/support/InterceptingHttpAccessor.java#L102
1) RestTemplate.getRequestFactory() return InterceptingClientHttpRequestFactory(interceptors). it contains interceptors
https://github.com/spring-projects/spring-framework/blob/e1c8a84fec02373b79f9023880a8a8c3dd8ef593/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java#L180-L185
2) But on RestClientBuilder(RestTemplate), we copy interceptors again that already contained in InterceptingClientHttpRequestFactory from restTemplate.getRequestFactory()
https://github.com/spring-projects/spring-framework/blob/e1c8a84fec02373b79f9023880a8a8c3dd8ef593/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java#L521
3) Finally, RestClient.createRequest() uses interceptors and InterceptingClientHttpRequestFactory from restTemplate.getRequestFactory() that already contains interceptors, so same interceptors from RestTemplate called twice!
My simple proposal
https://github.com/spring-projects/spring-framework/blob/e1c8a84fec02373b79f9023880a8a8c3dd8ef593/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java#L183-L185
if (!CollectionUtils.isEmpty(restTemplate.getInterceptors())) {
// ππ If RestTemplate's requestFactory already contains interceptors, don't copy it
if (!(this.requestFactory instanceof InterceptingClientHttpRequestFactory)) {
this.interceptors = new ArrayList<>(restTemplate.getInterceptors());
}
}
With above code, we don't copy RestTemplate's interceptors when restTemplate.requestFactory already contains that interceptors.
And I checked test also passed with above code.
So simply check my investigation and share your opinion~! I'm also ready to open PR. thanks a lot! cc. @sbrannen π
Comment From: tilm4nn
Hi @injae-kim, thank you very much for your investigation and proposal. I think the idea of the proposed solution is clear and it might be achieved in a simpler way. I can imagine only two possible paths:
- RestTemplate does hold some interceptors: then requestFactory will always be an InterceptingClientHttpRequestFactory here and we do not copy the interceptors list. (Only probably extremly rare exception: A subclass of RestTemplate is used that overrides getRequestFactory somehow)
- RestTemplate does not hold any interceptors: then we do not copy the interceptors list.
So as the outcome is always "we do not copy the interceptors list" wouldn't it be possible to just skip the copy of the interceptors list alltogether? Or am I missing something here?
One additional case comes to my mind:
var builder = RestClient.builder(restTemplateWithInterceptors);
builder.requestFactory(someSpecialRequestFactory)
// Here we have lost the interceptors of RestTemplate's InterceptingClientHttpRequestFactory and have to add them manually
builder.requestInterceptors(interceptors -> interceptors.addAll(restTemplateWithInterceptors.getInterceptors()))
But I think one can argue that the manual step is justified in this case.
Comment From: tilm4nn
And here is another workaround that also works independent of how the RestTemplate is used otherwise:
RestClient.builder(restTemplate).requestInterceptors(List::clear).build();
Comment From: injae-kim
So as the outcome is always "we do not copy the interceptors list" wouldn't it be possible to "just skip the copy of the interceptors list" all together?
Hi @tilm4nn ! as you suggested above, we can fix this issue by "just skip the copy of the interceptors list from RestTemplate".
RestTemplate does hold some interceptors: then requestFactory will always be an InterceptingClientHttpRequestFactory here and we do not copy the interceptors list. (Only probably extremly rare exception: A subclass of RestTemplate is used that overrides getRequestFactory somehow)
But I worry about this case. in the future I think we can change RestTemplate.requestFactory()'s return type that doesn't include interceptors, and then RestTemplate.interceptors are never copied!
(For maintainability, maybe it's better to not strongly rely on assumption that RestTemplate.requestFactory() always contains interceptors)
So let's wait maintainer's opinion about this~ thanks! π
Comment From: poutsma
@tilm4nn Thanks for reporting this, it should now be fixed.
@injae-kim Changing what RestTemplate.getRequestFactory() returns might have solved this particular issue, but it would also break backward compatibility for existing users that rely on the current behavior, especially considering the fact that this method comes from InterceptingHttpAccessor, an abstract base class that could be in use in various other places, not just RestTemplate. So I decided to take a different route in https://github.com/spring-projects/spring-framework/commit/c820e44a995b7a6c4b76af67e51e86f90358c561.
Comment From: injae-kim
https://github.com/spring-projects/spring-framework/commit/c820e44a995b7a6c4b76af67e51e86f90358c561 Unwrap request factory when creating RestClient from RestTemplate
aha~ I understood. thanks for the kind explanation! I agree with your fix. it looks more explicit way! π