Ordering of WebClient.filter(s) [SPR-15657]

[spring-issuemaster]

Joe Grandja opened SPR-15657 and commented

Given this filter configuration for WebClient:

this.webClient
   .filter(filter1())
   .filter(filter2())
   .filter(filter3())
   .filter(filter4())

The expectation is that the filters would be applied in the following order: filter1, filter2, filter3, filter4. However, that is not the case, as they are applied in reverse order.

It seems a bit confusing compared to the way a Reactive stream is defined and executed in a top-down approach (the way the code reads).

Does it make sense to have the filters applied in the order they are defined - top-down approach?

---

Affects: 5.0 RC1

Issue Links:

• #20250 Simplify applying a strategy to requests from WebTestClient and WebTestClient

0 votes, 5 watchers

[spring-issuemaster]

Rossen Stoyanchev commented

I can certainly relate to the expectation, seeing what looks like a list of filters. Fundamentally however this is a decoration-based mechanism. If you look inisde, WebClient is only ever aware of the next ExchangeFilterFunction and the same is true for any filters along the way.

Remember also that filter can be called at different times. When using a decoration approach, as a filter is added that link becomes immutable and never has to change. If we were to follow the order of declaration if you later add filter5 it would have to be inserted between filter4 and the WebClient. So you lose immutability.

Another benefit of decoration is you can have the same WebClient instance used with a different set of filters. If I'm given a WebClient with filter1 and filter2, I can then decorate it with filterA and filterB and that won't affect any users of the WebClient passed to me (decorated with only filter1 and filter2).

Considering that the word "filter" carries a strong association to a chain of Servlet Filter's perhaps we can consider changing the name filter to something else? Especially since filter is also the name of the method that does actual filtering in ExchangeFilterFunction. Perhaps apply or compose, wdyt Arjen Poutsma?

[spring-issuemaster]

Joe Grandja commented

I think naming the method to compose might help, however, I feel the ordering is still not that clear to the user.

Here's a more concrete example:

this.webClient
	.filter(applyAuthorizationHeader())
	.filter(exchangeAndHandleAccessTokenErrorWithRefresh())

As a user, I might read this as follows:

1. applyAuthorizationHeader() is called first which set the bearer token in the Authorization header.
2. exchangeAndHandleAccessTokenErrorWithRefresh is called next which initiates the exchange (next.exchange) and evaluates the response for token errors. If there is an access token related error then initiate a new client request to the token endpoint to refresh the access token. Upon receiving the new access token, the original request is exchanged again with the new token in auth header.

But I will know quickly that this won't work because the applyAuthorizationHeader() is applied after exchangeAndHandleAccessTokenErrorWithRefresh() thus the bearer token is never set on the request and the refresh token will always get triggered.
To fix this, I switch the ordering and all is good. But it reads a little different and might not make sense...at least from my perspective.

I understand the ExchangeFilterFunction is using a decoration-based approach as I took a look at the code. It is convenient to allow new WebClient instances to leverage the existing configured filters, for example, an ExchangeFilterFunction that populates the Authorization header on the request. But there may be use cases that you want a new WebClient instance with all the configured settings except the configured filters. You may want to provide a new set of 1 or more filters that do not decorate the existing ones. Looking at the code, I'm not aware of how I can do this. Any suggestions?

[spring-issuemaster]

Rossen Stoyanchev commented

But there may be use cases that you want a new WebClient instance with all the configured settings except the configured filters.

Isn't this just:

WebClient webClientA = WebClient.create("http://localhost:8080");
// pass this where you want instance without filters...

WebClient webClientB = webClientA.filter(filter1).filter(filter2);
// pass this where you want instance with filters...

[spring-issuemaster]

Joe Grandja commented

Let's assume webClientA has 2 filters then webClientB gets those 2 and then is decorated with the 2 new ones.

What webClientB really wants is all the same configuration (for example, headers, base uri, etc) of webClientA but wants to configure a new set of filters without decorating the 2 that exist in webClientA

[spring-issuemaster]

Rossen Stoyanchev commented

I get that you want to be able to unwrap the target WebClient without any filters but I don't see how this has anything to do with filter ordering.

[spring-issuemaster]

Arjen Poutsma commented

Here's a more concrete example:

this.webClient
	.filter(applyAuthorizationHeader())
	.filter(exchangeAndHandleAccessTokenErrorWithRefresh())

