opendev/gerrit
Code Issues Proposed changes

Remove support for reading accounts from ReviewDb

Browse Source 
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 <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2017-08-23 11:45:03 +02:00
parent 4edaf0eda1
commit 30af94f4ce
8 changed files with 33 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

@@ -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();

6
gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java
View File

@@ -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;
}

2
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java
View File

@@ -208,7 +208,7 @@ public class AccountCacheImpl implements AccountCache {
private Optional<AccountState> 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();
}

9
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java
View File

@@ -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.
*

60
gerrit-server/src/main/java/com/google/gerrit/server/account/Accounts.java
View File

@@ -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<Account> get(ReviewDb db, Collection<Account.Id> accountIds)
throws OrmException, IOException, ConfigInvalidException {
if (readFromGit) {
List<Account> accounts = new ArrayList<>(accountIds.size());
try (Repository repo = repoManager.openRepository(allUsersName)) {
for (Account.Id accountId : accountIds) {
accounts.add(read(repo, accountId));
}
public List<Account> get(Collection<Account.Id> accountIds)
throws IOException, ConfigInvalidException {
List<Account> 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<Account> all(ReviewDb db) throws OrmException, IOException {
if (readFromGit) {
Set<Account.Id> accountIds = allIds();
List<Account> 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<Account> all() throws IOException {
Set<Account.Id> accountIds = allIds();
List<Account> 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;
}
/**

12
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java
View File

@@ -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<ReviewDb> dbProvider;
private final Accounts accounts;
private final ExternalIds externalIds;
@Inject
AccountsConsistencyChecker(
Provider<ReviewDb> dbProvider, Accounts accounts, ExternalIds externalIds) {
this.dbProvider = dbProvider;
AccountsConsistencyChecker(Accounts accounts, ExternalIds externalIds) {
this.accounts = accounts;
this.externalIds = externalIds;
}
public List<ConsistencyProblemInfo> check() throws OrmException, IOException {
public List<ConsistencyProblemInfo> check() throws IOException {
List<ConsistencyProblemInfo> problems = new ArrayList<>();
for (Account account : accounts.all(dbProvider.get())) {
for (Account account : accounts.all()) {
if (account.getPreferredEmail() != null) {
if (!externalIds
.byAccount(account.getId())

4
gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
View File

@@ -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);
}
}

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

@@ -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));