From 2a1d82da9baf48441600a84b83ee9357bf6c3b29 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 12 Jul 2016 18:22:15 -0400 Subject: [PATCH] Implement consistency checker for NoteDb Change the public interface of ConsistencyChecker to only take a ChangeControl, since this always includes a pre-loaded ChangeNotes. Not all callers have a ChangeData available, and it's wasteful to load a Change just to turn around and load a ChangeNotes from it again. This simplifies the interface and implementation, but does mean that certain types of corruption, such as complete garbage in a NoteDb commit message, will result in an error in the caller rather than a nicely-formatted message. However, when rewriting the tests, I found that there were not really any situations we cared about that are really and truly corrupt data. Most issues were due to the database and the underlying repo getting out of sync. From the perspective of NoteDb, it's pretty easy to generate these corrupt states. The only place we had to go behind the back of normal change operations during tests was when pointing a patch set at a nonexisting object, since otherwise PatchSetUtil will try to parse the object to get the subject. It was easy enough to manually construct a commit message there. The most complicated fix was for deleting patch sets pointing to nonexistent objects. I think this actually turned out nicer, since we were able to decompose the problem into a set of independent deletion operations that then get packaged together into a single BatchUpdate. The one and only test that really cannot be replicated in NoteDb is when the change destination repository is missing, since that's where the NoteDb data would be stored. This one it's ok to skip. Since I was rewriting the tests pretty much completely anyway, I simplified some code by implementing ProblemInfo#equals so we can use assertThat(problems).containsExactly(...) for more compact assertions. Change-Id: I1d77f9202c053127089b4926383abf02aa24466b --- .../server/change/ConsistencyCheckerIT.java | 897 ++++++++++-------- .../gerrit/extensions/common/ProblemInfo.java | 18 + .../gerrit/server/change/ChangeJson.java | 22 +- .../google/gerrit/server/change/Check.java | 15 +- .../server/change/ConsistencyChecker.java | 394 ++++---- 5 files changed, 780 insertions(+), 566 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index f84615105f..b88278eabc 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -14,19 +14,22 @@ package com.google.gerrit.acceptance.server.change; -import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; -import static com.google.gerrit.testutil.TestChanges.newChange; +import static com.google.gerrit.extensions.common.ProblemInfo.Status.FIXED; +import static com.google.gerrit.extensions.common.ProblemInfo.Status.FIX_FAILED; import static com.google.gerrit.testutil.TestChanges.newPatchSet; import static java.util.Collections.singleton; -import com.google.common.base.Function; -import com.google.common.collect.Lists; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; +import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.common.FooterConstants; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.api.changes.FixInput; +import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ProblemInfo; @@ -34,22 +37,39 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; +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.change.ChangeInserter; import com.google.gerrit.server.change.ConsistencyChecker; -import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.change.PatchSetInserter; +import com.google.gerrit.server.config.AnonymousCowardName; +import com.google.gerrit.server.git.BatchUpdate; +import com.google.gerrit.server.git.BatchUpdate.ChangeContext; +import com.google.gerrit.server.git.BatchUpdate.RepoContext; +import com.google.gerrit.server.git.validators.CommitValidators; +import com.google.gerrit.server.notedb.ChangeNoteUtil; import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.testutil.InMemoryRepositoryManager; import com.google.gerrit.testutil.TestChanges; +import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; 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.RefUpdate; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.transport.ReceiveCommand; import org.junit.Before; import org.junit.Test; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; import java.util.List; @NoHttpd @@ -57,26 +77,37 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { @Inject private ChangeControl.GenericFactory changeControlFactory; - @Inject - private ChangeUpdate.Factory changeUpdateFactory; - @Inject private Provider checkerProvider; @Inject private IdentifiedUser.GenericFactory userFactory; + @Inject + private BatchUpdate.Factory updateFactory; + + @Inject + private ChangeInserter.Factory changeInserterFactory; + + @Inject + private PatchSetInserter.Factory patchSetInserterFactory; + + @Inject + private ChangeNoteUtil noteUtil; + + @Inject + @AnonymousCowardName + private String anonymousCowardName; + + @Inject + private Sequences sequences; + private RevCommit tip; private Account.Id adminId; private ConsistencyChecker checker; @Before public void setUp() throws Exception { - // TODO(dborowitz): Re-enable when ConsistencyChecker supports NoteDb. - // Note that we *do* want to enable these tests with GERRIT_CHECK_NOTEDB, as - // we need to be able to convert old, corrupt changes. However, those tests - // don't necessarily need to pass. - assume().that(notesMigration.enabled()).isFalse(); // Ignore client clone of project; repurpose as server-side TestRepository. testRepo = new TestRepository<>( (InMemoryRepository) repoManager.openRepository(project)); @@ -88,62 +119,56 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { @Test public void validNewChange() throws Exception { - Change c = insertChange(); - insertPatchSet(c); - incrementPatchSet(c); - insertPatchSet(c); - assertProblems(c); + assertNoProblems(insertChange(), null); } @Test public void validMergedChange() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - insertPatchSet(c); - incrementPatchSet(c); - - incrementPatchSet(c); - RevCommit commit2 = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).create(); - PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit2, adminId); - db.patchSets().insert(singleton(ps2)); - - testRepo.branch(c.getDest().get()).update(commit2); - assertProblems(c); + ChangeControl ctl = mergeChange(incrementPatchSet(insertChange())); + assertNoProblems(ctl, null); } @Test public void missingOwner() throws Exception { - Change c = newChange(project, new Account.Id(2)); - db.changes().insert(singleton(c)); - RevCommit commit = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - db.patchSets().insert(singleton(ps)); + TestAccount owner = accounts.create("missing"); + ChangeControl ctl = insertChange(owner); + db.accounts().deleteKeys(singleton(owner.getId())); - assertProblems(c, "Missing change owner: 2"); + assertProblems(ctl, null, + problem("Missing change owner: " + owner.getId())); } @Test public void missingRepo() throws Exception { - Change c = newChange(new Project.NameKey("otherproject"), adminId); - db.changes().insert(singleton(c)); - insertMissingPatchSet(c, "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); - assertProblems(c, "Destination repository not found: otherproject"); + // NoteDb can't have a change without a repo. + assume().that(notesMigration.enabled()).isFalse(); + + ChangeControl ctl = insertChange(); + Project.NameKey name = ctl.getProject().getNameKey(); + ((InMemoryRepositoryManager) repoManager).deleteRepository(name); + + assertProblems( + ctl, null, + problem("Destination repository not found: " + name)); } @Test public void invalidRevision() throws Exception { - Change c = insertChange(); + // NoteDb always parses the revision when inserting a patch set, so we can't + // create an invalid patch set. + assume().that(notesMigration.enabled()).isFalse(); - db.patchSets().insert(singleton(newPatchSet(c.currentPatchSetId(), - "fooooooooooooooooooooooooooooooooooooooo", adminId))); - incrementPatchSet(c); - insertPatchSet(c); + ChangeControl ctl = insertChange(); + PatchSet ps = newPatchSet( + ctl.getChange().currentPatchSetId(), + "fooooooooooooooooooooooooooooooooooooooo", + adminId); + db.patchSets().update(singleton(ps)); - assertProblems(c, - "Invalid revision on patch set 1:" - + " fooooooooooooooooooooooooooooooooooooooo"); + assertProblems( + ctl, null, + problem("Invalid revision on patch set 1:" + + " fooooooooooooooooooooooooooooooooooooooo")); } // No test for ref existing but object missing; InMemoryRepository won't let @@ -151,414 +176,429 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { @Test public void patchSetObjectAndRefMissing() throws Exception { - Change c = insertChange(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), - ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), adminId); - db.patchSets().insert(singleton(ps)); - - assertProblems(c, - "Ref missing: " + ps.getId().toRefName(), - "Object missing: patch set 1: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + ChangeControl ctl = insertChange(); + PatchSet ps = insertMissingPatchSet(ctl, rev); + ctl = reload(ctl); + assertProblems( + ctl, null, + problem("Ref missing: " + ps.getId().toRefName()), + problem( + "Object missing: patch set 2:" + + " deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); } @Test public void patchSetObjectAndRefMissingWithFix() throws Exception { - Change c = insertChange(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), - ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), adminId); - db.patchSets().insert(singleton(ps)); + String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + ChangeControl ctl = insertChange(); + PatchSet ps = insertMissingPatchSet(ctl, rev); + ctl = reload(ctl); String refName = ps.getId().toRefName(); - List problems = checker.check(c, new FixInput()).problems(); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo("Ref missing: " + refName); - assertThat(p.status).isNull(); + assertProblems( + ctl, new FixInput(), + problem("Ref missing: " + refName), + problem("Object missing: patch set 2: " + rev)); } @Test public void patchSetRefMissing() throws Exception { - Change c = insertChange(); - PatchSet ps = insertPatchSet(c); - String refName = ps.getId().toRefName(); - testRepo.update("refs/other/foo", ObjectId.fromString(ps.getRevision().get())); + ChangeControl ctl = insertChange(); + testRepo.update( + "refs/other/foo", + ObjectId.fromString( + psUtil.current(db, ctl.getNotes()).getRevision().get())); + String refName = ctl.getChange().currentPatchSetId().toRefName(); deleteRef(refName); - assertProblems(c, "Ref missing: " + refName); + assertProblems(ctl, null, problem("Ref missing: " + refName)); } @Test public void patchSetRefMissingWithFix() throws Exception { - Change c = insertChange(); - PatchSet ps = insertPatchSet(c); - String refName = ps.getId().toRefName(); - testRepo.update("refs/other/foo", ObjectId.fromString(ps.getRevision().get())); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + testRepo.update("refs/other/foo", ObjectId.fromString(rev)); + String refName = ctl.getChange().currentPatchSetId().toRefName(); deleteRef(refName); - List problems = checker.check(c, new FixInput()).problems(); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo("Ref missing: " + refName); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Repaired patch set ref"); - + assertProblems( + ctl, new FixInput(), + problem("Ref missing: " + refName, FIXED, "Repaired patch set ref")); assertThat(testRepo.getRepository().exactRef(refName).getObjectId().name()) - .isEqualTo(ps.getRevision().get()); + .isEqualTo(rev); } @Test public void patchSetObjectAndRefMissingWithDeletingPatchSet() throws Exception { - Change c = insertChange(); - PatchSet ps1 = insertPatchSet(c); - incrementPatchSet(c); - PatchSet ps2 = insertMissingPatchSet(c, - "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + ChangeControl ctl = insertChange(); + PatchSet ps1 = psUtil.current(db, ctl.getNotes()); + + String rev2 = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + PatchSet ps2 = insertMissingPatchSet(ctl, rev2); + ctl = reload(ctl); FixInput fix = new FixInput(); fix.deletePatchSetIfCommitMissing = true; - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(2); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo("Ref missing: " + ps2.getId().toRefName()); - assertThat(p.status).isNull(); - p = problems.get(1); - assertThat(p.message).isEqualTo( - "Object missing: patch set 2: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Deleted patch set"); + assertProblems( + ctl, fix, + problem("Ref missing: " + ps2.getId().toRefName()), + problem("Object missing: patch set 2: " + rev2, + FIXED, "Deleted patch set")); - c = notesFactory.createChecked(db, c).getChange(); - assertThat(c.currentPatchSetId().get()).isEqualTo(1); - assertThat(getPatchSet(ps1.getId())).isNotNull(); - assertThat(getPatchSet(ps2.getId())).isNull(); + ctl = reload(ctl); + assertThat(ctl.getChange().currentPatchSetId().get()).isEqualTo(1); + assertThat(psUtil.get(db, ctl.getNotes(), ps1.getId())).isNotNull(); + assertThat(psUtil.get(db, ctl.getNotes(), ps2.getId())).isNull(); } @Test public void patchSetMultipleObjectsMissingWithDeletingPatchSets() throws Exception { - Change c = insertChange(); - PatchSet ps1 = insertPatchSet(c); + ChangeControl ctl = insertChange(); + PatchSet ps1 = psUtil.current(db, ctl.getNotes()); - incrementPatchSet(c); - PatchSet ps2 = insertMissingPatchSet(c, - "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + String rev2 = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + PatchSet ps2 = insertMissingPatchSet(ctl, rev2); - incrementPatchSet(c); - PatchSet ps3 = insertPatchSet(c); + ctl = incrementPatchSet(reload(ctl)); + PatchSet ps3 = psUtil.current(db, ctl.getNotes()); - incrementPatchSet(c); - PatchSet ps4 = insertMissingPatchSet(c, - "c0ffeeeec0ffeeeec0ffeeeec0ffeeeec0ffeeee"); + String rev4 = "c0ffeeeec0ffeeeec0ffeeeec0ffeeeec0ffeeee"; + PatchSet ps4 = insertMissingPatchSet(ctl, rev4); + ctl = reload(ctl); FixInput fix = new FixInput(); fix.deletePatchSetIfCommitMissing = true; - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(4); + assertProblems( + ctl, fix, + problem("Ref missing: " + ps2.getId().toRefName()), + problem("Object missing: patch set 2: " + rev2, + FIXED, "Deleted patch set"), + problem("Ref missing: " + ps4.getId().toRefName()), + problem("Object missing: patch set 4: " + rev4, + FIXED, "Deleted patch set")); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo("Ref missing: " + ps4.getId().toRefName()); - assertThat(p.status).isNull(); - - p = problems.get(1); - assertThat(p.message).isEqualTo( - "Object missing: patch set 4: c0ffeeeec0ffeeeec0ffeeeec0ffeeeec0ffeeee"); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Deleted patch set"); - - p = problems.get(2); - assertThat(p.message).isEqualTo("Ref missing: " + ps2.getId().toRefName()); - assertThat(p.status).isNull(); - - p = problems.get(3); - assertThat(p.message).isEqualTo( - "Object missing: patch set 2: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Deleted patch set"); - - c = notesFactory.createChecked(db, c).getChange(); - assertThat(c.currentPatchSetId().get()).isEqualTo(3); - assertThat(getPatchSet(ps1.getId())).isNotNull(); - assertThat(getPatchSet(ps2.getId())).isNull(); - assertThat(getPatchSet(ps3.getId())).isNotNull(); - assertThat(getPatchSet(ps4.getId())).isNull(); + ctl = reload(ctl); + assertThat(ctl.getChange().currentPatchSetId().get()).isEqualTo(3); + assertThat(psUtil.get(db, ctl.getNotes(), ps1.getId())).isNotNull(); + assertThat(psUtil.get(db, ctl.getNotes(), ps2.getId())).isNull(); + assertThat(psUtil.get(db, ctl.getNotes(), ps3.getId())).isNotNull(); + assertThat(psUtil.get(db, ctl.getNotes(), ps4.getId())).isNull(); } @Test public void onlyPatchSetObjectMissingWithFix() throws Exception { - Change c = insertChange(); - PatchSet ps1 = insertMissingPatchSet(c, - "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + Change c = TestChanges.newChange( + project, admin.getId(), sequences.nextChangeId()); + PatchSet.Id psId = c.currentPatchSetId(); + String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + PatchSet ps = newPatchSet(psId, rev, adminId); + + db.changes().insert(singleton(c)); + db.patchSets().insert(singleton(ps)); + addNoteDbCommit( + c.getId(), + "Create change\n" + + "\n" + + "Patch-set: 1\n" + + "Branch: " + c.getDest().get() + "\n" + + "Change-id: " + c.getKey().get() + "\n" + + "Subject: Bogus subject\n" + + "Commit: " + rev + "\n" + + "Groups: " + rev + "\n"); + indexer.index(db, c.getProject(), c.getId()); + IdentifiedUser user = userFactory.create(admin.getId()); + ChangeControl ctl = changeControlFactory.controlFor( + db, c.getProject(), c.getId(), user); FixInput fix = new FixInput(); fix.deletePatchSetIfCommitMissing = true; - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(2); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo("Ref missing: " + ps1.getId().toRefName()); - assertThat(p.status).isNull(); - p = problems.get(1); - assertThat(p.message).isEqualTo( - "Object missing: patch set 1: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIX_FAILED); - assertThat(p.outcome) - .isEqualTo("Cannot delete patch set; no patch sets would remain"); + assertProblems( + ctl, fix, + problem("Ref missing: " + ps.getId().toRefName()), + problem("Object missing: patch set 1: " + rev, + FIX_FAILED, "Cannot delete patch set; no patch sets would remain")); - c = notesFactory.createChecked(db, c).getChange(); - assertThat(c.currentPatchSetId().get()).isEqualTo(1); - assertThat(getPatchSet(ps1.getId())).isNotNull(); + ctl = reload(ctl); + assertThat(ctl.getChange().currentPatchSetId().get()).isEqualTo(1); + assertThat(psUtil.current(db, ctl.getNotes())).isNotNull(); } @Test public void currentPatchSetMissing() throws Exception { - Change c = insertChange(); - assertProblems(c, "Current patch set 1 not found"); + // NoteDb can't create a change without a patch set. + assume().that(notesMigration.enabled()).isFalse(); + + ChangeControl ctl = insertChange(); + db.patchSets().deleteKeys(singleton(ctl.getChange().currentPatchSetId())); + assertProblems(ctl, null, problem("Current patch set 1 not found")); } @Test public void duplicatePatchSetRevisions() throws Exception { - Change c = insertChange(); - PatchSet ps1 = insertPatchSet(c); + ChangeControl ctl = insertChange(); + PatchSet ps1 = psUtil.current(db, ctl.getNotes()); String rev = ps1.getRevision().get(); - incrementPatchSet(c); - PatchSet ps2 = insertMissingPatchSet(c, rev); - updatePatchSetRef(ps2); - assertProblems(c, - "Multiple patch sets pointing to " + rev + ": [1, 2]"); + ctl = incrementPatchSet( + ctl, testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev))); + + assertProblems( + ctl, null, + problem("Multiple patch sets pointing to " + rev + ": [1, 2]")); } @Test public void missingDestRef() throws Exception { + ChangeControl ctl = insertChange(); + String ref = "refs/heads/master"; // Detach head so we're allowed to delete ref. testRepo.reset(testRepo.getRepository().exactRef(ref).getObjectId()); RefUpdate ru = testRepo.getRepository().updateRef(ref); ru.setForceUpdate(true); assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED); - Change c = insertChange(); - RevCommit commit = testRepo.commit().create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - updatePatchSetRef(ps); - db.patchSets().insert(singleton(ps)); - assertProblems(c, "Destination ref not found (may be new branch): " + ref); + assertProblems( + ctl, null, + problem("Destination ref not found (may be new branch): " + ref)); } @Test public void mergedChangeIsNotMerged() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - PatchSet ps = insertPatchSet(c); - String rev = ps.getRevision().get(); + ChangeControl ctl = insertChange(); - assertProblems(c, - "Patch set 1 (" + rev + ") is not merged into destination ref" - + " refs/heads/master (" + tip.name() - + "), but change status is MERGED"); + try (BatchUpdate bu = newUpdate(adminId)) { + bu.addOp(ctl.getId(), new BatchUpdate.Op() { + @Override + public boolean updateChange(ChangeContext ctx) throws OrmException { + ctx.getChange().setStatus(Change.Status.MERGED); + ctx.getUpdate(ctx.getChange().currentPatchSetId()) + .fixStatus(Change.Status.MERGED); + return true; + } + }); + bu.execute(); + } + ctl = reload(ctl); + + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + ObjectId tip = getDestRef(ctl); + assertProblems( + ctl, null, + problem( + "Patch set 1 (" + rev + ") is not merged into destination ref" + + " refs/heads/master (" + tip.name() + + "), but change status is MERGED")); } @Test public void newChangeIsMerged() throws Exception { - Change c = insertChange(); - RevCommit commit = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - db.patchSets().insert(singleton(ps)); - testRepo.branch(c.getDest().get()).update(commit); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + testRepo.branch(ctl.getChange().getDest().get()) + .update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev))); - assertProblems(c, - "Patch set 1 (" + commit.name() + ") is merged into destination ref" - + " refs/heads/master (" + commit.name() - + "), but change status is NEW"); + assertProblems( + ctl, null, + problem( + "Patch set 1 (" + rev + ") is merged into destination ref" + + " refs/heads/master (" + rev + + "), but change status is NEW")); } @Test public void newChangeIsMergedWithFix() throws Exception { - Change c = insertChange(); - RevCommit commit = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - db.patchSets().insert(singleton(ps)); - testRepo.branch(c.getDest().get()).update(commit); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + testRepo.branch(ctl.getChange().getDest().get()) + .update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev))); - List problems = checker.check(c, new FixInput()).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "Patch set 1 (" + commit.name() + ") is merged into destination ref" - + " refs/heads/master (" + commit.name() - + "), but change status is NEW"); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Marked change as merged"); + assertProblems( + ctl, new FixInput(), + problem( + "Patch set 1 (" + rev + ") is merged into destination ref" + + " refs/heads/master (" + rev + + "), but change status is NEW", + FIXED, "Marked change as merged")); - c = notesFactory.createChecked(db, c).getChange(); - assertThat(c.getStatus()).isEqualTo(Change.Status.MERGED); - assertProblems(c); + ctl = reload(ctl); + assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED); + assertNoProblems(ctl, null); } @Test public void extensionApiReturnsUpdatedValueAfterFix() throws Exception { - Change c = insertChange(); - RevCommit commit = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - db.patchSets().insert(singleton(ps)); - testRepo.branch(c.getDest().get()).update(commit); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + testRepo.branch(ctl.getChange().getDest().get()) + .update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev))); ChangeInfo info = gApi.changes() - .id(c.getChangeId()) + .id(ctl.getId().get()) .info(); assertThat(info.status).isEqualTo(ChangeStatus.NEW); info = gApi.changes() - .id(c.getChangeId()) + .id(ctl.getId().get()) .check(new FixInput()); assertThat(info.status).isEqualTo(ChangeStatus.MERGED); } @Test public void expectedMergedCommitIsLatestPatchSet() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - PatchSet ps = insertPatchSet(c); - RevCommit commit = parseCommit(ps); - testRepo.update(c.getDest().get(), commit); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + testRepo.branch(ctl.getChange().getDest().get()) + .update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev))); FixInput fix = new FixInput(); - fix.expectMergedAs = commit.name(); - assertThat(checker.check(c, fix).problems()).isEmpty(); + fix.expectMergedAs = rev; + assertProblems( + ctl, fix, + problem( + "Patch set 1 (" + rev + ") is merged into destination ref" + + " refs/heads/master (" + rev + "), but change status is NEW", + FIXED, "Marked change as merged")); + + ctl = reload(ctl); + assertThat(ctl.getChange().getStatus()).isEqualTo(Change.Status.MERGED); + assertNoProblems(ctl, null); } @Test public void expectedMergedCommitNotMergedIntoDestination() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - PatchSet ps = insertPatchSet(c); - RevCommit commit = parseCommit(ps); - testRepo.update(c.getDest().get(), commit); + ChangeControl ctl = insertChange(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + RevCommit commit = + testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)); + testRepo.branch(ctl.getChange().getDest().get()).update(commit); FixInput fix = new FixInput(); RevCommit other = testRepo.commit().message(commit.getFullMessage()).create(); fix.expectMergedAs = other.name(); - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "Expected merged commit " + other.name() - + " is not merged into destination ref refs/heads/master" - + " (" + commit.name() + ")"); + assertProblems( + ctl, fix, + problem( + "Expected merged commit " + other.name() + + " is not merged into destination ref refs/heads/master" + + " (" + commit.name() + ")")); } @Test public void createNewPatchSetForExpectedMergeCommitWithNoChangeId() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - RevCommit parent = - testRepo.branch(c.getDest().get()).commit().message("parent").create(); - PatchSet ps = insertPatchSet(c); - RevCommit commit = parseCommit(ps); + ChangeControl ctl = insertChange(); + String dest = ctl.getChange().getDest().get(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + RevCommit commit = + testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)); - RevCommit mergedAs = testRepo.commit().parent(parent) + RevCommit mergedAs = testRepo.commit().parent(commit.getParent(0)) .message(commit.getShortMessage()).create(); testRepo.getRevWalk().parseBody(mergedAs); assertThat(mergedAs.getFooterLines(FooterConstants.CHANGE_ID)).isEmpty(); - testRepo.update(c.getDest().get(), mergedAs); + testRepo.update(dest, mergedAs); - assertProblems(c, "Patch set 1 (" + commit.name() + ") is not merged into" - + " destination ref refs/heads/master (" + mergedAs.name() - + "), but change status is MERGED"); + assertNoProblems(ctl, null); FixInput fix = new FixInput(); fix.expectMergedAs = mergedAs.name(); - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "No patch set found for merged commit " + mergedAs.name()); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Inserted as patch set 2"); + assertProblems( + ctl, fix, + problem( + "No patch set found for merged commit " + mergedAs.name(), + FIXED, "Inserted as patch set 2"), + problem( + "Expected merged commit " + mergedAs.name() + + " has no associated patch set", + FIXED, "Marked change as merged")); - c = notesFactory.createChecked(db, c).getChange(); - PatchSet.Id psId2 = new PatchSet.Id(c.getId(), 2); - assertThat(c.currentPatchSetId()).isEqualTo(psId2); - assertThat(getPatchSet(psId2).getRevision().get()) + ctl = reload(ctl); + PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2); + assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId2); + assertThat(psUtil.get(db, ctl.getNotes(), psId2).getRevision().get()) .isEqualTo(mergedAs.name()); - assertProblems(c); + assertNoProblems(ctl, null); } @Test public void createNewPatchSetForExpectedMergeCommitWithChangeId() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - RevCommit parent = - testRepo.branch(c.getDest().get()).commit().message("parent").create(); - PatchSet ps = insertPatchSet(c); - RevCommit commit = parseCommit(ps); + ChangeControl ctl = insertChange(); + String dest = ctl.getChange().getDest().get(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + RevCommit commit = + testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)); - RevCommit mergedAs = testRepo.commit().parent(parent) + RevCommit mergedAs = testRepo.commit().parent(commit.getParent(0)) .message(commit.getShortMessage() + "\n" + "\n" - + "Change-Id: " + c.getKey().get() + "\n").create(); + + "Change-Id: " + ctl.getChange().getKey().get() + "\n").create(); testRepo.getRevWalk().parseBody(mergedAs); assertThat(mergedAs.getFooterLines(FooterConstants.CHANGE_ID)) - .containsExactly(c.getKey().get()); - testRepo.update(c.getDest().get(), mergedAs); + .containsExactly(ctl.getChange().getKey().get()); + testRepo.update(dest, mergedAs); - assertProblems(c, "Patch set 1 (" + commit.name() + ") is not merged into" - + " destination ref refs/heads/master (" + mergedAs.name() - + "), but change status is MERGED"); + assertNoProblems(ctl, null); FixInput fix = new FixInput(); fix.expectMergedAs = mergedAs.name(); - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "No patch set found for merged commit " + mergedAs.name()); - assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); - assertThat(p.outcome).isEqualTo("Inserted as patch set 2"); + assertProblems( + ctl, fix, + problem( + "No patch set found for merged commit " + mergedAs.name(), + FIXED, "Inserted as patch set 2"), + problem( + "Expected merged commit " + mergedAs.name() + + " has no associated patch set", + FIXED, "Marked change as merged")); - c = notesFactory.createChecked(db, c).getChange(); - PatchSet.Id psId2 = new PatchSet.Id(c.getId(), 2); - assertThat(c.currentPatchSetId()).isEqualTo(psId2); - assertThat(getPatchSet(psId2).getRevision().get()) + ctl = reload(ctl); + PatchSet.Id psId2 = new PatchSet.Id(ctl.getId(), 2); + assertThat(ctl.getChange().currentPatchSetId()).isEqualTo(psId2); + assertThat(psUtil.get(db, ctl.getNotes(), psId2).getRevision().get()) .isEqualTo(mergedAs.name()); - assertProblems(c); + assertNoProblems(ctl, null); } @Test public void expectedMergedCommitIsOldPatchSetOfSameChange() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); - PatchSet ps1 = insertPatchSet(c); + ChangeControl ctl = insertChange(); + PatchSet ps1 = psUtil.current(db, ctl.getNotes()); String rev1 = ps1.getRevision().get(); - incrementPatchSet(c); - PatchSet ps2 = insertPatchSet(c); - testRepo.branch(c.getDest().get()).update(parseCommit(ps1)); + ctl = incrementPatchSet(ctl); + PatchSet ps2 = psUtil.current(db, ctl.getNotes()); + testRepo.branch(ctl.getChange().getDest().get()) + .update(testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev1))); FixInput fix = new FixInput(); fix.expectMergedAs = rev1; - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "Expected merged commit " + rev1 + " corresponds to patch set " - + ps1.getId() + ", which is not the current patch set " + ps2.getId()); + assertProblems( + ctl, fix, + problem( + "Expected merged commit " + rev1 + " corresponds to patch set " + + ps1.getId() + ", which is not the current patch set " + + ps2.getId())); } @Test public void expectedMergedCommitWithMismatchedChangeId() throws Exception { - Change c = insertChange(); - c.setStatus(Change.Status.MERGED); + ChangeControl ctl = insertChange(); + String dest = ctl.getChange().getDest().get(); RevCommit parent = - testRepo.branch(c.getDest().get()).commit().message("parent").create(); - PatchSet ps = insertPatchSet(c); - RevCommit commit = parseCommit(ps); + testRepo.branch(dest).commit().message("parent").create(); + String rev = psUtil.current(db, ctl.getNotes()).getRevision().get(); + RevCommit commit = + testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)); + testRepo.branch(dest).update(commit); String badId = "I0000000000000000000000000000000000000000"; RevCommit mergedAs = testRepo.commit().parent(parent) @@ -567,85 +607,140 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { + "Change-Id: " + badId + "\n") .create(); testRepo.getRevWalk().parseBody(mergedAs); - testRepo.update(c.getDest().get(), mergedAs); + assertThat(mergedAs.getFooterLines(FooterConstants.CHANGE_ID)) + .containsExactly(badId); + testRepo.update(dest, mergedAs); + + assertNoProblems(ctl, null); FixInput fix = new FixInput(); fix.expectMergedAs = mergedAs.name(); - List problems = checker.check(c, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "Expected merged commit " + mergedAs.name() + " has Change-Id: " - + badId + ", but expected " + c.getKey().get()); + assertProblems( + ctl, fix, + problem( + "Expected merged commit " + mergedAs.name() + " has Change-Id: " + + badId + ", but expected " + ctl.getChange().getKey().get())); } @Test public void expectedMergedCommitMatchesMultiplePatchSets() throws Exception { - Change c1 = insertChange(); - c1.setStatus(Change.Status.MERGED); - insertPatchSet(c1); + ChangeControl ctl1 = insertChange(); + PatchSet.Id psId1 = psUtil.current(db, ctl1.getNotes()).getId(); + String dest = ctl1.getChange().getDest().get(); + String rev = psUtil.current(db, ctl1.getNotes()).getRevision().get(); + RevCommit commit = + testRepo.getRevWalk().parseCommit(ObjectId.fromString(rev)); + testRepo.branch(dest).update(commit); - RevCommit commit = testRepo.branch(c1.getDest().get()).commit().create(); - Change c2 = insertChange(); - PatchSet ps2 = newPatchSet(c2.currentPatchSetId(), commit, adminId); - updatePatchSetRef(ps2); - db.patchSets().insert(singleton(ps2)); + ChangeControl ctl2 = insertChange(); + ctl2 = incrementPatchSet(ctl2, commit); + PatchSet.Id psId2 = psUtil.current(db, ctl2.getNotes()).getId(); - Change c3 = insertChange(); - PatchSet ps3 = newPatchSet(c3.currentPatchSetId(), commit, adminId); - updatePatchSetRef(ps3); - db.patchSets().insert(singleton(ps3)); + ChangeControl ctl3 = insertChange(); + ctl3 = incrementPatchSet(ctl3, commit); + PatchSet.Id psId3 = psUtil.current(db, ctl3.getNotes()).getId(); FixInput fix = new FixInput(); fix.expectMergedAs = commit.name(); - List problems = checker.check(c1, fix).problems(); - assertThat(problems).hasSize(1); - ProblemInfo p = problems.get(0); - assertThat(p.message).isEqualTo( - "Multiple patch sets for expected merged commit " + commit.name() - + ": [" + ps2.getId() + ", " + ps3.getId() + "]"); + assertProblems( + ctl1, fix, + problem( + "Multiple patch sets for expected merged commit " + commit.name() + + ": [" + psId1 + ", " + psId2 + ", " + psId3 + "]")); } - private Change insertChange() throws Exception { - Change c = newChange(project, adminId); - db.changes().insert(singleton(c)); - indexer.index(db, c); - - ChangeUpdate u = changeUpdateFactory.create( - changeControlFactory.controlFor(db, c, userFactory.create(adminId))); - u.setBranch(c.getDest().get()); - u.setChangeId(c.getKey().get()); - u.commit(); - - return c; + private BatchUpdate newUpdate(Account.Id owner) { + return updateFactory.create( + db, project, userFactory.create(owner), TimeUtil.nowTs()); } - private void incrementPatchSet(Change c) throws Exception { - TestChanges.incrementPatchSet(c); - db.changes().upsert(singleton(c)); + private ChangeControl insertChange() throws Exception { + return insertChange(admin); } - private PatchSet insertPatchSet(Change c) throws Exception { - db.changes().upsert(singleton(c)); - RevCommit commit = testRepo.branch(c.currentPatchSetId().toRefName()).commit() - .parent(tip).message("Change " + c.getId().get()).create(); - PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, adminId); - updatePatchSetRef(ps); + + private ChangeControl insertChange(TestAccount owner) throws Exception { + return insertChange(owner, "refs/heads/master"); + } + + private ChangeControl insertChange(TestAccount owner, String dest) + throws Exception { + Change.Id id = new Change.Id(sequences.nextChangeId()); + ChangeInserter ins; + try (BatchUpdate bu = newUpdate(owner.getId())) { + RevCommit commit = patchSetCommit(new PatchSet.Id(id, 1)); + ins = changeInserterFactory + .create(id, commit, dest) + .setValidatePolicy(CommitValidators.Policy.NONE) + .setNotify(NotifyHandling.NONE) + .setFireRevisionCreated(false) + .setSendMail(false); + bu.insertChange(ins).execute(); + } + // Return control for admin regardless of owner. + return changeControlFactory.controlFor( + db, ins.getChange(), userFactory.create(adminId)); + } + + private PatchSet.Id nextPatchSetId(ChangeControl ctl) { + return ChangeUtil.nextPatchSetId(ctl.getChange().currentPatchSetId()); + } + + private ChangeControl incrementPatchSet(ChangeControl ctl) throws Exception { + return incrementPatchSet(ctl, patchSetCommit(nextPatchSetId(ctl))); + } + + private ChangeControl incrementPatchSet(ChangeControl ctl, + RevCommit commit) throws Exception { + PatchSetInserter ins; + try (BatchUpdate bu = newUpdate(ctl.getChange().getOwner())) { + ins = patchSetInserterFactory.create(ctl, nextPatchSetId(ctl), commit) + .setValidatePolicy(CommitValidators.Policy.NONE) + .setFireRevisionCreated(false) + .setSendMail(false); + bu.addOp(ctl.getId(), ins).execute(); + } + return reload(ctl); + } + + private ChangeControl reload(ChangeControl ctl) throws Exception { + return changeControlFactory.controlFor( + db, ctl.getChange().getProject(), ctl.getId(), ctl.getUser()); + } + + private RevCommit patchSetCommit(PatchSet.Id psId) throws Exception { + RevCommit c = testRepo + .commit() + .parent(tip) + .message("Change " + psId) + .create(); + return testRepo.parseBody(c); + } + + private PatchSet insertMissingPatchSet(ChangeControl ctl, String rev) + throws Exception { + // Don't use BatchUpdate since we're manually updating the meta ref rather + // than using ChangeUpdate. + String subject = "Subject for missing commit"; + Change c = new Change(ctl.getChange()); + PatchSet.Id psId = nextPatchSetId(ctl); + c.setCurrentPatchSet(psId, subject, c.getOriginalSubject()); + + PatchSet ps = newPatchSet(psId, rev, adminId); db.patchSets().insert(singleton(ps)); - return ps; - } + db.changes().update(singleton(c)); - private PatchSet insertMissingPatchSet(Change c, String id) throws Exception { - PatchSet ps = newPatchSet(c.currentPatchSetId(), - ObjectId.fromString(id), adminId); - db.patchSets().insert(singleton(ps)); - return ps; - } + addNoteDbCommit( + c.getId(), + "Update patch set " + psId.get() + "\n" + + "\n" + + "Patch-set: " + psId.get() + "\n" + + "Commit: " + rev + "\n" + + "Subject: " + subject + "\n"); + indexer.index(db, c.getProject(), c.getId()); - private void updatePatchSetRef(PatchSet ps) throws Exception { - testRepo.update(ps.getId().toRefName(), - ObjectId.fromString(ps.getRevision().get())); + return ps; } private void deleteRef(String refName) throws Exception { @@ -654,22 +749,82 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED); } - private RevCommit parseCommit(PatchSet ps) throws Exception { - RevCommit commit = testRepo.getRevWalk() - .parseCommit(ObjectId.fromString(ps.getRevision().get())); - testRepo.getRevWalk().parseBody(commit); - return commit; + private void addNoteDbCommit(Change.Id id, String commitMessage) + throws Exception { + if (!notesMigration.commitChangeWrites()) { + return; + } + PersonIdent committer = serverIdent.get(); + PersonIdent author = noteUtil.newIdent( + accountCache.get(admin.getId()).getAccount(), + committer.getWhen(), + committer, + anonymousCowardName); + testRepo.branch(RefNames.changeMetaRef(id)) + .commit() + .author(author) + .committer(committer) + .message(commitMessage) + .create(); } - private void assertProblems(Change c, String... expected) { - assertThat(Lists.transform(checker.check(c).problems(), - new Function() { - @Override - public String apply(ProblemInfo in) { - checkArgument(in.status == null, - "Status is not null: " + in.message); - return in.message; - } - })).containsExactly((Object[]) expected); + private ObjectId getDestRef(ChangeControl ctl) throws Exception { + return testRepo.getRepository() + .exactRef(ctl.getChange().getDest().get()) + .getObjectId(); + } + + private ChangeControl mergeChange(ChangeControl ctl) throws Exception { + final ObjectId oldId = getDestRef(ctl); + final ObjectId newId = ObjectId.fromString( + psUtil.current(db, ctl.getNotes()).getRevision().get()); + final String dest = ctl.getChange().getDest().get(); + + try (BatchUpdate bu = newUpdate(adminId)) { + bu.addOp(ctl.getId(), new BatchUpdate.Op() { + @Override + public void updateRepo(RepoContext ctx) throws IOException { + ctx.addRefUpdate(new ReceiveCommand(oldId, newId, dest)); + } + + @Override + public boolean updateChange(ChangeContext ctx) throws OrmException { + ctx.getChange().setStatus(Change.Status.MERGED); + ctx.getUpdate(ctx.getChange().currentPatchSetId()) + .fixStatus(Change.Status.MERGED); + return true; + } + }); + bu.execute(); + } + return reload(ctl); + } + + private static ProblemInfo problem(String message) { + ProblemInfo p = new ProblemInfo(); + p.message = message; + return p; + } + + private static ProblemInfo problem(String message, + ProblemInfo.Status status, String outcome) { + ProblemInfo p = problem(message); + p.status = checkNotNull(status); + p.outcome = checkNotNull(outcome); + return p; + } + + private void assertProblems(ChangeControl ctl, @Nullable FixInput fix, + ProblemInfo first, ProblemInfo... rest) { + List expected = new ArrayList<>(1 + rest.length); + expected.add(first); + expected.addAll(Arrays.asList(rest)); + assertThat(checker.check(ctl, fix).problems()) + .containsExactlyElementsIn(expected) + .inOrder(); + } + + private void assertNoProblems(ChangeControl ctl, @Nullable FixInput fix) { + assertThat(checker.check(ctl, fix).problems()).isEmpty(); } } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java index 4dd910d147..ff04fdcf80 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java @@ -14,6 +14,8 @@ package com.google.gerrit.extensions.common; +import java.util.Objects; + public class ProblemInfo { public enum Status { FIXED, FIX_FAILED @@ -23,6 +25,22 @@ public class ProblemInfo { public Status status; public String outcome; + @Override + public int hashCode() { + return Objects.hash(message, status, outcome); + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof ProblemInfo)) { + return false; + } + ProblemInfo p = (ProblemInfo) o; + return Objects.equals(message, p.message) + && Objects.equals(status, p.status) + && Objects.equals(outcome, p.outcome); + } + @Override public String toString() { StringBuilder sb = new StringBuilder(getClass().getSimpleName()) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index ce381867a4..158758b3b2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -355,7 +355,21 @@ public class ChangeJson { } private ChangeInfo checkOnly(ChangeData cd) { - ConsistencyChecker.Result result = checkerProvider.get().check(cd, fix); + ChangeControl ctl; + try { + ctl = cd.changeControl().forUser(userProvider.get()); + } catch (OrmException e) { + String msg = "Error loading change"; + log.warn(msg + " " + cd.getId(), e); + ChangeInfo info = new ChangeInfo(); + info._number = cd.getId().get(); + ProblemInfo p = new ProblemInfo(); + p.message = msg; + info.problems = Lists.newArrayList(p); + return info; + } + + ConsistencyChecker.Result result = checkerProvider.get().check(ctl, fix); ChangeInfo info; Change c = result.change(); if (c != null) { @@ -384,9 +398,11 @@ public class ChangeJson { Optional limitToPsId) throws PatchListNotAvailableException, GpgException, OrmException, IOException { ChangeInfo out = new ChangeInfo(); + CurrentUser user = userProvider.get(); + ChangeControl ctl = cd.changeControl().forUser(user); if (has(CHECK)) { - out.problems = checkerProvider.get().check(cd.change(), fix).problems(); + out.problems = checkerProvider.get().check(ctl, fix).problems(); // If any problems were fixed, the ChangeData needs to be reloaded. for (ProblemInfo p : out.problems) { if (p.status == ProblemInfo.Status.FIXED) { @@ -397,8 +413,6 @@ public class ChangeJson { } Change in = cd.change(); - CurrentUser user = userProvider.get(); - ChangeControl ctl = cd.changeControl().forUser(user); out.project = in.getProject().get(); out.branch = in.getDest().getShortName(); out.topic = in.getTopic(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java index 465ce95418..f4869bedb1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java @@ -18,12 +18,10 @@ import com.google.gerrit.extensions.api.changes.FixInput; import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.restapi.AuthException; -import com.google.gerrit.extensions.restapi.NotImplementedException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.RestReadView; -import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -32,27 +30,22 @@ import java.util.EnumSet; public class Check implements RestReadView, RestModifyView { - private final NotesMigration notesMigration; private final ChangeJson.Factory jsonFactory; @Inject - Check(NotesMigration notesMigration, - ChangeJson.Factory json) { - this.notesMigration = notesMigration; + Check(ChangeJson.Factory json) { this.jsonFactory = json; } @Override public Response apply(ChangeResource rsrc) throws RestApiException, OrmException { - checkEnabled(); return Response.withMustRevalidate(newChangeJson().format(rsrc)); } @Override public Response apply(ChangeResource rsrc, FixInput input) throws RestApiException, OrmException { - checkEnabled(); ChangeControl ctl = rsrc.getControl(); if (!ctl.isOwner() && !ctl.getProjectControl().isOwner() @@ -65,10 +58,4 @@ public class Check implements RestReadView, private ChangeJson newChangeJson() { return jsonFactory.create(EnumSet.of(ListChangesOption.CHECK)); } - - private void checkEnabled() throws NotImplementedException { - if (notesMigration.readChanges()) { - throw new NotImplementedException("check not implemented for NoteDb"); - } - } } 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 c0a92ba11a..b16d8b8555 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 @@ -14,7 +14,8 @@ package com.google.gerrit.server.change; -import static com.google.common.base.Preconditions.checkState; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES; import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering; import static com.google.gerrit.server.ChangeUtil.PS_ID_ORDER; @@ -39,23 +40,22 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.git.BatchUpdate; +import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.git.validators.CommitValidators; -import com.google.gerrit.server.index.change.ChangeIndexer; import com.google.gerrit.server.notedb.ChangeNotes; -import com.google.gerrit.server.notedb.ChangeUpdate; -import com.google.gerrit.server.notedb.NotesMigration; +import com.google.gerrit.server.notedb.PatchSetState; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; -import com.google.gerrit.server.query.change.ChangeData; -import com.google.gwtorm.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -78,9 +78,10 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Objects; +import java.util.Set; /** * Checks changes for various kinds of inconsistency and corruption. @@ -94,12 +95,10 @@ public class ConsistencyChecker { @AutoValue public abstract static class Result { - private static Result create(Change.Id id, List problems) { - return new AutoValue_ConsistencyChecker_Result(id, null, problems); - } - - private static Result create(Change c, List problems) { - return new AutoValue_ConsistencyChecker_Result(c.getId(), c, problems); + private static Result create(ChangeControl ctl, + List problems) { + return new AutoValue_ConsistencyChecker_Result( + ctl.getId(), ctl.getChange(), problems); } public abstract Change.Id id(); @@ -110,22 +109,20 @@ public class ConsistencyChecker { public abstract List problems(); } - private final Provider db; - private final GitRepositoryManager repoManager; - private final NotesMigration notesMigration; - private final Provider user; - private final Provider serverIdent; - private final PatchSetInfoFactory patchSetInfoFactory; - private final PatchSetInserter.Factory patchSetInserterFactory; private final BatchUpdate.Factory updateFactory; - private final ChangeIndexer indexer; private final ChangeControl.GenericFactory changeControlFactory; private final ChangeNotes.Factory notesFactory; - private final ChangeUpdate.Factory changeUpdateFactory; private final DynamicItem accountPatchReviewStore; + private final GitRepositoryManager repoManager; + private final PatchSetInfoFactory patchSetInfoFactory; + private final PatchSetInserter.Factory patchSetInserterFactory; + private final PatchSetUtil psUtil; + private final Provider user; + private final Provider serverIdent; + private final Provider db; private FixInput fix; - private Change change; + private ChangeControl ctl; private Repository repo; private RevWalk rw; @@ -137,67 +134,51 @@ public class ConsistencyChecker { private List problems; @Inject - ConsistencyChecker(Provider db, - GitRepositoryManager repoManager, - NotesMigration notesMigration, - Provider user, + ConsistencyChecker( @GerritPersonIdent Provider serverIdent, - PatchSetInfoFactory patchSetInfoFactory, - PatchSetInserter.Factory patchSetInserterFactory, BatchUpdate.Factory updateFactory, - ChangeIndexer indexer, ChangeControl.GenericFactory changeControlFactory, ChangeNotes.Factory notesFactory, - ChangeUpdate.Factory changeUpdateFactory, - DynamicItem accountPatchReviewStore) { + DynamicItem accountPatchReviewStore, + GitRepositoryManager repoManager, + PatchSetInfoFactory patchSetInfoFactory, + PatchSetInserter.Factory patchSetInserterFactory, + PatchSetUtil psUtil, + Provider user, + Provider db) { + this.accountPatchReviewStore = accountPatchReviewStore; + this.changeControlFactory = changeControlFactory; this.db = db; - this.notesMigration = notesMigration; - this.repoManager = repoManager; - this.user = user; - this.serverIdent = serverIdent; + this.notesFactory = notesFactory; this.patchSetInfoFactory = patchSetInfoFactory; this.patchSetInserterFactory = patchSetInserterFactory; + this.psUtil = psUtil; + this.repoManager = repoManager; + this.serverIdent = serverIdent; this.updateFactory = updateFactory; - this.indexer = indexer; - this.changeControlFactory = changeControlFactory; - this.notesFactory = notesFactory; - this.changeUpdateFactory = changeUpdateFactory; - this.accountPatchReviewStore = accountPatchReviewStore; + this.user = user; reset(); } private void reset() { - change = null; + ctl = null; repo = null; rw = null; problems = new ArrayList<>(); } - public Result check(ChangeData cd) { - return check(cd, null); + private Change change() { + return ctl.getChange(); } - public Result check(ChangeData cd, @Nullable FixInput f) { - reset(); - try { - return check(cd.change(), f); - } catch (OrmException e) { - error("Error looking up change", e); - return Result.create(cd.getId(), problems); - } - } - - public Result check(Change c) { - return check(c, null); - } - - public Result check(Change c, @Nullable FixInput f) { - reset(); - fix = f; - change = c; + public Result check(ChangeControl cc, @Nullable FixInput f) { + checkNotNull(cc); try { + reset(); + ctl = cc; + fix = f; checkImpl(); - return Result.create(c, problems); + return result(); } finally { if (rw != null) { rw.close(); @@ -209,8 +190,6 @@ public class ConsistencyChecker { } private void checkImpl() { - checkState(!notesMigration.readChanges(), - "ConsistencyChecker for NoteDb not yet implemented"); checkOwner(); checkCurrentPatchSetEntity(); @@ -226,8 +205,8 @@ public class ConsistencyChecker { private void checkOwner() { try { - if (db.get().accounts().get(change.getOwner()) == null) { - problem("Missing change owner: " + change.getOwner()); + if (db.get().accounts().get(change().getOwner()) == null) { + problem("Missing change owner: " + change().getOwner()); } } catch (OrmException e) { error("Failed to look up owner", e); @@ -236,10 +215,10 @@ public class ConsistencyChecker { private void checkCurrentPatchSetEntity() { try { - PatchSet.Id psId = change.currentPatchSetId(); - currPs = db.get().patchSets().get(psId); + currPs = psUtil.current(db.get(), ctl.getNotes()); if (currPs == null) { - problem(String.format("Current patch set %d not found", psId.get())); + problem(String.format("Current patch set %d not found", + change().currentPatchSetId().get())); } } catch (OrmException e) { error("Failed to look up current patch set", e); @@ -247,7 +226,7 @@ public class ConsistencyChecker { } private boolean openRepo() { - Project.NameKey project = change.getDest().getParentKey(); + Project.NameKey project = change().getDest().getParentKey(); try { repo = repoManager.openRepository(project); rw = new RevWalk(repo); @@ -262,13 +241,11 @@ public class ConsistencyChecker { private boolean checkPatchSets() { List all; try { - all = Lists.newArrayList(db.get().patchSets().byChange(change.getId())); + // Iterate in descending order. + all = PS_ID_ORDER.sortedCopy(psUtil.byChange(db.get(), ctl.getNotes())); } catch (OrmException e) { return error("Failed to look up patch sets", e); } - // Iterate in descending order so deletePatchSet can assume the latest patch - // set exists. - Collections.sort(all, PS_ID_ORDER.reverse()); patchSetsBySha = MultimapBuilder.hashKeys(all.size()) .treeSetValues(PS_ID_ORDER) .build(); @@ -287,6 +264,7 @@ public class ConsistencyChecker { refs = Collections.emptyMap(); } + List deletePatchSetOps = new ArrayList<>(); for (PatchSet ps : all) { // Check revision format. int psNum = ps.getId().get(); @@ -317,17 +295,21 @@ public class ConsistencyChecker { objId, String.format("patch set %d", psNum)); if (psCommit == null) { if (fix != null && fix.deletePatchSetIfCommitMissing) { - deletePatchSet(lastProblem(), change.getProject(), ps.getId()); + deletePatchSetOps.add( + new DeletePatchSetFromDbOp(lastProblem(), ps.getId())); } continue; } else if (refProblem != null && fix != null) { fixPatchSetRef(refProblem, ps); } - if (ps.getId().equals(change.currentPatchSetId())) { + if (ps.getId().equals(change().currentPatchSetId())) { currPsCommit = psCommit; } } + // Delete any bad patch sets found above, in a single update. + deletePatchSets(deletePatchSetOps); + // Check for duplicates. for (Map.Entry> e : patchSetsBySha.asMap().entrySet()) { @@ -342,7 +324,7 @@ public class ConsistencyChecker { } private void checkMerged() { - String refName = change.getDest().get(); + String refName = change().getDest().get(); Ref dest; try { dest = repo.getRefDatabase().exactRef(refName); @@ -375,22 +357,27 @@ public class ConsistencyChecker { } } + private ProblemInfo wrongChangeStatus(PatchSet.Id psId, RevCommit commit) { + String refName = change().getDest().get(); + return problem(String.format( + "Patch set %d (%s) is merged into destination ref %s (%s), but change" + + " status is %s", psId.get(), commit.name(), + refName, tip.name(), change().getStatus())); + } + private void checkMergedBitMatchesStatus(PatchSet.Id psId, RevCommit commit, boolean merged) { - String refName = change.getDest().get(); - if (merged && change.getStatus() != Change.Status.MERGED) { - ProblemInfo p = problem(String.format( - "Patch set %d (%s) is merged into destination ref %s (%s), but change" - + " status is %s", psId.get(), commit.name(), - refName, tip.name(), change.getStatus())); + String refName = change().getDest().get(); + if (merged && change().getStatus() != Change.Status.MERGED) { + ProblemInfo p = wrongChangeStatus(psId, commit); if (fix != null) { fixMerged(p); } - } else if (!merged && change.getStatus() == Change.Status.MERGED) { + } else if (!merged && change().getStatus() == Change.Status.MERGED) { problem(String.format("Patch set %d (%s) is not merged into" + " destination ref %s (%s), but change status is %s", currPs.getId().get(), commit.name(), refName, tip.name(), - change.getStatus())); + change().getStatus())); } } @@ -401,16 +388,12 @@ public class ConsistencyChecker { if (commit == null) { return; } - if (Objects.equals(commit, currPsCommit)) { - // Caller gave us latest patch set SHA-1; verified in checkPatchSets. - return; - } try { if (!rw.isMergedInto(commit, tip)) { problem(String.format("Expected merged commit %s is not merged into" + " destination ref %s (%s)", - commit.name(), change.getDest().get(), tip.name())); + commit.name(), change().getDest().get(), tip.name())); return; } @@ -425,8 +408,8 @@ public class ConsistencyChecker { } try { Change c = notesFactory.createChecked( - db.get(), change.getProject(), psId.getParentKey()).getChange(); - if (!c.getDest().equals(change.getDest())) { + db.get(), change().getProject(), psId.getParentKey()).getChange(); + if (!c.getDest().equals(change().getDest())) { continue; } } catch (OrmException | NoSuchChangeException e) { @@ -442,16 +425,17 @@ public class ConsistencyChecker { String changeId = Iterables.getFirst( commit.getFooterLines(FooterConstants.CHANGE_ID), null); // Missing Change-Id footer is ok, but mismatched is not. - if (changeId != null && !changeId.equals(change.getKey().get())) { + if (changeId != null && !changeId.equals(change().getKey().get())) { problem(String.format("Expected merged commit %s has Change-Id: %s," + " but expected %s", - commit.name(), changeId, change.getKey().get())); + commit.name(), changeId, change().getKey().get())); return; } - PatchSet.Id psId = insertPatchSet(commit); - if (psId != null) { - checkMergedBitMatchesStatus(psId, commit, true); - } + // TODO(dborowitz): Combine into one op. + insertPatchSet(commit); + fixMerged(problem(String.format( + "Expected merged commit %s has no associated patch set", + commit.name()))); break; case 1: @@ -460,10 +444,12 @@ public class ConsistencyChecker { // TODO(dborowitz): This could be fixed if it's an older patch set of // the current change. PatchSet.Id id = psIds.get(0); - if (!id.equals(change.currentPatchSetId())) { + if (id.equals(change().currentPatchSetId())) { + fixMerged(wrongChangeStatus(id, commit)); + } else { problem(String.format("Expected merged commit %s corresponds to" + " patch set %s, which is not the current patch set %s", - commit.name(), id, change.currentPatchSetId())); + commit.name(), id, change().currentPatchSetId())); } break; @@ -490,17 +476,14 @@ public class ConsistencyChecker { } try { - ChangeControl ctl = changeControlFactory - .controlFor(db.get(), change, user.get()); PatchSet.Id psId = - ChangeUtil.nextPatchSetId(repo, change.currentPatchSetId()); + ChangeUtil.nextPatchSetId(repo, change().currentPatchSetId()); PatchSetInserter inserter = patchSetInserterFactory.create(ctl, psId, commit); - try (BatchUpdate bu = updateFactory.create( - db.get(), change.getProject(), ctl.getUser(), TimeUtil.nowTs()); + try (BatchUpdate bu = newBatchUpdate(); ObjectInserter oi = repo.newObjectInserter()) { bu.setRepository(repo, rw, oi); - bu.addOp(change.getId(), inserter + bu.addOp(ctl.getId(), inserter .setValidatePolicy(CommitValidators.Policy.NONE) .setFireRevisionCreated(false) .setSendMail(false) @@ -509,7 +492,8 @@ public class ConsistencyChecker { "Patch set for merged commit inserted by consistency checker")); bu.execute(); } - change = inserter.getChange(); + ctl = changeControlFactory.controlFor( + db.get(), inserter.getChange(), ctl.getUser()); p.status = Status.FIXED; p.outcome = "Inserted as patch set " + psId.get(); return psId; @@ -523,30 +507,33 @@ public class ConsistencyChecker { } private void fixMerged(ProblemInfo p) { - try { - change = db.get().changes().atomicUpdate(change.getId(), - new AtomicUpdate() { - @Override - public Change update(Change c) { - c.setStatus(Change.Status.MERGED); - return c; - } - }); - ChangeUpdate changeUpdate = - changeUpdateFactory.create( - changeControlFactory.controlFor(db.get(), change, user.get())); - changeUpdate.fixStatus(Change.Status.MERGED); - changeUpdate.commit(); - indexer.index(db.get(), change); + try (BatchUpdate bu = newBatchUpdate(); + ObjectInserter oi = repo.newObjectInserter()) { + bu.setRepository(repo, rw, oi); + bu.addOp(ctl.getId(), new BatchUpdate.Op() { + @Override + public boolean updateChange(ChangeContext ctx) throws OrmException { + ctx.getChange().setStatus(Change.Status.MERGED); + ctx.getUpdate(ctx.getChange().currentPatchSetId()) + .fixStatus(Change.Status.MERGED); + return true; + } + }); + bu.execute(); p.status = Status.FIXED; p.outcome = "Marked change as merged"; - } catch (OrmException | IOException | NoSuchChangeException e) { - log.warn("Error marking " + change.getId() + "as merged", e); + } catch (UpdateException | RestApiException e) { + log.warn("Error marking " + ctl.getId() + "as merged", e); p.status = Status.FIX_FAILED; p.outcome = "Error updating status to merged"; } } + private BatchUpdate newBatchUpdate() { + return updateFactory.create( + db.get(), change().getProject(), ctl.getUser(), TimeUtil.nowTs()); + } + private void fixPatchSetRef(ProblemInfo p, PatchSet ps) { try { RefUpdate ru = repo.updateRef(ps.getId().toRefName()); @@ -582,59 +569,108 @@ public class ConsistencyChecker { } } - private void deletePatchSet(ProblemInfo p, Project.NameKey project, - PatchSet.Id psId) { - ReviewDb db = this.db.get(); - Change.Id cid = psId.getParentKey(); - try { - db.changes().beginTransaction(cid); - try { - ChangeNotes notes = notesFactory.createChecked(db, project, cid); - Change c = notes.getChange(); - if (psId.equals(c.currentPatchSetId())) { - List all = Lists.newArrayList(db.patchSets().byChange(cid)); - if (all.size() == 1 && all.get(0).getId().equals(psId)) { - p.status = Status.FIX_FAILED; - p.outcome = "Cannot delete patch set; no patch sets would remain"; - return; - } - // If there were multiple missing patch sets, assumes deletePatchSet - // has been called in decreasing order, so the max remaining PatchSet - // is the effective current patch set. - Collections.sort(all, PS_ID_ORDER.reverse()); - PatchSet.Id latest = null; - for (PatchSet ps : all) { - latest = ps.getId(); - if (!ps.getId().equals(psId)) { - break; - } - } - c.setCurrentPatchSet(patchSetInfoFactory.get(db, notes, latest)); - db.changes().update(Collections.singleton(c)); - } - - // Delete dangling primary key references. Don't delete ChangeMessages, - // which don't use patch sets as a primary key, and may provide useful - // historical information. - accountPatchReviewStore.get().clearReviewed(psId); - db.patchSetApprovals().delete( - db.patchSetApprovals().byPatchSet(psId)); - db.patchComments().delete( - db.patchComments().byPatchSet(psId)); - db.patchSets().deleteKeys(Collections.singleton(psId)); - db.commit(); - - p.status = Status.FIXED; - p.outcome = "Deleted patch set"; - } finally { - db.rollback(); + private void deletePatchSets(List ops) { + try (BatchUpdate bu = newBatchUpdate(); + ObjectInserter oi = repo.newObjectInserter()) { + bu.setRepository(repo, rw, oi); + for (DeletePatchSetFromDbOp op : ops) { + checkArgument(op.psId.getParentKey().equals(ctl.getId())); + bu.addOp(ctl.getId(), op); } - } catch (PatchSetInfoNotAvailableException | OrmException - | NoSuchChangeException e) { + bu.addOp(ctl.getId(), new UpdateCurrentPatchSetOp(ops)); + bu.execute(); + } catch (NoPatchSetsWouldRemainException e) { + for (DeletePatchSetFromDbOp op : ops) { + op.p.status = Status.FIX_FAILED; + op.p.outcome = e.getMessage(); + } + } catch (UpdateException | RestApiException e) { String msg = "Error deleting patch set"; - log.warn(msg + ' ' + psId, e); - p.status = Status.FIX_FAILED; - p.outcome = msg; + log.warn(msg + " of change " + ops.get(0).psId.getParentKey(), e); + for (DeletePatchSetFromDbOp op : ops) { + // Overwrite existing statuses that were set before the transaction was + // rolled back. + op.p.status = Status.FIX_FAILED; + op.p.outcome = msg; + } + } + } + + private class DeletePatchSetFromDbOp extends BatchUpdate.Op { + private final ProblemInfo p; + private final PatchSet.Id psId; + + private DeletePatchSetFromDbOp(ProblemInfo p, PatchSet.Id psId) { + this.p = p; + this.psId = psId; + } + + @Override + public boolean updateChange(ChangeContext ctx) + throws OrmException, PatchSetInfoNotAvailableException { + // Delete dangling key references. + ReviewDb db = DeleteDraftChangeOp.unwrap(ctx.getDb()); + accountPatchReviewStore.get().clearReviewed(psId); + db.changeMessages().delete( + db.changeMessages().byChange(psId.getParentKey())); + db.patchSetApprovals().delete( + db.patchSetApprovals().byPatchSet(psId)); + db.patchComments().delete( + db.patchComments().byPatchSet(psId)); + db.patchSets().deleteKeys(Collections.singleton(psId)); + + // NoteDb requires no additional fiddling; setting the state to deleted is + // sufficient to filter everything else out. + ctx.getUpdate(psId).setPatchSetState(PatchSetState.DELETED); + + p.status = Status.FIXED; + p.outcome = "Deleted patch set"; + return true; + } + } + + private static class NoPatchSetsWouldRemainException + extends RestApiException { + private static final long serialVersionUID = 1L; + + private NoPatchSetsWouldRemainException() { + super("Cannot delete patch set; no patch sets would remain"); + } + } + + private class UpdateCurrentPatchSetOp extends BatchUpdate.Op { + private final Set toDelete; + + private UpdateCurrentPatchSetOp(List deleteOps) { + toDelete = new HashSet<>(); + for (DeletePatchSetFromDbOp op : deleteOps) { + toDelete.add(op.psId); + } + } + + @Override + public boolean updateChange(ChangeContext ctx) + throws OrmException, PatchSetInfoNotAvailableException, + NoPatchSetsWouldRemainException { + if (!toDelete.contains(ctx.getChange().currentPatchSetId())) { + return false; + } + Set all = new HashSet<>(); + // Doesn't make any assumptions about the order in which deletes happen + // and whether they are seen by this op; we are already given the full set + // of patch sets that will eventually be deleted in this update. + for (PatchSet ps : psUtil.byChange(ctx.getDb(), ctx.getNotes())) { + if (!toDelete.contains(ps.getId())) { + all.add(ps.getId()); + } + } + if (all.isEmpty()) { + throw new NoPatchSetsWouldRemainException(); + } + PatchSet.Id latest = ReviewDbUtil.intKeyOrdering().max(all); + ctx.getChange().setCurrentPatchSet( + patchSetInfoFactory.get(ctx.getDb(), ctx.getNotes(), latest)); + return true; } } @@ -687,6 +723,10 @@ public class ConsistencyChecker { } private void warn(Throwable t) { - log.warn("Error in consistency check of change " + change.getId(), t); + log.warn("Error in consistency check of change " + ctl.getId(), t); + } + + private Result result() { + return Result.create(ctl, problems); } }