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

Commit

Permalink
fix!: Fix PATCH being unsupported (#1465)
Browse files Browse the repository at this point in the history
  • Loading branch information
@vam-google
vam-google committed Aug 23, 2021
1 parent dcfbd93 commit 2c6ac24
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 14 deletions.
40 changes: 38 additions & 2 deletions gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpRequestRunnable.java
View file
Expand Up @@ -33,6 +33,7 @@
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.HttpMethods;
import com.google.api.client.http.HttpRequest;
import com.google.api.client.http.HttpRequestFactory;
import com.google.api.client.http.HttpResponse;
Expand Down Expand Up @@ -112,15 +113,50 @@ HttpRequest createHttpRequest() throws IOException {
}
}

HttpRequest httpRequest =
requestFactory.buildRequest(getApiMethodDescriptor().getHttpMethod(), url, jsonHttpContent);
HttpRequest httpRequest = buildRequest(requestFactory, url, jsonHttpContent);

for (HttpJsonHeaderEnhancer enhancer : getHeaderEnhancers()) {
enhancer.enhance(httpRequest.getHeaders());
}
httpRequest.setParser(new JsonObjectParser(getJsonFactory()));
return httpRequest;
}

private HttpRequest buildRequest(
HttpRequestFactory requestFactory, GenericUrl url, HttpContent jsonHttpContent)
throws IOException {
// A workaround to support PATCH request. This assumes support of "X-HTTP-Method-Override"
// header on the server side, which GCP services usually do.
//
// Long story short, the problems is as follows: gax-httpjson depends on NetHttpTransport class
// from google-http-client, which depends on JDK standard java.net.HttpUrlConnection, which does
// not support PATCH http method.
//
// It is a won't fix for JDK8: https://bugs.openjdk.java.net/browse/JDK-8207840.
// A corresponding google-http-client issue:
// https://github.com/googleapis/google-http-java-client/issues/167
//
// In JDK11 there is java.net.http.HttpRequest with PATCH method support but, gax-httpjson must
// remain compatible with Java 8.
//
// Using "X-HTTP-Method-Override" header is probably the cleanest way to fix it. Other options
// would be: hideous reflection hacks (not a safe option in a generic library, which
// gax-httpjson is), writing own implementation of HttpUrlConnection (fragile and a lot of
// work), depending on v2.ApacheHttpTransport (it has many extra dependencies, does not support
// mtls etc).
String actualHttpMethod = getApiMethodDescriptor().getHttpMethod();
String originalHttpMethod = actualHttpMethod;
if (HttpMethods.PATCH.equals(actualHttpMethod)) {
actualHttpMethod = HttpMethods.POST;
}
HttpRequest httpRequest = requestFactory.buildRequest(actualHttpMethod, url, jsonHttpContent);
if (originalHttpMethod != null && !originalHttpMethod.equals(actualHttpMethod)) {
HttpHeadersUtils.setHeader(
httpRequest.getHeaders(), "X-HTTP-Method-Override", originalHttpMethod);
}
return httpRequest;
}

// This will be frequently executed, so avoiding using regexps if not necessary.
private String normalizeEndpoint(String endpoint) {
String normalized = endpoint;
Expand Down
57 changes: 47 additions & 10 deletions gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpRequestRunnableTest.java
View file
Expand Up @@ -56,10 +56,8 @@ public class HttpRequestRunnableTest {
private static HttpJsonCallOptions fakeCallOptions;
private static CatMessage catMessage;
private static final String ENDPOINT = "https://www.googleapis.com/animals/v1/projects/";
private static HttpRequestRunnable httpRequestRunnable;
private static HttpRequestFormatter<CatMessage> catFormatter;
private static HttpResponseParser<EmptyMessage> catParser;
private static ApiMethodDescriptor<CatMessage, EmptyMessage> methodDescriptor;
private static PathTemplate nameTemplate = PathTemplate.create("name/{name}");
private static Set<String> queryParams =
Sets.newTreeSet(Lists.newArrayList("food", "size", "gibberish"));
Expand Down Expand Up @@ -139,29 +137,29 @@ public String serialize(EmptyMessage response) {
return null;
}
};
}

methodDescriptor =
@Test
public void testRequestUrl() throws IOException {
ApiMethodDescriptor<CatMessage, EmptyMessage> methodDescriptor =
ApiMethodDescriptor.<CatMessage, EmptyMessage>newBuilder()
.setFullMethodName("house.cat.get")
.setHttpMethod(null)
.setRequestFormatter(catFormatter)
.setResponseParser(catParser)
.build();

httpRequestRunnable =
HttpRequestRunnable<CatMessage, EmptyMessage> httpRequestRunnable =
HttpRequestRunnable.<CatMessage, EmptyMessage>newBuilder()
.setHttpJsonCallOptions(fakeCallOptions)
.setEndpoint(ENDPOINT)
.setRequest(catMessage)
.setApiMethodDescriptor(methodDescriptor)
.setHttpTransport(new MockHttpTransport())
.setJsonFactory(new GsonFactory())
.setResponseFuture(SettableApiFuture.<EmptyMessage>create())
.setResponseFuture(SettableApiFuture.create())
.build();
}

