opendev/gerrit
Code Issues Proposed changes

Remove AllUsersName from AccountState

Browse Source 
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
This commit is contained in:
Patrick Hiesel
2019-07-12 09:58:20 +02:00
parent a02aa0ac5d
commit fd905ba8f0
11 changed files with 32 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
java/com/google/gerrit/pgm/init/InitAdminUser.java
View File

@@ -20,7 +20,6 @@ import com.google.common.base.Strings;
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.reviewdb.client.Account;
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 @@ import org.apache.commons.validator.routines.EmailValidator;
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 @@ public class InitAdminUser implements InitStep {
InitAdminUser(
InitFlags flags,
ConsoleUI ui,
AllUsersNameOnInitProvider allUsers,
AccountsOnInit accounts,
VersionedAuthorizedKeysOnInit.Factory authorizedKeysFactory,
ExternalIdsOnInit externalIds,
@@ -70,7 +66,6 @@ public class InitAdminUser implements InitStep {
GroupsOnInit groupsOnInit) {
this.flags = flags;
this.ui = ui;
this.allUsers = allUsers;
this.accounts = accounts;
this.authorizedKeysFactory = authorizedKeysFactory;
this.externalIds = externalIds;
@@ -140,7 +135,7 @@ public class InitAdminUser implements InitStep {
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);
}

6
java/com/google/gerrit/server/account/AccountCacheImpl.java
View File

@@ -26,7 +26,6 @@ import com.google.gerrit.server.FanOutExecutor;
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 @@ public class AccountCacheImpl implements AccountCache {
};
}
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 @@ public class AccountCacheImpl implements AccountCache {
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>> {

38
java/com/google/gerrit/server/account/AccountState.java
View File

@@ -36,7 +36,6 @@ import com.google.gerrit.server.account.ProjectWatches.ProjectWatchKey;
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 @@ public class AccountState {
/**
* 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 @@ public class AccountState {
* 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 @@ public class AccountState {
* @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 @@ public class AccountState {
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 @@ public class AccountState {
EditPreferencesInfo.defaults());
}
private final AllUsersName allUsersName;
private final Account account;
private final ImmutableSet<ExternalId> externalIds;
private final Optional<String> userName;
@@ -167,14 +149,12 @@ public class AccountState {
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 @@ public class AccountState {
this.editPreferences = editPreferences;
}
public AllUsersName getAllUsersNameForIndexing() {
return allUsersName;
}
/** Get the cached account metadata. */
public Account getAccount() {
return account;

4
java/com/google/gerrit/server/account/Accounts.java
View File

@@ -134,9 +134,7 @@ public class Accounts {
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 {

7
java/com/google/gerrit/server/account/AccountsUpdate.java
View File

@@ -321,7 +321,7 @@ public class AccountsUpdate {
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 @@ public class AccountsUpdate {
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 class AccountsUpdate {
}
public AccountState getAccount() throws IOException {
return AccountState.fromAccountConfig(allUsersName, externalIds, accountConfig, extIdNotes)
.get();
return AccountState.fromAccountConfig(externalIds, accountConfig, extIdNotes).get();
}
public ExternalIdNotes getExternalIdNotes() {

8
java/com/google/gerrit/server/index/account/AccountField.java
View File

@@ -31,6 +31,8 @@ import com.google.gerrit.index.SchemaUtil;
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 @@ public class AccountField {
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)));
});
/**

7
java/com/google/gerrit/server/index/account/StalenessChecker.java
View File

@@ -32,6 +32,7 @@ import com.google.gerrit.reviewdb.client.RefNames;
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 @@ public class StalenessChecker {
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;

4
java/com/google/gerrit/testing/FakeAccountCache.java
View File

@@ -20,8 +20,6 @@ import com.google.gerrit.common.Nullable;
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 @@ public class FakeAccountCache implements AccountCache {
}
private static AccountState newState(Account account) {
return AccountState.forAccount(new AllUsersName(AllUsersNameProvider.DEFAULT), account);
return AccountState.forAccount(account);
}
}

6
javatests/com/google/gerrit/server/account/AccountResolverTest.java
View File

@@ -27,7 +27,6 @@ import com.google.gerrit.server.account.AccountResolver.Result;
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 @@ public class AccountResolverTest {
}
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) {

5
javatests/com/google/gerrit/server/index/account/AccountFieldTest.java
View File

@@ -39,8 +39,7 @@ public class AccountFieldTest {
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 @@ public class AccountFieldTest {
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);

4
javatests/com/google/gerrit/server/mail/send/FromAddressGeneratorProviderTest.java
View File

@@ -25,8 +25,6 @@ import com.google.gerrit.mail.Address;
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 @@ public class FromAddressGeneratorProviderTest {
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);
}
}