Support JDK HttpClient in ClientHttpRequestFactories

[poutsma]

This commit introduces support for the JdkClientHttpRequestFactory in ClientHttpRequestFactories.

Notes:

• On Java 11+, JdkClientHttpRequestFactory is preferred to the SimpleClientHttpRequestFactory if the java.net.http module is loaded. As a consequence, several tests that expect SimpleClientHttpRequestFactory fail, and because @ClassPathExclusions cannot be used in this situation, I am unsure how to proceed. Perhaps a new @ModuleExclusions annotation is necessary?
• As with Support Jetty in ClientHttpRequestFactories #36116, this PR uses code that will be in Spring Framework 6.1-M2, hence the snapshot version change in gradle.properties.

[wilkinsona]

I've opened #36120 so that @ClassPathExclusions can be used to exclude individual packages as well as whole jars from the classpath.

[spencergibb]

Any chance this can make it into boot 3.2.0-M1?

[wilkinsona]

I would hope so, yeah.

[poutsma]

@wilkinsona Thanks, using ClassPathExclusions.packages all tests are green.

[wilkinsona]

smoketest.actuator.ui.SampleActuatorUiApplicationTests.testCss() is failing with these changes. This is the test:

spring-boot/spring-boot-tests/spring-boot-smoke-tests/spring-boot-smoke-test-actuator-ui/src/test/java/smoketest/actuator/ui/SampleActuatorUiApplicationTests.java

Lines 58 to 63 in 9273a76

	@Test
	void testCss() {
	ResponseEntity<String> entity = this.restTemplate.getForEntity("/css/bootstrap.min.css", String.class);
	assertThat(entity.getStatusCode()).isEqualTo(HttpStatus.OK);
	assertThat(entity.getBody()).contains("body");
	}

Previously it used SimpleClientHttpRequestFactory but now it's using JdkClientHttpRequestFactory. Unlike the former, the latter does not follow redirects automatically. The request is redirected to /login by Spring Security and this means that the status code assertion now fails.

Given that anyone who was using SimpleClientHttpRequestFactory through Boot's auto-configuration is now very likely to use JdkClientHttpRequestFactory instead, I think this change in behavior could cause problems when people upgrade. I wonder if it's possible to configure the JDK's HTTP client to follow redirects by default as well.

[poutsma]

See https://docs.oracle.com/en/java/javase/11/docs/api/java.net.http/java/net/http/HttpClient.Builder.html#followRedirects(java.net.http.HttpClient.Redirect). The default is NEVER, do you think we should change that in framework to provide better compatibility with the other request factories?

[wilkinsona]

Yeah, I think so. Specifically, aligning with the behavior of SimpleClientHttpRequestFactory which enables redirects (for GET requests) feels like a good idea to me as many users of SimpleClientHttpRequestFactory will become users of JdkClientHttpRequestFactory once these changes are merged.

[poutsma]

The default HttpClient created by JdkClientHttpRequestFactory now follows redirects, see spring-projects/spring-framework@7c37f4b

[philwebb]

@poutsma Do you think it's worth adding a Framework test as well? It might be hard to catch regressions if SampleActuatorUiApplicationTests is the only thing catching this.

[wilkinsona]

Thanks for the redirect change, @poutsma. Unfortunately, I've now hit another problem that's related to redirects. This time it involves the set-cookie header.

We have a test that makes a POST request to /login. It receives a redirect response with, among others, these two headers:

location: http://localhost:53640/
set-cookie: SESSION=ZGYzMDhhM2ItOTk4Yy00OThkLTg2ZDktM2U1MmFiNDA2YmI3; Path=/; HttpOnly; SameSite=Lax

Unlike the simple client that only follows redirects for GET requests, this redirect is followed automatically and a GET request is made to http://localhost:53640/. No cookie header is sent as the HttpClient has no cookie manager configured. The response to this GET request has, among others, these headers:

location: http://localhost:53640/login
set-cookie: SESSION=NTI3YTUwMDgtMjIyNS00OTI1LWFkOWUtYTM3ZjVjYzkxNGFm; Path=/; HttpOnly; SameSite=Lax

This redirect is also followed so a GET request is now made to http://localhost:53640/login. Once again, no cookie header is sent. The response to this is a 200 with the HTML login page that has no set-cookie header in the response. This following of redirects prevents the test from getting the set-cookie header that was sent in response to the original POST request, making logging in impossible with the current HttpClient configuration.

