From e17cb52bf77a6135bee5bfa1f2bab8e43f4b2b0f Mon Sep 17 00:00:00 2001 From: Lai Jiang Date: Mon, 22 Jul 2019 14:09:49 -0400 Subject: [PATCH] Fail gracefully when copying detailed reports (#181) * Fail gracefully when copying detailed reports When the detailed reports are copied from GCS to registrars' Drive folders, do not fail the entire copy operation when a single registrar fails. Instead, send an alert email about the failure, and continue to copy the rest of the reports. Also, instead of creating duplicates, overwrite the existing files on Drive. BUG=127690361 --- .../billing/CopyDetailReportsAction.java | 32 +++++++++-- .../storage/drive/DriveConnection.java | 4 ++ .../billing/CopyDetailReportsActionTest.java | 55 +++++++++++++------ 3 files changed, 69 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/google/registry/reporting/billing/CopyDetailReportsAction.java b/core/src/main/java/google/registry/reporting/billing/CopyDetailReportsAction.java index 08f118d93..8da9be513 100644 --- a/core/src/main/java/google/registry/reporting/billing/CopyDetailReportsAction.java +++ b/core/src/main/java/google/registry/reporting/billing/CopyDetailReportsAction.java @@ -22,6 +22,7 @@ import static javax.servlet.http.HttpServletResponse.SC_OK; import com.google.appengine.tools.cloudstorage.GcsFilename; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.flogger.FluentLogger; import com.google.common.io.ByteStreams; @@ -38,6 +39,7 @@ import google.registry.util.Retrier; import java.io.IOException; import java.io.InputStream; import java.util.Optional; +import java.util.stream.Collectors; import javax.inject.Inject; /** Copy all registrar detail reports in a given bucket's subdirectory from GCS to Drive. */ @@ -95,6 +97,8 @@ public final class CopyDetailReportsAction implements Runnable { response.setPayload(String.format("Failure, encountered %s", e.getMessage())); return; } + ImmutableMap.Builder copyErrorsBuilder = + new ImmutableMap.Builder(); for (String detailReportName : detailReportObjectNames) { // The standard report format is "invoice_details_yyyy-MM_registrarId_tld.csv // TODO(larryruili): Determine a safer way of enforcing this. @@ -117,7 +121,7 @@ public final class CopyDetailReportsAction implements Runnable { try (InputStream input = gcsUtils.openInputStream( new GcsFilename(billingBucket, invoiceDirectoryPrefix + detailReportName))) { - driveConnection.createFile( + driveConnection.createOrUpdateFile( detailReportName, MediaType.CSV_UTF_8, driveFolderId, @@ -129,15 +133,31 @@ public final class CopyDetailReportsAction implements Runnable { }, IOException.class); } catch (Throwable e) { - emailUtils.sendAlertEmail( + String alertMessage = String.format( - "Warning: CopyDetailReportsAction failed.\nEncountered: %s on file: %s", - getRootCause(e).getMessage(), detailReportName)); - throw e; + "Warning: CopyDetailReportsAction failed for registrar %s.\n" + + "Encountered: %s on file: %s", + registrarId, getRootCause(e).getMessage(), detailReportName); + copyErrorsBuilder.put(registrarId, e); + logger.atSevere().withCause(e).log(alertMessage); } } response.setStatus(SC_OK); response.setContentType(MediaType.PLAIN_TEXT_UTF_8); - response.setPayload("Copied detail reports."); + StringBuilder payload = new StringBuilder().append("Copied detail reports.\n"); + ImmutableMap copyErrors = copyErrorsBuilder.build(); + if (!copyErrors.isEmpty()) { + payload.append("The following errors were encountered:\n"); + payload.append( + copyErrors.entrySet().stream() + .map( + entrySet -> + String.format( + "Registrar: %s\nError: %s\n", + entrySet.getKey(), entrySet.getValue().getMessage())) + .collect(Collectors.joining())); + } + response.setPayload(payload.toString()); + emailUtils.sendAlertEmail(payload.toString()); } } diff --git a/core/src/main/java/google/registry/storage/drive/DriveConnection.java b/core/src/main/java/google/registry/storage/drive/DriveConnection.java index 822ec8c8a..a9e9f9db7 100644 --- a/core/src/main/java/google/registry/storage/drive/DriveConnection.java +++ b/core/src/main/java/google/registry/storage/drive/DriveConnection.java @@ -54,6 +54,10 @@ public class DriveConnection { /** * Creates a file with the given parent. * + *

If a file with the same path already exists, a duplicate is created. If overwriting the + * existing file is the desired behavior, use {@link #createOrUpdateFile(String, MediaType, + * String, byte[])} instead. + * * @returns the file id. */ public String createFile(String title, MediaType mimeType, String parentFolderId, byte[] bytes) diff --git a/core/src/test/java/google/registry/reporting/billing/CopyDetailReportsActionTest.java b/core/src/test/java/google/registry/reporting/billing/CopyDetailReportsActionTest.java index 20f30b5ff..e601f2d47 100644 --- a/core/src/test/java/google/registry/reporting/billing/CopyDetailReportsActionTest.java +++ b/core/src/test/java/google/registry/reporting/billing/CopyDetailReportsActionTest.java @@ -18,10 +18,10 @@ import static com.google.common.truth.Truth.assertThat; import static google.registry.testing.DatastoreHelper.loadRegistrar; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.GcsTestingUtils.writeGcsFile; -import static google.registry.testing.JUnitBackports.assertThrows; import static java.nio.charset.StandardCharsets.UTF_8; import static javax.servlet.http.HttpServletResponse.SC_OK; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -68,6 +68,7 @@ public class CopyDetailReportsActionTest { @Before public void setUp() { persistResource(loadRegistrar("TheRegistrar").asBuilder().setDriveFolderId("0B-12345").build()); + persistResource(loadRegistrar("NewRegistrar").asBuilder().setDriveFolderId("0B-54321").build()); response = new FakeResponse(); driveConnection = mock(DriveConnection.class); emailUtils = mock(BillingEmailUtils.class); @@ -96,21 +97,21 @@ public class CopyDetailReportsActionTest { action.run(); verify(driveConnection) - .createFile( + .createOrUpdateFile( "invoice_details_2017-10_TheRegistrar_test.csv", MediaType.CSV_UTF_8, "0B-12345", "hello,world\n1,2".getBytes(UTF_8)); verify(driveConnection) - .createFile( + .createOrUpdateFile( "invoice_details_2017-10_TheRegistrar_hello.csv", MediaType.CSV_UTF_8, "0B-12345", "hola,mundo\n3,4".getBytes(UTF_8)); assertThat(response.getStatus()).isEqualTo(SC_OK); assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); - assertThat(response.getPayload()).isEqualTo("Copied detail reports."); + assertThat(response.getPayload()).isEqualTo("Copied detail reports.\n"); } @Test @@ -126,7 +127,7 @@ public class CopyDetailReportsActionTest { "hello,world\n1,2".getBytes(UTF_8)); action.run(); verify(driveConnection) - .createFile( + .createOrUpdateFile( "invoice_details_2017-10_TheRegistrar_hello.csv", MediaType.CSV_UTF_8, "0B-12345", @@ -135,7 +136,7 @@ public class CopyDetailReportsActionTest { verifyNoMoreInteractions(driveConnection); assertThat(response.getStatus()).isEqualTo(SC_OK); assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); - assertThat(response.getPayload()).isEqualTo("Copied detail reports."); + assertThat(response.getPayload()).isEqualTo("Copied detail reports.\n"); } @Test @@ -144,41 +145,63 @@ public class CopyDetailReportsActionTest { gcsService, new GcsFilename("test-bucket", "results/invoice_details_2017-10_TheRegistrar_hello.csv"), "hola,mundo\n3,4".getBytes(UTF_8)); - when(driveConnection.createFile(any(), any(), any(), any())) + when(driveConnection.createOrUpdateFile(any(), any(), any(), any())) .thenThrow(new IOException("expected")) .thenReturn("success"); action.run(); verify(driveConnection, times(2)) - .createFile( + .createOrUpdateFile( "invoice_details_2017-10_TheRegistrar_hello.csv", MediaType.CSV_UTF_8, "0B-12345", "hola,mundo\n3,4".getBytes(UTF_8)); assertThat(response.getStatus()).isEqualTo(SC_OK); assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); - assertThat(response.getPayload()).isEqualTo("Copied detail reports."); + assertThat(response.getPayload()).isEqualTo("Copied detail reports.\n"); } @Test - public void testFail_tooManyFailures_sendsAlertEmail() throws IOException { + public void testFail_tooManyFailures_sendsAlertEmail_continues() throws IOException { writeGcsFile( gcsService, new GcsFilename("test-bucket", "results/invoice_details_2017-10_TheRegistrar_hello.csv"), "hola,mundo\n3,4".getBytes(UTF_8)); - when(driveConnection.createFile(any(), any(), any(), any())) + writeGcsFile( + gcsService, + new GcsFilename("test-bucket", "results/invoice_details_2017-10_NewRegistrar_test.csv"), + "hello,world\n1,2".getBytes(UTF_8)); + when(driveConnection.createOrUpdateFile( + eq("invoice_details_2017-10_TheRegistrar_hello.csv"), any(), any(), any())) .thenThrow(new IOException("expected")); - RuntimeException thrown = assertThrows(RuntimeException.class, action::run); - assertThat(thrown).hasMessageThat().isEqualTo("java.io.IOException: expected"); + action.run(); verify(driveConnection, times(3)) - .createFile( + .createOrUpdateFile( "invoice_details_2017-10_TheRegistrar_hello.csv", MediaType.CSV_UTF_8, "0B-12345", "hola,mundo\n3,4".getBytes(UTF_8)); - verify(emailUtils).sendAlertEmail("Warning: CopyDetailReportsAction failed.\nEncountered: " - + "expected on file: invoice_details_2017-10_TheRegistrar_hello.csv"); + verify(driveConnection) + .createOrUpdateFile( + "invoice_details_2017-10_NewRegistrar_test.csv", + MediaType.CSV_UTF_8, + "0B-54321", + "hello,world\n1,2".getBytes(UTF_8)); + verify(emailUtils) + .sendAlertEmail( + "Copied detail reports.\n" + + "The following errors were encountered:\n" + + "Registrar: TheRegistrar\n" + + "Error: java.io.IOException: expected\n"); + assertThat(response.getStatus()).isEqualTo(SC_OK); + assertThat(response.getContentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8); + assertThat(response.getPayload()) + .isEqualTo( + "Copied detail reports.\n" + + "The following errors were encountered:\n" + + "Registrar: TheRegistrar\n" + + "Error: java.io.IOException: expected\n"); } @Test