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

Error in RestTemplate when setting the same HTTP header through ClientHttpRequestInterceptor and HttpEntity [SPR-15066] #19632

Closed
spring-issuemaster opened this issue Dec 29, 2016 · 11 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Dec 29, 2016

Einar Pehrson [Atlassian] opened SPR-15066 and commented

When using HttpEntity to make a request with RestTemplate, the HttpHeaders passed to ClientHttpRequestInterceptors is mutable but with immutable values.

Steps to Reproduce

  1. Create a RestTemplate with a ClientHttpRequestInterceptor that adds a value for a particular header
  2. Create an HttpEntity with an HttpHeaders containing a different value for that same header
  3. Make a request using any of the RestTemplate methods that accepts an HttpEntity

Actual Results

An UnsupportedOperationException is thrown.

java.lang.UnsupportedOperationException
	at java.util.Collections$UnmodifiableCollection.add(Collections.java:1055)
	at org.springframework.http.HttpHeaders.add(HttpHeaders.java:1343)
	at ...intercept(...)
	at org.springframework.http.client.InterceptingClientHttpRequest$InterceptingRequestExecution.execute(InterceptingClientHttpRequest.java:85)
	at org.springframework.http.client.InterceptingClientHttpRequest.executeInternal(InterceptingClientHttpRequest.java:69)
	at org.springframework.http.client.AbstractBufferingClientHttpRequest.executeInternal(AbstractBufferingClientHttpRequest.java:48)
	at org.springframework.http.client.AbstractClientHttpRequest.execute(AbstractClientHttpRequest.java:53)
	at org.springframework.web.client.RestTemplate.doExecute(RestTemplate.java:666)
	at org.springframework.web.client.RestTemplate.execute(RestTemplate.java:627)
	at org.springframework.web.client.RestTemplate.exchange(RestTemplate.java:545)

Expected Results

The request is sent with both values for the header.


Affects: 4.3.5

Reference URL: #1277

Backported to: 4.3.14

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 29, 2016

Einar Pehrson [Atlassian] commented

Sébastien, thanks for triaging this issue. I couldn't assign it to myself, but I'm in the process of submitting a patch to fix this bug.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 29, 2016

Sébastien Deleuze commented

You're welcome. That said, I am not sure this is bug. The Javadoc explains you need to wrap the request in a HttpRequestWrapper to modify it.

Any thoughts?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 29, 2016

Einar Pehrson [Atlassian] commented

That crossed my mind, but I interpreted "Optionally wrap the request to filter HTTP attributes." as the wrapping being optional, since you can add additional HTTP headers without wrapping the request.

More importantly though, BasicAuthorizationInterceptor uses HttpHeaders#add(String, String) instead of wrapping the request.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 29, 2016

Sébastien Deleuze commented

Ok thanks for the PR.

Arjen Poutsma Could we have your feedback on this issue and the related PR?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 8, 2017

HerrDerb commented

Are there already any updates for this issue? I just ran into the same UnmodifiableCollection wall...

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 8, 2017

Sébastien Deleuze commented

After a deeper look, I agree that this half mutable HttpHeaders instance is probably not ideal, and I also think the intent is to provide a mutable headers to the interceptor. I have slightly modified the implementation.

Arjen Poutsma Can you please check that you are ok with the proposed changes?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 11, 2017

Einar Pehrson [Atlassian] commented

Sébastien, I see you've made use of part of my PR from a year ago. But the parts you left out fixed this bug for requests that have a body. Was that on purpose? I realize I only added a unit test for requests without a body.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 12, 2017

Sébastien Deleuze commented

Good point, I will fix that and add another test for that use case.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 13, 2017

Sébastien Deleuze commented

Done

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 15, 2017

Einar Pehrson [Atlassian] commented

Sébastien, I see that you have now incorporated all the changes from my PR 1277 in your commits 554662e and 73a81f9.

I have closed that PR, but could you please tell me what I should do differently to have my PR approved and merged, if I were to attempt to contribute to Spring Framework another time? Since there was never any feedback on my PR, I simply forgot about it until HerrDerb commented last week.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 16, 2017

Sébastien Deleuze commented

Einar Pehrson [Atlassian] No you did the right thing, this time I changed the implementation too much to reuse it directly. Thanks again for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.