Always update accounts atomically

This prevents that we unintentionally overwrite concurrent updates, e.g.
updates that were done by a racing request or updates we didn't see
because we read from a stale cache.

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

gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java

@@ -37,7 +37,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountManager;
-import com.google.gerrit.server.account.Accounts;
 import com.google.gerrit.server.account.AccountsUpdate;
 import com.google.gerrit.server.account.AuthRequest;
 import com.google.gerrit.server.account.externalids.ExternalId;
@@ -71,8 +70,6 @@ import org.junit.Test;
 
 /** Unit tests for {@link GerritPublicKeyChecker}. */
 public class GerritPublicKeyCheckerTest {
-  @Inject private Accounts accounts;
-
   @Inject private AccountsUpdate.Server accountsUpdate;
 
   @Inject private AccountManager accountManager;
@@ -117,10 +114,8 @@ public class GerritPublicKeyCheckerTest {
     db = schemaFactory.open();
     schemaCreator.create(db);
     userId = accountManager.authenticate(AuthRequest.forUser("user")).getAccountId();
-    Account userAccount = accounts.get(db, userId);
     // Note: does not match any key in TestKeys.
-    userAccount.setPreferredEmail("user@example.com");
+    accountsUpdate.create().atomicUpdate(db, userId, a -> a.setPreferredEmail("user@example.com"));
-    accountsUpdate.create().update(db, userAccount);
     user = reloadUser();
 
     requestContext.setContext(

gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java

@@ -38,10 +38,13 @@ import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.List;
 import java.util.Optional;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Consumer;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 import org.slf4j.Logger;
@@ -149,7 +152,7 @@ public class AccountManager {
   private void update(ReviewDb db, AuthRequest who, ExternalId extId)
       throws OrmException, IOException, ConfigInvalidException {
     IdentifiedUser user = userFactory.create(extId.accountId());
-    Account toUpdate = null;
+    List<Consumer<Account>> accountUpdates = new ArrayList<>();
 
     // If the email address was modified by the authentication provider,
     // update our records to match the changed email.
@@ -158,8 +161,7 @@ public class AccountManager {
     String oldEmail = extId.email();
     if (newEmail != null && !newEmail.equals(oldEmail)) {
       if (oldEmail != null && oldEmail.equals(user.getAccount().getPreferredEmail())) {
-        toUpdate = load(toUpdate, user.getAccountId(), db);
+        accountUpdates.add(a -> a.setPreferredEmail(newEmail));
-        toUpdate.setPreferredEmail(newEmail);
       }
 
       externalIdsUpdateFactory
@@ -171,8 +173,7 @@ public class AccountManager {
     if (!realm.allowsEdit(AccountFieldName.FULL_NAME)
         && !Strings.isNullOrEmpty(who.getDisplayName())
         && !eq(user.getAccount().getFullName(), who.getDisplayName())) {
-      toUpdate = load(toUpdate, user.getAccountId(), db);
+      accountUpdates.add(a -> a.setFullName(who.getDisplayName()));
-      toUpdate.setFullName(who.getDisplayName());
     }
 
     if (!realm.allowsEdit(AccountFieldName.USER_NAME)
@@ -183,8 +184,12 @@ public class AccountManager {
               "Not changing already set username %s to %s", user.getUserName(), who.getUserName()));
     }
 
-    if (toUpdate != null) {
+    if (!accountUpdates.isEmpty()) {
-      accountsUpdateFactory.create().update(db, toUpdate);
+      Account account =
+          accountsUpdateFactory.create().atomicUpdate(db, user.getAccountId(), accountUpdates);
+      if (account == null) {
+        throw new OrmException("Account " + user.getAccountId() + " has been deleted");
+      }
     }
 
     if (newEmail != null && !newEmail.equals(oldEmail)) {
@@ -193,17 +198,6 @@ public class AccountManager {
     }
   }
 
-  private Account load(Account toUpdate, Account.Id accountId, ReviewDb db)
-      throws OrmException, IOException, ConfigInvalidException {
-    if (toUpdate == null) {
-      toUpdate = accounts.get(db, accountId);
-      if (toUpdate == null) {
-        throw new OrmException("Account " + accountId + " has been deleted");
-      }
-    }
-    return toUpdate;
-  }
-
   private static boolean eq(String a, String b) {
     return (a == null && b == null) || (a != null && a.equals(b));
   }
@@ -369,11 +363,16 @@ public class AccountManager {
             .insert(ExternalId.createWithEmail(who.getExternalIdKey(), to, who.getEmailAddress()));
 
         if (who.getEmailAddress() != null) {
-          Account a = accounts.get(db, to);
+          accountsUpdateFactory
+              .create()
+              .atomicUpdate(
+                  db,
+                  to,
+                  a -> {
                     if (a.getPreferredEmail() == null) {
                       a.setPreferredEmail(who.getEmailAddress());
-            accountsUpdateFactory.create().update(db, a);
                     }
+                  });
           byEmailCache.evict(who.getEmailAddress());
         }
       }
@@ -433,12 +432,17 @@ public class AccountManager {
         externalIdsUpdateFactory.create().delete(extId);
 
         if (who.getEmailAddress() != null) {
-          Account a = accounts.get(db, from);
+          accountsUpdateFactory
+              .create()
+              .atomicUpdate(
+                  db,
+                  from,
+                  a -> {
                     if (a.getPreferredEmail() != null
                         && a.getPreferredEmail().equals(who.getEmailAddress())) {
                       a.setPreferredEmail(null);
-            accountsUpdateFactory.create().update(db, a);
                     }
+                  });
           byEmailCache.evict(who.getEmailAddress());
         }
 

gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsUpdate.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.account;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Account;
@@ -37,6 +38,7 @@ import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
 import java.sql.Timestamp;
+import java.util.List;
 import java.util.function.Consumer;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.CommitBuilder;
@@ -251,20 +253,41 @@ public class AccountsUpdate {
    */
   public Account atomicUpdate(ReviewDb db, Account.Id accountId, Consumer<Account> consumer)
       throws OrmException, IOException, ConfigInvalidException {
+    return atomicUpdate(db, accountId, ImmutableList.of(consumer));
+  }
+
+  /**
+   * Gets the account and updates it atomically.
+   *
+   * <p>Changing the registration date of an account is not supported.
+   *
+   * @param db ReviewDb
+   * @param accountId ID of the account
+   * @param consumers consumers to update the account, only invoked if the account exists
+   * @return the updated account, {@code null} if the account doesn't exist
+   * @throws OrmException if updating the account fails
+   */
+  public Account atomicUpdate(ReviewDb db, Account.Id accountId, List<Consumer<Account>> consumers)
+      throws OrmException, IOException, ConfigInvalidException {
+    if (consumers.isEmpty()) {
+      return null;
+    }
+
     // Update in ReviewDb
     db.accounts()
         .atomicUpdate(
             accountId,
             a -> {
-              consumer.accept(a);
+              consumers.stream().forEach(c -> c.accept(a));
               return a;
             });
 
     // Update in NoteDb
     AccountConfig accountConfig = read(accountId);
     Account account = accountConfig.getAccount();
-    consumer.accept(account);
+    consumers.stream().forEach(c -> c.accept(account));
     commit(accountConfig);
+
     return account;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -84,7 +84,6 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.Sequences;
 import com.google.gerrit.server.account.AccountResolver;
-import com.google.gerrit.server.account.Accounts;
 import com.google.gerrit.server.account.AccountsUpdate;
 import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.SetHashtagsOp;
@@ -302,7 +301,6 @@ public class ReceiveCommits {
   private final Sequences seq;
   private final Provider<InternalChangeQuery> queryProvider;
   private final ChangeNotes.Factory notesFactory;
-  private final Accounts accounts;
   private final AccountsUpdate.Server accountsUpdate;
   private final AccountResolver accountResolver;
   private final PermissionBackend permissionBackend;
@@ -377,7 +375,6 @@ public class ReceiveCommits {
       Sequences seq,
       Provider<InternalChangeQuery> queryProvider,
       ChangeNotes.Factory notesFactory,
-      Accounts accounts,
       AccountsUpdate.Server accountsUpdate,
       AccountResolver accountResolver,
       PermissionBackend permissionBackend,
@@ -417,7 +414,6 @@ public class ReceiveCommits {
     this.seq = seq;
     this.queryProvider = queryProvider;
     this.notesFactory = notesFactory;
-    this.accounts = accounts;
     this.accountsUpdate = accountsUpdate;
     this.accountResolver = accountResolver;
     this.permissionBackend = permissionBackend;
@@ -2788,11 +2784,20 @@ public class ReceiveCommits {
 
         if (defaultName && user.hasEmailAddress(c.getCommitterIdent().getEmailAddress())) {
           try {
-            Account a = accounts.get(db, user.getAccountId());
+            String committerName = c.getCommitterIdent().getName();
-            if (a != null && Strings.isNullOrEmpty(a.getFullName())) {
+            Account account =
-              a.setFullName(c.getCommitterIdent().getName());
+                accountsUpdate
-              accountsUpdate.create().update(db, a);
+                    .create()
-              user.getAccount().setFullName(a.getFullName());
+                    .atomicUpdate(
+                        db,
+                        user.getAccountId(),
+                        a -> {
+                          if (Strings.isNullOrEmpty(a.getFullName())) {
+                            a.setFullName(committerName);
+                          }
+                        });
+            if (account != null && Strings.isNullOrEmpty(account.getFullName())) {
+              user.getAccount().setFullName(account.getFullName());
             }
           } catch (OrmException | IOException | ConfigInvalidException e) {
             logWarn("Cannot default full_name", e);

gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java

@@ -496,11 +496,16 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
       if (email != null) {
         accountManager.link(id, AuthRequest.forEmail(email));
       }
-      Account a = accounts.get(db, id);
+      accountsUpdate
+          .create()
+          .atomicUpdate(
+              db,
+              id,
+              a -> {
                 a.setFullName(fullName);
                 a.setPreferredEmail(email);
                 a.setActive(active);
-      accountsUpdate.create().update(db, a);
+              });
       return id;
     }
   }

gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java

@@ -208,13 +208,11 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     db = schemaFactory.open();
 
     userId = accountManager.authenticate(AuthRequest.forUser("user")).getAccountId();
-    Account userAccount = accounts.get(db, userId);
     String email = "user@example.com";
     externalIdsUpdate.create().insert(ExternalId.createEmail(userId, email));
-    userAccount.setPreferredEmail(email);
+    accountsUpdate.create().atomicUpdate(db, userId, a -> a.setPreferredEmail(email));
-    accountsUpdate.create().update(db, userAccount);
     user = userFactory.create(userId);
-    requestContext.setContext(newRequestContext(userAccount.getId()));
+    requestContext.setContext(newRequestContext(userId));
   }
 
   protected RequestContext newRequestContext(Account.Id requestUserId) {
@@ -2340,11 +2338,16 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
       if (email != null) {
         accountManager.link(id, AuthRequest.forEmail(email));
       }
-      Account a = accounts.get(db, id);
+      accountsUpdate
+          .create()
+          .atomicUpdate(
+              db,
+              id,
+              a -> {
                 a.setFullName(fullName);
                 a.setPreferredEmail(email);
                 a.setActive(active);
-      accountsUpdate.create().update(db, a);
+              });
       return id;
     }
   }

gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java

@@ -319,11 +319,16 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
       if (email != null) {
         accountManager.link(id, AuthRequest.forEmail(email));
       }
-      Account a = accounts.get(db, id);
+      accountsUpdate
+          .create()
+          .atomicUpdate(
+              db,
+              id,
+              a -> {
                 a.setFullName(fullName);
                 a.setPreferredEmail(email);
                 a.setActive(active);
-      accountsUpdate.create().update(db, a);
+              });
       return id;
     }
   }