From 618fa6394afe2a12edc0ba290bd3548ff14fe026 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 13 Dec 2019 09:15:32 +0100 Subject: [PATCH 1/2] Document EmailArguments While adding the javadoc I was wondering why this class is not a singleton. Adding a todo to check this later. Change-Id: Ie7639e2d4fff62972873e4c6457e7d00f3606d84 Signed-off-by: Edwin Kempin --- .../gerrit/server/mail/send/EmailArguments.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/java/com/google/gerrit/server/mail/send/EmailArguments.java b/java/com/google/gerrit/server/mail/send/EmailArguments.java index ede5765993..4f07bbebaf 100644 --- a/java/com/google/gerrit/server/mail/send/EmailArguments.java +++ b/java/com/google/gerrit/server/mail/send/EmailArguments.java @@ -50,6 +50,19 @@ import java.util.List; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.PersonIdent; +/** + * Arguments used for sending notification emails. + * + *

Notification emails are sent by out by {@link OutgoingEmail} and it's subclasses, so called + * senders. To construct an email the sender class needs to get various other classes injected. + * Instead of injecting these classes into the sender classes directly, they only get {@code + * EmailArguments} injected and {@code EmailArguments} provides them all dependencies that they + * need. + * + *

This class is public because plugins need access to it for sending emails. + */ +// TODO(ekempin): Can we make this class a singleton so that we don't need to instantiate it for +// each and every email? @UsedAt(UsedAt.Project.PLUGINS_ALL) public class EmailArguments { final GitRepositoryManager server; From 87f26075700ef92026e1ee9df22e82d759fd58f8 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 13 Dec 2019 13:23:47 +0100 Subject: [PATCH 2/2] Make EmailArguments a singleton Most of the things that get injected into EmailArguments are singletons themselves, the others can be turned into providers. Making EmailArguments a singleton prevents that we have to create a new instance of this class whenever an email is sent. Change-Id: I2459a3ea79d94d8ba8203a385cfcca8bb99cfeaf Signed-off-by: Edwin Kempin --- .../server/mail/send/EmailArguments.java | 26 +++++++++---------- .../server/mail/send/OutgoingEmail.java | 18 +++++++------ .../gerrit/server/mail/send/ProjectWatch.java | 4 +-- 3 files changed, 25 insertions(+), 23 deletions(-) diff --git a/java/com/google/gerrit/server/mail/send/EmailArguments.java b/java/com/google/gerrit/server/mail/send/EmailArguments.java index 4f07bbebaf..808d6a4d77 100644 --- a/java/com/google/gerrit/server/mail/send/EmailArguments.java +++ b/java/com/google/gerrit/server/mail/send/EmailArguments.java @@ -45,6 +45,7 @@ import com.google.gerrit.server.ssh.SshAdvertisedAddresses; import com.google.gerrit.server.validators.OutgoingEmailValidationListener; import com.google.inject.Inject; import com.google.inject.Provider; +import com.google.inject.Singleton; import com.google.template.soy.jbcsrc.api.SoySauce; import java.util.List; import org.eclipse.jgit.lib.Config; @@ -61,8 +62,7 @@ import org.eclipse.jgit.lib.PersonIdent; * *