@Test
public void testRequestUrl() throws IOException {
HttpRequest httpRequest = httpRequestRunnable.createHttpRequest();
Truth.assertThat(httpRequest.getContent()).isInstanceOf(EmptyContent.class);
String expectedUrl = ENDPOINT + "name/feline" + "?food=bird&food=mouse&size=small";
Expand All @@ -170,21 +168,60 @@ public void testRequestUrl() throws IOException {

@Test
public void testRequestUrlUnnormalized() throws IOException {
httpRequestRunnable =
ApiMethodDescriptor<CatMessage, EmptyMessage> methodDescriptor =
ApiMethodDescriptor.<CatMessage, EmptyMessage>newBuilder()
.setFullMethodName("house.cat.get")
.setHttpMethod("PUT")
.setRequestFormatter(catFormatter)
.setResponseParser(catParser)
.build();

HttpRequestRunnable<CatMessage, EmptyMessage> httpRequestRunnable =
HttpRequestRunnable.<CatMessage, EmptyMessage>newBuilder()
.setHttpJsonCallOptions(fakeCallOptions)
.setEndpoint("www.googleapis.com/animals/v1/projects")
.setRequest(catMessage)
.setApiMethodDescriptor(methodDescriptor)
.setHttpTransport(new MockHttpTransport())
.setJsonFactory(new GsonFactory())
.setResponseFuture(SettableApiFuture.create())
.build();
HttpRequest httpRequest = httpRequestRunnable.createHttpRequest();
Truth.assertThat(httpRequest.getContent()).isInstanceOf(EmptyContent.class);
String expectedUrl =
"https://www.googleapis.com/animals/v1/projects/name/feline?food=bird&food=mouse&size=small";
Truth.assertThat(httpRequest.getUrl().toString()).isEqualTo(expectedUrl);
Truth.assertThat(httpRequest.getRequestMethod()).isEqualTo("PUT");
Truth.assertThat(httpRequest.getHeaders().get("X-HTTP-Method-Override")).isNull();
}

@Test
public void testRequestUrlUnnormalizedPatch() throws IOException {
ApiMethodDescriptor<CatMessage, EmptyMessage> methodDescriptor =
ApiMethodDescriptor.<CatMessage, EmptyMessage>newBuilder()
.setFullMethodName("house.cat.get")
.setHttpMethod("PATCH")
.setRequestFormatter(catFormatter)
.setResponseParser(catParser)
.build();

HttpRequestRunnable<CatMessage, EmptyMessage> httpRequestRunnable =
HttpRequestRunnable.<CatMessage, EmptyMessage>newBuilder()
.setHttpJsonCallOptions(fakeCallOptions)
.setEndpoint("www.googleapis.com/animals/v1/projects")
.setRequest(catMessage)
.setApiMethodDescriptor(methodDescriptor)
.setHttpTransport(new MockHttpTransport())
.setJsonFactory(new GsonFactory())
.setResponseFuture(SettableApiFuture.<EmptyMessage>create())
.setResponseFuture(SettableApiFuture.create())
.build();
HttpRequest httpRequest = httpRequestRunnable.createHttpRequest();
Truth.assertThat(httpRequest.getContent()).isInstanceOf(EmptyContent.class);
String expectedUrl =
"https://www.googleapis.com/animals/v1/projects/name/feline?food=bird&food=mouse&size=small";
Truth.assertThat(httpRequest.getUrl().toString()).isEqualTo(expectedUrl);
Truth.assertThat(httpRequest.getRequestMethod()).isEqualTo("POST");
Truth.assertThat(httpRequest.getHeaders().get("X-HTTP-Method-Override")).isEqualTo("PATCH");
}

// TODO(andrealin): test request body
Expand Down
11 changes: 9 additions & 2 deletions gax-httpjson/src/test/java/com/google/api/gax/httpjson/testing/MockHttpService.java
View file
Expand Up @@ -29,6 +29,7 @@
*/
package com.google.api.gax.httpjson.testing;

import com.google.api.client.http.HttpMethods;
import com.google.api.client.http.LowLevelHttpRequest;
import com.google.api.client.http.LowLevelHttpResponse;
import com.google.api.client.testing.http.MockHttpTransport;
Expand Down Expand Up @@ -100,8 +101,14 @@ public MockLowLevelHttpResponse getHttpResponse(String httpMethod, String fullTa
String relativePath = getRelativePath(fullTargetUrl);

for (ApiMethodDescriptor methodDescriptor : serviceMethodDescriptors) {
if (!httpMethod.equals(methodDescriptor.getHttpMethod())) {
continue;
// Check the comment in com.google.api.gax.httpjson.HttpRequestRunnable.buildRequest()
// method for details why it is needed.
String descriptorHttpMethod = methodDescriptor.getHttpMethod();
if (!httpMethod.equals(descriptorHttpMethod)) {
if (!(HttpMethods.PATCH.equals(descriptorHttpMethod)
&& HttpMethods.POST.equals(httpMethod))) {
continue;
}
}

PathTemplate pathTemplate = methodDescriptor.getRequestFormatter().getPathTemplate();
Expand Down

0 comments on commit 2c6ac24

Please sign in to comment.