Update external account IDs in a single commit

Currently when Gerrit updates users external IDs it will create two
commits in `refs/meta/external-ids` branch of `All-Users.git`
repository. First commit is removing the old value, and follow up commit
adds new ID.

This approach increase the number of git objects in `All-Users.git`
repository. It also adds a slight delay for the GitHub authorization
flow.

This change ensures that updating external account IDs is done in a
single commit.

Bug: Issue 309849905
Release-Notes: Account external IDs are updated in a single commit
Change-Id: I9c0160f07d92f8049a9bad9e1f4d4d669a7af64a

2 files changed, 52 insertions, 10 deletions

diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java
index 50234134c6..e32a0eb06f 100644
--- a/java/com/google/gerrit/server/account/AccountManager.java
+++ b/java/com/google/gerrit/server/account/AccountManager.java
@@ -54,6 +54,7 @@ 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.Objects;
 import java.util.Optional;
@@ -505,28 +506,30 @@ public class AccountManager {
    */
   public AuthResult updateLink(Account.Id to, AuthRequest who)
       throws AccountException, IOException, ConfigInvalidException {
+    Optional<ExternalId> optionalExtId = externalIds.get(who.getExternalIdKey());
+    if (optionalExtId.filter(extId -> !extId.accountId().equals(to)).isPresent()) {
+      throw new AccountException(
+          "Identity '" + optionalExtId.get().key().get() + "' in use by another account");
+    }
+
     accountsUpdateProvider
         .get()
         .update(
-            "Delete External IDs on Update Link",
+            "Update External IDs on Update Link",
             to,
             (a, u) -> {
               Set<ExternalId> filteredExtIdsByScheme =
                   a.externalIds().stream()
                       .filter(e -> e.key().isScheme(who.getExternalIdKey().scheme()))
                       .collect(toImmutableSet());
-              if (filteredExtIdsByScheme.isEmpty()) {
-                return;
-              }
+              ExternalId newExtId =
+                  externalIdFactory.createWithEmail(
+                      who.getExternalIdKey(), to, who.getEmailAddress());
 
-              if (filteredExtIdsByScheme.size() > 1
-                  || filteredExtIdsByScheme.stream()
-                      .noneMatch(e -> e.key().equals(who.getExternalIdKey()))) {
-                u.deleteExternalIds(filteredExtIdsByScheme);
-              }
+              u.replaceExternalIds(filteredExtIdsByScheme, Collections.singletonList(newExtId));
             });
 
-    return link(to, who);
+    return new AuthResult(to, who.getExternalIdKey(), false);
   }
 
   /**
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java
index ee2db28f9d..39fa918189 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java
@@ -18,12 +18,14 @@ import static com.google.common.truth.OptionalSubject.optionals;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.common.truth.Truth8.assertThat;
+import static com.google.gerrit.entities.RefNames.REFS_EXTERNAL_IDS;
 import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_GOOGLE_OAUTH;
 import static com.google.gerrit.testing.GerritJUnit.assertThrows;
 import static com.google.gerrit.testing.TestActionRefUpdateContext.openTestRefUpdateContext;
 import static java.util.stream.Collectors.toSet;
 
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.config.GerritConfig;
 import com.google.gerrit.common.Nullable;
@@ -50,8 +52,12 @@ import com.google.gerrit.server.ssh.SshKeyCache;
 import com.google.gerrit.server.update.context.RefUpdateContext;
 import com.google.inject.Inject;
 import com.google.inject.util.Providers;
+import java.io.IOException;
 import java.util.Optional;
 import java.util.Set;
+import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 import org.junit.Test;
 
@@ -781,6 +787,39 @@ public class AccountManagerIT extends AbstractDaemonTest {
     assertThat(result.getAccountId().get()).isEqualTo(accountId.get());
   }
 
+  @Test
+  public void updateLinkInSingleCommit() throws Exception {
+    String username = "baz";
+    String email1 = "baz@foo.com";
+    String email2 = "baz@bar.com";
+
+    ExternalId.Key mailExtIdKey = externalIdKeyFactory.create(ExternalId.SCHEME_MAILTO, username);
+    Account.Id accountId = Account.id(seq.nextAccountId());
+    accountsUpdate.insert(
+        "Create Test Account",
+        accountId,
+        u -> u.addExternalId(externalIdFactory.create(mailExtIdKey, accountId)));
+
+    accountManager.link(accountId, authRequestFactory.createForEmail(email1));
+
+    try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+        Git git = new Git(allUsersRepo)) {
+      int initialCommits = getCommitsInExternalIds(git, allUsersRepo);
+
+      accountManager.updateLink(accountId, authRequestFactory.createForEmail(email2));
+
+      int afterUpdateCommits = getCommitsInExternalIds(git, allUsersRepo);
+
+      assertThat(afterUpdateCommits).isEqualTo(initialCommits + 1);
+    }
+  }
+
+  private static int getCommitsInExternalIds(Git git, Repository allUsersRepo)
+      throws GitAPIException, IOException {
+    ObjectId refsMetaExternalIdsHead = allUsersRepo.exactRef(REFS_EXTERNAL_IDS).getObjectId();
+    return Iterables.size(git.log().add(refsMetaExternalIdsHead).call());
+  }
+
   private void assertNoSuchExternalIds(ExternalId.Key... extIdKeys) throws Exception {
     for (ExternalId.Key extIdKey : extIdKeys) {
       assertWithMessage(extIdKey.get())