This class is public because plugins need access to it for sending emails. */ -// TODO(ekempin): Can we make this class a singleton so that we don't need to instantiate it for -// each and every email? +@Singleton @UsedAt(UsedAt.Project.PLUGINS_ALL) public class EmailArguments { final GitRepositoryManager server; @@ -73,22 +73,22 @@ public class EmailArguments { final PatchListCache patchListCache; final PatchSetUtil patchSetUtil; final ApprovalsUtil approvalsUtil; - final FromAddressGenerator fromAddressGenerator; + final Provider fromAddressGenerator; final EmailSender emailSender; final PatchSetInfoFactory patchSetInfoFactory; final IdentifiedUser.GenericFactory identifiedUserFactory; final ChangeNotes.Factory changeNotesFactory; - final AnonymousUser anonymousUser; + final Provider anonymousUser; final String anonymousCowardName; - final PersonIdent gerritPersonIdent; + final Provider gerritPersonIdent; final DynamicItem urlFormatter; final AllProjectsName allProjectsName; final List sshAddresses; final SitePaths site; - final ChangeQueryBuilder queryBuilder; + final Provider queryBuilder; final ChangeData.Factory changeDataFactory; - final SoySauce soySauce; + final Provider soySauce; final EmailSettings settings; final DynamicSet outgoingEmailValidationListeners; final Provider accountQueryProvider; @@ -106,19 +106,19 @@ public class EmailArguments { PatchListCache patchListCache, PatchSetUtil patchSetUtil, ApprovalsUtil approvalsUtil, - FromAddressGenerator fromAddressGenerator, + Provider fromAddressGenerator, EmailSender emailSender, PatchSetInfoFactory patchSetInfoFactory, GenericFactory identifiedUserFactory, ChangeNotes.Factory changeNotesFactory, - AnonymousUser anonymousUser, + Provider anonymousUser, @AnonymousCowardName String anonymousCowardName, - GerritPersonIdentProvider gerritPersonIdentProvider, + GerritPersonIdentProvider gerritPersonIdent, DynamicItem urlFormatter, AllProjectsName allProjectsName, - ChangeQueryBuilder queryBuilder, + Provider queryBuilder, ChangeData.Factory changeDataFactory, - @MailTemplates SoySauce soySauce, + @MailTemplates Provider soySauce, EmailSettings settings, @SshAdvertisedAddresses List sshAddresses, SitePaths site, @@ -142,7 +142,7 @@ public class EmailArguments { this.changeNotesFactory = changeNotesFactory; this.anonymousUser = anonymousUser; this.anonymousCowardName = anonymousCowardName; - this.gerritPersonIdent = gerritPersonIdentProvider.get(); + this.gerritPersonIdent = gerritPersonIdent; this.urlFormatter = urlFormatter; this.allProjectsName = allProjectsName; this.queryBuilder = queryBuilder; diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index e81f7f404a..d48ed5985a 100644 --- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java @@ -52,6 +52,7 @@ import java.util.Optional; import java.util.Set; import java.util.StringJoiner; import org.apache.james.mime4j.dom.field.FieldName; +import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.util.SystemReader; /** Sends an email to one or more interested parties. */ @@ -256,7 +257,7 @@ public abstract class OutgoingEmail { protected void init() throws EmailException { setupSoyContext(); - smtpFromAddress = args.fromAddressGenerator.from(fromId); + smtpFromAddress = args.fromAddressGenerator.get().from(fromId); setHeader(FieldName.DATE, new Date()); headers.put(FieldName.FROM, new EmailHeader.AddressList(smtpFromAddress)); headers.put(FieldName.TO, new EmailHeader.AddressList()); @@ -273,7 +274,7 @@ public abstract class OutgoingEmail { textBody = new StringBuilder(); htmlBody = new StringBuilder(); - if (fromId != null && args.fromAddressGenerator.isGenericAddress(fromId)) { + if (fromId != null && args.fromAddressGenerator.get().isGenericAddress(fromId)) { appendText(getFromLine()); } } @@ -353,7 +354,7 @@ public abstract class OutgoingEmail { /** Lookup a human readable name for an account, usually the "full name". */ protected String getNameFor(@Nullable Account.Id accountId) { if (accountId == null) { - return args.gerritPersonIdent.getName(); + return args.gerritPersonIdent.get().getName(); } Optional account = args.accountCache.get(accountId).map(AccountState::account); @@ -379,10 +380,8 @@ public abstract class OutgoingEmail { */ protected String getNameEmailFor(@Nullable Account.Id accountId) { if (accountId == null) { - return args.gerritPersonIdent.getName() - + " <" - + args.gerritPersonIdent.getEmailAddress() - + ">"; + PersonIdent gerritIdent = args.gerritPersonIdent.get(); + return gerritIdent.getName() + " <" + gerritIdent.getEmailAddress() + ">"; } Optional account = args.accountCache.get(accountId).map(AccountState::account); @@ -591,7 +590,10 @@ public abstract class OutgoingEmail { /** Configures a soy renderer for the given template name and rendering data map. */ private SoySauce.Renderer configureRenderer(String templateName) { - return args.soySauce.renderTemplate(SOY_TEMPLATE_NAMESPACE + templateName).setData(soyContext); + return args.soySauce + .get() + .renderTemplate(SOY_TEMPLATE_NAMESPACE + templateName) + .setData(soyContext); } protected void removeUser(Account user) { diff --git a/java/com/google/gerrit/server/mail/send/ProjectWatch.java b/java/com/google/gerrit/server/mail/send/ProjectWatch.java index 934a0a00a0..be36f55ec2 100644 --- a/java/com/google/gerrit/server/mail/send/ProjectWatch.java +++ b/java/com/google/gerrit/server/mail/send/ProjectWatch.java @@ -241,9 +241,9 @@ public class ProjectWatch { Predicate p = null; if (user == null) { - qb = args.queryBuilder.asUser(args.anonymousUser); + qb = args.queryBuilder.get().asUser(args.anonymousUser.get()); } else { - qb = args.queryBuilder.asUser(user); + qb = args.queryBuilder.get().asUser(user); p = qb.is_visible(); }