From 30af94f4ce20cfd69426381d3da81173443be2d7 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 23 Aug 2017 11:45:03 +0200 Subject: [PATCH] Remove support for reading accounts from ReviewDb Accounts have been fully migrated to NoteDb. However inside of Google we still have code that depends on having all accounts in the Accounts table. This means we must still write account updates to the Accounts table. On the other hand also for us Gerrit server is always reading the accounts from NoteDb. This means the code for reading accounts from ReviewDb can be removed already now. This is why removing the support for accounts in ReviewDb is done in 2 parts: 1. remove support for reading accounts from ReviewDb (this change) 2. remove support for writing accounts to ReviewDb (follow-up change) Change-Id: Id51a0504635de30f95205ec76d51e2be8ff6f462 Signed-off-by: Edwin Kempin --- .../acceptance/api/accounts/AccountIT.java | 5 +- .../become/BecomeAnyAccountLoginServlet.java | 6 +- .../server/account/AccountCacheImpl.java | 2 +- .../server/account/AccountResolver.java | 9 +-- .../gerrit/server/account/Accounts.java | 60 +++++++------------ .../account/AccountsConsistencyChecker.java | 12 +--- .../server/change/ConsistencyChecker.java | 4 +- .../account/AbstractQueryAccountsTest.java | 2 +- 8 files changed, 33 insertions(+), 67 deletions(-) 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 0d68f4abed..570621f8da 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 @@ -45,7 +45,6 @@ import com.google.common.io.BaseEncoding; import com.google.common.util.concurrent.AtomicLongMap; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.AccountCreator; -import com.google.gerrit.acceptance.GerritConfig; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.Sandboxed; import com.google.gerrit.acceptance.TestAccount; @@ -1024,7 +1023,6 @@ public class AccountIT extends AbstractDaemonTest { @Test @Sandboxed - @GerritConfig(name = "user.readAccountsFromGit", value = "true") public void deleteUserBranchWithAccessDatabaseCapability() throws Exception { allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); grant( @@ -1282,10 +1280,9 @@ public class AccountIT extends AbstractDaemonTest { } @Test - @GerritConfig(name = "user.readAccountsFromGit", value = "true") public void checkMetaId() throws Exception { // metaId is set when account is loaded - assertThat(accounts.get(db, admin.getId()).getMetaId()).isEqualTo(getMetaId(admin.getId())); + assertThat(accounts.get(admin.getId()).getMetaId()).isEqualTo(getMetaId(admin.getId())); // metaId is set when account is created AccountsUpdate au = accountsUpdate.create(); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java index 01aec6e010..1f095e0540 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java @@ -239,9 +239,9 @@ class BecomeAnyAccountLoginServlet extends HttpServlet { } catch (NumberFormatException nfe) { return null; } - try (ReviewDb db = schema.open()) { - return auth(accounts.get(db, id)); - } catch (OrmException | IOException | ConfigInvalidException e) { + try { + return auth(accounts.get(id)); + } catch (IOException | ConfigInvalidException e) { getServletContext().log("cannot query database", e); return null; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java index 16901ed213..5ac7ac4cb5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java @@ -208,7 +208,7 @@ public class AccountCacheImpl implements AccountCache { private Optional load(ReviewDb db, Account.Id who) throws OrmException, IOException, ConfigInvalidException { - Account account = accounts.get(db, who); + Account account = accounts.get(who); if (account == null) { return Optional.empty(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java index 894f7a18e4..94f63a7231 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java @@ -98,7 +98,7 @@ public class AccountResolver { Matcher m = Pattern.compile("^.* \\(([1-9][0-9]*)\\)$").matcher(nameOrEmail); if (m.matches()) { Account.Id id = Account.Id.parse(m.group(1)); - if (exists(db, id)) { + if (accounts.get(id) != null) { return Collections.singleton(id); } return Collections.emptySet(); @@ -106,7 +106,7 @@ public class AccountResolver { if (nameOrEmail.matches("^[1-9][0-9]*$")) { Account.Id id = Account.Id.parse(nameOrEmail); - if (exists(db, id)) { + if (accounts.get(id) != null) { return Collections.singleton(id); } return Collections.emptySet(); @@ -122,11 +122,6 @@ public class AccountResolver { return findAllByNameOrEmail(db, nameOrEmail); } - private boolean exists(ReviewDb db, Account.Id id) - throws OrmException, IOException, ConfigInvalidException { - return accounts.get(db, id) != null; - } - /** * Locate exactly one account matching the name or name/email string. * diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/Accounts.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/Accounts.java index cb8287926d..cc35438541 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/Accounts.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/Accounts.java @@ -20,12 +20,9 @@ import static java.util.stream.Collectors.toSet; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.config.AllUsersName; -import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.mail.send.OutgoingEmailValidator; -import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; @@ -36,7 +33,6 @@ import java.util.Objects; import java.util.Set; import java.util.stream.Stream; import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Repository; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -46,47 +42,35 @@ import org.slf4j.LoggerFactory; public class Accounts { private static final Logger log = LoggerFactory.getLogger(Accounts.class); - private final boolean readFromGit; private final GitRepositoryManager repoManager; private final AllUsersName allUsersName; private final OutgoingEmailValidator emailValidator; @Inject Accounts( - @GerritServerConfig Config cfg, GitRepositoryManager repoManager, AllUsersName allUsersName, OutgoingEmailValidator emailValidator) { - this.readFromGit = cfg.getBoolean("user", null, "readAccountsFromGit", true); this.repoManager = repoManager; this.allUsersName = allUsersName; this.emailValidator = emailValidator; } - public Account get(ReviewDb db, Account.Id accountId) - throws OrmException, IOException, ConfigInvalidException { - if (readFromGit) { - try (Repository repo = repoManager.openRepository(allUsersName)) { - return read(repo, accountId); - } + public Account get(Account.Id accountId) throws IOException, ConfigInvalidException { + try (Repository repo = repoManager.openRepository(allUsersName)) { + return read(repo, accountId); } - - return db.accounts().get(accountId); } - public List get(ReviewDb db, Collection accountIds) - throws OrmException, IOException, ConfigInvalidException { - if (readFromGit) { - List accounts = new ArrayList<>(accountIds.size()); - try (Repository repo = repoManager.openRepository(allUsersName)) { - for (Account.Id accountId : accountIds) { - accounts.add(read(repo, accountId)); - } + public List get(Collection accountIds) + throws IOException, ConfigInvalidException { + List accounts = new ArrayList<>(accountIds.size()); + try (Repository repo = repoManager.openRepository(allUsersName)) { + for (Account.Id accountId : accountIds) { + accounts.add(read(repo, accountId)); } - return accounts; } - - return db.accounts().get(accountIds).toList(); + return accounts; } /** @@ -94,23 +78,19 @@ public class Accounts { * * @return all accounts */ - public List all(ReviewDb db) throws OrmException, IOException { - if (readFromGit) { - Set accountIds = allIds(); - List accounts = new ArrayList<>(accountIds.size()); - try (Repository repo = repoManager.openRepository(allUsersName)) { - for (Account.Id accountId : accountIds) { - try { - accounts.add(read(repo, accountId)); - } catch (Exception e) { - log.error(String.format("Ignoring invalid account %s", accountId.get()), e); - } + public List all() throws IOException { + Set accountIds = allIds(); + List accounts = new ArrayList<>(accountIds.size()); + try (Repository repo = repoManager.openRepository(allUsersName)) { + for (Account.Id accountId : accountIds) { + try { + accounts.add(read(repo, accountId)); + } catch (Exception e) { + log.error(String.format("Ignoring invalid account %s", accountId.get()), e); } } - return accounts; } - - return db.accounts().all().toList(); + return accounts; } /** diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java index 2f3f657fc5..0085303f4e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java @@ -16,11 +16,8 @@ package com.google.gerrit.server.account; import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo; import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.account.externalids.ExternalIds; -import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; -import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.util.ArrayList; @@ -28,22 +25,19 @@ import java.util.List; @Singleton public class AccountsConsistencyChecker { - private final Provider dbProvider; private final Accounts accounts; private final ExternalIds externalIds; @Inject - AccountsConsistencyChecker( - Provider dbProvider, Accounts accounts, ExternalIds externalIds) { - this.dbProvider = dbProvider; + AccountsConsistencyChecker(Accounts accounts, ExternalIds externalIds) { this.accounts = accounts; this.externalIds = externalIds; } - public List check() throws OrmException, IOException { + public List check() throws IOException { List problems = new ArrayList<>(); - for (Account account : accounts.all(dbProvider.get())) { + for (Account account : accounts.all()) { if (account.getPreferredEmail() != null) { if (!externalIds .byAccount(account.getId()) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java index 56f637e9bd..46d80630bb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -224,10 +224,10 @@ public class ConsistencyChecker { private void checkOwner() { try { - if (accounts.get(db.get(), change().getOwner()) == null) { + if (accounts.get(change().getOwner()) == null) { problem("Missing change owner: " + change().getOwner()); } - } catch (OrmException | IOException | ConfigInvalidException e) { + } catch (IOException | ConfigInvalidException e) { error("Failed to look up owner", 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 c5eea0f29a..8fec96a0e8 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 @@ -400,7 +400,7 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests { // update account in ReviewDb without reindex so that account index is stale String newName = "Test User"; Account.Id accountId = new Account.Id(user1._accountId); - Account account = accounts.get(db, accountId); + Account account = accounts.get(accountId); account.setFullName(newName); db.accounts().update(ImmutableSet.of(account));