Add option to skip logging invalid recipients when sending emails

cescoffier · cescoffier · commit fa28a69d855a · 2024-10-16T10:49:20.000+02:00
This commits detects and prevents logging invalid recipients before delegating to the underlying SMTP client, avoiding the overhead of processing attachments for emails that won’t be sent as well as acquiring an unnecessary connection.

When the option is enabled (default), the invalid recipients is not logged and not part of the thrown exception.
diff --git a/extensions/mailer/deployment/pom.xml b/extensions/mailer/deployment/pom.xml
@@ -41,6 +41,11 @@
             <artifactId>quarkus-junit5-internal</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.assertj</groupId>
+            <artifactId>assertj-core</artifactId>
+            <scope>test</scope>
+        </dependency>
 
     </dependencies>
 
diff --git a/extensions/mailer/deployment/src/test/java/io/quarkus/mailer/InvalidEmailTest.java b/extensions/mailer/deployment/src/test/java/io/quarkus/mailer/InvalidEmailTest.java
@@ -0,0 +1,82 @@
+package io.quarkus.mailer;
+
+import java.time.Duration;
+import java.util.List;
+
+import jakarta.inject.Inject;
+import jakarta.inject.Singleton;
+
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.quarkus.mailer.reactive.ReactiveMailer;
+import io.quarkus.test.QuarkusUnitTest;
+import io.smallrye.mutiny.Uni;
+
+public class InvalidEmailTest {
+
+    @RegisterExtension
+    static final QuarkusUnitTest config = new QuarkusUnitTest()
+            .withApplicationRoot(root -> root
+                    .addClasses(Sender.class));
+
+    @Inject
+    MockMailbox mockMailbox;
+
+    @Inject
+    Sender sender;
+
+    @Test
+    public void testInvalidTo() {
+        List<String> to = List.of("clement@test.io", "inv alid@quarkus.io", "max@test.io");
+        List<String> cc = List.of();
+        List<String> bcc = List.of();
+        Assertions.assertThatThrownBy(() -> sender.send(to, cc, bcc).await().atMost(Duration.ofSeconds(5)))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("Unable to send an email, an email address is invalid")
+                .hasMessageNotContaining("@");
+        Assertions.assertThat(mockMailbox.getTotalMessagesSent()).isEqualTo(0);
+    }
+
+    @Test
+    public void testInvalidCC() {
+        List<String> cc = List.of("clement@test.io", "inv alid@quarkus.io", "max@test.io");
+        List<String> to = List.of();
+        List<String> bcc = List.of();
+        Assertions.assertThatThrownBy(() -> sender.send(to, cc, bcc).await().atMost(Duration.ofSeconds(5)))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("Unable to send an email, an email address is invalid")
+                .hasMessageNotContaining("@");
+        Assertions.assertThat(mockMailbox.getTotalMessagesSent()).isEqualTo(0);
+    }
+
+    @Test
+    public void testInvalidBCC() {
+        List<String> bcc = List.of("clement@test.io", "inv alid@quarkus.io", "max@test.io");
+        List<String> to = List.of();
+        List<String> cc = List.of();
+        Assertions.assertThatThrownBy(() -> sender.send(to, cc, bcc).await().atMost(Duration.ofSeconds(5)))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("Unable to send an email, an email address is invalid")
+                .hasMessageNotContaining("@");
+        Assertions.assertThat(mockMailbox.getTotalMessagesSent()).isEqualTo(0);
+    }
+
+    @Singleton
+    static class Sender {
+
+        @Inject
+        ReactiveMailer mailer;
+
+        Uni<Void> send(List<String> to, List<String> cc, List<String> bcc) {
+            Mail mail = new Mail()
+                    .setTo(to)
+                    .setCc(cc)
+                    .setBcc(bcc)
+                    .setSubject("Test")
+                    .setText("Hello!");
+            return mailer.send(mail);
+        }
+    }
+}
diff --git a/extensions/mailer/deployment/src/test/java/io/quarkus/mailer/LoggedInvalidEmailTest.java b/extensions/mailer/deployment/src/test/java/io/quarkus/mailer/LoggedInvalidEmailTest.java
@@ -0,0 +1,86 @@
+package io.quarkus.mailer;
+
+import java.time.Duration;
+import java.util.List;
+
+import jakarta.inject.Inject;
+import jakarta.inject.Singleton;
+
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.quarkus.mailer.reactive.ReactiveMailer;
+import io.quarkus.test.QuarkusUnitTest;
+import io.smallrye.mutiny.Uni;
+
+public class LoggedInvalidEmailTest {
+
+    @RegisterExtension
+    static final QuarkusUnitTest config = new QuarkusUnitTest()
+            .withApplicationRoot(root -> root
+                    .addClasses(Sender.class))
+            .overrideRuntimeConfigKey("quarkus.mailer.log-invalid-recipients", "true");
+
+    @Inject
+    MockMailbox mockMailbox;
+
+    @Inject
+    Sender sender;
+
+    @Test
+    public void testInvalidTo() {
+        List<String> to = List.of("clement@test.io", "inv alid@quarkus.io", "max@test.io");
+        List<String> cc = List.of();
+        List<String> bcc = List.of();
+        Assertions.assertThatThrownBy(() -> sender.send(to, cc, bcc).await().atMost(Duration.ofSeconds(5)))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("Unable to send an email")
+                .hasStackTraceContaining("inv alid@quarkus.io")
+                .hasMessageNotContaining("@text.io");
+        Assertions.assertThat(mockMailbox.getTotalMessagesSent()).isEqualTo(0);
+    }
+
+    @Test
+    public void testInvalidCC() {
+        List<String> cc = List.of("clement@test.io", "inv alid@quarkus.io", "max@test.io");
+        List<String> to = List.of();
+        List<String> bcc = List.of();
+        Assertions.assertThatThrownBy(() -> sender.send(to, cc, bcc).await().atMost(Duration.ofSeconds(5)))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("Unable to send an email")
+                .hasStackTraceContaining("inv alid@quarkus.io")
+                .hasMessageNotContaining("@text.io");
+        Assertions.assertThat(mockMailbox.getTotalMessagesSent()).isEqualTo(0);
+    }
+
+    @Test
+    public void testInvalidBCC() {
+        List<String> bcc = List.of("clement@test.io", "inv alid@quarkus.io", "max@test.io");
+        List<String> to = List.of();
+        List<String> cc = List.of();
+        Assertions.assertThatThrownBy(() -> sender.send(to, cc, bcc).await().atMost(Duration.ofSeconds(5)))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("Unable to send an email")
+                .hasStackTraceContaining("inv alid@quarkus.io")
+                .hasMessageNotContaining("@text.io");
+        Assertions.assertThat(mockMailbox.getTotalMessagesSent()).isEqualTo(0);
+    }
+
+    @Singleton
+    static class Sender {
+
+        @Inject
+        ReactiveMailer mailer;
+
+        Uni<Void> send(List<String> to, List<String> cc, List<String> bcc) {
+            Mail mail = new Mail()
+                    .setTo(to)
+                    .setCc(cc)
+                    .setBcc(bcc)
+                    .setSubject("Test")
+                    .setText("Hello!");
+            return mailer.send(mail);
+        }
+    }
+}
diff --git a/extensions/mailer/runtime/src/main/java/io/quarkus/mailer/runtime/MailerRuntimeConfig.java b/extensions/mailer/runtime/src/main/java/io/quarkus/mailer/runtime/MailerRuntimeConfig.java
@@ -272,4 +272,13 @@ public class MailerRuntimeConfig {
      */
     @ConfigItem(defaultValue = "false")
     public boolean logRejectedRecipients = false;
+
+    /**
+     * Log invalid recipients as warnings.
+     * <p>
+     * If false, the invalid recipients will not be logged and the thrown exception will not contain the invalid email address.
+     *
+     */
+    @ConfigItem(defaultValue = "false")
+    public boolean logInvalidRecipients = false;
 }
