Continue Spec11 emails even if one fails

One failure should not stop the rest from sending.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=241760092

java/google/registry/reporting/spec11/Spec11EmailUtils.java

@@ -14,16 +14,12 @@
 
 package google.registry.reporting.spec11;
 
-import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.google.common.base.Throwables.getRootCause;
 import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.io.Resources.getResource;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Sets;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.net.MediaType;
 import com.google.template.soy.SoyFileSet;
@@ -87,40 +83,40 @@ public class Spec11EmailUtils {
       SoyTemplateInfo soyTemplateInfo,
       String subject,
       Set<RegistrarThreatMatches> registrarThreatMatchesSet) {
-    // Null/empty email address shouldn't throw an exception, but we should warn if they occur.
+    ImmutableMap.Builder<RegistrarThreatMatches, Throwable> failedMatchesBuilder =
-    ImmutableSet<RegistrarThreatMatches> matchesWithoutEmails =
+        ImmutableMap.builder();
-        registrarThreatMatchesSet.stream()
+    for (RegistrarThreatMatches registrarThreatMatches : registrarThreatMatchesSet) {
-            .filter(matches -> isNullOrEmpty(matches.registrarEmailAddress()))
-            .collect(toImmutableSet());
       try {
-      for (RegistrarThreatMatches registrarThreatMatches :
+        // Handle exceptions individually per registrar so that one failed email doesn't prevent the
-          Sets.difference(registrarThreatMatchesSet, matchesWithoutEmails)) {
+        // rest from being sent.
         emailRegistrar(date, soyTemplateInfo, subject, registrarThreatMatches);
-      }
       } catch (Throwable e) {
-      // Send an alert with the root cause, unwrapping the retrier's RuntimeException
+        failedMatchesBuilder.put(registrarThreatMatches, getRootCause(e));
+      }
+    }
+    ImmutableMap<RegistrarThreatMatches, Throwable> failedMatches = failedMatchesBuilder.build();
+    if (!failedMatches.isEmpty()) {
+      ImmutableList<Map.Entry<RegistrarThreatMatches, Throwable>> failedMatchesList =
+          failedMatches.entrySet().asList();
+      // Send an alert email and throw a RuntimeException with the first failure as the cause,
+      // but log the rest so that we have that information.
+      Throwable firstThrowable = failedMatchesList.get(0).getValue();
       sendAlertEmail(
           String.format("Spec11 Emailing Failure %s", date),
-          String.format("Emailing spec11 reports failed due to %s", getRootCause(e).getMessage()));
+          String.format("Emailing Spec11 reports failed due to %s", firstThrowable.getMessage()));
-      throw new RuntimeException("Emailing spec11 report failed", e);
+      for (int i = 1; i < failedMatches.size(); i++) {
+        // TODO(b/129401965): Use only client IDs in this message
+        logger.atSevere().withCause(failedMatchesList.get(i).getValue()).log(
+            "Additional exception thrown when sending email to registrar %s, in addition to the"
+                + " re-thrown exception",
+            failedMatchesList.get(i).getKey().registrarEmailAddress());
+      }
+      throw new RuntimeException(
+          "Emailing Spec11 reports failed, first exception:", firstThrowable);
     }
