From daa1880567f3b1ce79a6b73342c33024009c1bdc Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 31 Oct 2019 11:33:11 +0900 Subject: [PATCH 1/4] OutgoingEmail#send: Check first if email sending is enabled Move the check for email sending being disabled to the beginning of the method. There's no point doing anything else if it's disabled. Change-Id: I97cf1ed64453067ac9330aaf4216fdb18ea0d5d3 --- .../com/google/gerrit/server/mail/send/OutgoingEmail.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index 7420611522..debe684aa1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java @@ -111,16 +111,16 @@ public abstract class OutgoingEmail { * @throws EmailException */ public void send() throws EmailException { - if (NotifyHandling.NONE.equals(notify) && accountsToNotify.isEmpty()) { - return; - } - if (!args.emailSender.isEnabled()) { // Server has explicitly disabled email sending. // return; } + if (NotifyHandling.NONE.equals(notify) && accountsToNotify.isEmpty()) { + return; + } + init(); if (useHtml()) { appendHtml(soyHtmlTemplate("HeaderHtml")); From fcfeee87707e396f4a8d0c37d705d10899cdf73d Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 31 Oct 2019 11:41:03 +0900 Subject: [PATCH 2/4] OutgoingEmail: Log the reason that email is not being sent There are various conditions that can result in an email not being sent. Add debug logs to make it easier to track down. Change-Id: I96af52d6e0105661304a5ffcf8e5e1ee3f7498b0 --- .../gerrit/server/mail/send/OutgoingEmail.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index debe684aa1..99fb0b7cd5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java @@ -54,6 +54,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.StringJoiner; +import java.util.function.Supplier; import org.apache.commons.lang.StringUtils; import org.apache.velocity.Template; import org.apache.velocity.VelocityContext; @@ -114,10 +115,12 @@ public abstract class OutgoingEmail { if (!args.emailSender.isEnabled()) { // Server has explicitly disabled email sending. // + logNotSending(() -> "Email sending is disabled by server config"); return; } if (NotifyHandling.NONE.equals(notify) && accountsToNotify.isEmpty()) { + logNotSending(() -> "Notify handling is NONE"); return; } @@ -164,6 +167,7 @@ public abstract class OutgoingEmail { new Address(thisUser.getFullName(), thisUser.getPreferredEmail())); } if (smtpRcptTo.isEmpty() && smtpRcptToPlaintextOnly.isEmpty()) { + logNotSending(() -> "No SMTP recipients"); return; } } @@ -202,6 +206,8 @@ public abstract class OutgoingEmail { try { validator.validateOutgoingEmail(va); } catch (ValidationException e) { + logNotSending( + () -> String.format("Rejected by outgoing email validator: %s", e.getMessage())); return; } } @@ -405,6 +411,7 @@ public abstract class OutgoingEmail { protected boolean shouldSendMessage() { if (textBody.length() == 0) { // If we have no message body, don't send. + logNotSending(() -> "No message body"); return false; } @@ -412,6 +419,7 @@ public abstract class OutgoingEmail { // If we have nobody to send this message to, then all of our // selection filters previously for this type of message were // unable to match a destination. Don't bother sending it. + logNotSending(() -> "No recipients"); return false; } @@ -421,12 +429,19 @@ public abstract class OutgoingEmail { && rcptTo.contains(fromId)) { // If the only recipient is also the sender, don't bother. // + logNotSending(() -> "Sender is only recipient"); return false; } return true; } + private void logNotSending(Supplier reason) { + if (log.isDebugEnabled()) { + log.debug("Not sending '{}': {}", messageClass, reason.get()); + } + } + /** Schedule this message for delivery to the listed accounts. */ protected void add(RecipientType rt, Collection list) { add(rt, list, false); From 8497b332f58df4aadfb1ca99eb2676c94290ac83 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 31 Oct 2019 13:56:15 +0900 Subject: [PATCH 3/4] OutgoingEmail: Log when email is sent as multipart and plaintext Change-Id: I7ef03e50d80a1a534bcf466e0754857cc1abd2fe --- .../java/com/google/gerrit/server/mail/send/OutgoingEmail.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index 99fb0b7cd5..51a48a9af4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java @@ -214,10 +214,12 @@ public abstract class OutgoingEmail { if (!smtpRcptTo.isEmpty()) { // Send multipart message + log.debug("Sending multipart '{}'", messageClass); args.emailSender.send(va.smtpFromAddress, va.smtpRcptTo, va.headers, va.body, va.htmlBody); } if (!smtpRcptToPlaintextOnly.isEmpty()) { + log.debug("Sending plaintext '{}'", messageClass); // Send plaintext message Map shallowCopy = new HashMap<>(); shallowCopy.putAll(headers); From fd8b0f2ff2bb3377985d75be850f96022f2b8b02 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 31 Oct 2019 13:57:23 +0900 Subject: [PATCH 4/4] OutgoingEmail: Sanity check recipients of multipart and plaintext The email gets sent twice; once to users who have requested HTML/multipart and again to users who have requested plain text. Add a sanity check and log an error if there is any intersection between the two sets of users. If this is the case it means those users will get the mail twice. Change-Id: Id8fafe335872235a84c0f34905a7ad3215ad0c2f --- .../com/google/gerrit/server/mail/send/OutgoingEmail.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index 51a48a9af4..26b8b9c588 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/OutgoingEmail.java @@ -21,6 +21,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ListMultimap; +import com.google.common.collect.Sets; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.extensions.api.changes.NotifyHandling; @@ -212,6 +213,11 @@ public abstract class OutgoingEmail { } } + Set
intersection = Sets.intersection(smtpRcptTo, smtpRcptToPlaintextOnly); + if (!intersection.isEmpty()) { + log.error("Email '{}' will be sent twice to {}", messageClass, intersection); + } + if (!smtpRcptTo.isEmpty()) { // Send multipart message log.debug("Sending multipart '{}'", messageClass);