diff --git a/extensions/mailer/runtime/src/main/java/io/quarkus/mailer/runtime/Mailers.java b/extensions/mailer/runtime/src/main/java/io/quarkus/mailer/runtime/Mailers.java
@@ -75,7 +75,8 @@ public Mailers(Vertx vertx, io.vertx.mutiny.core.Vertx mutinyVertx, MailersRunti
                             mailersRuntimeConfig.defaultMailer.mock.orElse(launchMode.isDevOrTest()),
                             mailersRuntimeConfig.defaultMailer.approvedRecipients.orElse(List.of()).stream()
                                     .filter(Objects::nonNull).collect(Collectors.toList()),
-                            mailersRuntimeConfig.defaultMailer.logRejectedRecipients));
+                            mailersRuntimeConfig.defaultMailer.logRejectedRecipients,
+                            mailersRuntimeConfig.defaultMailer.logInvalidRecipients));
         }
 
         for (String name : mailerSupport.namedMailers) {
@@ -97,7 +98,8 @@ public Mailers(Vertx vertx, io.vertx.mutiny.core.Vertx mutinyVertx, MailersRunti
                             namedMailerRuntimeConfig.mock.orElse(false),
                             namedMailerRuntimeConfig.approvedRecipients.orElse(List.of()).stream()
                                     .filter(p -> p != null).collect(Collectors.toList()),
-                            namedMailerRuntimeConfig.logRejectedRecipients));
+                            namedMailerRuntimeConfig.logRejectedRecipients,
+                            namedMailerRuntimeConfig.logInvalidRecipients));
         }
 
         this.clients = Collections.unmodifiableMap(localClients);