-    if (matchesWithoutEmails.isEmpty()) {
     sendAlertEmail(
         String.format("Spec11 Pipeline Success %s", date),
         "Spec11 reporting completed successfully.");
-    } else {
-      logger.atSevere().log(
-          "Some Spec11 threat matches had no associated email addresses: %s", matchesWithoutEmails);
-      // TODO(b/129401965): Use only client IDs in this message
-      sendAlertEmail(
-          String.format("Spec11 Pipeline Warning %s", date),
-          String.format(
-              "No errors occurred but the following matches had no associated email: \n"
-                  + "%s\n\n"
-                  + "This should not occur; please make sure we have email addresses for these"
-                  + " registrar(s).",
-              matchesWithoutEmails));
-    }
   }
 
   private void emailRegistrar(

javatests/google/registry/reporting/spec11/Spec11EmailUtilsTest.java

@@ -14,8 +14,9 @@
 
 package google.registry.reporting.spec11;
 
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.reporting.spec11.Spec11RegistrarThreatMatchesParserTest.getMatchA;
+import static google.registry.reporting.spec11.Spec11RegistrarThreatMatchesParserTest.getMatchB;
 import static google.registry.reporting.spec11.Spec11RegistrarThreatMatchesParserTest.sampleThreatMatches;
 import static google.registry.testing.JUnitBackports.assertThrows;
 import static org.mockito.Mockito.doThrow;
@@ -25,11 +26,11 @@ import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.net.MediaType;
 import google.registry.reporting.spec11.soy.Spec11EmailSoyInfo;
 import google.registry.util.EmailMessage;
 import google.registry.util.SendEmailService;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Optional;
 import javax.mail.MessagingException;
@@ -46,6 +47,64 @@ import org.mockito.ArgumentCaptor;
 public class Spec11EmailUtilsTest {
 
   private static final ImmutableList<String> FAKE_RESOURCES = ImmutableList.of("foo");
+  private static final String DAILY_EMAIL_FORMAT =
+      "Dear registrar partner,"
+          + ""
+          + "<p>Super Cool Registry conducts a daily analysis of all domains registered in its "
+          + "TLDs to identify potential security concerns. On 2018-07-15, the following domains "
+          + "that your registrar manages were flagged for potential security concerns:</p>"
+          + ""
+          + "<table>"
+          + "<tr><th>Domain Name</th><th>Threat Type</th></tr>"
+          + "%s"
+          + "</table>"
+          + "<p><b>Please communicate these findings to the registrant and work with the "
+          + "registrant to mitigate any security issues and have the domains delisted.</b></p>"
+          + ""
+          + "Some helpful resources for getting off a blocked list include:"
+          + "<ul><li>foo</li></ul><p>"
+          + ""
+          + "You will continue to receive daily notices when new domains managed by your registrar "
+          + "are flagged for abuse, as well as a monthly summary of all of your domains under "
+          + "management that remain flagged for abuse. Once the registrant has resolved the "
+          + "security issues and followed the steps to have his or her domain reviewed and "
+          + "delisted it will automatically be removed from our reporting.</p>"
+          + ""
+          + "<p>If you would like to change the email to which these notices are sent please "
+          + "update your abuse contact using your registrar portal account.</p>"
+          + ""
+          + "<p>If you have any questions regarding this notice, please contact "
+          + "my-reply-to@test.com.</p>";
+  private static final String MONTHLY_EMAIL_FORMAT =
+      "Dear registrar partner,"
+          + ""
+          + "<p>Super Cool Registry previously notified you when the following "
+          + "domains managed by your registrar were flagged for potential security concerns.</p>"
+          + "<p>The following domains that you manage continue to be flagged by our analysis "
+          + "for potential security concerns. This may be because the registrants have not "
+          + "completed the requisite steps to mitigate the potential security abuse and/or have "
+          + "it reviewed and delisted.</p>"
+          + ""
+          + "<table>"
+          + "<tr><th>Domain Name</th><th>Threat Type</th></tr>"
+          + "%s"
+          + "</table>"
+          + ""
+          + "<p>Please work with the registrant to mitigate any security issues "
+          + "and have the domains delisted.</p>"
+          + ""
+          + "Some helpful resources for getting off a blocked list include:"
+          + "<ul><li>foo</li></ul><p>"
+          + ""
+          + "You will continue to receive a monthly summary of all domains managed by your "
+          + "registrar that remain on our lists of potential security threats. You will "
+          + "additionally receive a daily notice when any new domains that are added to these "
+          + "lists. Once the registrant has resolved the security issues and followed the steps to "
+          + "have his or her domain reviewed and delisted it will automatically be removed from "
+          + "our monthly reporting.</p>"
+          + ""
+          + "<p>If you have any questions regarding this notice, please contact "
+          + "my-reply-to@test.com.</p>";
 
   private SendEmailService emailService;
   private Spec11EmailUtils emailUtils;
@@ -79,30 +138,13 @@ public class Spec11EmailUtilsTest {
     // We inspect individual parameters because Message doesn't implement equals().
     verify(emailService, times(3)).sendEmail(contentCaptor.capture());
     List<EmailMessage> capturedContents = contentCaptor.getAllValues();
-    String emailFormat =
-        "Dear registrar partner,<p>Super Cool Registry previously notified you when the following "
-            + "domains managed by your registrar were flagged for potential security concerns."
-            + "</p><p>The following domains that you manage continue to be flagged by our analysis "
-            + "for potential security concerns. This may be because the registrants have not "
-            + "completed the requisite steps to mitigate the potential security abuse and/or have "
-            + "it reviewed and delisted.</p><table><tr><th>Domain Name</th><th>Threat Type</th>"
-            + "</tr>%s</table><p>Please work with the registrant to mitigate any security issues "
-            + "and have the domains delisted.</p>Some helpful resources for getting off a blocked "
-            + "list include:<ul><li>foo</li></ul><p>You will continue to receive a monthly summary "
-            + "of all domains managed by your registrar that remain on our lists of potential "
-            + "security threats. You will additionally receive a daily notice when any new domains "
-            + "that are added to these lists. Once the registrant has resolved the security issues "
-            + "and followed the steps to have his or her domain reviewed and delisted it will "
-            + "automatically be removed from our monthly reporting.</p><p>If you have any q"
-            + "uestions regarding this notice, please contact my-reply-to@test.com.</p>";
-
     validateMessage(
         capturedContents.get(0),
         "my-sender@test.com",
         "a@fake.com",
         Optional.of("my-reply-to@test.com"),
         "Super Cool Registry Monthly Threat Detector [2018-07-15]",
-        String.format(emailFormat, "<tr><td>a.com</td><td>MALWARE</td></tr>"),
+        String.format(MONTHLY_EMAIL_FORMAT, "<tr><td>a.com</td><td>MALWARE</td></tr>"),
         Optional.of(MediaType.HTML_UTF_8));
     validateMessage(
         capturedContents.get(1),
@@ -111,7 +153,7 @@ public class Spec11EmailUtilsTest {
         Optional.of("my-reply-to@test.com"),
         "Super Cool Registry Monthly Threat Detector [2018-07-15]",
         String.format(
-            emailFormat,
+            MONTHLY_EMAIL_FORMAT,
             "<tr><td>b.com</td><td>MALWARE</td></tr><tr><td>c.com</td><td>MALWARE</td></tr>"),
         Optional.of(MediaType.HTML_UTF_8));
     validateMessage(
@@ -134,31 +176,13 @@ public class Spec11EmailUtilsTest {
     // We inspect individual parameters because Message doesn't implement equals().
     verify(emailService, times(3)).sendEmail(contentCaptor.capture());
     List<EmailMessage> capturedMessages = contentCaptor.getAllValues();
-    String emailFormat =
-        "Dear registrar partner,<p>"
-            + "Super Cool Registry conducts a daily analysis of all domains registered in its TLDs "
-            + "to identify potential security concerns. On 2018-07-15, the following domains that "
-            + "your registrar manages were flagged for potential security concerns:</p><table><tr>"
-            + "<th>Domain Name</th><th>Threat Type</th></tr>%s</table><p><b>Please communicate "
-            + "these findings to the registrant and work with the registrant to mitigate any "
-            + "security issues and have the domains delisted.</b></p>Some helpful resources for "
-            + "getting off a blocked list include:<ul><li>foo</li></ul><p>You will continue to "
-            + "receive daily notices when new domains managed by your registrar are flagged for "
-            + "abuse, as well as a monthly summary of all of your domains under management that "
-            + "remain flagged for abuse. Once the registrant has resolved the security issues and "
-            + "followed the steps to have his or her domain reviewed and delisted it will "
-            + "automatically be removed from our reporting.</p><p>If you would like to change "
-            + "the email to which these notices are sent please update your abuse contact using "
-            + "your registrar portal account.</p><p>If you have any questions regarding this "
-            + "notice, please contact my-reply-to@test.com.</p>";
-
     validateMessage(
         capturedMessages.get(0),
         "my-sender@test.com",
         "a@fake.com",
         Optional.of("my-reply-to@test.com"),
         "Super Cool Registry Daily Threat Detector [2018-07-15]",
-        String.format(emailFormat, "<tr><td>a.com</td><td>MALWARE</td></tr>"),
+        String.format(DAILY_EMAIL_FORMAT, "<tr><td>a.com</td><td>MALWARE</td></tr>"),
         Optional.of(MediaType.HTML_UTF_8));
     validateMessage(
         capturedMessages.get(1),
@@ -167,7 +191,7 @@ public class Spec11EmailUtilsTest {
         Optional.of("my-reply-to@test.com"),
         "Super Cool Registry Daily Threat Detector [2018-07-15]",
         String.format(
-            emailFormat,
+            DAILY_EMAIL_FORMAT,
             "<tr><td>b.com</td><td>MALWARE</td></tr><tr><td>c.com</td><td>MALWARE</td></tr>"),
         Optional.of(MediaType.HTML_UTF_8));
     validateMessage(
@@ -181,8 +205,13 @@ public class Spec11EmailUtilsTest {
   }
 
   @Test
-  public void testFailure_emailsAlert() throws MessagingException {
+  public void testOneFailure_sendsAlert() throws Exception {
+    // If there is one failure, we should still send the other message and then an alert email
+    LinkedHashSet<RegistrarThreatMatches> matches = new LinkedHashSet<>();
+    matches.add(getMatchA());
+    matches.add(getMatchB());
     doThrow(new RuntimeException(new MessagingException("expected")))
+        .doNothing()
         .doNothing()
         .when(emailService)
         .sendEmail(contentCaptor.capture());
@@ -191,21 +220,42 @@ public class Spec11EmailUtilsTest {
             RuntimeException.class,
             () ->
                 emailUtils.emailSpec11Reports(
-                    date, Spec11EmailSoyInfo.MONTHLY_SPEC_11_EMAIL, "bar", sampleThreatMatches()));
+                    date,
-    assertThat(thrown).hasMessageThat().isEqualTo("Emailing spec11 report failed");
+                    Spec11EmailSoyInfo.MONTHLY_SPEC_11_EMAIL,
+                    "Super Cool Registry Monthly Threat Detector [2018-07-15]",
+                    matches));
     assertThat(thrown)
-        .hasCauseThat()
         .hasMessageThat()
-        .isEqualTo("javax.mail.MessagingException: expected");
+        .isEqualTo("Emailing Spec11 reports failed, first exception:");
+    assertThat(thrown).hasCauseThat().hasMessageThat().isEqualTo("expected");
     // Verify we sent an e-mail alert
-    verify(emailService, times(2)).sendEmail(contentCaptor.capture());
+    verify(emailService, times(3)).sendEmail(contentCaptor.capture());
+    List<EmailMessage> capturedMessages = contentCaptor.getAllValues();
     validateMessage(
-        contentCaptor.getValue(),
+        capturedMessages.get(0),
+        "my-sender@test.com",
+        "a@fake.com",
+        Optional.of("my-reply-to@test.com"),
+        "Super Cool Registry Monthly Threat Detector [2018-07-15]",
+        String.format(MONTHLY_EMAIL_FORMAT, "<tr><td>a.com</td><td>MALWARE</td></tr>"),
+        Optional.of(MediaType.HTML_UTF_8));
+    validateMessage(
+        capturedMessages.get(1),
+        "my-sender@test.com",
+        "b@fake.com",
+        Optional.of("my-reply-to@test.com"),
+        "Super Cool Registry Monthly Threat Detector [2018-07-15]",
+        String.format(
+            MONTHLY_EMAIL_FORMAT,
+            "<tr><td>b.com</td><td>MALWARE</td></tr><tr><td>c.com</td><td>MALWARE</td></tr>"),
+        Optional.of(MediaType.HTML_UTF_8));
+    validateMessage(
+        capturedMessages.get(2),
         "my-sender@test.com",
         "my-receiver@test.com",
         Optional.empty(),
         "Spec11 Emailing Failure 2018-07-15",
-        "Emailing spec11 reports failed due to expected",
+        "Emailing Spec11 reports failed due to expected",
         Optional.empty());
   }
 
@@ -223,36 +273,6 @@ public class Spec11EmailUtilsTest {
         Optional.empty());
   }
 
-  @Test
-  public void testWarning_emptyEmailAddressForRegistrars() throws Exception {
-    ImmutableSet<RegistrarThreatMatches> matchesWithoutEmails =
-        sampleThreatMatches().stream()
-            .map(matches -> RegistrarThreatMatches.create("", matches.threatMatches()))
-            .collect(toImmutableSet());
-    emailUtils.emailSpec11Reports(
-        date,
-        Spec11EmailSoyInfo.DAILY_SPEC_11_EMAIL,
-        "Super Cool Registry Daily Threat Detector [2018-07-15]",
-        matchesWithoutEmails);
-    verify(emailService).sendEmail(contentCaptor.capture());
-    validateMessage(
-        contentCaptor.getValue(),
-        "my-sender@test.com",
-        "my-receiver@test.com",
-        Optional.empty(),
-        "Spec11 Pipeline Warning 2018-07-15",
-        "No errors occurred but the following matches had no associated email: \n"
-            + "[RegistrarThreatMatches{registrarEmailAddress=, threatMatches=[ThreatMatch"
-            + "{threatType=MALWARE, platformType=ANY_PLATFORM, metadata=NONE,"
-            + " fullyQualifiedDomainName=a.com}]}, RegistrarThreatMatches{registrarEmailAddress=,"
-            + " threatMatches=[ThreatMatch{threatType=MALWARE, platformType=ANY_PLATFORM,"
-            + " metadata=NONE, fullyQualifiedDomainName=b.com}, ThreatMatch{threatType=MALWARE,"
-            + " platformType=ANY_PLATFORM, metadata=NONE, fullyQualifiedDomainName=c.com}]}]\n\n"
-            + "This should not occur; please make sure we have email addresses for these"
-            + " registrar(s).",
-        Optional.empty());
-  }
-
   private void validateMessage(
       EmailMessage message,
       String from,

javatests/google/registry/reporting/spec11/Spec11RegistrarThreatMatchesParserTest.java

@@ -78,28 +78,11 @@ public class Spec11RegistrarThreatMatchesParserTest {
   }
 
   /** The expected contents of the sample spec11 report file */
-  public static ImmutableSet<RegistrarThreatMatches> sampleThreatMatches() throws Exception {
+  static ImmutableSet<RegistrarThreatMatches> sampleThreatMatches() throws Exception {
     return ImmutableSet.of(getMatchA(), getMatchB());
   }
 
-  private void setupFile(String fileWithContent, String fileDate) {
+  static RegistrarThreatMatches getMatchA() throws Exception {
-    GcsFilename gcsFilename =
-        new GcsFilename(
-            "test-bucket",
-            String.format("icann/spec11/2018-07/SPEC11_MONTHLY_REPORT_%s", fileDate));
-    when(gcsUtils.existsAndNotEmpty(gcsFilename)).thenReturn(true);
-    when(gcsUtils.openInputStream(gcsFilename))
-        .thenAnswer(
-            (args) ->
-                new ByteArrayInputStream(
-                    loadFile(fileWithContent).getBytes(StandardCharsets.UTF_8)));
-  }
-
-  private static String loadFile(String filename) {
-    return TestDataHelper.loadFile(Spec11EmailUtils.class, filename);
-  }
-
-  private static RegistrarThreatMatches getMatchA() throws Exception {
     return RegistrarThreatMatches.create(
         "a@fake.com",
         ImmutableList.of(
@@ -112,7 +95,7 @@ public class Spec11RegistrarThreatMatchesParserTest {
                         "fullyQualifiedDomainName", "a.com")))));
   }
 
-  private static RegistrarThreatMatches getMatchB() throws Exception {
+  static RegistrarThreatMatches getMatchB() throws Exception {
     return RegistrarThreatMatches.create(
         "b@fake.com",
         ImmutableList.of(
@@ -131,4 +114,21 @@ public class Spec11RegistrarThreatMatchesParserTest {
                         "threatEntryMetadata", "NONE",
                         "fullyQualifiedDomainName", "c.com")))));
   }
+
+  private void setupFile(String fileWithContent, String fileDate) {
+    GcsFilename gcsFilename =
+        new GcsFilename(
+            "test-bucket",
+            String.format("icann/spec11/2018-07/SPEC11_MONTHLY_REPORT_%s", fileDate));
+    when(gcsUtils.existsAndNotEmpty(gcsFilename)).thenReturn(true);
+    when(gcsUtils.openInputStream(gcsFilename))
+        .thenAnswer(
+            (args) ->
+                new ByteArrayInputStream(
+                    loadFile(fileWithContent).getBytes(StandardCharsets.UTF_8)));
+  }
+
+  private static String loadFile(String filename) {
+    return TestDataHelper.loadFile(Spec11EmailUtils.class, filename);
+  }
 }