Dynamic decorator of the default header for `WebClient`

[tomatophobia]

Motivation:

• Currently, WebClient can only have static values as default headers. Therefore, whenever the headers need to change, there is an inconvenience of having to create a new request header each time.

Modifications:

• Added a method WebClientBuilder#defaultHeaderDecorator(CharSequence, Supplier<CompletableFuture<Object>>) that takes an argument name and creates the default header with a matching name.
• Added test code for the newly added method.

Result:

• Closes #5032.
• Since a decorator for dynamically changing headers is used, there is no need to recreate request headers manually each time.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 96.77419% with 3 lines in your changes missing coverage. Please review.

Project coverage is 74.04%. Comparing base (b8eb810) to head (17f4f7f). Report is 273 commits behind head on main.

:exclamation: Current head 17f4f7f differs from pull request most recent head 933c0a6

Please upload reports for the commit 933c0a6 to get more accurate results.

Files	Patch %	Lines
...linecorp/armeria/client/HeadersUpdatingClient.java	96.15%	2 Missing :warning:
...java/com/linecorp/armeria/common/HttpResponse.java	87.50%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5160      +/-   ##
============================================
+ Coverage     73.95%   74.04%   +0.09%     
- Complexity    20115    20814     +699     
============================================
  Files          1730     1803      +73     
  Lines         74161    76666    +2505     
  Branches       9465     9760     +295     
============================================
+ Hits          54847    56770    +1923     
- Misses        14837    15273     +436     
- Partials       4477     4623     +146     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[github-actions[bot]]

🔍 Build Scan® (commit: 933c0a63ee2a01f354e2295b658b4277d7e18646)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/zqloaloxmtdaw
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/qgnfy7we7lbae
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/s6v7bae5h2wj4
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/4rm4con5ejhfo
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/y5zuczbnnh7pg
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/v7daiihpehcii
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/o2pnd6fkmupnc
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/dlyfrc23zooni

[jrhee17]

Talked with @ikhoon

What do you think of:

• Instead of adding a method to WebClient, create a decorator HeadersUpdatingClient which updates headers of requests
• If a CompletableFuture fails, the request is failed (along with the corresponding ctx.requestLog
• Add addHeader[s]/setHeader[s] builder methods
• If a duplicate key exists, they are applied in order

e.g.

HeadersUpdatingClient
  .builder()
  .requestHeaders()
  .add(key, 'a')
  .addAsync(key, key -> CompletableFuture.complete('b'))
  .add(key, key -> 'c') // key is eventually 'c'
  .responseHeaders()
  .add...
  .newDecorator();

[tomatophobia]

@jrhee17 Thank you for the great suggestion! 👍

Is the following the correct order of precedence for applied headers?

1. Headers set at the moment of sending a request using WebClient.execute(RequestHeaders). (Highest)
2. Headers set using the decorator created with HeadersUpdatingClient.
3. Default static headers set using WebClientBuilder.addHeaders. (Lowest)

[jrhee17]

Is the following the correct order of precedence for applied headers?

There are a couple places where the header can be set, but more or less I think the priority you specified is correct.

• ClientFactory < WebClient < Request < ClientRequestContext

My assumption was that the Request level will be where the decorator will be modifying the header (maybe @ikhoon has a different idea though 😄 )

[ikhoon]

The default headers should have lower priority than the request headers. If we update request headers with .mapHeaders(), we can't call them default. How about adding a new API to mutate the existing default headers?

interface ClientRequestContext {
  void mutateDefaultRequestHeaders(Consumer<HttpHeadersBuilder> mutator);
}

mutateDefaultRequestHeaders() could be used by the headers updating decorator instead of .mapHeaders() for the dynamic headers.

public class WebClientBuilder {
  WebClientBuilder addHeaders(CharSequence name,
                              Supplier<CompletableFuture<Object>> supplier) {
    var decorator = (delegate, ctx, req) -> {
       supplier.get().handle(... -> {
          ctx.mutateDefaultRequestHeaders(builder -> builder.addObject(name, value)
       }
       ...
   });
   ...
}

[jrhee17]

mutateDefaultRequestHeaders() could be used by the headers updating decorator instead of .mapHeaders() for the dynamic headers.

I assume you are suggesting a completely different design from the discussed one.

I think the design is fine, but some points worth noting are:

• The order of setHeader(string, string) vs. setHeader(string, CF) may need to be considered
• We need to decide where to wait for the CF completion (I assume you are suggesting to add an "internal" decorator)

If these two comments are addressed, I think the direction you suggested is also fine.

[ikhoon]

I assume you are suggesting a completely different design from the discussed one.

Sorry for confusing you. I think it would be a good idea to focus on implementing HeadersUpdatingClient for this issue and think about dynamic default headers as a different issue.

What made me think about this issue was how I could set up a token that expires, such as an oauth2 token, in WebClient Builder. Because AuthToken is an immutable class, its value cannot be changed dynamically. I imagined that it would be easier to implement if WebClient provided AuthTokenProvider: () -> CompletableFuture<AuthToken> that has the same priority as AuthToken.

[jrhee17]

I imagined that it would be easier to implement if WebClient provided AuthTokenProvider: () -> CompletableFuture<AuthToken> that has the same priority as AuthToken.

I agree having such an API would be useful for users. Perhaps we can do something similar to how RedirectingClient is implemented.

We can deprecate ClientOptions#HEADERS and add a ClientOptions#HEADER_UPDATING_DECORATOR which WebClient#*header methods delegate to. (for compatibility, ClientOptions#HEADERS will also be applied)