diff --git a/extensions/mailer/runtime/src/main/java/io/quarkus/mailer/runtime/MockMailboxImpl.java b/extensions/mailer/runtime/src/main/java/io/quarkus/mailer/runtime/MockMailboxImpl.java
@@ -9,6 +9,7 @@
 import io.quarkus.mailer.MockMailbox;
 import io.smallrye.mutiny.Uni;
 import io.vertx.ext.mail.MailMessage;
+import io.vertx.ext.mail.mailencoder.EmailAddress;
 
 /**
  * Mock mailbox bean, will be populated if mocking emails.
@@ -22,22 +23,30 @@ public class MockMailboxImpl implements MockMailbox {
     Uni<Void> send(Mail email, MailMessage mailMessage) {
         if (email.getTo() != null) {
             for (String to : email.getTo()) {
+                validateEmailAddress(to);
                 send(email, mailMessage, to);
             }
         }
         if (email.getCc() != null) {
             for (String to : email.getCc()) {
+                validateEmailAddress(to);
                 send(email, mailMessage, to);
             }
         }
         if (email.getBcc() != null) {
             for (String to : email.getBcc()) {
+                validateEmailAddress(to);
                 send(email, mailMessage, to);
             }
         }
         return Uni.createFrom().item(() -> null);
     }
 
+    private void validateEmailAddress(String to) {
+        // Just here to validate the email address.
+        new EmailAddress(to);
+    }
+
     private void send(Mail sentMail, MailMessage sentMailMessage, String to) {
         sentMails.computeIfAbsent(to, k -> new ArrayList<>()).add(sentMail);
         sentMailMessages.computeIfAbsent(to, k -> new ArrayList<>()).add(sentMailMessage);
diff --git a/extensions/mailer/runtime/src/main/java/io/quarkus/mailer/runtime/MutinyMailerImpl.java b/extensions/mailer/runtime/src/main/java/io/quarkus/mailer/runtime/MutinyMailerImpl.java
@@ -25,6 +25,7 @@
 import io.vertx.core.file.OpenOptions;
 import io.vertx.ext.mail.MailAttachment;
 import io.vertx.ext.mail.MailMessage;
+import io.vertx.ext.mail.mailencoder.EmailAddress;
 import io.vertx.mutiny.core.Vertx;
 import io.vertx.mutiny.core.file.AsyncFile;
 import io.vertx.mutiny.ext.mail.MailClient;
@@ -47,11 +48,13 @@ public class MutinyMailerImpl implements ReactiveMailer {
 
     private final List<Pattern> approvedRecipients;
 
-    private boolean logRejectedRecipients;
+    private final boolean logRejectedRecipients;
+
+    private final boolean logInvalidRecipients;
 
     MutinyMailerImpl(Vertx vertx, MailClient client, MockMailboxImpl mockMailbox,
             String from, String bounceAddress, boolean mock, List<Pattern> approvedRecipients,
-            boolean logRejectedRecipients) {
+            boolean logRejectedRecipients, boolean logInvalidRecipients) {
         this.vertx = vertx;
         this.client = client;
         this.mockMailbox = mockMailbox;
@@ -60,6 +63,7 @@ public class MutinyMailerImpl implements ReactiveMailer {
         this.mock = mock;
         this.approvedRecipients = approvedRecipients;
         this.logRejectedRecipients = logRejectedRecipients;
+        this.logInvalidRecipients = logInvalidRecipients;
     }
 
     @Override
@@ -149,6 +153,11 @@ private Uni<MailMessage> toMailMessage(Mail mail) {
         message.setTo(mail.getTo());
         message.setCc(mail.getCc());
         message.setBcc(mail.getBcc());
+
+        // Validate that the email addresses are valid
+        // We do that early to avoid having to read attachments if an email is invalid
+        validate(mail.getTo(), mail.getCc(), mail.getBcc());
+
         message.setSubject(mail.getSubject());
         message.setText(mail.getText());
         message.setHtml(mail.getHtml());
@@ -177,13 +186,39 @@ private Uni<MailMessage> toMailMessage(Mail mail) {
             return Uni.createFrom().item(message);
         }
 
-        return Uni.combine().all().unis(stages).combinedWith(res -> {
+        return Uni.combine().all().unis(stages).with(res -> {
             message.setAttachment(attachments);
             message.setInlineAttachment(inline);
             return message;
         });
     }
 
+    private void validate(List<String> to, List<String> cc, List<String> bcc) {
+        try {
+            for (String email : to) {
+                new EmailAddress(email);
+            }
+            for (String email : cc) {
+                new EmailAddress(email);
+            }
+            for (String email : bcc) {
+                new EmailAddress(email);
+            }
+        } catch (IllegalArgumentException e) {
+            // One of the email addresses is invalid
+            if (logInvalidRecipients) {
+                // We are allowed to log the invalid email address
+                // The exception message contains the invalid email address.
+                LOGGER.warn("Unable to send an email", e);
+                throw new IllegalArgumentException("Unable to send an email", e);
+            } else {
+                // Do not print the invalid email address.
+                LOGGER.warn("Unable to send an email, an email address is invalid");
+                throw new IllegalArgumentException("Unable to send an email, an email address is invalid");
+            }
+        }
+    }
+
     private MultiMap toMultimap(Map<String, List<String>> headers) {
         MultiMap mm = MultiMap.caseInsensitiveMultiMap();
         headers.forEach(mm::add);
diff --git a/extensions/mailer/runtime/src/test/java/io/quarkus/mailer/runtime/MailerImplTest.java b/extensions/mailer/runtime/src/test/java/io/quarkus/mailer/runtime/MailerImplTest.java
@@ -59,7 +59,7 @@ void init() {
         mailer = new MutinyMailerImpl(vertx,
                 MailClient.createShared(vertx,
                         new MailConfig().setPort(wiser.getServer().getPort())),
-                null, FROM, null, false, List.of(), false);
+                null, FROM, null, false, List.of(), false, false);
 
         wiser.getMessages().clear();
     }
diff --git a/extensions/mailer/runtime/src/test/java/io/quarkus/mailer/runtime/MailerWithMultipartImplTest.java b/extensions/mailer/runtime/src/test/java/io/quarkus/mailer/runtime/MailerWithMultipartImplTest.java
@@ -62,7 +62,7 @@ static void stopWiser() {
     void init() {
         mailer = new MutinyMailerImpl(vertx, MailClient.createShared(vertx,
                 new MailConfig().setPort(wiser.getServer().getPort()).setMultiPartOnly(true)), null,
-                FROM, null, false, List.of(), false);
+                FROM, null, false, List.of(), false, false);
 
         wiser.getMessages().clear();
     }
diff --git a/extensions/mailer/runtime/src/test/java/io/quarkus/mailer/runtime/MockMailerImplTest.java b/extensions/mailer/runtime/src/test/java/io/quarkus/mailer/runtime/MockMailerImplTest.java
@@ -35,7 +35,7 @@ static void stop() {
     @BeforeEach
     void init() {
         mockMailbox = new MockMailboxImpl();
-        mailer = new MutinyMailerImpl(vertx, null, mockMailbox, FROM, null, true, List.of(), false);
+        mailer = new MutinyMailerImpl(vertx, null, mockMailbox, FROM, null, true, List.of(), false, false);
     }
 
     @Test

Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,7 @@`
`9`	`9`	`import io.quarkus.mailer.MockMailbox;`
`10`	`10`	`import io.smallrye.mutiny.Uni;`
`11`	`11`	`import io.vertx.ext.mail.MailMessage;`
	`12`	`+import io.vertx.ext.mail.mailencoder.EmailAddress;`
`12`	`13`
`13`	`14`	`/**`
`14`	`15`	`* Mock mailbox bean, will be populated if mocking emails.`
`@@ -22,22 +23,30 @@ public class MockMailboxImpl implements MockMailbox {`
`22`	`23`	`Uni<Void> send(Mail email, MailMessage mailMessage) {`
`23`	`24`	`if (email.getTo() != null) {`
`24`	`25`	`for (String to : email.getTo()) {`
	`26`	`+ validateEmailAddress(to);`
`25`	`27`	`send(email, mailMessage, to);`
`26`	`28`	`}`
`27`	`29`	`}`
`28`	`30`	`if (email.getCc() != null) {`
`29`	`31`	`for (String to : email.getCc()) {`
	`32`	`+ validateEmailAddress(to);`
`30`	`33`	`send(email, mailMessage, to);`
`31`	`34`	`}`
`32`	`35`	`}`
`33`	`36`	`if (email.getBcc() != null) {`
`34`	`37`	`for (String to : email.getBcc()) {`
	`38`	`+ validateEmailAddress(to);`
`35`	`39`	`send(email, mailMessage, to);`
`36`	`40`	`}`
`37`	`41`	`}`
`38`	`42`	`return Uni.createFrom().item(() -> null);`
`39`	`43`	`}`
`40`	`44`
	`45`	`+ private void validateEmailAddress(String to) {`
	`46`	`+ // Just here to validate the email address.`
	`47`	`+ new EmailAddress(to);`
	`48`	`+ }`
	`49`	`+`
`41`	`50`	`private void send(Mail sentMail, MailMessage sentMailMessage, String to) {`
`42`	`51`	`sentMails.computeIfAbsent(to, k -> new ArrayList<>()).add(sentMail);`
`43`	`52`	`sentMailMessages.computeIfAbsent(to, k -> new ArrayList<>()).add(sentMailMessage);`
Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,7 @@ void init() {`
`59`	`59`	`mailer = new MutinyMailerImpl(vertx,`
`60`	`60`	`MailClient.createShared(vertx,`
`61`	`61`	`new MailConfig().setPort(wiser.getServer().getPort())),`
`62`		`- null, FROM, null, false, List.of(), false);`
	`62`	`+ null, FROM, null, false, List.of(), false, false);`
`63`	`63`
`64`	`64`	`wiser.getMessages().clear();`
`65`	`65`	`}`
Original file line number	Diff line number	Diff line change
`@@ -62,7 +62,7 @@ static void stopWiser() {`
`62`	`62`	`void init() {`
`63`	`63`	`mailer = new MutinyMailerImpl(vertx, MailClient.createShared(vertx,`
`64`	`64`	`new MailConfig().setPort(wiser.getServer().getPort()).setMultiPartOnly(true)), null,`
`65`		`- FROM, null, false, List.of(), false);`
	`65`	`+ FROM, null, false, List.of(), false, false);`
`66`	`66`
`67`	`67`	`wiser.getMessages().clear();`
`68`	`68`	`}`
Original file line number	Diff line number	Diff line change
`@@ -35,7 +35,7 @@ static void stop() {`
`35`	`35`	`@BeforeEach`
`36`	`36`	`void init() {`
`37`	`37`	`mockMailbox = new MockMailboxImpl();`
`38`		`- mailer = new MutinyMailerImpl(vertx, null, mockMailbox, FROM, null, true, List.of(), false);`
	`38`	`+ mailer = new MutinyMailerImpl(vertx, null, mockMailbox, FROM, null, true, List.of(), false, false);`
`39`	`39`	`}`
`40`	`40`
`41`	`41`	`@Test`