Allow multiple executions of ClientHttpRequestInterceptors

[Russell-Allen]

org.springframework.http.client.ClientHttpRequestInterceptor javadoc describes the interceptor as a "chain", and I assumed that the ClientHttpRequestExecution execution argument was a reference to the next link/interceptor in the chain (or the terminal request execution.)

The above assumption fails if the interceptor makes more than one call to execution.execute(request,body). That is, all calls after the first will skip any following interceptors.

Issue Demo Code

import org.springframework.http.client.ClientHttpRequestInterceptor;
import org.springframework.web.client.RestClient;

public class ClientHttpRequestInterceptor_IssueDemo {

  /**
   * Example interceptor that induces the issue by calling the ClientHttpRequestExecution#execute method
   * more than once.
   */
  private static final ClientHttpRequestInterceptor REPEATING_INTERCEPTOR = (request, body, execution) -> {
    System.out.println("Request 1...");
    execution.execute(request, body);
    System.out.println("Request 2...");
    execution.execute(request, body);
    System.out.println("Request 3...");
    return execution.execute(request, body);
  };

  /**
   * Our last interceptor just logs the request being executed.
   */
  private static final ClientHttpRequestInterceptor LOG_INTERCEPTOR = (request, body, execution) -> {
    System.out.println("Executing request.");
    return execution.execute(request, body);
  };

  public static void main(String[] args) {
    RestClient.builder()
        .requestInterceptor(REPEATING_INTERCEPTOR)
        .requestInterceptor(LOG_INTERCEPTOR)
        .build()
        .get().uri("https://www.google.com")
        .retrieve()
        .toEntity(String.class);

    /*
        Actual:
            Request 1...
            Executing request.
            Request 2...
            Request 3...
        Expected:
            Request 1...
            Executing request.
            Request 2...
            Executing request.
            Request 3...
            Executing request.
     */
  }

}

Root Cause?

I traced the issue down to the org.springframework.http.client.InterceptingClientHttpRequest.InterceptingRequestExecution class which is the type being passed into the interceptors as the execution argument. Here's an annotated snippet of that class showing the issue:

	private class InterceptingRequestExecution implements ClientHttpRequestExecution {

		private final Iterator<ClientHttpRequestInterceptor> iterator;

		public InterceptingRequestExecution() {
			this.iterator = interceptors.iterator();
		}

		@Override
		public ClientHttpResponse execute(HttpRequest request, byte[] body) throws IOException {
			if (this.iterator.hasNext()) {
				ClientHttpRequestInterceptor nextInterceptor = this.iterator.next();
				return nextInterceptor.intercept(request, body, this);
			}
			else {
			  ...  // terminal request execution here

The use of an iterator and iterator.hasNext() results in a visitor behavioral pattern instead of an execution chain.

My expected behavior for a chain pattern is that any node in the chain has a reference to the next node in the chain and that there is no state outside of the chain arguments. It seems quite odd that calls to execute the next interceptor in the chain has the side-effect of changing the next reference.

Example Use Case

Using a request interceptor, detect a request that results in a 401 Unauthorized response and replay the request with a refreshed token in the authorization header. For example:

  private static final ClientHttpRequestInterceptor AUTH_INTERCEPTOR = (request, body, execution) -> {
    // Presume existing token, previously inserted likely from cache, is still valid and allow the request to proceed...
    ClientHttpResponse response = execution.execute(request, body);
    // if the server responds with a 401 UNAUTHORIZED then refresh the token and try again (just once with expected good token)...
    if (response.getStatusCode().value() == 401) {
      response.close(); // be kind
      request.getHeaders().setBearerAuth(refreshTokenSupplier.get());
      response = execution.execute(request, body);
    }
    return response;
  };

Workarounds

1. Inject the RestClient into the repeating interceptor - If the interceptor had a reference to the otherwise fully configured rest client, it could use it to make follow on requests. Note that those requests would repeat the interceptor chain from the start instead of here forward. It also tightly couples the injected rest client to the interceptor; in fact, that injection would likely need to happen per request which would require the calling code to create an instance of the repeating interceptor (with a reference to the in flight rest client) and add that instance to the rest client (circular references!). This smells.
2. Don't do it. - Only call execute once, and then we can pretend it is a chain. But this means whatever business logic (like the auth example) was neatly solved in an interceptor now has to move up to the calling code. While we can of course keep our code DRY, this still requires every place where the behavior is desired to shim in a call to wherever that DRY behavior is located.
3. Fix it. - The closest accessible class is InterceptingClientHttpRequestFactory which is easy enough to replicate and have it use a fixed version of InterceptingClientHttpRequest, but ... I don't see a way to set my version into DefaultRestClient.interceptingRequestFactory; a lazily set field in a package-private final class. While I could use reflection to overcome the accessibility issues, its a brittle solution that will earn me the stink eye from peers.

[Russell-Allen]

I found a workaround. It's not an ideal solution, but it does change the behavior to be chain like.

Define and add this interceptor as the first interceptor in the chain:

  private static final ClientHttpRequestInterceptor FORCED_CHAINING = (request, body, terminalExecution) -> {
    ClientHttpRequestExecution first = terminalExecution;
    try {
      Class<?> targetClass = Class.forName("org.springframework.http.client.InterceptingClientHttpRequest$InterceptingRequestExecution");
      if (targetClass.isInstance(terminalExecution)) {
        Field iteratorField = targetClass.getDeclaredField("iterator");
        iteratorField.setAccessible(true);
        Iterator<ClientHttpRequestInterceptor> iterator = (Iterator<ClientHttpRequestInterceptor>) iteratorField.get(terminalExecution);

        // A chained version of ClientHttpRequestExecution
        class InterceptedExecution implements ClientHttpRequestExecution {
          ClientHttpRequestInterceptor interceptor;
          ClientHttpRequestExecution next;
          @Override public ClientHttpResponse execute(HttpRequest request, byte[] body) throws IOException {
            // when executing, run the interceptor and point it to the next intercepting execution or the terminal if no next.
            return interceptor.intercept(request, body, next == null ? terminalExecution : next);
          }
        }

        // Build the intercepting execution chain in iteration order
        InterceptedExecution previous = null;
        while (iterator.hasNext()) {
          InterceptedExecution current = new InterceptedExecution();
          current.interceptor = iterator.next();
          if (previous != null) previous.next = current; else first = current;
          previous = current;
        }
      }
    } catch (Exception ignored) {}
    return first.execute(request, body);
  };

Application code must add this interceptor as the first in the list, and it will then manage the execution of the remaining list items in chained fashion (followed by the terminal request execution.)

This interceptor will not affect request processing if the concrete spring class is not the specific target class of the workaround. However, this uses reflection to grab the private iterator using within the target spring class and this exhausts that iterator. That kills two birds with one stone; it causes the spring class to think there are no interceptors left for it to run and it allows this code to discover and build a proper chain structure for remaining interceptors.

Dropping this interceptor into the issue demo code results in the expected output.

[bclozel]

Hello @Russell-Allen and thanks for the detailed report.
The ClientHttpRequestInterceptor feature was introduced in Spring Framework 3.1 (see #12152) and while this behavior wasn't intentionally implemented, it's been like this for a long time and the team declined this enhancement request already because we didn't want to introduce a behavior change that might break people (see #32525).

Now we are working on the next 7.0 major generation and this change seems acceptable there.