Move ChangeUtil#deleteDraftPatchSet body to its Op implementation

Delegate to another Op if the whole change needs to be deleted.

This ends up removing a bunch of boilerplate in ChangeUtil that was
duplicating work from BatchUpdate. Now ChangeUtil is ever so slightly
less of a kitchen sink.

Change-Id: Ica09906925f33857de3fd08d71772571d7238199

gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java

@@ -31,7 +31,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.ChangeMessages;
 import com.google.gerrit.server.change.ChangeTriplet;
-import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.UpdateException;
@@ -53,19 +52,15 @@ import com.google.inject.Singleton;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
-import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.CommitBuilder;
-import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefDatabase;
-import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
-import org.eclipse.jgit.transport.ReceiveCommand;
 import org.eclipse.jgit.util.ChangeIdUtil;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -184,11 +179,9 @@ public class ChangeUtil {
   private final RevertedSender.Factory revertedSenderFactory;
   private final ChangeInserter.Factory changeInserterFactory;
   private final GitRepositoryManager gitManager;
-  private final GitReferenceUpdated gitRefUpdated;
   private final BatchUpdate.Factory updateFactory;
   private final ChangeMessagesUtil changeMessagesUtil;
   private final ChangeUpdate.Factory changeUpdateFactory;
-  private final StarredChangesUtil starredChangesUtil;
 
   @Inject
   ChangeUtil(Provider<CurrentUser> user,
@@ -198,11 +191,9 @@ public class ChangeUtil {
       RevertedSender.Factory revertedSenderFactory,
       ChangeInserter.Factory changeInserterFactory,
       GitRepositoryManager gitManager,
-      GitReferenceUpdated gitRefUpdated,
       BatchUpdate.Factory updateFactory,
       ChangeMessagesUtil changeMessagesUtil,
-      ChangeUpdate.Factory changeUpdateFactory,
+      ChangeUpdate.Factory changeUpdateFactory) {
-      StarredChangesUtil starredChangesUtil) {
     this.user = user;
     this.db = db;
     this.queryProvider = queryProvider;
@@ -210,11 +201,9 @@ public class ChangeUtil {
     this.revertedSenderFactory = revertedSenderFactory;
     this.changeInserterFactory = changeInserterFactory;
     this.gitManager = gitManager;
-    this.gitRefUpdated = gitRefUpdated;
     this.updateFactory = updateFactory;
     this.changeMessagesUtil = changeMessagesUtil;
     this.changeUpdateFactory = changeUpdateFactory;
-    this.starredChangesUtil = starredChangesUtil;
   }
 
   public Change.Id revert(ChangeControl ctl, PatchSet.Id patchSetId,
@@ -341,81 +330,6 @@ public class ChangeUtil {
     }
   }
 
-  public void deleteDraftChange(Change change)
-      throws NoSuchChangeException, OrmException, IOException {
-    ReviewDb db = this.db.get();
-    Change.Id changeId = change.getId();
-    if (change.getStatus() != Change.Status.DRAFT) {
-      // TODO(dborowitz): ResourceConflictException.
-      throw new NoSuchChangeException(changeId);
-    }
-    List<PatchSet> patchSets = db.patchSets().byChange(changeId).toList();
-    for (PatchSet ps : patchSets) {
-      if (!ps.isDraft()) {
-        // TODO(dborowitz): ResourceConflictException.
-        throw new NoSuchChangeException(changeId);
-      }
-      db.accountPatchReviews().delete(
-          db.accountPatchReviews().byPatchSet(ps.getId()));
-    }
-
-    // No need to delete from notedb; draft patch sets will be filtered out.
-    db.patchComments().delete(db.patchComments().byChange(changeId));
-
-    db.patchSetApprovals().delete(db.patchSetApprovals().byChange(changeId));
-    db.patchSets().delete(patchSets);
-    db.changeMessages().delete(db.changeMessages().byChange(changeId));
-    starredChangesUtil.unstarAll(changeId);
-    db.changes().delete(Collections.singleton(change));
-
-    // Delete all refs at once.
-    try (Repository repo = gitManager.openRepository(change.getProject());
-        RevWalk rw = new RevWalk(repo)) {
-      String prefix = new PatchSet.Id(changeId, 1).toRefName();
-      prefix = prefix.substring(0, prefix.length() - 1);
-      BatchRefUpdate ru = repo.getRefDatabase().newBatchUpdate();
-      for (Ref ref : repo.getRefDatabase().getRefs(prefix).values()) {
-        ru.addCommand(
-            new ReceiveCommand(
-              ref.getObjectId(), ObjectId.zeroId(), ref.getName()));
-      }
-      ru.execute(rw, NullProgressMonitor.INSTANCE);
-      for (ReceiveCommand cmd : ru.getCommands()) {
-        if (cmd.getResult() != ReceiveCommand.Result.OK) {
-          throw new IOException("failed: " + cmd + ": " + cmd.getResult());
-        }
-      }
-    }
-  }
-
-  public void deleteOnlyDraftPatchSet(PatchSet patch, Change change)
-      throws NoSuchChangeException, OrmException, IOException {
-    PatchSet.Id patchSetId = patch.getId();
-    if (!patch.isDraft()) {
-      throw new NoSuchChangeException(patchSetId.getParentKey());
-    }
-
-    try (Repository repo = gitManager.openRepository(change.getProject())) {
-      RefUpdate update = repo.updateRef(patch.getRefName());
-      update.setForceUpdate(true);
-      update.disableRefLog();
-      switch (update.delete()) {
-        case NEW:
-        case FAST_FORWARD:
-        case FORCED:
-        case NO_CHANGE:
-          // Successful deletion.
-          break;
-        default:
-          throw new IOException("Failed to delete ref " + patch.getRefName() +
-              " in " + repo.getDirectory() + ": " + update.getResult());
-      }
-      gitRefUpdated.fire(change.getProject(), update, ReceiveCommand.Type.DELETE);
-    }
-
-    deleteOnlyDraftPatchSetPreserveRef(this.db.get(), patch);
-  }
-
   /**
    * Find changes matching the given identifier.
    *
@@ -471,22 +385,6 @@ public class ChangeUtil {
     return ctls;
   }
 
-  private static void deleteOnlyDraftPatchSetPreserveRef(ReviewDb db,
-      PatchSet patch) throws NoSuchChangeException, OrmException {
-    PatchSet.Id patchSetId = patch.getId();
-    if (!patch.isDraft()) {
-      throw new NoSuchChangeException(patchSetId.getParentKey());
-    }
-
-    db.accountPatchReviews().delete(db.accountPatchReviews().byPatchSet(patchSetId));
-    db.changeMessages().delete(db.changeMessages().byPatchSet(patchSetId));
-    // No need to delete from notedb; draft patch sets will be filtered out.
-    db.patchComments().delete(db.patchComments().byPatchSet(patchSetId));
-    db.patchSetApprovals().delete(db.patchSetApprovals().byPatchSet(patchSetId));
-
-    db.patchSets().delete(Collections.singleton(patch));
-  }
-
   public static PatchSet.Id nextPatchSetId(PatchSet.Id id) {
     return new PatchSet.Id(id.getParentKey(), id.get() + 1);
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java

@@ -19,7 +19,6 @@ import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
@@ -33,16 +32,18 @@ import com.google.gerrit.server.change.DeleteDraftPatchSet.Input;
 import com.google.gerrit.server.config.GerritServerConfig;
 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.UpdateException;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
-import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
 
 import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.transport.ReceiveCommand;
 
 import java.io.IOException;
 import java.util.Collections;
@@ -56,19 +57,19 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
   private final Provider<ReviewDb> db;
   private final BatchUpdate.Factory updateFactory;
   private final PatchSetInfoFactory patchSetInfoFactory;
-  private final ChangeUtil changeUtil;
+  private final Provider<DeleteDraftChangeOp> deleteChangeOpProvider;
   private final boolean allowDrafts;
 
   @Inject
   public DeleteDraftPatchSet(Provider<ReviewDb> db,
       BatchUpdate.Factory updateFactory,
       PatchSetInfoFactory patchSetInfoFactory,
-      ChangeUtil changeUtil,
+      Provider<DeleteDraftChangeOp> deleteChangeOpProvider,
       @GerritServerConfig Config cfg) {
     this.db = db;
     this.updateFactory = updateFactory;
     this.patchSetInfoFactory = patchSetInfoFactory;
-    this.changeUtil = changeUtil;
+    this.deleteChangeOpProvider = deleteChangeOpProvider;
     this.allowDrafts = cfg.getBoolean("change", "allowDrafts", true);
   }
 
@@ -77,6 +78,7 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
       throws RestApiException, UpdateException {
     try (BatchUpdate bu = updateFactory.create(
         db.get(), rsrc.getProject(), rsrc.getUser(), TimeUtil.nowTs())) {
+      bu.setOrder(BatchUpdate.Order.DB_BEFORE_REPO);
       bu.addOp(rsrc.getChange().getId(), new Op(rsrc.getPatchSet().getId()));
       bu.execute();
     }
@@ -86,6 +88,9 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
   private class Op extends BatchUpdate.Op {
     private final PatchSet.Id psId;
 
+    private PatchSet patchSet;
+    private DeleteDraftChangeOp deleteChangeOp;
+
     private Op(PatchSet.Id psId) {
       this.psId = psId;
     }
@@ -93,7 +98,7 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
     @Override
     public void updateChange(ChangeContext ctx)
         throws RestApiException, OrmException, IOException {
-      PatchSet patchSet = ctx.getDb().patchSets().get(psId);
+      patchSet = ctx.getDb().patchSets().get(psId);
       if (patchSet == null) {
         return; // Nothing to do.
       }
@@ -101,7 +106,7 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
         throw new ResourceConflictException("Patch set is not a draft");
       }
       if (!allowDrafts) {
-        throw new MethodNotAllowedException("draft workflow is disabled");
+        throw new MethodNotAllowedException("Draft workflow is disabled");
       }
       if (!ctx.getChangeControl().canDeleteDraft(ctx.getDb())) {
         throw new AuthException("Not permitted to delete this draft patch set");
@@ -111,21 +116,36 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
       deleteOrUpdateDraftChange(ctx);
     }
 
-    private void deleteDraftPatchSet(PatchSet patchSet, ChangeContext ctx)
+    @Override
-        throws ResourceNotFoundException, OrmException, IOException {
+    public void updateRepo(RepoContext ctx) throws IOException {
-      try {
+      if (deleteChangeOp != null) {
-        changeUtil.deleteOnlyDraftPatchSet(patchSet, ctx.getChange());
+        deleteChangeOp.updateRepo(ctx);
-      } catch (NoSuchChangeException e) {
+        return;
-        throw new ResourceNotFoundException(e.getMessage());
       }
+      ctx.getBatchRefUpdate().addCommand(
+          new ReceiveCommand(
+              ObjectId.fromString(patchSet.getRevision().get()),
+              ObjectId.zeroId(),
+              patchSet.getRefName()));
+    }
+
+    private void deleteDraftPatchSet(PatchSet patchSet, ChangeContext ctx)
+        throws OrmException {
+      ReviewDb db = ctx.getDb();
+      db.accountPatchReviews().delete(db.accountPatchReviews().byPatchSet(psId));
+      db.changeMessages().delete(db.changeMessages().byPatchSet(psId));
+      // No need to delete from notedb; draft patch sets will be filtered out.
+      db.patchComments().delete(db.patchComments().byPatchSet(psId));
+      db.patchSetApprovals().delete(db.patchSetApprovals().byPatchSet(psId));
+      db.patchSets().delete(Collections.singleton(patchSet));
     }
 
     private void deleteOrUpdateDraftChange(ChangeContext ctx)
-        throws OrmException, ResourceNotFoundException, IOException {
+        throws OrmException, RestApiException {
       Change c = ctx.getChange();
       if (Iterables.isEmpty(ctx.getDb().patchSets().byChange(c.getId()))) {
-        deleteDraftChange(c);
+        deleteChangeOp = deleteChangeOpProvider.get();
-        ctx.markDeleted();
+        deleteChangeOp.updateChange(ctx);
         return;
       }
       if (c.currentPatchSetId().equals(psId)) {
@@ -135,15 +155,6 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
       ctx.getDb().changes().update(Collections.singleton(c));
     }
 
-    private void deleteDraftChange(Change change)
-        throws OrmException, IOException, ResourceNotFoundException {
-      try {
-        changeUtil.deleteDraftChange(change);
-      } catch (NoSuchChangeException e) {
-        throw new ResourceNotFoundException(e.getMessage());
-      }
-    }
-
     private PatchSetInfo previousPatchSetInfo(ChangeContext ctx)
         throws OrmException {
       try {