Fix NPE when deleting current patch set and previous patch set doesn't exist
When the current patch set was deleted we used patch set ID - 1 as ID
for the previous patch set but this patch set may not exist (e.g. it was
a draft that was deleted).
Change-Id: Ief0776d131d805ed403fb45bb6112d953dc5564d
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java
index 244efbf..11ff612 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteDraftPatchSetIT.java
@@ -188,6 +188,26 @@
}
@Test
+ public void deleteCurrentDraftPatchSetWhenPreviousPatchSetDoesNotExist() throws Exception {
+ PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
+ String changeId = push.to("refs/for/master").getChangeId();
+ pushFactory
+ .create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT, "b.txt", "foo", changeId)
+ .to("refs/drafts/master");
+ pushFactory
+ .create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT, "b.txt", "bar", changeId)
+ .to("refs/drafts/master");
+
+ deletePatchSet(changeId, 2);
+ deletePatchSet(changeId, 3);
+
+ ChangeData cd = getChange(changeId);
+ assertThat(cd.patchSets()).hasSize(1);
+ assertThat(Iterables.getOnlyElement(cd.patchSets()).getId().get()).isEqualTo(1);
+ assertThat(cd.currentPatchSet().getId().get()).isEqualTo(1);
+ }
+
+ @Test
public void deleteDraftPatchSetAndPushNewDraftPatchSet() throws Exception {
String ref = "refs/drafts/master";
@@ -263,6 +283,10 @@
}
private void deletePatchSet(String changeId, PatchSet ps) throws Exception {
- gApi.changes().id(changeId).revision(ps.getId().get()).delete();
+ deletePatchSet(changeId, ps.getId().get());
+ }
+
+ private void deletePatchSet(String changeId, int ps) throws Exception {
+ gApi.changes().id(changeId).revision(ps).delete();
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
index 222230b..d452489 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.change;
+import static com.google.common.base.Preconditions.checkNotNull;
+
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -45,6 +47,7 @@
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Collection;
+import java.util.Map;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.transport.ReceiveCommand;
@@ -106,7 +109,8 @@
@Override
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException, IOException, NoSuchChangeException {
- patchSet = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
+ Map<PatchSet.Id, PatchSet> patchSets = psUtil.byChangeAsMap(db.get(), ctx.getNotes());
+ patchSet = patchSets.get(psId);
if (patchSet == null) {
return false; // Nothing to do.
}
@@ -120,9 +124,9 @@
throw new AuthException("Not permitted to delete this draft patch set");
}
- patchSetsBeforeDeletion = psUtil.byChange(ctx.getDb(), ctx.getNotes());
+ patchSetsBeforeDeletion = patchSets.values();
deleteDraftPatchSet(patchSet, ctx);
- deleteOrUpdateDraftChange(ctx);
+ deleteOrUpdateDraftChange(ctx, patchSets);
return true;
}
@@ -152,7 +156,7 @@
db.patchSetApprovals().delete(db.patchSetApprovals().byPatchSet(psId));
}
- private void deleteOrUpdateDraftChange(ChangeContext ctx)
+ private void deleteOrUpdateDraftChange(ChangeContext ctx, Map<PatchSet.Id, PatchSet> patchSets)
throws OrmException, RestApiException, IOException, NoSuchChangeException {
Change c = ctx.getChange();
if (deletedOnlyPatchSet()) {
@@ -161,7 +165,7 @@
return;
}
if (c.currentPatchSetId().equals(psId)) {
- c.setCurrentPatchSet(previousPatchSetInfo(ctx));
+ c.setCurrentPatchSet(previousPatchSetInfo(ctx, patchSets));
}
}
@@ -170,12 +174,22 @@
&& patchSetsBeforeDeletion.iterator().next().getId().equals(psId);
}
- private PatchSetInfo previousPatchSetInfo(ChangeContext ctx) throws OrmException {
+ private PatchSetInfo previousPatchSetInfo(
+ ChangeContext ctx, Map<PatchSet.Id, PatchSet> patchSets) throws OrmException {
+ PatchSet.Id prevPsId = null;
+ for (PatchSet.Id id : patchSets.keySet()) {
+ if (id.get() < psId.get() && (prevPsId == null || id.get() > prevPsId.get())) {
+ prevPsId = id;
+ }
+ }
+
try {
// TODO(dborowitz): Get this in a way that doesn't involve re-opening
// the repo after the updateRepo phase.
return patchSetInfoFactory.get(
- ctx.getDb(), ctx.getNotes(), new PatchSet.Id(psId.getParentKey(), psId.get() - 1));
+ ctx.getDb(),
+ ctx.getNotes(),
+ new PatchSet.Id(psId.getParentKey(), checkNotNull(prevPsId).get()));
} catch (PatchSetInfoNotAvailableException e) {
throw new OrmException(e);
}
