diff --git a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java index 524af50e0d..5f1486b1c8 100644 --- a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java +++ b/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().update(db, userAccount); + accountsUpdate.create().atomicUpdate(db, userId, a -> a.setPreferredEmail("user@example.com")); user = reloadUser(); requestContext.setContext( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java index fccb7e8d88..d7e4f9b209 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java +++ b/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> 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); - toUpdate.setPreferredEmail(newEmail); + accountUpdates.add(a -> a.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); - toUpdate.setFullName(who.getDisplayName()); + accountUpdates.add(a -> a.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) { - accountsUpdateFactory.create().update(db, toUpdate); + if (!accountUpdates.isEmpty()) { + 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); - if (a.getPreferredEmail() == null) { - a.setPreferredEmail(who.getEmailAddress()); - accountsUpdateFactory.create().update(db, a); - } + accountsUpdateFactory + .create() + .atomicUpdate( + db, + to, + a -> { + if (a.getPreferredEmail() == null) { + a.setPreferredEmail(who.getEmailAddress()); + } + }); 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); - if (a.getPreferredEmail() != null - && a.getPreferredEmail().equals(who.getEmailAddress())) { - a.setPreferredEmail(null); - accountsUpdateFactory.create().update(db, a); - } + accountsUpdateFactory + .create() + .atomicUpdate( + db, + from, + a -> { + if (a.getPreferredEmail() != null + && a.getPreferredEmail().equals(who.getEmailAddress())) { + a.setPreferredEmail(null); + } + }); byEmailCache.evict(who.getEmailAddress()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsUpdate.java index c1b3538cbf..f5ece30b10 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsUpdate.java +++ b/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 consumer) throws OrmException, IOException, ConfigInvalidException { + return atomicUpdate(db, accountId, ImmutableList.of(consumer)); + } + + /** + * Gets the account and updates it atomically. + * + *

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> 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; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 69ebbe3c13..dac5331606 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/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 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 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()); - if (a != null && Strings.isNullOrEmpty(a.getFullName())) { - a.setFullName(c.getCommitterIdent().getName()); - accountsUpdate.create().update(db, a); - user.getAccount().setFullName(a.getFullName()); + String committerName = c.getCommitterIdent().getName(); + Account account = + accountsUpdate + .create() + .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); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java index b5188be412..c07d3367b7 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java +++ b/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); - a.setFullName(fullName); - a.setPreferredEmail(email); - a.setActive(active); - accountsUpdate.create().update(db, a); + accountsUpdate + .create() + .atomicUpdate( + db, + id, + a -> { + a.setFullName(fullName); + a.setPreferredEmail(email); + a.setActive(active); + }); return id; } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index fdb00917a0..6f0d5e5ab9 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/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().update(db, userAccount); + accountsUpdate.create().atomicUpdate(db, userId, a -> a.setPreferredEmail(email)); 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); - a.setFullName(fullName); - a.setPreferredEmail(email); - a.setActive(active); - accountsUpdate.create().update(db, a); + accountsUpdate + .create() + .atomicUpdate( + db, + id, + a -> { + a.setFullName(fullName); + a.setPreferredEmail(email); + a.setActive(active); + }); return id; } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java index fe2a8f3e92..4953eeb66b 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java +++ b/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); - a.setFullName(fullName); - a.setPreferredEmail(email); - a.setActive(active); - accountsUpdate.create().update(db, a); + accountsUpdate + .create() + .atomicUpdate( + db, + id, + a -> { + a.setFullName(fullName); + a.setPreferredEmail(email); + a.setActive(active); + }); return id; } }