You're missing an quite important piece in that sample: the assignment of the return value from filter. In order for that code to work, it has to be something like:

WebClient filteredClient = this.webClient
	.filter(applyAuthorizationHeader())
	.filter(exchangeAndHandleAccessTokenErrorWithRefresh())

or, to make it even clearer:

WebClient filteredClient = this.webClient.filter(applyAuthorizationHeader());
filteredClient = filtedClient.filter(exchangeAndHandleAccessTokenErrorWithRefresh());

here, you are creating a filtered client by applying the applyAuthorizationHeader filter, which in term is filtered with the exchangeAndHandleAccessTokenErrorWithRefresh filter. Looking at it this way, it becomes quite apparent to see why the filters are executed in this particular order.

The reason we took this approach, as Rossen already eluded to, is thread-safety: if filter would not return a filtered client, but instead would return void and register the filter in an internal filter list, then we would make WebClient mutable, opening the door to all kinds of thread-safety issues. Chaining functions like these to decorate immutable objects (like WebClient) is quite common in functional programming, but I can understand it takes some getting used to.

That said, I still think we can improve the overall filtering experience by making it part of WebClient's building process. Unlike WebClient itself, its builder is mutable, and therefore, if we add an addFilter(ExchangeFilterFunction) method to WebClient.Builder, we can store the registered filters in a list in the builder, and use that list to compose a filtered WebClient when WebClient.Builder.build() is called. So that we can get to something like this:

WebClient webClient = webClient.builder()
    .addFilter(filter1)
    .addFilter(filter2)
    .build();

where filter1 and filter2 are called in order.

Whether we want to keep WebClient.filter after adding WebClient.Builder.addFilter is a separate question: personally I think it is still useful to have.

As for getting access to the unfiltered WebClient: I see that as the responsibility of the user. If you can add a filter to a client, you can also keep a reference to the unfiltered client, like Rossen indicated in his comment.

[spring-issuemaster]

Arjen Poutsma commented

Considering that the word "filter" carries a strong association to a chain of Servlet Filter's perhaps we can consider changing the name filter to something else? Especially since filter is also the name of the method that does actual filtering in ExchangeFilterFunction. Perhaps apply or compose, wdyt Arjen Poutsma?

filter was the name suggested by Rob Winch when he requested this method. I am not against using a different name, but I do think filter is preferable to both apply and compose: those are a bit too generic for my taste. Also, apply is already used on the "other side" of things, see ExchangeFilterFunction.apply(ExchangeFunction).

[spring-issuemaster]

Arjen Poutsma commented

One more comment regarding the order of filter: note that you can also use andThen to compose two filters into one:

ExchangeFilterFunction filter = applyAuthorizationHeader().andThen(exchangeAndHandleAccessTokenErrorWithRefresh());
WebClient filteredClient = this.webClient.filter(filter);

Here the applyAuthorizationHeader filter is executed before the exchangeAndHandleAccessTokenErrorWithRefresh filter, as you expect.

[spring-issuemaster]

Joe Grandja commented

Rossen, in reference to your comment:

I get that you want to be able to unwrap the target WebClient without any filters but I don't see how this has anything to do with filter ordering.

This allows me to control the order of filters, by starting fresh and re-adding the filters I want in a specific order. If I reuse a WebClient with filter(s) already composed than I have less control and may want to place a filter before/after the one that is already composed within.

[spring-issuemaster]

Joe Grandja commented

Arjen, in reference to your comment:

You're missing an quite important piece in that sample: the assignment of the return value from filter. In order for that code to work, it has to be something like:

Totally aware of the required variable assignment of WebClient. I know filter() returns a new instance of WebClient with the new filter decorating the existing ones (if there are existing ones). I was only providing a minimal sample code snippet to start the ordering conversation.

I understand the reason for immutability and that totally makes sense. However, when filter is called then filter ordering can take effect whatever that strategy is and a new WebClient instance is returned to keep things immutable.

[spring-issuemaster]

Joe Grandja commented

Arjen, I forgot about the option of andThen for composition. This ordering is clear to me using that approach and may be what I end up using. Thanks for the tip.

[spring-issuemaster]

Rossen Stoyanchev commented

Arjen Poutsma I'm in favor of having addFilter(ExchangeFilterFunction) on the builder although I can see where it re-enforces Joe's perception issue since after using the builder you might expect that the filter method also works like a list.

