Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

Always add Content-Length header #648

Merged
merged 2 commits into from
Jan 31, 2019
Merged

Always add Content-Length header #648

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c10d38b
add Content-Length header always
andreamlin Jan 29, 2019
257ba13
just set Content body to EmptyContent
andreamlin Jan 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpRequestRunnable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
package com.google.api.gax.httpjson;

import com.google.api.client.http.GenericUrl;
import com.google.api.client.http.HttpContent;
import com.google.api.client.http.HttpMediaType;
import com.google.api.client.http.HttpRequest;
import com.google.api.client.http.HttpRequestFactory;
Expand All @@ -51,6 +52,7 @@
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import javax.annotation.Nullable;

/** A runnable object that creates and executes an HTTP request. */
@AutoValue
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed all of this.

if (contentLengthHeader != null) {
contentLengthHeader.enhance(httpRequest.getHeaders());
}

httpRequest.setParser(new JsonObjectParser(getJsonFactory()));
return httpRequest;
}

@Nullable
private HttpJsonHeaderEnhancer getContentLengthHeader(@Nullable HttpContent requestBody) {
long contentLength = 0;
try {
if (requestBody != null) {
contentLength = requestBody.getLength();
}
} catch (IOException e) {
// Could not determine content length.
return null;
}

return HttpJsonHeaderEnhancers.create("Content-Length", String.valueOf(contentLength));
}

@Override
public void run() {
try {
Expand Down
3 changes: 3 additions & 0 deletions gax-httpjson/src/test/java/com/google/api/gax/httpjson/ApiMessageHttpRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use 0L literal instead of (long) 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this .


OutputStream outputStream = new PrintableOutputStream();
httpRequest.getContent().writeTo(outputStream);
Expand Down