fix!: Fix PATCH being unsupported (#1465)

Author: vam-google

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpRequestRunnable.java

@@ -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;
@@ -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;

gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpRequestRunnableTest.java

@@ -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"));
@@ -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";
@@ -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

gax-httpjson/src/test/java/com/google/api/gax/httpjson/testing/MockHttpService.java

@@ -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;
@@ -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();