Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TestRestTemplate replaces custom HttpClients #8697

Closed
abelsromero opened this issue Mar 22, 2017 · 13 comments
Closed

TestRestTemplate replaces custom HttpClients #8697

abelsromero opened this issue Mar 22, 2017 · 13 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@abelsromero
Copy link
Contributor

abelsromero commented Mar 22, 2017

Spring-Boot version 1.5.2.RELEASE

Problem:
When apache httpclient jar is in the classpath, TestRestTemplate replaces any HttpClient set by the user in a RestTemplate. Alseo, since the field is final there's no way to change that, even with reflection.
See this https://github.com/spring-projects/spring-boot/blob/master/spring-boot-test/src/main/java/org/springframework/boot/test/web/client/TestRestTemplate.java#L127-L130.

In my case, I am building a client for mutual authentication and some headers. However, the same applications uses httpclient for other integrations, not only that, previous tests were written in RestAssured which also includes that dependency.

Is it possible to remove that validation, make it optional or just apply it if there's no requestFactory set?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 22, 2017
@philwebb
Copy link
Member

Yeah, that's a bit nasty. You might be able to change it back afterwards until we can find a better fix:

RestTemplate rt = new RestTemplate();
TestRestTemplate trt = new TestRestTemplate(rt);
rt.setRequestFactory(...);

@philwebb philwebb added priority: normal type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 22, 2017
@philwebb philwebb added this to the 1.5.3 milestone Mar 22, 2017
@abelsromero
Copy link
Contributor Author

That's true! I did not think of that way to undo it.

Thanks a lot.

@snicoll snicoll changed the title RestTestTemplate replaces custom HttpClients TestRestTemplate replaces custom HttpClients Mar 28, 2017
@snicoll snicoll self-assigned this Apr 11, 2017
@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Apr 11, 2017
@snicoll
Copy link
Member

snicoll commented Apr 11, 2017

It really is tricky and I think the current code is a bit wrong but I can't see a way to fix it without breaking backward compat.

So one thing we could do is check if there are any HttpClientOptions before overriding the factory. If there aren't any, let's go with whatever we have. Also, rather than creating an empty RestTemplate like we do if none is provided, let's use the builder internally so that we benefit from the same algorithm that detects the factory to use.

If we do that, TestRestTemplate now follow redirects all the sudden. This is because our override of the request factory checks if the redirect option is set and if it isn't then the flag is false. Long story short, TestRestTemplate does not follow redirect while a sample RestTemplate does.

I don't know at this point if those inconsistencies are made on purpose but surely deviating from RestTemplate default doesn't sound very good to me. Not overriding the factory if no options is provided is also a bit fragile because we override it again if SSL is configured for instance.

@philwebb philwebb modified the milestones: 2.0.0.M1, 1.5.3 Apr 12, 2017
@philwebb philwebb self-assigned this Apr 12, 2017
@snicoll snicoll removed the for: team-attention An issue we'd like other members of the team to review label Apr 12, 2017
@philwebb philwebb modified the milestones: 2.0.0.M2, 2.0.0.M1 May 16, 2017
@binkley
Copy link

binkley commented Jun 5, 2017

Along these lines, I'm having difficulty with redirects in TestRestTemplate. Using 1.5.3.RELEASE I do not get a redirect for 302 (which is good: my test is specifically checking for this status code), but with 2.0.0.M1 I am getting a redirect.

I hunted for some way to customize the injected TestRestTemplate bean (using the @SpringBootTest(webEnvironment = RANDOM_PORT) annotation), or to replace it with one customized to not redirect, but didn't find what I needed.

The current docs say TestRestTemplate should not redirect, but in my test it is:

https://docs.spring.io/spring-boot/docs/current/reference/html/boot-features-testing.html#boot-features-rest-templates-test-utility

An example:

binkley/sproingk@49456e5

(See https://github.com/binkley/sproingk/blob/49456e50eb3c855ea6b284b9565337d593b63994/src/test/kotlin/hm/binkley/labs/GreetingControllerIT.kt#L140 and remove the @Disabled annotation.)

@wilkinsona
Copy link
Member

@binkley That sounds like a separate problem. Can you please open a new issue? AFAIK, both 1.5.3.RELEASE and 2.0.0.M1 will not follow a redirect as long as you have Apache's HTTP client on the class path.

@binkley
Copy link

binkley commented Jun 5, 2017

@wilkinsona OK, filed #9410. I do not explicitly ask for Apache HTTP client in my classpath, relying on Spring Boot starter to handle this.

@snicoll snicoll modified the milestones: 2.0.0.M2, 2.0.0.M3 Jun 16, 2017
@snicoll snicoll removed their assignment Jun 19, 2017
@wilkinsona wilkinsona modified the milestones: 2.0.0.M3, 2.0.0.M4 Jul 26, 2017
@snicoll snicoll removed this from the 2.0.0.M4 milestone Aug 23, 2017
@snicoll snicoll modified the milestones: 2.0.0.M5, 2.0.0.M4 Aug 23, 2017
@philwebb philwebb modified the milestones: 2.0.0.M5, 2.0.0.RC1, 2.0.0.M6 Sep 20, 2017
@philwebb philwebb modified the milestones: 2.0.0.M6, 2.0.0.RC1, 2.0.0.M7 Nov 1, 2017
@philwebb philwebb modified the milestones: 2.0.0.RC1, 2.0.0.RC2 Jan 17, 2018
@philwebb philwebb removed their assignment Feb 9, 2018
@bclozel
Copy link
Member

bclozel commented Feb 16, 2018

Maybe I'm missing something here, but #11872 removed all TestRestTemplate constructor variants using a RestTemplate argument. Is this issue gone, then?

@wilkinsona
Copy link
Member

wilkinsona commented Feb 16, 2018

No, it's still there. The private constructor will clobber any request factory that was configured via a RestTemplateBuilder.

@bclozel bclozel self-assigned this Feb 16, 2018
@abelsromero
Copy link
Contributor Author

So, the TL;DR; is to use a new builder in the new release?

@bclozel
Copy link
Member

bclozel commented Feb 16, 2018

Yes, we've removed all constructors accepting a RestTemplate argument - you can now use RestTemplateBuilder variants instead.

bclozel added a commit that referenced this issue Feb 16, 2018
This commit fixes two issues in `TestRestTemplate`:

* it improves the detection of the underlying request factory, using
reflection to look inside the intercepting request factory if
interceptors were configured

* it avoids reusing the same request factory when creating a new
`TestRestTemplate` with `withBasicAuth`. Sharing the same instance would
result in sharing authentication state (HTTP cookies). Since the
original request factory can't be detected consistently, a new one is
selected automatically

See gh-8697
@bvulaj
Copy link

bvulaj commented May 14, 2018

Am I right in assuming this issue was not resolved for 1.5.X? Autowiring TestRestTemplate into my integration tests, with a RestTemplateBuilder bean wired, still invokes a ctor that negates my customizations.

@philwebb
Copy link
Member

@bvulaj Unfortunately we needed to introduce a breaking change so 2.0 was the only option.

@bvulaj
Copy link

bvulaj commented May 15, 2018

@philwebb Understood. Will use the workaround for now. Thanks for the response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

8 participants