fix: googphotos http request handling to be self-consistent (#1445)

factors out the common HTTP logic to a middle-tier private func
`makeHttpRequest` to get to a _mostly_ noop cleanup/DRYing of our HTTP
handling. (motivation: some issues we're debugging). Things that prevent
this from being just a true **noop** refactor:

- `makePostRequest` used to only respect its users' request for special
HTTP headers on the _first_ request, but not on subsequent
(token-refresh driven) requests. Now it'll respect it for all requests.
- similar to previous point: _rate limiting_ wasn't consistently
applied, now it's always applied for every write request, and not just
the first.
- similarly, check for HTTP 200 was only done on some request paths, now
it's always done; it's also now a consistent stack trace that will
hopefully be more useful (like the other stack traces we get for bad
responses).
- unlike the above/extant http-200 check (despite not getting an error),
other checks like permission-denied were not always done; now they're
done regardless of getting a response or an error

Author: jzacsh

extensions/data-transfer/portability-data-transfer-google/src/main/java/org/datatransferproject/datatransfer/google/photos/GooglePhotosInterface.java

@@ -23,6 +23,7 @@
 import com.google.api.client.http.ByteArrayContent;
 import com.google.api.client.http.GenericUrl;
 import com.google.api.client.http.HttpContent;
+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;
@@ -67,11 +68,13 @@
 
 // TODO (#1307): Find a way to consolidate all 3P API interfaces
 public class GooglePhotosInterface {
-
   public static final String ERROR_HASH_MISMATCH = "Hash mismatch";
   private static final String GOOG_ERROR_HASH_MISMATCH_LEGACY = "Checksum from header does not match received payload content.";
   private static final String GOOG_ERROR_HASH_MISMATCH_UNIFIED = "User-provided checksum does not match received payload content.";
 
+  private static final String GOOGPHOTOS_ALBUMS_PERMISSION_ERROR = "The caller does not have permission";
+  private static final String GOOGPHOTOS_PHOTO_PERMISSION_ERROR = "Google Photos is disabled for the user";
+
   private static final String BASE_URL = "https://photoslibrary.googleapis.com/v1/";
   private static final int ALBUM_PAGE_SIZE = 20; // TODO
   private static final int MEDIA_PAGE_SIZE = 50; // TODO
@@ -196,57 +199,42 @@ public BatchMediaItemResponse createPhotos(NewMediaItemUpload newMediaItemUpload
   private <T> T makeGetRequest(String url, Optional<Map<String, String>> parameters, Class<T> clazz)
       throws IOException, InvalidTokenException, PermissionDeniedException {
     HttpRequestFactory requestFactory = httpTransport.createRequestFactory();
-    HttpRequest getRequest =
-        requestFactory.buildGetRequest(
-            new GenericUrl(url + "?" + generateParamsString(parameters)));
 
     HttpResponse response;
     try {
-      response = getRequest.execute();
-    } catch (HttpResponseException e) {
-      response =
-          handleHttpResponseException(
-              () ->
+      response = makeHttpRequest(() ->
                   requestFactory.buildGetRequest(
-                      new GenericUrl(url + "?" + generateParamsString(parameters))),
-              e);
+                      new GenericUrl(url + "?" + generateParamsString(parameters))));
+    } catch (UploadErrorException e) {
+      throw new IllegalStateException("GET request unexpectedly produced Upload exception", e);
     }
 
-    Preconditions.checkState(response.getStatusCode() == 200);
     String result =
         CharStreams.toString(new InputStreamReader(response.getContent(), StandardCharsets.UTF_8));
     return objectMapper.readValue(result, clazz);
   }
 
-  public <T> T makePostRequest(String url, Optional<Map<String, String>> parameters,
-      Optional<Map<String, String>> extraHeaders, HttpContent httpContent, Class<T> clazz)
+  public <T> T makePostRequest(
+      String url,
+      Optional<Map<String, String>> parameters,
+      Optional<Map<String, String>> extraHeaders,
+      HttpContent httpContent,
+      Class<T> clazz)
       throws IOException, InvalidTokenException, PermissionDeniedException, UploadErrorException {
-    // Wait for write permit before making request
-    writeRateLimiter.acquire();
-
     HttpRequestFactory requestFactory = httpTransport.createRequestFactory();
-    HttpRequest postRequest =
+    HttpResponse response = makeHttpRequest(() -> {
+      // Wait for write permit before making request
+      writeRateLimiter.acquire();
+
+      HttpRequest postRequest =
         requestFactory.buildPostRequest(
             new GenericUrl(url + "?" + generateParamsString(parameters)), httpContent);
-    extraHeaders.ifPresent(stringStringMap -> stringStringMap.forEach(
-        (key, value) -> postRequest.getHeaders().set(key, value)));
-    postRequest.setReadTimeout(2 * 60000); // 2 minutes read timeout
-    HttpResponse response;
-
-    try {
-      response = postRequest.execute();
-    } catch (HttpResponseException e) {
-      maybeRethrowAsUploadError(e);
-
-      response =
-          handleHttpResponseException(
-              () ->
-                  requestFactory.buildPostRequest(
-                      new GenericUrl(url + "?" + generateParamsString(parameters)), httpContent),
-              e);
-    }
+      extraHeaders.ifPresent(stringStringMap -> stringStringMap.forEach(
+            (key, value) -> postRequest.getHeaders().set(key, value)));
+      postRequest.setReadTimeout(2 * 60000); // 2 minutes read timeout
+      return postRequest;
+    });
 
-    Preconditions.checkState(response.getStatusCode() == 200);
     String result =
         CharStreams.toString(new InputStreamReader(response.getContent(), StandardCharsets.UTF_8));
     if (clazz.isAssignableFrom(String.class)) {
@@ -256,55 +244,115 @@ public <T> T makePostRequest(String url, Optional<Map<String, String>> parameter
     }
   }
 
-  /**
-   * Converting {@link HttpResponseException} to upload-related exceptions. Current this is only
-   * used for payload hash verifications.
-   *
-   * Note that making this a separate method to avoid polluting throw lists.
-   */
-  private void maybeRethrowAsUploadError(HttpResponseException e) throws UploadErrorException {
-    if (e.getStatusCode() == 400) {
-      if (e.getContent().contains(GOOG_ERROR_HASH_MISMATCH_LEGACY) || e.getContent()
-          .contains(GOOG_ERROR_HASH_MISMATCH_UNIFIED)) {
-        throw new UploadErrorException(ERROR_HASH_MISMATCH, e);
+  private HttpResponse makeHttpRequest(SupplierWithIO<HttpRequest> httpRequest)
+  throws IOException, InvalidTokenException, PermissionDeniedException, UploadErrorException {
+
+    HttpResponse response = null;
+    HttpRequest firstReq = httpRequest.getWithIO();
+    try {
+      response = firstReq.execute();
+    } catch (HttpResponseException firstReqException) {
+      Optional<HttpResponse> maybeTokenRefreshedRetry = Optional.empty();
+      try {
+        maybeTokenRefreshedRetry =
+            maybeRetryWithFreshToken(httpRequest, firstReqException);
+      } catch (HttpResponseException tokenRefreshedRetryException) {
+        rethrowForDtpStandards(
+            tokenRefreshedRetryException.getStatusCode(),
+            Optional.of(tokenRefreshedRetryException),
+            Optional.empty() /*maybeResponse*/);
       }
-      // Delegate other 400 errors and non-400 errors to {@link #handleHttpResponseException}.
+
+      if (maybeTokenRefreshedRetry.isPresent()) {
+        response = maybeTokenRefreshedRetry.get();
+      } else {
+        rethrowForDtpStandards(
+            firstReqException.getStatusCode(),
+            Optional.of(firstReqException),
+            Optional.empty() /*maybeResponse*/);
+      }
+    }
+    Preconditions.checkNotNull(
+        response,
+        "bug? response should be set, else DTP error already thrown, but neither happened?");
+
+    if (response.getStatusCode() != 200) {
+      rethrowForDtpStandards(
+        response.getStatusCode(),
+        Optional.empty() /*maybeException*/,
+        Optional.ofNullable(response));
     }
+
+    return response;
   }
 
-  private HttpResponse handleHttpResponseException(
+  private Optional<HttpResponse> maybeRetryWithFreshToken(
       SupplierWithIO<HttpRequest> httpRequest, HttpResponseException e)
-      throws IOException, InvalidTokenException, PermissionDeniedException {
+      throws IOException, InvalidTokenException {
     // if the response is "unauthorized", refresh the token and try the request again
     final int statusCode = e.getStatusCode();
 
-    if (statusCode == 401) {
-      monitor.info(() -> String.format("GooglePhotosInterface: Attempting to refresh authorization token due to HTTP response code=%s, %s\n", statusCode, e));
-      // if the credential refresh failed, let the error bubble up via the IOException that gets
-      // thrown
-      credential = credentialFactory.refreshCredential(credential);
-      monitor.info(() -> "GooglePhotosInterface: Refreshed authorization token successfully");
+    if (statusCode != 401) {
+      return Optional.empty();
+    }
+
+    monitor.info(() -> String.format("GooglePhotosInterface: Attempting to refresh authorization token due to HTTP response code=%s, %s\n", statusCode, e));
+    // if the credential refresh failed, let the error bubble up via the IOException that gets
+    // thrown
+    credential = credentialFactory.refreshCredential(credential);
+    monitor.info(() -> "GooglePhotosInterface: Refreshed authorization token successfully");
 
-      // if the second attempt throws an error, then something else is wrong, and we bubble up the
-      // response errors
-      return httpRequest.getWithIO().execute();
+    // if the second attempt throws an error, then something else is wrong, and we bubble up the
+    // response errors
+    return Optional.of(httpRequest.getWithIO().execute());
+  }
+
+  /**
+   * Tries to throw a DTP-standard exception for a request that's failed, given whatever info we
+   * have about the failure (HTTP response code, if nothing else) .
+   */
+  // TODO: jzacsh rework this class to interact with HTTP in an easily
+  // loggable/testable/re-usable way, like we did with MicrosoftApiResponse.
+  private void rethrowForDtpStandards(
+    int statusCode,
+    Optional<HttpResponseException> maybeException,
+    Optional<HttpResponse> maybeResponse)
+  throws IOException, InvalidTokenException, PermissionDeniedException, UploadErrorException { 
+    final String emptyServerMessage = "[no server message: have neither response nor exception]";
+    final Optional<String> serverMessage;
+    if (maybeException.isPresent()) {
+      serverMessage = Optional.of(maybeException.get().getContent());
+    } else if (maybeResponse.isPresent()) {
+      serverMessage = Optional.of(maybeResponse.get().getStatusMessage());
+    } else {
+      serverMessage = Optional.empty();
     }
-    // "The caller does not have permission" is potential error for albums.
-    // "Google Photos is disabled for the user" is potential error for photos.
+
     if (statusCode == 403 &&
-            (e.getContent().contains("The caller does not have permission") ||
-             e.getContent().contains("Google Photos is disabled for the user"))) {
-      throw new PermissionDeniedException("User permission to google photos was denied", e);
-    } else {
-      // something else is wrong, bubble up the error
-      throw new IOException(
-          "Bad status code: "
-              + e.getStatusCode()
-              + " Error: '"
-              + e.getStatusMessage()
-              + "' Content: "
-              + e.getContent());
+        (serverMessage.orElse("").contains(GOOGPHOTOS_ALBUMS_PERMISSION_ERROR) ||
+        serverMessage.orElse("").contains(GOOGPHOTOS_PHOTO_PERMISSION_ERROR))) {
+      throw new PermissionDeniedException("User permission to google photos was denied", maybeException.orElse(null));
     }
+
+   // Upload-related exceptions; currently this is only used for payload hash verifications.
+    if (statusCode == 400 &&
+      (serverMessage.orElse("").contains(GOOG_ERROR_HASH_MISMATCH_LEGACY) ||
+      serverMessage.orElse("").contains(GOOG_ERROR_HASH_MISMATCH_UNIFIED))) {
+    Throwable throwableForBadResponse =
+        new IOException(String.format(
+          "non-error HTTP response statusCode=%s: %s",
+          statusCode,
+          serverMessage.orElse(emptyServerMessage)));
+    Throwable cause = maybeException.map(e -> (Throwable) e /*downcast*/).orElse(throwableForBadResponse);
+    throw new UploadErrorException(ERROR_HASH_MISMATCH, cause);
+    }
+
+    // something else is wrong, bubble up the error
+    throw new IOException(
+        String.format("Bad HTTP response: status code=%s, Error='%s' Content: %s",
+            statusCode,
+            maybeException.map(e -> e.getStatusMessage()).orElse("[no HTTP error]"),
+            serverMessage.orElse(emptyServerMessage)));
   }
 
   private String generateParamsString(Optional<Map<String, String>> params) {