I still think the filter method name is part of the issue. ExchangeFilterFunction#filter(request, next does actually filter. WebClient#filter(ExchangeFilterFunction) decorates the WebClient with a filter rather than filtering. Even if we compare to an operator like filter (or conceptually map) the issue again is this is right-to-left actually so . so perhaps filter is also too general. How about prependFilter to suggest more strongly we're decorating, or perhaps you have another idea along those lines?

[spring-issuemaster]

Rob Winch commented

I'm not actually that fond of filter myself. I think that was just the name that was acceptable on both sides. What about with?

[spring-issuemaster]

Arjen Poutsma commented

Here's what I suggest I do to resolve this issue: first, we add addFilter(ExchangeFilterFunction) to the builder. Then, I think we have two options:

• Remove WebClient.filter altogether, or
• Rename WebClient.filter to a different name.

I am starting to think that the issues we are seeing with filter's semantics, combined with the difficulty we are having to find a new name is a smell. After all, while it is common to see compositional operators on functional interfaces (like ExchangeFunction, HandlerFunction and java.util.function.*); it is less common to see them on a non-functional interface such as WebClient. And I think the compositionality might be the cause for all the confusion, as you don't expect it on an interface like WebClient.

So I suggest to drop WebClient.filter for now, in favour of adding addFilter(ExchangeFilterFunction) to the builder. Then we can see if this change is good enough, or if we still need WebClient.filter, though with a different name.

[spring-issuemaster]

Arjen Poutsma commented

Rossen Stoyanchev suggested during today's meeting that removing WebClient.filter would mean that users cannot add filters to an existing WebClient. In order to fix this, we should add a method to WebClient that returns a WebClient.Builder with the "current" configuration.

[spring-issuemaster]

Joe Grandja commented

I do like the option of adding the filters via the Builder with the expectation that they are called in list order. Also, the ability to return the builder with the current configuration of the WebClient is a good option as well.

I'm going to put out another option to consider.

Let's say we keep the filter(ExchangeFilterFunction) method but change it to this:

public WebClient withFilter(ExchangeFilterFunction filterFunction) {
		ExchangeFunction filteredExchangeFunction = this.exchangeFunction.andThen(filterFunction);

This would allow the ordering to be consistent between Builder.addFilter(ExchangeFilterFunction) and withFilter(ExchangeFilterFunction, in list order.

However, this means we would need to add the default method ExchangeFunction.andThen which I'm not sure is a good idea or not?

Either way, another option to consider.

[spring-issuemaster]

Arjen Poutsma commented

However, this means we would need to add the default method ExchangeFunction.andThen which I'm not sure is a good idea or not?

The ExchangeFunction represent the actual exchange from a client-side request to a future response. It is executed after all filters had an opportunity to manipulate the request, and as such is the last element in the filter chain. After the exchange, filters have an opportunity to manipulate the received response, in reverse order.

Adding a method ExchangeFunction andThen(ExchangeFilterFunction) to ExchangeFunction would imply that the filter is executed after the exchange function. As such, it would lose its capability to manipulate the request, as the exchange has already been executed by that point.

[spring-issuemaster]

Joe Grandja commented

Arjen, thanks for the explanation. That totally makes sense. I did not put much thought into my suggestion for adding ExchangeFunction.andThen. This obviously would not work.

Given the way function chaining works via filter(ExchangeFilterFunction), from right-to-left, it seems the only way to get the filters ordered in a list would be through the WebClient.Builder.build().

[spring-issuemaster]

Arjen Poutsma commented

Fixed in 4a0597d

[spring-issuemaster]

Arjen Poutsma commented

I wasn't sure if we needed WebTestClient.mutate() as well, given that it's not that hard to create a new WebTestClient for a different scenario. We can always introduce it later.

[spring-issuemaster]

Rob Winch commented

Arjen Poutsma Can we add the WebTestClient.mutate() method? The security tests heavily rely on a "per request" style use of a ExchangeFilterFunction on WebTestClient. I expect that many users of security will also have this requirement since they will be wanting to test with different permutations of ExchangeFilterFunction (i.e. different user combinations).

[spring-issuemaster]

Arjen Poutsma commented

robwinch Done, see a95cf07

[spring-issuemaster]

Artem Bilan commented

There is an NPE in the newly introduced headers(Consumer<HttpHeaders> headersConsumer). The getHeaders() should be called instead of direct this.headers usage.

[spring-issuemaster]

Arjen Poutsma commented

NPE fixed in 4f39edc