Repository: firebase/firebase-admin-java
Pull request #459 (2020-07-27 19:39:13)

fix: Handling http method override in ErrorHandlingHttpClient
Some HTTP methods (e.g. PATCH) requires some special handling due to the limitations in transport implementations like `NetHttpTransport`. This code is currently duplicated in several places. I'm moving this to `ErrorHandlingHttpClient` and `HttpRequestInfo` where it can be handled in a centralized manner.


Patch:
diff --git a/src/main/java/com/google/firebase/auth/internal/AuthHttpClient.java b/src/main/java/com/google/firebase/auth/internal/AuthHttpClient.java
index 70f9495..fe1c820 100644
--- a/src/main/java/com/google/firebase/auth/internal/AuthHttpClient.java
+++ b/src/main/java/com/google/firebase/auth/internal/AuthHttpClient.java
@@ -16,16 +16,12 @@
 
 package com.google.firebase.auth.internal;
 
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkNotNull;
-
 import com.google.api.client.http.GenericUrl;
 import com.google.api.client.http.HttpContent;
 import com.google.api.client.http.HttpRequestFactory;
 import com.google.api.client.http.HttpResponseInterceptor;
 import com.google.api.client.http.json.JsonHttpContent;
 import com.google.api.client.json.JsonFactory;
-import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.firebase.IncomingHttpResponse;
 import com.google.firebase.auth.FirebaseAuthException;
