Don't fail on account update if the user branch is missing
If the account does not exist AccountsUpdate#update should return null
and the provided consumer to update the account should not be invoked
(if it's invoked with a null account the consumer likely fails with a
NullPointerException).
If the account only exists in ReviewDb the missing user branch should be
created.
At Google we run into this problem because we haven't migrated all of
our accounts to NoteDb yet and hence for a few accounts the user branch
is still missing.
Change-Id: Ia0a9c390684cacba5825190f9bb6ec7b00dbf3c2
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 7cd27bf..f145839 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -47,6 +47,7 @@
import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.UseSsh;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.accounts.EmailInput;
@@ -76,12 +77,14 @@
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.account.AccountByEmailCache;
import com.google.gerrit.server.account.AccountConfig;
+import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.account.WatchConfig;
import com.google.gerrit.server.account.WatchConfig.NotifyType;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.account.externalids.ExternalIdsUpdate;
import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilderImpl;
import com.google.gerrit.server.project.RefPattern;
@@ -100,6 +103,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
import org.bouncycastle.bcpg.ArmoredOutputStream;
import org.bouncycastle.openpgp.PGPPublicKey;
import org.bouncycastle.openpgp.PGPPublicKeyRing;
@@ -134,6 +138,8 @@
@Inject private AllUsersName allUsers;
+ @Inject private AccountsUpdate.Server accountsUpdate;
+
@Inject private AccountByEmailCache byEmailCache;
@Inject private ExternalIds externalIds;
@@ -225,56 +231,93 @@
assertThat(info.username).isEqualTo(name);
assertThat(info.name).isEqualTo(name);
accountIndexedCounter.assertReindexOf(foo, expectedAccountReindexCalls);
-
- // check user branch
- try (Repository repo = repoManager.openRepository(allUsers);
- RevWalk rw = new RevWalk(repo);
- ObjectReader or = repo.newObjectReader()) {
- Ref ref = repo.exactRef(RefNames.refsUsers(foo.getId()));
- assertThat(ref).isNotNull();
- RevCommit c = rw.parseCommit(ref.getObjectId());
- long timestampDiffMs =
- Math.abs(
- c.getCommitTime() * 1000L
- - accountCache.get(foo.getId()).getAccount().getRegisteredOn().getTime());
- assertThat(timestampDiffMs).isAtMost(ChangeRebuilderImpl.MAX_WINDOW_MS);
-
- // Check the 'account.config' file.
- try (TreeWalk tw = TreeWalk.forPath(or, AccountConfig.ACCOUNT_CONFIG, c.getTree())) {
- assertThat(tw).isNotNull();
- Config cfg = new Config();
- cfg.fromText(new String(or.open(tw.getObjectId(0), OBJ_BLOB).getBytes(), UTF_8));
- assertThat(cfg.getString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_FULL_NAME))
- .isEqualTo(name);
- }
- }
+ assertUserBranch(foo.getId(), name, null);
}
@Test
public void createAnonymousCoward() throws Exception {
TestAccount anonymousCoward = accountCreator.create();
accountIndexedCounter.assertReindexOf(anonymousCoward);
+ assertUserBranchWithoutAccountConfig(anonymousCoward.getId());
+ }
- // check user branch
+ @Test
+ public void updateNonExistingAccount() throws Exception {
+ Account.Id nonExistingAccountId = new Account.Id(999999);
+ AtomicBoolean consumerCalled = new AtomicBoolean();
+ Account account =
+ accountsUpdate.create().update(db, nonExistingAccountId, a -> consumerCalled.set(true));
+ assertThat(account).isNull();
+ assertThat(consumerCalled.get()).isFalse();
+ }
+
+ @Test
+ public void updateAccountThatIsMissingInNoteDb() throws Exception {
+ String name = "bar";
+ TestAccount bar = accountCreator.create(name);
+ assertUserBranch(bar.getId(), name, null);
+
+ // delete user branch
+ try (Repository repo = repoManager.openRepository(allUsers)) {
+ AccountsUpdate.deleteUserBranch(
+ repo, allUsers, GitReferenceUpdated.DISABLED, null, serverIdent.get(), bar.getId());
+ assertThat(repo.exactRef(RefNames.refsUsers(bar.getId()))).isNull();
+ }
+
+ String status = "OOO";
+ Account account = accountsUpdate.create().update(db, bar.getId(), a -> a.setStatus(status));
+ assertThat(account).isNotNull();
+ assertThat(account.getFullName()).isEqualTo(name);
+ assertThat(account.getStatus()).isEqualTo(status);
+ assertUserBranch(bar.getId(), name, status);
+ }
+
+ @Test
+ public void updateAccountWithoutAccountConfigNoteDb() throws Exception {
+ TestAccount anonymousCoward = accountCreator.create();
+ assertUserBranchWithoutAccountConfig(anonymousCoward.getId());
+
+ String status = "OOO";
+ Account account =
+ accountsUpdate.create().update(db, anonymousCoward.getId(), a -> a.setStatus(status));
+ assertThat(account).isNotNull();
+ assertThat(account.getFullName()).isNull();
+ assertThat(account.getStatus()).isEqualTo(status);
+ assertUserBranch(anonymousCoward.getId(), null, status);
+ }
+
+ private void assertUserBranchWithoutAccountConfig(Account.Id accountId) throws Exception {
+ assertUserBranch(accountId, null, null);
+ }
+
+ private void assertUserBranch(
+ Account.Id accountId, @Nullable String name, @Nullable String status) throws Exception {
try (Repository repo = repoManager.openRepository(allUsers);
RevWalk rw = new RevWalk(repo);
ObjectReader or = repo.newObjectReader()) {
- Ref ref = repo.exactRef(RefNames.refsUsers(anonymousCoward.getId()));
+ Ref ref = repo.exactRef(RefNames.refsUsers(accountId));
assertThat(ref).isNotNull();
RevCommit c = rw.parseCommit(ref.getObjectId());
long timestampDiffMs =
Math.abs(
c.getCommitTime() * 1000L
- - accountCache
- .get(anonymousCoward.getId())
- .getAccount()
- .getRegisteredOn()
- .getTime());
+ - accountCache.get(accountId).getAccount().getRegisteredOn().getTime());
assertThat(timestampDiffMs).isAtMost(ChangeRebuilderImpl.MAX_WINDOW_MS);
- // No account properties were set, hence an 'account.config' file was not created.
+ // Check the 'account.config' file.
try (TreeWalk tw = TreeWalk.forPath(or, AccountConfig.ACCOUNT_CONFIG, c.getTree())) {
- assertThat(tw).isNull();
+ if (name != null || status != null) {
+ assertThat(tw).isNotNull();
+ Config cfg = new Config();
+ cfg.fromText(new String(or.open(tw.getObjectId(0), OBJ_BLOB).getBytes(), UTF_8));
+ assertThat(cfg.getString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_FULL_NAME))
+ .isEqualTo(name);
+ assertThat(cfg.getString(AccountConfig.ACCOUNT, null, AccountConfig.KEY_STATUS))
+ .isEqualTo(status);
+ } else {
+ // No account properties were set, hence an 'account.config' file was not created.
+ assertThat(tw).isNull();
+ }
}
}
}
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 1a08090..f9fdcfc 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
@@ -250,21 +250,28 @@
}
// Update in ReviewDb
- db.accounts()
- .atomicUpdate(
- accountId,
- a -> {
- consumers.stream().forEach(c -> c.accept(a));
- return a;
- });
+ Account reviewDbAccount =
+ db.accounts()
+ .atomicUpdate(
+ accountId,
+ a -> {
+ consumers.stream().forEach(c -> c.accept(a));
+ return a;
+ });
// Update in NoteDb
AccountConfig accountConfig = read(accountId);
Account account = accountConfig.getAccount();
- consumers.stream().forEach(c -> c.accept(account));
- commit(accountConfig);
+ if (account != null) {
+ consumers.stream().forEach(c -> c.accept(account));
+ commit(accountConfig);
+ } else if (reviewDbAccount != null) {
+ // user branch doesn't exist yet
+ accountConfig.setAccount(reviewDbAccount);
+ commitNew(accountConfig);
+ }
- return account;
+ return reviewDbAccount;
}
/**
