Support dynamic default uri variables for RestClient

[quaff]

Closes GH-34189

[bclozel]

I'm declining this PR because I don't think we should take this approach to solve this problem.

The UriComponents infrastructure is shared for both RestClient and WebClient. They're both declaring defaultUriVariables(Map<String, ?> defaultUriVariables) methods that provide static values as a Map for the entire lifetime of the client. Supporting Function or Supplier means that such values will be calculated at runtime, possibly depending on the execution context. The background information in #34189 shows MDC usage which is a typical example of this. This will create concurrency and lifecycle problems as URLs might be built on a different thread.

I think this use case should be supported by ExchangeFilterFunction + reactor context for WebClient, and ClientHttpRequestInterceptor + request attributes for RestTemplate and RestClient.

[quaff]

This will create concurrency and lifecycle problems as URLs might be built on a different thread.

It's safe for RestClient and RestTemplate, It may be a problem for WebClient, but it's the developer's choice to or not to use Supplier/Function as default variable value.

I think this use case should be supported by ExchangeFilterFunction + reactor context for WebClient, and ClientHttpRequestInterceptor + request attributes for RestTemplate and RestClient.

Could you reopen #34189 or create new issue to address it officially?

[quaff]

Another thought, introduce a subclass of UriComponentsBuilder to accept Supplier/Function, let application construct their own DefaultUriBuilderFactory, for example:

UriComponentsBuilder ucb = UriComponentsBuilder.fromUriString("http://{partition}.163.com").acceptFunctions();
DefaultUriBuilderFactory  uriBuilderFactory = new DefaultUriBuilderFactory(ucb);
uriBuilderFactory.setDefaultUriVariables(defaultUriVariables);
RestClient restClient = RestClient.builder().uriBuilderFactory(uriBuilderFactory).build();

WDYT @bclozel

[bclozel]

Could you reopen #34189 or create new issue to address it officially?

I meant that this should be already possible with an interceptor and a better for for this use case.

[quaff]

Could you reopen #34189 or create new issue to address it officially?

I meant that this should be already possible with an interceptor and a better for for this use case.

I tried with ClientHttpRequestInterceptor:

		RestClient restClient = RestClient.builder().baseUrl("http://{partition}.example.com")
				.requestInterceptor(new ClientHttpRequestInterceptor() {
					@Override
					public ClientHttpResponse intercept(HttpRequest request, byte[] body,
							ClientHttpRequestExecution execution) throws IOException {
						return execution.execute(new HttpRequestWrapper(request) {
							@Override
							public URI getURI() {
								return UriComponentsBuilder.fromUri(request.getURI())
										.buildAndExpand(Map.of("partition", MDC.get("partition"))).toUri();
							}
						}, body);
					}
				}).build();


		restClient.get().uri("/index.html").retrieve().body(String.class)

It's not working, and not elegant, please rethink the solution I proposed.

Exception in thread "main" java.lang.IllegalArgumentException: Not enough variable values available to expand 'partition'
	at org.springframework.web.util.UriComponents$VarArgsTemplateVariables.getValue(UriComponents.java:370)
	at org.springframework.web.util.UriComponents.expandUriComponent(UriComponents.java:263)
	at org.springframework.web.util.HierarchicalUriComponents.expandInternal(HierarchicalUriComponents.java:438)
	at org.springframework.web.util.HierarchicalUriComponents.expandInternal(HierarchicalUriComponents.java:53)
	at org.springframework.web.util.UriComponents.expand(UriComponents.java:172)
	at org.springframework.web.util.DefaultUriBuilderFactory$DefaultUriBuilder.build(DefaultUriBuilderFactory.java:459)
	at org.springframework.web.util.DefaultUriBuilderFactory.expand(DefaultUriBuilderFactory.java:204)
	at org.springframework.web.client.DefaultRestClient$DefaultRequestBodyUriSpec.uri(DefaultRestClient.java:317)
	at org.springframework.web.client.DefaultRestClient$DefaultRequestBodyUriSpec.uri(DefaultRestClient.java:288)

[bclozel]

I don't think the uri template should leave a path variable in. The interceptor should change the host instead.

[quaff]

I don't think the uri template should leave a path variable in. The interceptor should change the host instead.

OK, it will works, but not elegant.

@Override
public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException {
	return execution.execute(new HttpRequestWrapper(request) {
		@Override
		public URI getURI() {
			return UriComponentsBuilder.fromUri(request.getURI()).host(MDC.get("partition") + "." + request.getURI().getHost()).build().toUri();
		}
	}, body);
}

[bclozel]

I'll discuss #34189 with the team.

[quaff]

I'll discuss #34189 with the team.

Sorry to disturb, any updates?