Always add Content-Length header #648
Conversation
googlebot
added
the
cla: yes
|
PTAL |
| @@ -143,6 +143,9 @@ public String getFieldValue(String s) { | |||
| HttpRequest httpRequest = httpRequestRunnable.createHttpRequest(); | |||
| String expectedUrl = ENDPOINT + "name/tree_frog" + "?requestId=request57"; | |||
| Truth.assertThat(httpRequest.getUrl().toString()).isEqualTo(expectedUrl); | |||
| Truth.assertThat(httpRequest.getHeaders().getContentLength()) | |||
| .isEqualTo(httpRequest.getContent().getLength()); | |||
| Truth.assertThat(httpRequest.getHeaders().getContentLength()).isGreaterThan((long) 0); | |||
There was a problem hiding this comment.
Please use 0L literal instead of (long) 0
| @@ -110,10 +112,32 @@ HttpRequest createHttpRequest() throws IOException { | |||
| for (HttpJsonHeaderEnhancer enhancer : getHeaderEnhancers()) { | |||
| enhancer.enhance(httpRequest.getHeaders()); | |||
| } | |||
|
|
|||
| // Set Content-Length header to avoid 411s. | |||
| HttpJsonHeaderEnhancer contentLengthHeader = getContentLengthHeader(jsonHttpContent); | |||
There was a problem hiding this comment.
This is a little bit odd place for this. Few line earlier (L113) we already call the actual enhancers, but then, right after that we create one more on the fly just to call it once. Ideally we would want to have this enhancer as a default one, created in InstantiatingHttpJsonChannelProvider#createChannel() method (together with the other, static enhancers). The difference between this enhancer and the other ones (returned by getHeaderEnhancers() is that the other ones are static (do not depend on the actual body of the request) and this one is dynamic (does depend on the body, specifically on the size of the body).
I would suggest changing the HttpJsonHeaderEnhancer interface (it is @BetaApi, so should be ok) to accept the actual HttpRequest instead of HttpHeaders object.
Then create two enhancer implementations, one, which will be doing static headers (i.e. one single enhancer for all static headers) and one for dynamic headers, and instantiate both of those in InstantiatingHttpJsonChannelProvider#createChannel().
Then here, just call the enhancers normally in the loop (like on line 112), but pass the httpRequest as an argument (instead of httpRequest.getHeaders() as it is done now).
With the above suggestion in place, the HeaderEnhancers get their fair chunk of responsibility: setting headers (bot static and dynamic). Now they seem like be just doing a simple call to HttpHeadersUtils.setHeader() which is too little to justify existence of the small "family" of classes (the Enhancers class hierarchy).
There was a problem hiding this comment.
Removed all of this.
Codecov Report
@@ Coverage Diff @@
## master #648 +/- ##
===========================================
+ Coverage 75.45% 75.5% +0.05%
- Complexity 1000 1003 +3
===========================================
Files 185 185
Lines 4322 4332 +10
Branches 343 345 +2
===========================================
+ Hits 3261 3271 +10
+ Misses 906 905 -1
- Partials 155 156 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #648 +/- ##
===========================================
+ Coverage 75.45% 75.5% +0.05%
- Complexity 1000 1003 +3
===========================================
Files 185 185
Lines 4322 4332 +10
Branches 343 345 +2
===========================================
+ Hits 3261 3271 +10
+ Misses 906 905 -1
- Partials 155 156 +1
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #648 +/- ##
===========================================
+ Coverage 75.45% 75.5% +0.05%
- Complexity 1000 1003 +3
===========================================
Files 185 185
Lines 4322 4332 +10
Branches 343 345 +2
===========================================
+ Hits 3261 3271 +10
+ Misses 906 905 -1
- Partials 155 156 +1
Continue to review full report at Codecov.
|
|
@vam-google Hey Vadym, the previous rendition of the PR didn't actually work against my handwritten google-cloud-compute calls. I've scrapped all of that and made a simpler PR that does work against the repro steps listed in the google-cloud-java issue. PTAL |
Fixes googleapis/google-cloud-java#3735 and prevents 411 HTTP error codes.