Prevent duplicated/conflicting HTTP headers for HurlStack #193
Conversation
Values returned by getHeaders() should take precedence over both the additionalHeaders passed into performRequest (which come from the cache layer) and any Content-Type header set from getPostBodyType(). This allows the client app to hold ultimate control of all headers via getHeaders().
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
@houtianze FYI - I am assuming your consent from the initial PR in #139 where your commit was sourced from. |
| map.putAll(additionalHeaders); | ||
| map.putAll(request.getHeaders()); |
There was a problem hiding this comment.
Probably worth adding a comment about the ordering of these putAll() calls.
There was a problem hiding this comment.
Is there a unit test that verifies this behavior?
There was a problem hiding this comment.
I added comments and a conformance test of sorts. (It would be neat if there were some way to export the conformance test so it'd be easy to run against other HttpStack implementations, but I'm not sure how to do that short of creating a whole new volley-test library).
|
PTAL - comments addressed. |
|
@jpd236 Sorry for the late reply, sure, I consent. |
In google#193 we moved setting of request headers to after setting the connection parameters (Content-Type header and request body). However, setting a request body (or more precisely, calling HttpUrlConection#getOutputStream) prevents us from setting additional headers, because the connection is opened already. Instead, we just skip setting the Content-Type header if one has already been set via Request#getHeaders. This is a bit more error-prone, but unlikely to change in the future, and hard to express in a cleaner way because with the DEPRECATED_GET_OR_POST method, we need to call getPostBody to determine if the request will be a GET or POST request, and we should only call this once. Verified on a real device (as Robolectric did not catch this). Fixes google#198
In #193 we moved setting of request headers to after setting the connection parameters (Content-Type header and request body). However, setting a request body (or more precisely, calling HttpUrlConection#getOutputStream) prevents us from setting additional headers, because the connection is opened already. Instead, we just skip setting the Content-Type header if one has already been set via Request#getHeaders. This is a bit more error-prone, but unlikely to change in the future, and hard to express in a cleaner way because with the DEPRECATED_GET_OR_POST method, we need to call getPostBody to determine if the request will be a GET or POST request, and we should only call this once. Verified on a real device (as Robolectric did not catch this). Fixes #198
See #139 for more discussion and investigation.