Remove AllUsersName from AccountState
AccountState grew a field referencing the name of 'All-Users'. This
field was populated solely for indexing. We want to reduce account state
to the bare minimum information in a sense that it only carries fields
that relate to the account and make the entity immutable if possible.
This commit removes the field and migrates callers. It is perfectly fine
to index an account and build the REF_STATE from the default name. This
is only ever used for checking staleness of the account document where
we have the real All-Users name available for usage.
Change-Id: Idb91a15bef1427bd8026da23a345dd1601d3959d
diff --git a/java/com/google/gerrit/pgm/init/InitAdminUser.java b/java/com/google/gerrit/pgm/init/InitAdminUser.java
index 674f9c1..45c5ea6 100644
--- a/java/com/google/gerrit/pgm/init/InitAdminUser.java
+++ b/java/com/google/gerrit/pgm/init/InitAdminUser.java
@@ -20,7 +20,6 @@
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.exceptions.NoSuchGroupException;
import com.google.gerrit.extensions.client.AuthType;
-import com.google.gerrit.pgm.init.api.AllUsersNameOnInitProvider;
import com.google.gerrit.pgm.init.api.ConsoleUI;
import com.google.gerrit.pgm.init.api.InitFlags;
import com.google.gerrit.pgm.init.api.InitStep;
@@ -29,7 +28,6 @@
import com.google.gerrit.server.account.AccountSshKey;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.externalids.ExternalId;
-import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.index.account.AccountIndex;
import com.google.gerrit.server.index.account.AccountIndexCollection;
@@ -49,7 +47,6 @@
public class InitAdminUser implements InitStep {
private final InitFlags flags;
private final ConsoleUI ui;
- private final AllUsersNameOnInitProvider allUsers;
private final AccountsOnInit accounts;
private final VersionedAuthorizedKeysOnInit.Factory authorizedKeysFactory;
private final ExternalIdsOnInit externalIds;
@@ -62,7 +59,6 @@
InitAdminUser(
InitFlags flags,
ConsoleUI ui,
- AllUsersNameOnInitProvider allUsers,
AccountsOnInit accounts,
VersionedAuthorizedKeysOnInit.Factory authorizedKeysFactory,
ExternalIdsOnInit externalIds,
@@ -70,7 +66,6 @@
GroupsOnInit groupsOnInit) {
this.flags = flags;
this.ui = ui;
- this.allUsers = allUsers;
this.accounts = accounts;
this.authorizedKeysFactory = authorizedKeysFactory;
this.externalIds = externalIds;
@@ -140,7 +135,7 @@
authorizedKeys.save("Add SSH key for initial admin user\n");
}
- AccountState as = AccountState.forAccount(new AllUsersName(allUsers.get()), a, extIds);
+ AccountState as = AccountState.forAccount(a, extIds);
for (AccountIndex accountIndex : accountIndexCollection.getWriteIndexes()) {
accountIndex.replace(as);
}
diff --git a/java/com/google/gerrit/server/account/AccountCacheImpl.java b/java/com/google/gerrit/server/account/AccountCacheImpl.java
index 173d81d8..3d81052 100644
--- a/java/com/google/gerrit/server/account/AccountCacheImpl.java
+++ b/java/com/google/gerrit/server/account/AccountCacheImpl.java
@@ -26,7 +26,6 @@
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.cache.CacheModule;
-import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.logging.TraceContext.TraceTimer;
@@ -69,18 +68,15 @@
};
}
- private final AllUsersName allUsersName;
private final ExternalIds externalIds;
private final LoadingCache<Account.Id, Optional<AccountState>> byId;
private final ExecutorService executor;
@Inject
AccountCacheImpl(
- AllUsersName allUsersName,
ExternalIds externalIds,
@Named(BYID_NAME) LoadingCache<Account.Id, Optional<AccountState>> byId,
@FanOutExecutor ExecutorService executor) {
- this.allUsersName = allUsersName;
this.externalIds = externalIds;
this.byId = byId;
this.executor = executor;
@@ -171,7 +167,7 @@
private AccountState missing(Account.Id accountId) {
Account account = new Account(accountId, TimeUtil.nowTs());
account.setActive(false);
- return AccountState.forAccount(allUsersName, account);
+ return AccountState.forAccount(account);
}
static class ByIdLoader extends CacheLoader<Account.Id, Optional<AccountState>> {
diff --git a/java/com/google/gerrit/server/account/AccountState.java b/java/com/google/gerrit/server/account/AccountState.java
index 46fde8c..556185e 100644
--- a/java/com/google/gerrit/server/account/AccountState.java
+++ b/java/com/google/gerrit/server/account/AccountState.java
@@ -36,7 +36,6 @@
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIdNotes;
import com.google.gerrit.server.account.externalids.ExternalIds;
-import com.google.gerrit.server.config.AllUsersName;
import java.io.IOException;
import java.util.Collection;
import java.util.Optional;
@@ -56,16 +55,14 @@
/**
* Creates an AccountState from the given account config.
*
- * @param allUsersName the name of the All-Users repository
* @param externalIds class to access external IDs
* @param accountConfig the account config, must already be loaded
* @return the account state, {@link Optional#empty()} if the account doesn't exist
* @throws IOException if accessing the external IDs fails
*/
public static Optional<AccountState> fromAccountConfig(
- AllUsersName allUsersName, ExternalIds externalIds, AccountConfig accountConfig)
- throws IOException {
- return fromAccountConfig(allUsersName, externalIds, accountConfig, null);
+ ExternalIds externalIds, AccountConfig accountConfig) throws IOException {
+ return fromAccountConfig(externalIds, accountConfig, null);
}
/**
@@ -78,7 +75,6 @@
* updated the revision of the external IDs branch in account config is outdated. Hence after
* updating external IDs the external ID notes must be provided.
*
- * @param allUsersName the name of the All-Users repository
* @param externalIds class to access external IDs
* @param accountConfig the account config, must already be loaded
* @param extIdNotes external ID notes, must already be loaded, may be {@code null}
@@ -86,10 +82,7 @@
* @throws IOException if accessing the external IDs fails
*/
public static Optional<AccountState> fromAccountConfig(
- AllUsersName allUsersName,
- ExternalIds externalIds,
- AccountConfig accountConfig,
- @Nullable ExternalIdNotes extIdNotes)
+ ExternalIds externalIds, AccountConfig accountConfig, @Nullable ExternalIdNotes extIdNotes)
throws IOException {
if (!accountConfig.getLoadedAccount().isPresent()) {
return Optional.empty();
@@ -115,39 +108,29 @@
return Optional.of(
new AccountState(
- allUsersName,
- account,
- extIds,
- projectWatches,
- generalPreferences,
- diffPreferences,
- editPreferences));
+ account, extIds, projectWatches, generalPreferences, diffPreferences, editPreferences));
}
/**
* Creates an AccountState for a given account with no external IDs, no project watches and
* default preferences.
*
- * @param allUsersName the name of the All-Users repository
* @param account the account
* @return the account state
*/
- public static AccountState forAccount(AllUsersName allUsersName, Account account) {
- return forAccount(allUsersName, account, ImmutableSet.of());
+ public static AccountState forAccount(Account account) {
+ return forAccount(account, ImmutableSet.of());
}
/**
* Creates an AccountState for a given account with no project watches and default preferences.
*
- * @param allUsersName the name of the All-Users repository
* @param account the account
* @param extIds the external IDs
* @return the account state
*/
- public static AccountState forAccount(
- AllUsersName allUsersName, Account account, Collection<ExternalId> extIds) {
+ public static AccountState forAccount(Account account, Collection<ExternalId> extIds) {
return new AccountState(
- allUsersName,
account,
ImmutableSet.copyOf(extIds),
ImmutableMap.of(),
@@ -156,7 +139,6 @@
EditPreferencesInfo.defaults());
}
- private final AllUsersName allUsersName;
private final Account account;
private final ImmutableSet<ExternalId> externalIds;
private final Optional<String> userName;
@@ -167,14 +149,12 @@
private Cache<IdentifiedUser.PropertyKey<Object>, Object> properties;
private AccountState(
- AllUsersName allUsersName,
Account account,
ImmutableSet<ExternalId> externalIds,
ImmutableMap<ProjectWatchKey, ImmutableSet<NotifyType>> projectWatches,
GeneralPreferencesInfo generalPreferences,
DiffPreferencesInfo diffPreferences,
EditPreferencesInfo editPreferences) {
- this.allUsersName = allUsersName;
this.account = account;
this.externalIds = externalIds;
this.userName = ExternalId.getUserName(externalIds);
@@ -184,10 +164,6 @@
this.editPreferences = editPreferences;
}
- public AllUsersName getAllUsersNameForIndexing() {
- return allUsersName;
- }
-
/** Get the cached account metadata. */
public Account getAccount() {
return account;
diff --git a/java/com/google/gerrit/server/account/Accounts.java b/java/com/google/gerrit/server/account/Accounts.java
index f3758bf..dbe7ba7 100644
--- a/java/com/google/gerrit/server/account/Accounts.java
+++ b/java/com/google/gerrit/server/account/Accounts.java
@@ -134,9 +134,7 @@
private Optional<AccountState> read(Repository allUsersRepository, Account.Id accountId)
throws IOException, ConfigInvalidException {
return AccountState.fromAccountConfig(
- allUsersName,
- externalIds,
- new AccountConfig(accountId, allUsersName, allUsersRepository).load());
+ externalIds, new AccountConfig(accountId, allUsersName, allUsersRepository).load());
}
public static Stream<Account.Id> readUserRefs(Repository repo) throws IOException {
diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java
index 20a1c97..cd47945 100644
--- a/java/com/google/gerrit/server/account/AccountsUpdate.java
+++ b/java/com/google/gerrit/server/account/AccountsUpdate.java
@@ -321,7 +321,7 @@
AccountConfig accountConfig = read(r, accountId);
Account account =
accountConfig.getNewAccount(new Timestamp(committerIdent.getWhen().getTime()));
- AccountState accountState = AccountState.forAccount(allUsersName, account);
+ AccountState accountState = AccountState.forAccount(account);
InternalAccountUpdate.Builder updateBuilder = InternalAccountUpdate.builder();
updater.update(accountState, updateBuilder);
@@ -377,7 +377,7 @@
r -> {
AccountConfig accountConfig = read(r, accountId);
Optional<AccountState> account =
- AccountState.fromAccountConfig(allUsersName, externalIds, accountConfig);
+ AccountState.fromAccountConfig(externalIds, accountConfig);
if (!account.isPresent()) {
return null;
}
@@ -592,8 +592,7 @@
}
public AccountState getAccount() throws IOException {
- return AccountState.fromAccountConfig(allUsersName, externalIds, accountConfig, extIdNotes)
- .get();
+ return AccountState.fromAccountConfig(externalIds, accountConfig, extIdNotes).get();
}
public ExternalIdNotes getExternalIdNotes() {
diff --git a/java/com/google/gerrit/server/index/account/AccountField.java b/java/com/google/gerrit/server/index/account/AccountField.java
index ab3a96d..b881a35 100644
--- a/java/com/google/gerrit/server/index/account/AccountField.java
+++ b/java/com/google/gerrit/server/index/account/AccountField.java
@@ -31,6 +31,8 @@
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.AllUsersNameProvider;
import java.sql.Timestamp;
import java.util.Arrays;
import java.util.Collections;
@@ -144,7 +146,11 @@
RefState.create(
RefNames.refsUsers(a.getAccount().getId()),
ObjectId.fromString(a.getAccount().getMetaId()))
- .toByteArray(a.getAllUsersNameForIndexing()));
+ // We use the default AllUsers name to avoid having to pass around that
+ // variable just for indexing.
+ // This field is only used for staleness detection which will discover the
+ // default name and replace it with the actually configured name.
+ .toByteArray(new AllUsersName(AllUsersNameProvider.DEFAULT)));
});
/**
diff --git a/java/com/google/gerrit/server/index/account/StalenessChecker.java b/java/com/google/gerrit/server/index/account/StalenessChecker.java
index 4664700..0423bb9 100644
--- a/java/com/google/gerrit/server/index/account/StalenessChecker.java
+++ b/java/com/google/gerrit/server/index/account/StalenessChecker.java
@@ -32,6 +32,7 @@
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.index.IndexUtils;
import com.google.inject.Inject;
@@ -106,7 +107,11 @@
for (Map.Entry<Project.NameKey, RefState> e :
RefState.parseStates(result.get().getValue(AccountField.REF_STATE)).entries()) {
- try (Repository repo = repoManager.openRepository(e.getKey())) {
+ // Custom All-Users repository names are not indexed. Instead, the default name is used.
+ // Therefore, defer to the currently configured All-Users name.
+ Project.NameKey repoName =
+ e.getKey().get().equals(AllUsersNameProvider.DEFAULT) ? allUsersName : e.getKey();
+ try (Repository repo = repoManager.openRepository(repoName)) {
if (!e.getValue().match(repo)) {
// Ref was modified since the account was indexed.
return true;
diff --git a/java/com/google/gerrit/testing/FakeAccountCache.java b/java/com/google/gerrit/testing/FakeAccountCache.java
index b99a32d..c86bcf4 100644
--- a/java/com/google/gerrit/testing/FakeAccountCache.java
+++ b/java/com/google/gerrit/testing/FakeAccountCache.java
@@ -20,8 +20,6 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
-import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.util.time.TimeUtil;
import java.util.HashMap;
import java.util.Map;
@@ -78,6 +76,6 @@
}
private static AccountState newState(Account account) {
- return AccountState.forAccount(new AllUsersName(AllUsersNameProvider.DEFAULT), account);
+ return AccountState.forAccount(account);
}
}
diff --git a/javatests/com/google/gerrit/server/account/AccountResolverTest.java b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
index d4ad7d7..5470e3c 100644
--- a/javatests/com/google/gerrit/server/account/AccountResolverTest.java
+++ b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
@@ -27,7 +27,6 @@
import com.google.gerrit.server.account.AccountResolver.Searcher;
import com.google.gerrit.server.account.AccountResolver.StringSearcher;
import com.google.gerrit.server.account.AccountResolver.UnresolvableAccountException;
-import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.util.time.TimeUtil;
import java.util.Arrays;
import java.util.function.Predicate;
@@ -329,14 +328,13 @@
}
private AccountState newAccount(int id) {
- return AccountState.forAccount(
- new AllUsersName("All-Users"), new Account(Account.id(id), TimeUtil.nowTs()));
+ return AccountState.forAccount(new Account(Account.id(id), TimeUtil.nowTs()));
}
private AccountState newInactiveAccount(int id) {
Account a = new Account(Account.id(id), TimeUtil.nowTs());
a.setActive(false);
- return AccountState.forAccount(new AllUsersName("All-Users"), a);
+ return AccountState.forAccount(a);
}
private static ImmutableSet<Account.Id> ids(int... ids) {
diff --git a/javatests/com/google/gerrit/server/index/account/AccountFieldTest.java b/javatests/com/google/gerrit/server/index/account/AccountFieldTest.java
index 5573be7..0f73cc5 100644
--- a/javatests/com/google/gerrit/server/index/account/AccountFieldTest.java
+++ b/javatests/com/google/gerrit/server/index/account/AccountFieldTest.java
@@ -39,8 +39,7 @@
Account account = new Account(Account.id(1), TimeUtil.nowTs());
String metaId = "0e39795bb25dc914118224995c53c5c36923a461";
account.setMetaId(metaId);
- List<String> values =
- toStrings(AccountField.REF_STATE.get(AccountState.forAccount(allUsersName, account)));
+ List<String> values = toStrings(AccountField.REF_STATE.get(AccountState.forAccount(account)));
assertThat(values).hasSize(1);
String expectedValue =
allUsersName.get() + ":" + RefNames.refsUsers(account.getId()) + ":" + metaId;
@@ -68,7 +67,7 @@
List<String> values =
toStrings(
AccountField.EXTERNAL_ID_STATE.get(
- AccountState.forAccount(null, account, ImmutableSet.of(extId1, extId2))));
+ AccountState.forAccount(account, ImmutableSet.of(extId1, extId2))));
String expectedValue1 = extId1.key().sha1().name() + ":" + extId1.blobId().name();
String expectedValue2 = extId2.key().sha1().name() + ":" + extId2.blobId().name();
assertThat(values).containsExactly(expectedValue1, expectedValue2);
diff --git a/javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java b/javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java
index 128279f..0e04739 100644
--- a/javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java
+++ b/javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java
@@ -25,8 +25,6 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
-import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.util.time.TimeUtil;
import java.util.Arrays;
import java.util.List;
@@ -383,6 +381,6 @@
final Account account = new Account(userId, TimeUtil.nowTs());
account.setFullName(name);
account.setPreferredEmail(email);
- return AccountState.forAccount(new AllUsersName(AllUsersNameProvider.DEFAULT), account);
+ return AccountState.forAccount(account);
}
}