@@ -77,11 +73,8 @@ public final class AuthHttpClient {
 
   public IncomingHttpResponse sendRequest(
       String method, GenericUrl url, @Nullable Object content) throws FirebaseAuthException {
-    checkArgument(!Strings.isNullOrEmpty(method), "method must not be null or empty");
-    checkNotNull(url, "url must not be null");
-    String httpMethod = method.equals("PATCH") ? "POST" : method;
     HttpContent httpContent = content != null ? new JsonHttpContent(jsonFactory, content) : null;
-    HttpRequestInfo request = HttpRequestInfo.buildRequest(httpMethod, url, httpContent)
+    HttpRequestInfo request = HttpRequestInfo.buildRequest(method, url, httpContent)
         .addHeader(CLIENT_VERSION_HEADER, CLIENT_VERSION)
         .setResponseInterceptor(interceptor);
     if (method.equals("PATCH")) {
diff --git a/src/main/java/com/google/firebase/internal/HttpRequestInfo.java b/src/main/java/com/google/firebase/internal/HttpRequestInfo.java
index e92a76b..b7e00cc 100644
--- a/src/main/java/com/google/firebase/internal/HttpRequestInfo.java
+++ b/src/main/java/com/google/firebase/internal/HttpRequestInfo.java
@@ -44,9 +44,8 @@ public final class HttpRequestInfo {
 
   private HttpRequestInfo(String method, GenericUrl url, HttpContent content) {
     checkArgument(!Strings.isNullOrEmpty(method), "method must not be null");
-    checkNotNull(url, "url must not be null");
     this.method = method;
-    this.url = url;
+    this.url = checkNotNull(url, "url must not be null");
     this.content = content;
   }
 
@@ -77,16 +76,28 @@ public final class HttpRequestInfo {
     return buildRequest(HttpMethods.POST, url, content);
   }
 
-  public static HttpRequestInfo buildRequest(String method, String url, HttpContent content) {
+  public static HttpRequestInfo buildRequest(
+      String method, String url, @Nullable HttpContent content) {
     return buildRequest(method, new GenericUrl(url), content);
   }
 
-  public static HttpRequestInfo buildRequest(String method, GenericUrl url, HttpContent content) {
+  public static HttpRequestInfo buildRequest(
+      String method, GenericUrl url, @Nullable HttpContent content) {
     return new HttpRequestInfo(method, url, content);
   }
 
   HttpRequest newHttpRequest(HttpRequestFactory factory) throws IOException {
-    HttpRequest request = factory.buildRequest(method, url, content);
+    HttpRequest request;
+    if (factory.getTransport().supportsMethod(method)) {
+      request = factory.buildRequest(method, url, content);
+    } else {
+      // Some HttpTransport implementations (notably NetHttpTransport) don't support new methods
+      // like PATCH. We try to emulate such requests over POST by setting the method override
+      // header, which is recognized by most Google backend APIs.
+      request = factory.buildPostRequest(url, content);
+      request.getHeaders().set("X-HTTP-Method-Override", method);
+    }
+
     for (Map.Entry<String, String> entry : headers.entrySet()) {
       request.getHeaders().set(entry.getKey(), entry.getValue());
     }
diff --git a/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java b/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java
index e5bc94b..f954e2b 100644
--- a/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java
+++ b/src/main/java/com/google/firebase/messaging/FirebaseMessagingClientImpl.java
@@ -21,6 +21,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.api.client.googleapis.batch.BatchCallback;
 import com.google.api.client.googleapis.batch.BatchRequest;
+import com.google.api.client.googleapis.services.json.AbstractGoogleJsonClient;
 import com.google.api.client.http.GenericUrl;
 import com.google.api.client.http.HttpHeaders;
 import com.google.api.client.http.HttpMethods;
@@ -29,6 +30,7 @@ import com.google.api.client.http.HttpRequestFactory;
 import com.google.api.client.http.HttpRequestInitializer;
 import com.google.api.client.http.HttpResponseException;
 import com.google.api.client.http.HttpResponseInterceptor;
+import com.google.api.client.http.HttpTransport;
 import com.google.api.client.http.json.JsonHttpContent;
 import com.google.api.client.json.JsonFactory;
 import com.google.api.client.json.JsonObjectParser;
@@ -60,16 +62,10 @@ final class FirebaseMessagingClientImpl implements FirebaseMessagingClient {
 
   private static final String FCM_URL = "https://fcm.googleapis.com/v1/projects/%s/messages:send";
 
-  private static final String FCM_BATCH_URL = "https://fcm.googleapis.com/batch";
-
-  private static final String API_FORMAT_VERSION_HEADER = "X-GOOG-API-FORMAT-VERSION";
-
-  private static final String CLIENT_VERSION_HEADER = "X-Firebase-Client";
-
   private static final Map<String, String> COMMON_HEADERS =
       ImmutableMap.of(
-          API_FORMAT_VERSION_HEADER, "2",
-          CLIENT_VERSION_HEADER, "fire-admin-java/" + SdkUtils.getVersion());
+          "X-GOOG-API-FORMAT-VERSION", "2",
+          "X-Firebase-Client", "fire-admin-java/" + SdkUtils.getVersion());
 
   private final String fcmSendUrl;
   private final HttpRequestFactory requestFactory;
@@ -78,6 +74,7 @@ final class FirebaseMessagingClientImpl implements FirebaseMessagingClient {
   private final HttpResponseInterceptor responseInterceptor;
   private final MessagingErrorHandler errorHandler;
   private final ErrorHandlingHttpClient<FirebaseMessagingException> httpClient;
+  private final MessagingBatchClient batchClient;
 
   private FirebaseMessagingClientImpl(Builder builder) {
     checkArgument(!Strings.isNullOrEmpty(builder.projectId));
@@ -89,6 +86,7 @@ final class FirebaseMessagingClientImpl implements FirebaseMessagingClient {
     this.errorHandler = new MessagingErrorHandler(this.jsonFactory);
     this.httpClient = new ErrorHandlingHttpClient<>(
         this.requestFactory, this.jsonFactory, this.errorHandler);
+    this.batchClient = new MessagingBatchClient(requestFactory.getTransport(), jsonFactory);
   }
 
   @VisibleForTesting
@@ -141,7 +139,8 @@ final class FirebaseMessagingClientImpl implements FirebaseMessagingClient {
       batch.execute();
       return new BatchResponseImpl(callback.getResponses());
     } catch (HttpResponseException e) {
-      OutgoingHttpRequest req = new OutgoingHttpRequest(HttpMethods.POST, FCM_BATCH_URL);
+      OutgoingHttpRequest req = new OutgoingHttpRequest(
+          HttpMethods.POST, MessagingBatchClient.FCM_BATCH_URL);
       IncomingHttpResponse resp = new IncomingHttpResponse(e, req);
       throw errorHandler.handleHttpResponseException(e, resp);
     } catch (IOException e) {
@@ -152,10 +151,7 @@ final class FirebaseMessagingClientImpl implements FirebaseMessagingClient {
   private BatchRequest newBatchRequest(
       List<Message> messages, boolean dryRun, MessagingBatchCallback callback) throws IOException {
 
-    BatchRequest batch = new BatchRequest(
-        requestFactory.getTransport(), getBatchRequestInitializer());
-    batch.setBatchUrl(new GenericUrl(FCM_BATCH_URL));
-
+    BatchRequest batch = batchClient.batch(getBatchRequestInitializer());
     final JsonObjectParser jsonParser = new JsonObjectParser(this.jsonFactory);
     final GenericUrl sendUrl = new GenericUrl(fcmSendUrl);
     for (Message message : messages) {
@@ -319,4 +315,33 @@ final class FirebaseMessagingClientImpl implements FirebaseMessagingClient {
       return new MessagingServiceErrorResponse();
     }
   }
+
+  private static class MessagingBatchClient extends AbstractGoogleJsonClient {
+
+    private static final String FCM_ROOT_URL = "https://fcm.googleapis.com";
+    private static final String FCM_BATCH_PATH = "batch";
+    private static final String FCM_BATCH_URL = String.format(
+        "%s/%s", FCM_ROOT_URL, FCM_BATCH_PATH);
+
+    MessagingBatchClient(HttpTransport transport, JsonFactory jsonFactory) {
+      super(new Builder(transport, jsonFactory));
+    }
+
+    private MessagingBatchClient(Builder builder) {
+      super(builder);
+    }
+
+    private static class Builder extends AbstractGoogleJsonClient.Builder {
+      Builder(HttpTransport transport, JsonFactory jsonFactory) {
+        super(transport, jsonFactory, FCM_ROOT_URL, "", null, false);
+        setBatchPath(FCM_BATCH_PATH);
+        setApplicationName("fire-admin-java");
+      }
+
+      @Override
+      public AbstractGoogleJsonClient build() {
+        return new MessagingBatchClient(this);
+      }
+    }
+  }
 }
diff --git a/src/main/java/com/google/firebase/projectmanagement/HttpHelper.java b/src/main/java/com/google/firebase/projectmanagement/HttpHelper.java
index a9282ea..788dde4 100644
--- a/src/main/java/com/google/firebase/projectmanagement/HttpHelper.java
+++ b/src/main/java/com/google/firebase/projectmanagement/HttpHelper.java
@@ -16,11 +16,11 @@
 
 package com.google.firebase.projectmanagement;
 
+import com.google.api.client.http.HttpMethods;
 import com.google.api.client.http.HttpRequestFactory;
 import com.google.api.client.http.HttpResponseInterceptor;
 import com.google.api.client.http.json.JsonHttpContent;
 import com.google.api.client.json.JsonFactory;
-import com.google.common.annotations.VisibleForTesting;
 import com.google.firebase.FirebaseException;
 import com.google.firebase.IncomingHttpResponse;
 import com.google.firebase.internal.AbstractPlatformErrorHandler;
@@ -30,8 +30,6 @@ import com.google.firebase.internal.SdkUtils;
 
 final class HttpHelper {
 
-  @VisibleForTesting static final String PATCH_OVERRIDE_KEY = "X-HTTP-Method-Override";
-  @VisibleForTesting static final String PATCH_OVERRIDE_VALUE = "PATCH";
   private static final String CLIENT_VERSION_HEADER = "X-Client-Version";
 
   private final String clientVersion = "Java/Admin/" + SdkUtils.getVersion();
@@ -81,9 +79,8 @@ final class HttpHelper {
       T parsedResponseInstance,
       String requestIdentifier,
       String requestIdentifierDescription) throws FirebaseProjectManagementException {
-    HttpRequestInfo baseRequest = HttpRequestInfo.buildPostRequest(
-        url, new JsonHttpContent(jsonFactory, payload));
-    baseRequest.addHeader(PATCH_OVERRIDE_KEY, PATCH_OVERRIDE_VALUE);
+    HttpRequestInfo baseRequest = HttpRequestInfo.buildRequest(
+        HttpMethods.PATCH, url, new JsonHttpContent(jsonFactory, payload));
     makeRequest(
         baseRequest, parsedResponseInstance, requestIdentifier, requestIdentifierDescription);
   }


Test patch:
diff --git a/src/test/java/com/google/firebase/auth/FirebaseUserManagerTest.java b/src/test/java/com/google/firebase/auth/FirebaseUserManagerTest.java
index a71280b..38c3166 100644
--- a/src/test/java/com/google/firebase/auth/FirebaseUserManagerTest.java
+++ b/src/test/java/com/google/firebase/auth/FirebaseUserManagerTest.java
@@ -2765,13 +2765,7 @@ public class FirebaseUserManagerTest {
 
   private static void checkUrl(TestResponseInterceptor interceptor, String method, String url) {
     HttpRequest request = interceptor.getResponse().getRequest();
-    if (method.equals("PATCH")) {
-      assertEquals("PATCH",
-          request.getHeaders().getFirstHeaderStringValue("X-HTTP-Method-Override"));
-      assertEquals("POST", request.getRequestMethod());
-    } else {
-      assertEquals(method, request.getRequestMethod());
-    }
+    assertEquals(method, request.getRequestMethod());
     assertEquals(url, request.getUrl().toString().split("\\?")[0]);
   }
 
diff --git a/src/test/java/com/google/firebase/auth/multitenancy/FirebaseTenantClientTest.java b/src/test/java/com/google/firebase/auth/multitenancy/FirebaseTenantClientTest.java
index 86eb6b4..ec9cbe1 100644
--- a/src/test/java/com/google/firebase/auth/multitenancy/FirebaseTenantClientTest.java
+++ b/src/test/java/com/google/firebase/auth/multitenancy/FirebaseTenantClientTest.java
@@ -308,13 +308,7 @@ public class FirebaseTenantClientTest {
 
   private static void checkUrl(TestResponseInterceptor interceptor, String method, String url) {
     HttpRequest request = interceptor.getResponse().getRequest();
-    if (method.equals("PATCH")) {
-      assertEquals("PATCH",
-          request.getHeaders().getFirstHeaderStringValue("X-HTTP-Method-Override"));
-      assertEquals("POST", request.getRequestMethod());
-    } else {
-      assertEquals(method, request.getRequestMethod());
-    }
+    assertEquals(method, request.getRequestMethod());
     assertEquals(url, request.getUrl().toString().split("\\?")[0]);
   }
 
diff --git a/src/test/java/com/google/firebase/internal/ErrorHandlingHttpClientTest.java b/src/test/java/com/google/firebase/internal/ErrorHandlingHttpClientTest.java
index 31cc1d0..2f0490b 100644
--- a/src/test/java/com/google/firebase/internal/ErrorHandlingHttpClientTest.java
+++ b/src/test/java/com/google/firebase/internal/ErrorHandlingHttpClientTest.java
@@ -38,6 +38,7 @@ import com.google.api.client.util.GenericData;
 import com.google.auth.oauth2.AccessToken;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.firebase.ErrorCode;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.FirebaseException;
@@ -132,6 +133,32 @@ public class ErrorHandlingHttpClientTest {
     assertEquals("{\"key\":\"value\"}", out.toString());
   }
 
+  @Test
+  public void testUnsupportedMethod() throws FirebaseException, IOException {
+    MockHttpTransport transport = new MockHttpTransport.Builder()
+        .setLowLevelHttpResponse(new MockLowLevelHttpResponse().setContent("{}"))
+        .setSupportedMethods(ImmutableSet.of(HttpMethods.GET, HttpMethods.POST))
+        .build();
+    ErrorHandlingHttpClient<FirebaseException> client = new ErrorHandlingHttpClient<>(
+        transport.createRequestFactory(), DEFAULT_JSON_FACTORY, new TestHttpErrorHandler());
+    HttpRequestInfo patchRequest = HttpRequestInfo.buildRequest(
+        HttpMethods.PATCH,
+        "https://firebase.google.com",
+        new JsonHttpContent(DEFAULT_JSON_FACTORY, ImmutableMap.of("key", "value")));
+    TestResponseInterceptor interceptor = new TestResponseInterceptor();
+    patchRequest.setResponseInterceptor(interceptor);
+
+    client.sendAndParse(patchRequest, GenericData.class);
+
+    HttpRequest last = interceptor.getLastRequest();
+    assertEquals(HttpMethods.POST, last.getRequestMethod());
+    assertEquals(HttpMethods.PATCH, last.getHeaders().get("X-HTTP-Method-Override"));
+
+    ByteArrayOutputStream out = new ByteArrayOutputStream();
+    last.getContent().writeTo(out);
+    assertEquals("{\"key\":\"value\"}", out.toString());
+  }
+
   @Test
   public void testNetworkError() {
     final IOException exception = new IOException("Test");
diff --git a/src/test/java/com/google/firebase/projectmanagement/FirebaseProjectManagementServiceImplTest.java b/src/test/java/com/google/firebase/projectmanagement/FirebaseProjectManagementServiceImplTest.java
index 0c4cfca..8d227ef 100644
--- a/src/test/java/com/google/firebase/projectmanagement/FirebaseProjectManagementServiceImplTest.java
+++ b/src/test/java/com/google/firebase/projectmanagement/FirebaseProjectManagementServiceImplTest.java
@@ -18,8 +18,6 @@ package com.google.firebase.projectmanagement;
 
 import static com.google.firebase.projectmanagement.FirebaseProjectManagementServiceImpl.FIREBASE_PROJECT_MANAGEMENT_URL;
 import static com.google.firebase.projectmanagement.FirebaseProjectManagementServiceImpl.MAXIMUM_LIST_APPS_PAGE_SIZE;
-import static com.google.firebase.projectmanagement.HttpHelper.PATCH_OVERRIDE_KEY;
-import static com.google.firebase.projectmanagement.HttpHelper.PATCH_OVERRIDE_VALUE;
 import static com.google.firebase.projectmanagement.ShaCertificateType.SHA_1;
 import static com.google.firebase.projectmanagement.ShaCertificateType.SHA_256;
 import static org.junit.Assert.assertEquals;
@@ -530,11 +528,10 @@ public class FirebaseProjectManagementServiceImplTest {
         "%s/v1beta1/projects/-/iosApps/%s?update_mask=display_name",
         FIREBASE_PROJECT_MANAGEMENT_URL,
         IOS_APP_ID);
-    checkRequestHeader(expectedUrl, HttpMethod.POST);
+    checkRequestHeader(expectedUrl, HttpMethod.PATCH);
     ImmutableMap<String, String> payload =
         ImmutableMap.<String, String>builder().put("display_name", DISPLAY_NAME).build();
     checkRequestPayload(payload);
-    checkPatchRequest();
   }
 
   @Test
@@ -548,11 +545,10 @@ public class FirebaseProjectManagementServiceImplTest {
         "%s/v1beta1/projects/-/iosApps/%s?update_mask=display_name",
         FIREBASE_PROJECT_MANAGEMENT_URL,
         IOS_APP_ID);
-    checkRequestHeader(expectedUrl, HttpMethod.POST);
+    checkRequestHeader(expectedUrl, HttpMethod.PATCH);
     ImmutableMap<String, String> payload =
         ImmutableMap.<String, String>builder().put("display_name", DISPLAY_NAME).build();
     checkRequestPayload(payload);
-    checkPatchRequest();
   }
 
   @Test
@@ -872,11 +868,10 @@ public class FirebaseProjectManagementServiceImplTest {
         "%s/v1beta1/projects/-/androidApps/%s?update_mask=display_name",
         FIREBASE_PROJECT_MANAGEMENT_URL,
         ANDROID_APP_ID);
-    checkRequestHeader(expectedUrl, HttpMethod.POST);
+    checkRequestHeader(expectedUrl, HttpMethod.PATCH);
     ImmutableMap<String, String> payload =
         ImmutableMap.<String, String>builder().put("display_name", DISPLAY_NAME).build();
     checkRequestPayload(payload);
-    checkPatchRequest();
   }
 
   @Test
@@ -890,11 +885,10 @@ public class FirebaseProjectManagementServiceImplTest {
         "%s/v1beta1/projects/-/androidApps/%s?update_mask=display_name",
         FIREBASE_PROJECT_MANAGEMENT_URL,
         ANDROID_APP_ID);
-    checkRequestHeader(expectedUrl, HttpMethod.POST);
+    checkRequestHeader(expectedUrl, HttpMethod.PATCH);
     ImmutableMap<String, String> payload =
         ImmutableMap.<String, String>builder().put("display_name", DISPLAY_NAME).build();
     checkRequestPayload(payload);
-    checkPatchRequest();
   }
 
   @Test
@@ -1147,21 +1141,11 @@ public class FirebaseProjectManagementServiceImplTest {
     assertEquals(expected, parsed);
   }
 
-  private void checkPatchRequest() {
-    assertEquals(
-        "The number of HttpResponses is not equal to 1.", 1, interceptor.getNumberOfResponses());
-    checkPatchRequest(0);
-  }
-
-  private void checkPatchRequest(int index) {
-    assertTrue(interceptor.getResponse(index).getRequest().getHeaders()
-        .getHeaderStringValues(PATCH_OVERRIDE_KEY).contains(PATCH_OVERRIDE_VALUE));
-  }
-
   private enum HttpMethod {
     GET,
     POST,
-    DELETE
+    DELETE,
+    PATCH,
   }
 
   /**