Let ExternalIdsUpdate take care to evict accounts from the account cache
The account cache holds AccountState instances which contain the
external IDs of the accounts. Hence an account must be evicted from the
account cache when its external IDs are updated. At the moment it's the
responsibility of the caller to do the account eviction, but it can
easily be forgotten and it's more convenient if ExternalIdsUpdate takes
care of it. For some scenarios this may result in a few more cache
evictions (e.g. account creation), but for most operations the number of
account evictions should stay the same.
After updating external IDs the corresponding accounts also need to be
reindexed, but this is automatically done when accounts are evicted from
the account cache.
Change-Id: I1af02c7576eea81bb229a4663cb1e067ab137784
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
index e136bb3..20ae2d1 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
@@ -29,7 +29,6 @@
import com.google.gerrit.server.account.ExternalIdsUpdate;
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.account.VersionedAuthorizedKeys;
-import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.server.ssh.SshKeyCache;
import com.google.gerrit.testutil.SshMode;
import com.google.gwtorm.server.SchemaFactory;
@@ -56,7 +55,6 @@
private final SshKeyCache sshKeyCache;
private final AccountCache accountCache;
private final AccountByEmailCache byEmailCache;
- private final AccountIndexer indexer;
private final ExternalIdsUpdate.Server externalIdsUpdate;
@Inject
@@ -67,7 +65,6 @@
SshKeyCache sshKeyCache,
AccountCache accountCache,
AccountByEmailCache byEmailCache,
- AccountIndexer indexer,
ExternalIdsUpdate.Server externalIdsUpdate) {
accounts = new HashMap<>();
reviewDbProvider = schema;
@@ -76,7 +73,6 @@
this.sshKeyCache = sshKeyCache;
this.accountCache = accountCache;
this.byEmailCache = byEmailCache;
- this.indexer = indexer;
this.externalIdsUpdate = externalIdsUpdate;
}
@@ -120,11 +116,10 @@
sshKeyCache.evict(username);
}
+ accountCache.evict(id);
accountCache.evictByUsername(username);
byEmailCache.evict(email);
- indexer.index(id);
-
account = new TestAccount(id, username, email, fullName, sshKey, httpPass);
accounts.put(username, account);
return account;
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 2afc8c4..75fd448 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
@@ -140,8 +140,6 @@
externalIdsUpdate.delete(db, getExternalIds(user));
externalIdsUpdate.insert(db, savedExternalIds);
}
- accountCache.evict(admin.getId());
- accountCache.evict(user.getId());
}
@After
@@ -455,7 +453,6 @@
ExternalId.createWithEmail(ExternalId.Key.parse(extId1), admin.id, email),
ExternalId.createWithEmail(ExternalId.Key.parse(extId2), admin.id, email));
externalIdsUpdateFactory.create().insert(db, extIds);
- accountCache.evict(admin.id);
assertThat(
gApi.accounts().self().getExternalIds().stream().map(e -> e.identity).collect(toSet()))
.containsAllOf(extId1, extId2);
@@ -506,7 +503,6 @@
externalIdsUpdateFactory
.create()
.insert(db, ExternalId.createWithEmail(ExternalId.Key.parse("foo:bar"), admin.id, email));
- accountCache.evict(admin.id);
assertEmail(byEmailCache.get(email), admin);
// wrong case doesn't match
@@ -714,7 +710,6 @@
// Both users have a matching external ID for this key.
addExternalIdEmail(admin, "test5@example.com");
externalIdsUpdate.insert(db, ExternalId.create("foo", "myId", user.getId()));
- accountCache.evict(user.getId());
TestKey key = validKeyWithSecondUserId();
addGpgKey(key.getPublicKeyArmored());
@@ -940,8 +935,6 @@
checkNotNull(email);
externalIdsUpdate.insert(
db, ExternalId.createWithEmail(name("test"), email, account.getId(), email));
- // Clear saved AccountState and ExternalIds.
- accountCache.evict(account.getId());
setApiUser(account);
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
index 06b8f68..df44366 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
@@ -191,6 +191,7 @@
ExternalIdsUpdate update =
new ExternalIdsUpdate(
repoManager,
+ accountCache,
allUsers,
serverIdent.get(),
serverIdent.get(),
@@ -223,6 +224,7 @@
ExternalIdsUpdate update =
new ExternalIdsUpdate(
repoManager,
+ accountCache,
allUsers,
serverIdent.get(),
serverIdent.get(),
diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/DeleteGpgKey.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/DeleteGpgKey.java
index 50bf57b..f95cee2 100644
--- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/DeleteGpgKey.java
+++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/DeleteGpgKey.java
@@ -25,7 +25,6 @@
import com.google.gerrit.gpg.server.DeleteGpgKey.Input;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.GerritPersonIdent;
-import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.ExternalId;
import com.google.gerrit.server.account.ExternalIdsUpdate;
import com.google.gwtorm.server.OrmException;
@@ -45,7 +44,6 @@
private final Provider<PersonIdent> serverIdent;
private final Provider<ReviewDb> db;
private final Provider<PublicKeyStore> storeProvider;
- private final AccountCache accountCache;
private final ExternalIdsUpdate.User externalIdsUpdateFactory;
@Inject
@@ -53,12 +51,10 @@
@GerritPersonIdent Provider<PersonIdent> serverIdent,
Provider<ReviewDb> db,
Provider<PublicKeyStore> storeProvider,
- AccountCache accountCache,
ExternalIdsUpdate.User externalIdsUpdateFactory) {
this.serverIdent = serverIdent;
this.db = db;
this.storeProvider = storeProvider;
- this.accountCache = accountCache;
this.externalIdsUpdateFactory = externalIdsUpdateFactory;
}
@@ -74,7 +70,6 @@
rsrc.getUser().getAccountId(),
ExternalId.Key.create(
SCHEME_GPGKEY, BaseEncoding.base16().encode(key.getFingerprint())));
- accountCache.evict(rsrc.getUser().getAccountId());
try (PublicKeyStore store = storeProvider.get()) {
store.remove(rsrc.getKeyRing().getPublicKey().getFingerprint());
diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java
index 7b825b1..165402c 100644
--- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java
+++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java
@@ -44,7 +44,6 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountResource;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.ExternalId;
@@ -89,7 +88,6 @@
private final Provider<PublicKeyStore> storeProvider;
private final GerritPublicKeyChecker.Factory checkerFactory;
private final AddKeySender.Factory addKeyFactory;
- private final AccountCache accountCache;
private final Provider<InternalAccountQuery> accountQueryProvider;
private final ExternalIdsUpdate.User externalIdsUpdateFactory;
@@ -101,7 +99,6 @@
Provider<PublicKeyStore> storeProvider,
GerritPublicKeyChecker.Factory checkerFactory,
AddKeySender.Factory addKeyFactory,
- AccountCache accountCache,
Provider<InternalAccountQuery> accountQueryProvider,
ExternalIdsUpdate.User externalIdsUpdateFactory) {
this.serverIdent = serverIdent;
@@ -110,7 +107,6 @@
this.storeProvider = storeProvider;
this.checkerFactory = checkerFactory;
this.addKeyFactory = addKeyFactory;
- this.accountCache = accountCache;
this.accountQueryProvider = accountQueryProvider;
this.externalIdsUpdateFactory = externalIdsUpdateFactory;
}
@@ -148,7 +144,6 @@
externalIdsUpdateFactory
.create()
.replace(db.get(), rsrc.getUser().getAccountId(), extIdKeysToRemove, newExtIds);
- accountCache.evict(rsrc.getUser().getAccountId());
return toJson(newKeys, toRemove, store, rsrc.getUser());
}
}
diff --git a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java
index 862930f..d1bbc5e 100644
--- a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java
+++ b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java
@@ -407,7 +407,6 @@
assertThat(store.save(cb)).isAnyOf(NEW, FAST_FORWARD, FORCED);
externalIdsUpdateFactory.create().insert(db, newExtIds);
- accountCache.evict(user.getAccountId());
}
private TestKey add(TestKey k, IdentifiedUser user) throws Exception {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java
index 77d28f9..b0dcbe4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java
@@ -374,13 +374,10 @@
if (a.getPreferredEmail() == null) {
a.setPreferredEmail(who.getEmailAddress());
db.accounts().update(Collections.singleton(a));
+ byIdCache.evict(to);
}
- }
-
- if (who.getEmailAddress() != null) {
byEmailCache.evict(who.getEmailAddress());
}
- byIdCache.evict(to);
}
return new AuthResult(to, who.getExternalIdKey(), false);
@@ -449,9 +446,9 @@
&& a.getPreferredEmail().equals(who.getEmailAddress())) {
a.setPreferredEmail(null);
db.accounts().update(Collections.singleton(a));
+ byIdCache.evict(from);
}
byEmailCache.evict(who.getEmailAddress());
- byIdCache.evict(from);
}
} else {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java
index f60ee45..978331e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java
@@ -120,7 +120,6 @@
accountCache.evictByUsername(extId.key().id());
}
- accountCache.evict(user.getAccountId());
accountCache.evictByUsername(newUsername);
sshKeyCache.evict(newUsername);
return VoidResult.INSTANCE;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalIdsUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalIdsUpdate.java
index a596a8e..7746709 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalIdsUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalIdsUpdate.java
@@ -84,6 +84,9 @@
* </pre>
*
* For NoteDb each method call results in one commit on refs/meta/external-ids branch.
+ *
+ * <p>On updating external IDs this class takes care to evict affected accounts from the account
+ * cache and thus triggers reindex for them.
*/
public class ExternalIdsUpdate {
private static final String COMMIT_MSG = "Update external IDs";
@@ -97,22 +100,25 @@
@Singleton
public static class Server {
private final GitRepositoryManager repoManager;
+ private final AccountCache accountCache;
private final AllUsersName allUsersName;
private final Provider<PersonIdent> serverIdent;
@Inject
public Server(
GitRepositoryManager repoManager,
+ AccountCache accountCache,
AllUsersName allUsersName,
@GerritPersonIdent Provider<PersonIdent> serverIdent) {
this.repoManager = repoManager;
+ this.accountCache = accountCache;
this.allUsersName = allUsersName;
this.serverIdent = serverIdent;
}
public ExternalIdsUpdate create() {
PersonIdent i = serverIdent.get();
- return new ExternalIdsUpdate(repoManager, allUsersName, i, i);
+ return new ExternalIdsUpdate(repoManager, accountCache, allUsersName, i, i);
}
}
@@ -125,6 +131,7 @@
@Singleton
public static class User {
private final GitRepositoryManager repoManager;
+ private final AccountCache accountCache;
private final AllUsersName allUsersName;
private final Provider<PersonIdent> serverIdent;
private final Provider<IdentifiedUser> identifiedUser;
@@ -132,10 +139,12 @@
@Inject
public User(
GitRepositoryManager repoManager,
+ AccountCache accountCache,
AllUsersName allUsersName,
@GerritPersonIdent Provider<PersonIdent> serverIdent,
Provider<IdentifiedUser> identifiedUser) {
this.repoManager = repoManager;
+ this.accountCache = accountCache;
this.allUsersName = allUsersName;
this.serverIdent = serverIdent;
this.identifiedUser = identifiedUser;
@@ -144,7 +153,7 @@
public ExternalIdsUpdate create() {
PersonIdent i = serverIdent.get();
return new ExternalIdsUpdate(
- repoManager, allUsersName, createPersonIdent(i, identifiedUser.get()), i);
+ repoManager, accountCache, allUsersName, createPersonIdent(i, identifiedUser.get()), i);
}
private PersonIdent createPersonIdent(PersonIdent ident, IdentifiedUser user) {
@@ -166,6 +175,7 @@
private static final Retryer<Void> RETRYER = retryerBuilder().build();
private final GitRepositoryManager repoManager;
+ private final AccountCache accountCache;
private final AllUsersName allUsersName;
private final PersonIdent committerIdent;
private final PersonIdent authorIdent;
@@ -174,21 +184,31 @@
private ExternalIdsUpdate(
GitRepositoryManager repoManager,
+ AccountCache accountCache,
AllUsersName allUsersName,
PersonIdent committerIdent,
PersonIdent authorIdent) {
- this(repoManager, allUsersName, committerIdent, authorIdent, Runnables.doNothing(), RETRYER);
+ this(
+ repoManager,
+ accountCache,
+ allUsersName,
+ committerIdent,
+ authorIdent,
+ Runnables.doNothing(),
+ RETRYER);
}
@VisibleForTesting
public ExternalIdsUpdate(
GitRepositoryManager repoManager,
+ AccountCache accountCache,
AllUsersName allUsersName,
PersonIdent committerIdent,
PersonIdent authorIdent,
Runnable afterReadRevision,
Retryer<Void> retryer) {
this.repoManager = checkNotNull(repoManager, "repoManager");
+ this.accountCache = accountCache;
this.allUsersName = checkNotNull(allUsersName, "allUsersName");
this.committerIdent = checkNotNull(committerIdent, "committerIdent");
this.authorIdent = checkNotNull(authorIdent, "authorIdent");
@@ -222,6 +242,7 @@
insert(o.rw(), o.ins(), o.noteMap(), extId);
}
});
+ evictAccounts(extIds);
}
/**
@@ -249,6 +270,7 @@
upsert(o.rw(), o.ins(), o.noteMap(), extId);
}
});
+ evictAccounts(extIds);
}
/**
@@ -279,6 +301,7 @@
remove(o.rw(), o.noteMap(), extId);
}
});
+ evictAccounts(extIds);
}
/**
@@ -308,6 +331,7 @@
remove(o.rw(), o.noteMap(), accountId, extIdKey);
}
});
+ accountCache.evict(accountId);
}
/** Deletes all external IDs of the specified account. */
@@ -348,6 +372,7 @@
insert(o.rw(), o.ins(), o.noteMap(), extId);
}
});
+ accountCache.evict(accountId);
}
/**
@@ -587,6 +612,12 @@
return ins.insert(OBJ_TREE, new byte[] {});
}
+ private void evictAccounts(Collection<ExternalId> extIds) throws IOException {
+ for (Account.Id id : extIds.stream().map(ExternalId::accountId).collect(toSet())) {
+ accountCache.evict(id);
+ }
+ }
+
private static interface MyConsumer<T> {
void accept(T t) throws IOException, ConfigInvalidException, OrmException;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutHttpPassword.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutHttpPassword.java
index 435671f..29aac58 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutHttpPassword.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutHttpPassword.java
@@ -54,18 +54,15 @@
private final Provider<CurrentUser> self;
private final Provider<ReviewDb> dbProvider;
- private final AccountCache accountCache;
private final ExternalIdsUpdate.User externalIdsUpdate;
@Inject
PutHttpPassword(
Provider<CurrentUser> self,
Provider<ReviewDb> dbProvider,
- AccountCache accountCache,
ExternalIdsUpdate.User externalIdsUpdate) {
this.self = self;
this.dbProvider = dbProvider;
- this.accountCache = accountCache;
this.externalIdsUpdate = externalIdsUpdate;
}
@@ -122,7 +119,6 @@
ExternalId newExtId =
ExternalId.createWithPassword(extId.key(), extId.accountId(), extId.email(), newPassword);
externalIdsUpdate.create().upsert(dbProvider.get(), newExtId);
- accountCache.evict(user.getAccountId());
return Strings.isNullOrEmpty(newPassword) ? Response.<String>none() : Response.ok(newPassword);
}
