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