Remove usage of AccountByEmailCache
With change I1c24da1378 there is a new Emails class that allows looking
up accounts by email. To find accounts by email it gets external IDs by
email from the ExternalIdCache and extracts the account IDs from the
external IDs. This is exactly what AccountByEmailCacheImpl.Loader was
doing. In addition the Emails class does an index lookup to also find
accounts by preferred email (see commit message of change I1c24da1378
for an explanation of why this is needed).
Adapt all code to use the new Emails class instead of the
AccountByEmailCache.
Looking up accounts by email via the ExternalIdCache means that the SHA1
of the refs/meta/external-ids branch is read on each lookup (to verify
that the cache is up to date). To avoid reading the SHA1 of the
refs/meta/external-ids branch multiple times when looking up accounts
by email in a loop the Emails class offers a method that can lookup
accounts for several emails at once. This method is currently not used
by Gerrit core, but plugins may need it (e.g. the find-owners plugin).
When emails are changed the ExternalIdCache is automatically evicted
since it detects when the refs/meta/external-ids branch was updated,
hence manual cache eviction for this cache is not needed.
Accounts are also reindexed if the preferred email is changed so that
looking up accounts by preferred email via the account index always
returns up-to-date results.
The AccountByEmailCache is only removed in the follow-up change. This
allows Google to adapt internal code to use the new API before the
AccountByEmailCache is dropped.
Change-Id: I991d21b1acc11025a23504655b5a2c4fea795acf
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 8528853..6c1fb39 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -78,7 +78,6 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.Sequences;
-import com.google.gerrit.server.account.AccountByEmailCache;
import com.google.gerrit.server.account.AccountConfig;
import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.account.Emails;
@@ -145,8 +144,6 @@
@Inject private AccountsUpdate.Server accountsUpdate;
- @Inject private AccountByEmailCache byEmailCache;
-
@Inject private ExternalIds externalIds;
@Inject private ExternalIdsUpdate.User externalIdsUpdateFactory;
@@ -680,28 +677,6 @@
}
@Test
- public void lookUpFromCacheByEmail() throws Exception {
- // exact match with scheme "mailto:"
- assertEmail(byEmailCache.get(admin.email), admin);
-
- // exact match with other scheme
- String email = "foo.bar@example.com";
- externalIdsUpdateFactory
- .create()
- .insert(ExternalId.createWithEmail(ExternalId.Key.parse("foo:bar"), admin.id, email));
- assertEmail(byEmailCache.get(email), admin);
-
- // wrong case doesn't match
- assertThat(byEmailCache.get(admin.email.toUpperCase(Locale.US))).isEmpty();
-
- // prefix doesn't match
- assertThat(byEmailCache.get(admin.email.substring(0, admin.email.indexOf('@')))).isEmpty();
-
- // non-existing doesn't match
- assertThat(byEmailCache.get("non-existing@example.com")).isEmpty();
- }
-
- @Test
public void lookUpByEmail() throws Exception {
// exact match with scheme "mailto:"
assertEmail(emails.getAccountFor(admin.email), admin);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java
index 7f66b9c..894f7a1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java
@@ -36,22 +36,22 @@
public class AccountResolver {
private final Realm realm;
private final Accounts accounts;
- private final AccountByEmailCache byEmail;
private final AccountCache byId;
private final Provider<InternalAccountQuery> accountQueryProvider;
+ private final Emails emails;
@Inject
AccountResolver(
Realm realm,
Accounts accounts,
- AccountByEmailCache byEmail,
AccountCache byId,
- Provider<InternalAccountQuery> accountQueryProvider) {
+ Provider<InternalAccountQuery> accountQueryProvider,
+ Emails emails) {
this.realm = realm;
this.accounts = accounts;
- this.byEmail = byEmail;
this.byId = byId;
this.accountQueryProvider = accountQueryProvider;
+ this.emails = emails;
}
/**
@@ -136,7 +136,8 @@
* @return the single account that matches; null if no account matches or there are multiple
* candidates.
*/
- public Account findByNameOrEmail(ReviewDb db, String nameOrEmail) throws OrmException {
+ public Account findByNameOrEmail(ReviewDb db, String nameOrEmail)
+ throws OrmException, IOException {
Set<Account.Id> r = findAllByNameOrEmail(db, nameOrEmail);
return r.size() == 1 ? byId.get(r.iterator().next()).getAccount() : null;
}
@@ -149,11 +150,12 @@
* address ("email@example"), a full name ("Full Name").
* @return the accounts that match, empty collection if none. Never null.
*/
- public Set<Account.Id> findAllByNameOrEmail(ReviewDb db, String nameOrEmail) throws OrmException {
+ public Set<Account.Id> findAllByNameOrEmail(ReviewDb db, String nameOrEmail)
+ throws OrmException, IOException {
int lt = nameOrEmail.indexOf('<');
int gt = nameOrEmail.indexOf('>');
if (lt >= 0 && gt > lt && nameOrEmail.contains("@")) {
- Set<Account.Id> ids = byEmail.get(nameOrEmail.substring(lt + 1, gt));
+ Set<Account.Id> ids = emails.getAccountFor(nameOrEmail.substring(lt + 1, gt));
if (ids.isEmpty() || ids.size() == 1) {
return ids;
}
@@ -171,7 +173,7 @@
}
if (nameOrEmail.contains("@")) {
- return byEmail.get(nameOrEmail);
+ return emails.getAccountFor(nameOrEmail);
}
Account.Id id = realm.lookup(nameOrEmail);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java
index d8e46f4..9d9cf23 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java
@@ -19,20 +19,23 @@
import com.google.gerrit.extensions.client.AuthType;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.config.AuthConfig;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
+import java.io.IOException;
import java.util.Set;
@Singleton
public class DefaultRealm extends AbstractRealm {
private final EmailExpander emailExpander;
- private final AccountByEmailCache byEmail;
+ private final Provider<Emails> emails;
private final AuthConfig authConfig;
@Inject
- DefaultRealm(EmailExpander emailExpander, AccountByEmailCache byEmail, AuthConfig authConfig) {
+ DefaultRealm(EmailExpander emailExpander, Provider<Emails> emails, AuthConfig authConfig) {
this.emailExpander = emailExpander;
- this.byEmail = byEmail;
+ this.emails = emails;
this.authConfig = authConfig;
}
@@ -75,11 +78,15 @@
public void onCreateAccount(AuthRequest who, Account account) {}
@Override
- public Account.Id lookup(String accountName) {
+ public Account.Id lookup(String accountName) throws IOException {
if (emailExpander.canExpand(accountName)) {
- final Set<Account.Id> c = byEmail.get(emailExpander.expand(accountName));
- if (1 == c.size()) {
- return c.iterator().next();
+ try {
+ Set<Account.Id> c = emails.get().getAccountFor(emailExpander.expand(accountName));
+ if (1 == c.size()) {
+ return c.iterator().next();
+ }
+ } catch (OrmException e) {
+ throw new IOException("Failed to query accounts by email", e);
}
}
return null;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/Realm.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/Realm.java
index 5d551bc..b5e4cba 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/Realm.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/Realm.java
@@ -17,6 +17,7 @@
import com.google.gerrit.extensions.client.AccountFieldName;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.IdentifiedUser;
+import java.io.IOException;
import java.util.Set;
public interface Realm {
@@ -43,5 +44,5 @@
* where there is an {@link EmailExpander} configured that knows how to convert the accountName
* into an email address, and then locate the user by that email address.
*/
- Account.Id lookup(String accountName);
+ Account.Id lookup(String accountName) throws IOException;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
index ce04f26..d31c26d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
@@ -35,8 +35,8 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.GerritPersonIdent;
-import com.google.gerrit.server.account.AccountByEmailCache;
import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.change.ChangeKindCache;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.data.AccountAttribute;
@@ -83,9 +83,9 @@
private static final Logger log = LoggerFactory.getLogger(EventFactory.class);
private final AccountCache accountCache;
+ private final Emails emails;
private final Provider<String> urlProvider;
private final PatchListCache patchListCache;
- private final AccountByEmailCache byEmailCache;
private final PersonIdent myIdent;
private final ChangeData.Factory changeDataFactory;
private final ApprovalsUtil approvalsUtil;
@@ -96,8 +96,8 @@
@Inject
EventFactory(
AccountCache accountCache,
+ Emails emails,
@CanonicalWebUrl @Nullable Provider<String> urlProvider,
- AccountByEmailCache byEmailCache,
PatchListCache patchListCache,
@GerritPersonIdent PersonIdent myIdent,
ChangeData.Factory changeDataFactory,
@@ -106,9 +106,9 @@
Provider<InternalChangeQuery> queryProvider,
SchemaFactory<ReviewDb> schema) {
this.accountCache = accountCache;
+ this.emails = emails;
this.urlProvider = urlProvider;
this.patchListCache = patchListCache;
- this.byEmailCache = byEmailCache;
this.myIdent = myIdent;
this.changeDataFactory = changeDataFactory;
this.approvalsUtil = approvalsUtil;
@@ -497,7 +497,7 @@
}
}
p.kind = changeKindCache.getChangeKind(db, change, patchSet);
- } catch (IOException e) {
+ } catch (IOException | OrmException e) {
log.error("Cannot load patch set data for " + patchSet.getId(), e);
} catch (PatchListNotAvailableException e) {
log.error(String.format("Cannot get size information for %s.", pId), e);
@@ -507,7 +507,7 @@
// TODO: The same method exists in PatchSetInfoFactory, find a common place
// for it
- private UserIdentity toUserIdentity(PersonIdent who) {
+ private UserIdentity toUserIdentity(PersonIdent who) throws IOException, OrmException {
UserIdentity u = new UserIdentity();
u.setName(who.getName());
u.setEmail(who.getEmailAddress());
@@ -517,7 +517,7 @@
// If only one account has access to this email address, select it
// as the identity of the user.
//
- Set<Account.Id> a = byEmailCache.get(u.getEmail());
+ Set<Account.Id> a = emails.getAccountFor(u.getEmail());
if (a.size() == 1) {
u.setAccount(a.iterator().next());
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
index 9d2286e..3884d43 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -425,6 +425,7 @@
* @throws OrmException an error occurred reading or writing the database.
* @throws RestApiException if an error occurred.
* @throws PermissionBackendException if permissions can't be checked
+ * @throws IOException an error occurred reading from NoteDb.
*/
public void merge(
ReviewDb db,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergedByPushOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergedByPushOp.java
index 59017e7..a44d21c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergedByPushOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergedByPushOp.java
@@ -198,7 +198,7 @@
change, patchSet, ctx.getAccount(), patchSet.getRevision().get(), ctx.getWhen());
}
- private PatchSetInfo getPatchSetInfo(ChangeContext ctx) throws IOException {
+ private PatchSetInfo getPatchSetInfo(ChangeContext ctx) throws IOException, OrmException {
RevWalk rw = ctx.getRevWalk();
RevCommit commit =
rw.parseCommit(ObjectId.fromString(checkNotNull(patchSet).getRevision().get()));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index 40120df..0278cdd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -2546,7 +2546,7 @@
RefNames.refsEdit(user.getAccountId(), notes.getChangeId(), psId));
}
- private void newPatchSet() throws IOException {
+ private void newPatchSet() throws IOException, OrmException {
RevCommit newCommit = rp.getRevWalk().parseCommit(newCommitId);
psId =
ChangeUtil.nextPatchSetIdFromAllRefsMap(allRefs, notes.getChange().currentPatchSetId());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
index 49399ef..1b01c26 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
@@ -89,7 +89,8 @@
}
@Override
- protected void updateRepoImpl(RepoContext ctx) throws IntegrationException, IOException {
+ protected void updateRepoImpl(RepoContext ctx)
+ throws IntegrationException, IOException, OrmException {
// If there is only one parent, a cherry-pick can be done by taking the
// delta relative to that one parent and redoing that on the current merge
// tip.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java
index 5dae659..a752324 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java
@@ -24,6 +24,7 @@
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gwtorm.server.OrmException;
+import java.io.IOException;
import java.time.format.DateTimeFormatter;
import java.util.Collections;
import java.util.HashSet;
@@ -42,7 +43,7 @@
AccountResolver accountResolver,
boolean draftPatchSet,
List<FooterLine> footerLines)
- throws OrmException {
+ throws OrmException, IOException {
MailRecipients recipients = new MailRecipients();
if (!draftPatchSet) {
for (FooterLine footerLine : footerLines) {
@@ -70,7 +71,7 @@
private static Account.Id toAccountId(
ReviewDb db, AccountResolver accountResolver, String nameOrEmail)
- throws OrmException, NoSuchAccountException {
+ throws OrmException, NoSuchAccountException, IOException {
Account a = accountResolver.findByNameOrEmail(db, nameOrEmail);
if (a == null) {
throw new NoSuchAccountException("\"" + nameOrEmail + "\" is not registered");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index 24bae37..68bcda4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -36,8 +36,8 @@
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
-import com.google.gerrit.server.account.AccountByEmailCache;
import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.change.EmailReviewComments;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.extensions.events.CommentAdded;
@@ -58,6 +58,7 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
+import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
@@ -73,7 +74,7 @@
public class MailProcessor {
private static final Logger log = LoggerFactory.getLogger(MailProcessor.class);
- private final AccountByEmailCache accountByEmailCache;
+ private final Emails emails;
private final RetryHelper retryHelper;
private final ChangeMessagesUtil changeMessagesUtil;
private final CommentsUtil commentsUtil;
@@ -90,7 +91,7 @@
@Inject
public MailProcessor(
- AccountByEmailCache accountByEmailCache,
+ Emails emails,
RetryHelper retryHelper,
ChangeMessagesUtil changeMessagesUtil,
CommentsUtil commentsUtil,
@@ -104,7 +105,7 @@
CommentAdded commentAdded,
AccountCache accountCache,
@CanonicalWebUrl Provider<String> canonicalUrl) {
- this.accountByEmailCache = accountByEmailCache;
+ this.emails = emails;
this.retryHelper = retryHelper;
this.changeMessagesUtil = changeMessagesUtil;
this.commentsUtil = commentsUtil;
@@ -134,7 +135,7 @@
}
private void processImpl(BatchUpdate.Factory buf, MailMessage message)
- throws OrmException, UpdateException, RestApiException {
+ throws OrmException, UpdateException, RestApiException, IOException {
for (DynamicMap.Entry<MailFilter> filter : mailFilters) {
if (!filter.getProvider().get().shouldProcessMessage(message)) {
log.warn(
@@ -154,15 +155,15 @@
return;
}
- Set<Account.Id> accounts = accountByEmailCache.get(metadata.author);
- if (accounts.size() != 1) {
+ Set<Account.Id> accountIds = emails.getAccountFor(metadata.author);
+ if (accountIds.size() != 1) {
log.error(
String.format(
"Address %s could not be matched to a unique account. It was matched to %s. Will delete message.",
- metadata.author, accounts));
+ metadata.author, accountIds));
return;
}
- Account.Id account = accounts.iterator().next();
+ Account.Id account = accountIds.iterator().next();
if (!accountCache.get(account).getAccount().isActive()) {
log.warn(String.format("Mail: Account %s is inactive. Will delete message.", account));
return;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java
index cd9c4c3..41bade6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java
@@ -22,7 +22,7 @@
import com.google.gerrit.reviewdb.client.UserIdentity;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.PatchSetUtil;
-import com.google.gerrit.server.account.AccountByEmailCache;
+import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gwtorm.server.OrmException;
@@ -45,17 +45,17 @@
public class PatchSetInfoFactory {
private final GitRepositoryManager repoManager;
private final PatchSetUtil psUtil;
- private final AccountByEmailCache byEmailCache;
+ private final Emails emails;
@Inject
- public PatchSetInfoFactory(
- GitRepositoryManager repoManager, PatchSetUtil psUtil, AccountByEmailCache byEmailCache) {
+ public PatchSetInfoFactory(GitRepositoryManager repoManager, PatchSetUtil psUtil, Emails emails) {
this.repoManager = repoManager;
this.psUtil = psUtil;
- this.byEmailCache = byEmailCache;
+ this.emails = emails;
}
- public PatchSetInfo get(RevWalk rw, RevCommit src, PatchSet.Id psi) throws IOException {
+ public PatchSetInfo get(RevWalk rw, RevCommit src, PatchSet.Id psi)
+ throws IOException, OrmException {
rw.parseBody(src);
PatchSetInfo info = new PatchSetInfo(psi);
info.setSubject(src.getShortMessage());
@@ -84,13 +84,13 @@
PatchSetInfo info = get(rw, src, patchSet.getId());
info.setParents(toParentInfos(src.getParents(), rw));
return info;
- } catch (IOException e) {
+ } catch (IOException | OrmException e) {
throw new PatchSetInfoNotAvailableException(e);
}
}
// TODO: The same method exists in EventFactory, find a common place for it
- private UserIdentity toUserIdentity(PersonIdent who) {
+ private UserIdentity toUserIdentity(PersonIdent who) throws IOException, OrmException {
final UserIdentity u = new UserIdentity();
u.setName(who.getName());
u.setEmail(who.getEmailAddress());
@@ -100,7 +100,7 @@
// If only one account has access to this email address, select it
// as the identity of the user.
//
- final Set<Account.Id> a = byEmailCache.get(u.getEmail());
+ Set<Account.Id> a = emails.getAccountFor(u.getEmail());
if (a.size() == 1) {
u.setAccount(a.iterator().next());
}
