opendev/gerrit
Code Issues Proposed changes

AccountsUpdate: Remove methods to delete account

Browse Source 
Gerrit doesn't support deletion of accounts. AccountsUpdate only
supported the deletion of user branches to do a rollback if there was
an error during account creation. Since the account creation is now
done atomically we no longer need to manually rollback the creation of
user branches. Hence the delete methods from AccountsUpdate can be
removed.

The last remaining callers of these methods were a test and a schema
migration in which the needed functionality can be inlined.

Change-Id: I58d591dba50aa70d5423f1d79b8e9af47bf55281
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-01-02 08:30:45 +01:00
parent c5ee0fe7b4
commit 3e4dd8f278
3 changed files with 46 additions and 90 deletions
Download Patch File Download Diff File Expand all files Collapse all files

74
java/com/google/gerrit/server/account/AccountsUpdate.java
View File

@@ -24,8 +24,6 @@ import com.google.common.collect.Lists;
import com.google.common.util.concurrent.Runnables;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.externalids.ExternalIdNotes;
@@ -51,11 +49,7 @@ import java.util.Optional;
import java.util.function.Consumer;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.RefUpdate.Result;
import org.eclipse.jgit.lib.Repository;
/**
@@ -446,74 +440,6 @@ public class AccountsUpdate {
});
}
/**
* Deletes the account.
*
* @param account the account that should be deleted
* @throws IOException if deleting the user branch fails due to an IO error
* @throws OrmException if deleting the user branch fails
* @throws ConfigInvalidException
*/
public void delete(Account account) throws IOException, OrmException, ConfigInvalidException {
deleteByKey(account.getId());
}
/**
* Deletes the account.
*
* @param accountId the ID of the account that should be deleted
* @throws IOException if deleting the user branch fails due to an IO error
* @throws OrmException if deleting the user branch fails
* @throws ConfigInvalidException
*/
public void deleteByKey(Account.Id accountId)
throws IOException, OrmException, ConfigInvalidException {
deleteAccount(accountId);
}
private Account deleteAccount(Account.Id accountId)
throws IOException, OrmException, ConfigInvalidException {
return retryHelper.execute(
ActionType.ACCOUNT_UPDATE,
() -> {
deleteUserBranch(accountId);
return null;
});
}
private void deleteUserBranch(Account.Id accountId) throws IOException {
try (Repository repo = repoManager.openRepository(allUsersName)) {
deleteUserBranch(repo, allUsersName, gitRefUpdated, currentUser, authorIdent, accountId);
}
}
public static void deleteUserBranch(
Repository repo,
Project.NameKey project,
GitReferenceUpdated gitRefUpdated,
@Nullable IdentifiedUser user,
PersonIdent refLogIdent,
Account.Id accountId)
throws IOException {
String refName = RefNames.refsUsers(accountId);
Ref ref = repo.exactRef(refName);
if (ref == null) {
return;
}
RefUpdate ru = repo.updateRef(refName);
ru.setExpectedOldObjectId(ref.getObjectId());
ru.setNewObjectId(ObjectId.zeroId());
ru.setForceUpdate(true);
ru.setRefLogIdent(refLogIdent);
ru.setRefLogMessage("Delete Account", true);
Result result = ru.delete();
if (result != Result.FORCED) {
throw new IOException(String.format("Failed to delete ref %s: %s", refName, result.name()));
}
gitRefUpdated.fire(project, ru, user != null ? user.getAccount() : null);
}
private AccountConfig read(Repository allUsersRepo, Account.Id accountId)
throws IOException, ConfigInvalidException {
AccountConfig accountConfig = new AccountConfig(emailValidator, accountId);

35
java/com/google/gerrit/server/schema/Schema_147.java
View File

@@ -19,10 +19,7 @@ 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.GerritPersonIdent;
import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -34,25 +31,23 @@ import java.sql.Statement;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.RefUpdate.Result;
import org.eclipse.jgit.lib.Repository;
/** Delete user branches for which no account exists. */
public class Schema_147 extends SchemaVersion {
private final GitRepositoryManager repoManager;
private final AllUsersName allUsersName;
private final PersonIdent serverIdent;
@Inject
Schema_147(
Provider<Schema_146> prior,
GitRepositoryManager repoManager,
AllUsersName allUsersName,
@GerritPersonIdent PersonIdent serverIdent) {
Provider<Schema_146> prior, GitRepositoryManager repoManager, AllUsersName allUsersName) {
super(prior);
this.repoManager = repoManager;
this.allUsersName = allUsersName;
this.serverIdent = serverIdent;
}
@Override
@@ -69,8 +64,7 @@ public class Schema_147 extends SchemaVersion {
.collect(toSet());
accountIdsFromUserBranches.removeAll(accountIdsFromReviewDb);
for (Account.Id accountId : accountIdsFromUserBranches) {
AccountsUpdate.deleteUserBranch(
repo, allUsersName, GitReferenceUpdated.DISABLED, null, serverIdent, accountId);
deleteUserBranch(repo, accountId);
}
} catch (IOException e) {
throw new OrmException("Failed to delete user branches for non-existing accounts.", e);
@@ -87,4 +81,21 @@ public class Schema_147 extends SchemaVersion {
return ids;
}
}
private void deleteUserBranch(Repository allUsersRepo, Account.Id accountId) throws IOException {
String refName = RefNames.refsUsers(accountId);
Ref ref = allUsersRepo.exactRef(refName);
if (ref == null) {
return;
}
RefUpdate ru = allUsersRepo.updateRef(refName);
ru.setExpectedOldObjectId(ref.getObjectId());
ru.setNewObjectId(ObjectId.zeroId());
ru.setForceUpdate(true);
Result result = ru.delete();
if (result != Result.FORCED) {
throw new IOException(String.format("Failed to delete ref %s: %s", refName, result.name()));
}
}
}

27
javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
View File

@@ -41,7 +41,6 @@ import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.ConsistencyChecker;
import com.google.gerrit.server.change.PatchSetInserter;
@@ -67,7 +66,10 @@ import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.RefUpdate.Result;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
@@ -90,8 +92,6 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
@Inject private Sequences sequences;
@Inject private AccountsUpdate.Server accountsUpdate;
private RevCommit tip;
private Account.Id adminId;
private ConsistencyChecker checker;
@@ -126,7 +126,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
public void missingOwner() throws Exception {
TestAccount owner = accountCreator.create("missing");
ChangeNotes notes = insertChange(owner);
accountsUpdate.create().deleteByKey(owner.getId());
deleteUserBranch(owner.getId());
assertProblems(notes, null, problem("Missing change owner: " + owner.getId()));
}
@@ -958,4 +958,23 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
private void assertNoProblems(ChangeNotes notes, @Nullable FixInput fix) throws Exception {
assertThat(checker.check(notes, fix).problems()).isEmpty();
}
private void deleteUserBranch(Account.Id accountId) throws IOException {
try (Repository repo = repoManager.openRepository(allUsers)) {
String refName = RefNames.refsUsers(accountId);
Ref ref = repo.exactRef(refName);
if (ref == null) {
return;
}
RefUpdate ru = repo.updateRef(refName);
ru.setExpectedOldObjectId(ref.getObjectId());
ru.setNewObjectId(ObjectId.zeroId());
ru.setForceUpdate(true);
Result result = ru.delete();
if (result != Result.FORCED) {
throw new IOException(String.format("Failed to delete ref %s: %s", refName, result.name()));
}
}
}
}