AccountsUpdate: Provide AccountState instead of Account for account updates

Some callers need access to the external IDs in order to decide which
account updates should be done. At the moment these callers first read
the external IDs and then read and update the account. This way there is
a risk of a race that the external IDs are updated after they have been
read and before the account is read and updated. By providing an
AccountState that was consistently read for the account updates callers
have access to both the account and the external IDs (and also general
preferences and project watches). Adapting the callers to take advantage
of the provided AccountState will be done in a follow-up change.

Change-Id: Ia2bcd7df61bd8450e9f228afd01ffe7836a3270a
Signed-off-by: Edwin Kempin <ekempin@google.com>

java/com/google/gerrit/server/account/AccountManager.java

@@ -378,7 +378,7 @@ public class AccountManager {
               (a, u) -> {
                 u.addExternalId(
                     ExternalId.createWithEmail(who.getExternalIdKey(), to, who.getEmailAddress()));
-                if (who.getEmailAddress() != null && a.getPreferredEmail() == null) {
+                if (who.getEmailAddress() != null && a.getAccount().getPreferredEmail() == null) {
                   u.setPreferredEmail(who.getEmailAddress());
                 }
               });
@@ -468,8 +468,10 @@ public class AccountManager {
             from,
             (a, u) -> {
               u.deleteExternalIds(extIds);
-              if (a.getPreferredEmail() != null
+              if (a.getAccount().getPreferredEmail() != null
-                  && extIds.stream().anyMatch(e -> a.getPreferredEmail().equals(e.email()))) {
+                  && extIds
+                      .stream()
+                      .anyMatch(e -> a.getAccount().getPreferredEmail().equals(e.email()))) {
                 u.setPreferredEmail(null);
               }
             });

java/com/google/gerrit/server/account/AccountState.java

@@ -30,7 +30,10 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.WatchConfig.NotifyType;
 import com.google.gerrit.server.account.WatchConfig.ProjectWatchKey;
 import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.externalids.ExternalIds;
 import com.google.gerrit.server.config.AllUsersName;
+import java.io.IOException;
+import java.util.Optional;
 import org.apache.commons.codec.DecoderException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -38,6 +41,9 @@ import org.slf4j.LoggerFactory;
 /**
  * Superset of all information related to an Account. This includes external IDs, project watches,
  * and properties from the account config file. AccountState maps one-to-one to Account.
+ *
+ * <p>Most callers should not construct AccountStates directly but rather lookup accounts via the
+ * account cache (see {@link AccountCache#get(Account.Id)}).
  */
 public class AccountState {
   private static final Logger logger = LoggerFactory.getLogger(AccountState.class);
@@ -45,6 +51,24 @@ public class AccountState {
   public static final Function<AccountState, Account.Id> ACCOUNT_ID_FUNCTION =
       a -> a.getAccount().getId();
 
+  public static Optional<AccountState> fromAccountConfig(
+      AllUsersName allUsersName, ExternalIds externalIds, AccountConfig accountConfig)
+      throws IOException {
+    if (!accountConfig.getLoadedAccount().isPresent()) {
+      return Optional.empty();
+    }
+    Account account = accountConfig.getLoadedAccount().get();
+    return Optional.of(
+        new AccountState(
+            allUsersName,
+            account,
+            accountConfig.getExternalIdsRev().isPresent()
+                ? externalIds.byAccount(account.getId(), accountConfig.getExternalIdsRev().get())
+                : ImmutableSet.of(),
+            accountConfig.getProjectWatches(),
+            accountConfig.getGeneralPreferences()));
+  }
+
   private final AllUsersName allUsersName;
   private final Account account;
   private final ImmutableSet<ExternalId> externalIds;

java/com/google/gerrit/server/account/Accounts.java

@@ -18,7 +18,6 @@ import static java.util.Comparator.comparing;
 import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toSet;
 
-import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.RefNames;
@@ -136,20 +135,8 @@ public class Accounts {
 
   private Optional<AccountState> read(Repository allUsersRepository, Account.Id accountId)
       throws IOException, ConfigInvalidException {
-    AccountConfig accountConfig = new AccountConfig(accountId, allUsersRepository).load();
+    return AccountState.fromAccountConfig(
-    if (!accountConfig.getLoadedAccount().isPresent()) {
+        allUsersName, externalIds, new AccountConfig(accountId, allUsersRepository).load());
-      return Optional.empty();
-    }
-    Account account = accountConfig.getLoadedAccount().get();
-    return Optional.of(
-        new AccountState(
-            allUsersName,
-            account,
-            accountConfig.getExternalIdsRev().isPresent()
-                ? externalIds.byAccount(accountId, accountConfig.getExternalIdsRev().get())
-                : ImmutableSet.of(),
-            accountConfig.getProjectWatches(),
-            accountConfig.getGeneralPreferences()));
   }
 
   public static Stream<Account.Id> readUserRefs(Repository repo) throws IOException {

java/com/google/gerrit/server/account/AccountsUpdate.java

@@ -19,15 +19,19 @@ import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.util.concurrent.Runnables;
 import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.externalids.ExternalIdNotes;
 import com.google.gerrit.server.account.externalids.ExternalIdNotes.ExternalIdNotesLoader;
+import com.google.gerrit.server.account.externalids.ExternalIds;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.GitRepositoryManager;
@@ -82,10 +86,10 @@ public class AccountsUpdate {
      * <p>Use the provided account only to read the current state of the account. Don't do updates
      * to the account. For updates use the provided account update builder.
      *
-     * @param account the account that is being updated
+     * @param accountState the account that is being updated
      * @param update account update builder
      */
-    void update(Account account, InternalAccountUpdate.Builder update);
+    void update(AccountState accountState, InternalAccountUpdate.Builder update);
 
     static AccountUpdater join(List<AccountUpdater> updaters) {
       return (a, u) -> updaters.stream().forEach(updater -> updater.update(a, u));
@@ -111,6 +115,7 @@ public class AccountsUpdate {
     private final GitRepositoryManager repoManager;
     private final GitReferenceUpdated gitRefUpdated;
     private final AllUsersName allUsersName;
+    private final ExternalIds externalIds;
     private final Provider<PersonIdent> serverIdentProvider;
     private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
     private final RetryHelper retryHelper;
@@ -121,6 +126,7 @@ public class AccountsUpdate {
         GitRepositoryManager repoManager,
         GitReferenceUpdated gitRefUpdated,
         AllUsersName allUsersName,
+        ExternalIds externalIds,
         @GerritPersonIdent Provider<PersonIdent> serverIdentProvider,
         Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
         RetryHelper retryHelper,
@@ -128,6 +134,7 @@ public class AccountsUpdate {
       this.repoManager = repoManager;
       this.gitRefUpdated = gitRefUpdated;
       this.allUsersName = allUsersName;
+      this.externalIds = externalIds;
       this.serverIdentProvider = serverIdentProvider;
       this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory;
       this.retryHelper = retryHelper;
@@ -141,6 +148,7 @@ public class AccountsUpdate {
           gitRefUpdated,
           null,
           allUsersName,
+          externalIds,
           metaDataUpdateInternalFactory,
           retryHelper,
           extIdNotesFactory,
@@ -163,6 +171,7 @@ public class AccountsUpdate {
     private final GitRepositoryManager repoManager;
     private final GitReferenceUpdated gitRefUpdated;
     private final AllUsersName allUsersName;
+    private final ExternalIds externalIds;
     private final Provider<PersonIdent> serverIdentProvider;
     private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
     private final RetryHelper retryHelper;
@@ -173,6 +182,7 @@ public class AccountsUpdate {
         GitRepositoryManager repoManager,
         GitReferenceUpdated gitRefUpdated,
         AllUsersName allUsersName,
+        ExternalIds externalIds,
         @GerritPersonIdent Provider<PersonIdent> serverIdentProvider,
         Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
         RetryHelper retryHelper,
@@ -180,6 +190,7 @@ public class AccountsUpdate {
       this.repoManager = repoManager;
       this.gitRefUpdated = gitRefUpdated;
       this.allUsersName = allUsersName;
+      this.externalIds = externalIds;
       this.serverIdentProvider = serverIdentProvider;
       this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory;
       this.retryHelper = retryHelper;
@@ -193,6 +204,7 @@ public class AccountsUpdate {
           gitRefUpdated,
           null,
           allUsersName,
+          externalIds,
           metaDataUpdateInternalFactory,
           retryHelper,
           extIdNotesFactory,
@@ -212,6 +224,7 @@ public class AccountsUpdate {
     private final GitRepositoryManager repoManager;
     private final GitReferenceUpdated gitRefUpdated;
     private final AllUsersName allUsersName;
+    private final ExternalIds externalIds;
     private final Provider<PersonIdent> serverIdentProvider;
     private final Provider<IdentifiedUser> identifiedUser;
     private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
@@ -223,6 +236,7 @@ public class AccountsUpdate {
         GitRepositoryManager repoManager,
         GitReferenceUpdated gitRefUpdated,
         AllUsersName allUsersName,
+        ExternalIds externalIds,
         @GerritPersonIdent Provider<PersonIdent> serverIdentProvider,
         Provider<IdentifiedUser> identifiedUser,
         Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
@@ -231,6 +245,7 @@ public class AccountsUpdate {
       this.repoManager = repoManager;
       this.gitRefUpdated = gitRefUpdated;
       this.allUsersName = allUsersName;
+      this.externalIds = externalIds;
       this.serverIdentProvider = serverIdentProvider;
       this.identifiedUser = identifiedUser;
       this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory;
@@ -247,6 +262,7 @@ public class AccountsUpdate {
           gitRefUpdated,
           user,
           allUsersName,
+          externalIds,
           metaDataUpdateInternalFactory,
           retryHelper,
           extIdNotesFactory,
@@ -263,6 +279,7 @@ public class AccountsUpdate {
   private final GitReferenceUpdated gitRefUpdated;
   @Nullable private final IdentifiedUser currentUser;
   private final AllUsersName allUsersName;
+  private final ExternalIds externalIds;
   private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
   private final RetryHelper retryHelper;
   private final ExternalIdNotesLoader extIdNotesLoader;
@@ -275,6 +292,7 @@ public class AccountsUpdate {
       GitReferenceUpdated gitRefUpdated,
       @Nullable IdentifiedUser currentUser,
       AllUsersName allUsersName,
+      ExternalIds externalIds,
       Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
       RetryHelper retryHelper,
       ExternalIdNotesLoader extIdNotesLoader,
@@ -285,6 +303,7 @@ public class AccountsUpdate {
         gitRefUpdated,
         currentUser,
         allUsersName,
+        externalIds,
         metaDataUpdateInternalFactory,
         retryHelper,
         extIdNotesLoader,
@@ -299,6 +318,7 @@ public class AccountsUpdate {
       GitReferenceUpdated gitRefUpdated,
       @Nullable IdentifiedUser currentUser,
       AllUsersName allUsersName,
+      ExternalIds externalIds,
       Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
       RetryHelper retryHelper,
       ExternalIdNotesLoader extIdNotesLoader,
@@ -309,6 +329,7 @@ public class AccountsUpdate {
     this.gitRefUpdated = checkNotNull(gitRefUpdated, "gitRefUpdated");
     this.currentUser = currentUser;
     this.allUsersName = checkNotNull(allUsersName, "allUsersName");
+    this.externalIds = checkNotNull(externalIds, "externalIds");
     this.metaDataUpdateInternalFactory =
         checkNotNull(metaDataUpdateInternalFactory, "metaDataUpdateInternalFactory");
     this.retryHelper = checkNotNull(retryHelper, "retryHelper");
@@ -355,8 +376,15 @@ public class AccountsUpdate {
           AccountConfig accountConfig = read(r, accountId);
           Account account =
               accountConfig.getNewAccount(new Timestamp(committerIdent.getWhen().getTime()));
+          AccountState accountState =
+              new AccountState(
+                  allUsersName,
+                  account,
+                  ImmutableSet.of(),
+                  ImmutableMap.of(),
+                  GeneralPreferencesInfo.defaults());
           InternalAccountUpdate.Builder updateBuilder = InternalAccountUpdate.builder();
-          updater.update(account, updateBuilder);
+          updater.update(accountState, updateBuilder);
 
           InternalAccountUpdate update = updateBuilder.build();
           accountConfig.setAccountUpdate(update);
@@ -405,7 +433,8 @@ public class AccountsUpdate {
     return updateAccount(
         r -> {
           AccountConfig accountConfig = read(r, accountId);
-          Optional<Account> account = accountConfig.getLoadedAccount();
+          Optional<AccountState> account =
+              AccountState.fromAccountConfig(allUsersName, externalIds, accountConfig);
           if (!account.isPresent()) {
             return null;
           }

java/com/google/gerrit/server/account/SetInactiveFlag.java

@@ -46,7 +46,7 @@ public class SetInactiveFlag {
                 "Deactivate Account via API",
                 accountId,
                 (a, u) -> {
-                  if (!a.isActive()) {
+                  if (!a.getAccount().isActive()) {
                     alreadyInactive.set(true);
                   } else {
                     u.setActive(false);
@@ -71,7 +71,7 @@ public class SetInactiveFlag {
                 "Activate Account via API",
                 accountId,
                 (a, u) -> {
-                  if (a.isActive()) {
+                  if (a.getAccount().isActive()) {
                     alreadyActive.set(true);
                   } else {
                     u.setActive(true);

java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -2987,7 +2987,7 @@ class ReceiveCommits {
                   "Set Full Name on Receive Commits",
                   user.getAccountId(),
                   (a, u) -> {
-                    if (Strings.isNullOrEmpty(a.getFullName())) {
+                    if (Strings.isNullOrEmpty(a.getAccount().getFullName())) {
                       u.setFullName(setFullNameTo);
                     }
                   });

java/com/google/gerrit/server/restapi/account/PutPreferred.java

@@ -72,7 +72,7 @@ public class PutPreferred implements RestModifyView<AccountResource.Email, Input
                 "Set Preferred Email via API",
                 user.getAccountId(),
                 (a, u) -> {
-                  if (email.equals(a.getPreferredEmail())) {
+                  if (email.equals(a.getAccount().getPreferredEmail())) {
                     alreadyPreferred.set(true);
                   } else {
                     u.setPreferredEmail(email);

javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java

@@ -2021,6 +2021,7 @@ public class AccountIT extends AbstractDaemonTest {
             gitReferenceUpdated,
             null,
             allUsers,
+            externalIds,
             metaDataUpdateInternalFactory,
             new RetryHelper(
                 cfg,
@@ -2069,6 +2070,7 @@ public class AccountIT extends AbstractDaemonTest {
             gitReferenceUpdated,
             null,
             allUsers,
+            externalIds,
             metaDataUpdateInternalFactory,
             new RetryHelper(
                 cfg,