AccountByEmailCacheImpl: Consider emails from all external IDs on load
Since change I643827 the cache loader uses the account index instead
of ReviewDb for loading accounts by email, but due to this change
email addresses from schemes other than SCHEME_MAILTO are ignored now.
Fix this by using the email predicate when searching for accounts by
email.
Since the EMAIL field in the account index is a prefix field and the
email predicate is case-insensitive, the search result may contain
accounts where the email address does not match exactly. This is why
we must check for each account that was found if it has an email
address that matches exactly before the account is included into the
result set.
Change-Id: Ib522a86c2438f9b220ea90841f4c23414a80745d
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 82aa576..2be7b6d 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
@@ -35,6 +35,7 @@
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.common.io.BaseEncoding;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.AccountCreator;
@@ -63,6 +64,7 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountExternalId;
import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.account.AccountByEmailCache;
import com.google.gerrit.server.account.WatchConfig;
import com.google.gerrit.server.account.WatchConfig.NotifyType;
import com.google.gerrit.server.config.AllUsersName;
@@ -81,6 +83,7 @@
import java.util.EnumSet;
import java.util.Iterator;
import java.util.List;
+import java.util.Locale;
import java.util.Map;
import java.util.Set;
import org.bouncycastle.bcpg.ArmoredOutputStream;
@@ -111,6 +114,8 @@
@Inject private AllUsersName allUsers;
+ @Inject private AccountByEmailCache byEmailCache;
+
private List<AccountExternalId> savedExternalIds;
@Before
@@ -487,6 +492,27 @@
}
@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";
+ db.accountExternalIds().insert(ImmutableList.of(createExternalIdWithEmail("foo:bar", email)));
+ accountCache.evict(admin.id);
+ 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 putStatus() throws Exception {
List<String> statuses = ImmutableList.of("OOO", "Busy");
AccountInfo info;
@@ -937,4 +963,9 @@
extId.setEmailAddress(email);
return extId;
}
+
+ private void assertEmail(Set<Account.Id> accounts, TestAccount expectedAccount) {
+ assertThat(accounts).hasSize(1);
+ assertThat(Iterables.getOnlyElement(accounts)).isEqualTo(expectedAccount.getId());
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountByEmailCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountByEmailCacheImpl.java
index 56c41e0..29ee20d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountByEmailCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountByEmailCacheImpl.java
@@ -18,7 +18,6 @@
import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.AccountExternalId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.query.account.InternalAccountQuery;
@@ -94,12 +93,15 @@
for (Account a : db.accounts().byPreferredEmail(email)) {
r.add(a.getId());
}
- for (AccountState accountState :
- accountQueryProvider
- .get()
- .byExternalId(
- (new AccountExternalId.Key(AccountExternalId.SCHEME_MAILTO, email)).get())) {
- r.add(accountState.getAccount().getId());
+ for (AccountState accountState : accountQueryProvider.get().byEmailPrefix(email)) {
+ if (accountState
+ .getExternalIds()
+ .stream()
+ .filter(e -> email.equals(e.getEmailAddress()))
+ .findAny()
+ .isPresent()) {
+ r.add(accountState.getAccount().getId());
+ }
}
return ImmutableSet.copyOf(r);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java
index 1c336d4..3e34ac2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java
@@ -67,6 +67,10 @@
return query(AccountPredicates.defaultPredicate(query));
}
+ public List<AccountState> byEmailPrefix(String emailPrefix) throws OrmException {
+ return query(AccountPredicates.email(emailPrefix));
+ }
+
public List<AccountState> byExternalId(String externalId) throws OrmException {
return query(AccountPredicates.externalId(externalId));
}
