RestClient calls interceptors from RestTemplate twice

[tilm4nn]

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

[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.

[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.

spring-framework/spring-web/src/main/java/org/springframework/http/client/support/InterceptingHttpAccessor.java

Line 102 in e1c8a84

factory = new InterceptingClientHttpRequestFactory(super.getRequestFactory(), interceptors);

1. RestTemplate.getRequestFactory() return InterceptingClientHttpRequestFactory(interceptors). it contains interceptors

spring-framework/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java

Lines 180 to 185 in e1c8a84

	this.requestFactory = restTemplate.getRequestFactory();
	this.messageConverters = new ArrayList<>(restTemplate.getMessageConverters());
	if (!CollectionUtils.isEmpty(restTemplate.getInterceptors())) {
	this.interceptors = new ArrayList<>(restTemplate.getInterceptors());
	}

2. But on RestClientBuilder(RestTemplate), we copy interceptors again that already contained in InterceptingClientHttpRequestFactory from restTemplate.getRequestFactory()

spring-framework/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java

Line 521 in e1c8a84

factory = new InterceptingClientHttpRequestFactory(DefaultRestClient.this.clientRequestFactory, DefaultRestClient.this.interceptors);

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

spring-framework/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java

Lines 183 to 185 in e1c8a84

	if (!CollectionUtils.isEmpty(restTemplate.getInterceptors())) {
	this.interceptors = new ArrayList<>(restTemplate.getInterceptors());
	}

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 🙇

[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:

1. 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)
2. 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.

[tilm4nn]

And here is another workaround that also works independent of how the RestTemplate is used otherwise:

RestClient.builder(restTemplate).requestInterceptors(List::clear).build();

[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! 😃

[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 c820e44.

[injae-kim]

c820e44 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! 👍