It looks to me like we have two competing requirements that cannot both be satisfied. To match the behavior of the simple request factory we need to follow redirects but only for GET requests and that isn't supported by the new HttpClient. This leaves us in an awkward situation in Boot.

If we introduce support for JdkClientHttpRequestFactory that's used in preference to SimpleClientHttpRequestFactory I think there's a strong possibility that it will be a breaking change for those currently using SimpleClientHttpRequestFactory. If we prefer SimpleClientHttpRequestFactory to JdkClientHttpRequestFactory, the latter will never be chosen by auto-detection as the classes that SimpleClientHttpRequestFactory requires will always be present. It doesn't feel like there's a good answer here. Configuring the HttpClient with a cookie manager might help but I don't think it'll fully solve the problem.

[poutsma]

From a Framework perspective, I'm not that comfortable with changing more defaults in the JdkClientHttpRequestFactory

Would it make sense to have different defaults? SimpleClientHttpRequestFactory for RestTemplate, but JdkClientHttpRequestFactory for RestClient?

[nkonev]

Just five cents from an user
From my point of view, it's better to have an unified approach with JdkClientHttpRequestFactory for both RestTemplate and RestClient

It' s a new version (3.2), I think it's okay to change the behaviour

[wilkinsona]

From a Framework perspective, I'm not that comfortable with changing more defaults in the JdkClientHttpRequestFactory.

Understood. I'd prefer to align with Framework's defaults in Boot as well if we can, I imagine for similar reasons.

Would it make sense to have different defaults? SimpleClientHttpRequestFactory for RestTemplate, but JdkClientHttpRequestFactory for RestClient?

I'd prefer not to do that. I think it may be confusing and it would mean that ClientHttpRequestFactories needs to behave differently depending on the purposes of the caller.

If this comes down to a straight choice between HttpClient with all of its defaults and HttpClient with a NORMAL redirect policy, I think the former is slightly better. It feels less bad to me than the problem described above where access to the Set-Cookie header is lost when the response to the POST is automatically redirected. More objectively, it also brings us back to being fully aligned with the JDK's defaults. I find changing those defaults harder to justify when it's swapping one problem for another rather than solving/not causing either problem.

That will leave us with a problem in Boot for those who were using SimpleClientHttpRequestFactory through auto-detection. They change to JdkClientHttpRequestFactory is likely to cause some breaking but perhaps we can address that through the release notes. It feels to me to be the least bad option at the moment.

[poutsma]

If this comes down to a straight choice between HttpClient with all of its defaults and HttpClient with a NORMAL redirect policy, I think the former is slightly better. It feels less bad to me than the problem described above where access to the Set-Cookie header is lost when the response to the POST is automatically redirected. More objectively, it also brings us back to being fully aligned with the JDK's defaults. I find changing those defaults harder to justify when it's swapping one problem for another rather than solving/not causing either problem.

Agreed. I must admit that I already felt a bit bothered about changing the default redirect policy, because I don't think we change defaults in any other http client. ClientHttpRequestFactory classes are not meant to be drop-in replacements, they all have their own defaults. Changing those from our side essentially says: "We know better than the developer of the http client library", which is fine, but does require us to keep up to date with all the latests security updates and other best practices of said client library. I'd rather stick with the defaults.

That will leave us with a problem in Boot for those who were using SimpleClientHttpRequestFactory through auto-detection. They change to JdkClientHttpRequestFactory is likely to cause some breaking but perhaps we can address that through the release notes. It feels to me to be the least bad option at the moment.

Agreed.

[wilkinsona]

Thanks, @poutsma. Sounds like we're in agreement then and that, I assume, spring-projects/spring-framework@7c37f4b will be reverted.

[poutsma]

I assume, spring-projects/spring-framework@7c37f4b will be reverted.

Indeed.

[wilkinsona]

The Boot team discussed this a bit today and decided that we think our best option for now is to add support for JdkClientHttpRequestFactory without making it part of the auto-detection. This will allow people to easily opt into its use without having the change forced upon them. I've opened #36266 to make it easier to override the auto-detection. When that is implemented, we could then add JdkClientHttpRequestFactory to the auto-detection as it will be easy to override.