Refactor DeleteDraft into gerrit-server

This change moves logic that was duplicated over the ssh command and the
rpc handler into a DeleteDraft class in gerrit-server.

Change-Id: I308f5371697776cd210199910fc787fc1220fbf1

gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewResult.java

@@ -66,11 +66,17 @@ public class ReviewResult {
       /** Not permitted to publish this draft patch set */
       PUBLISH_NOT_PERMITTED,
 
+      /** Not permitted to delete this draft patch set */
+      DELETE_NOT_PERMITTED,
+
       /** Review operation not permitted by rule. */
       RULE_ERROR,
 
       /** Review operation invalid because patch set is not a draft. */
       NOT_A_DRAFT,
+
+      /** Error writing change to git repository */
+      GIT_ERROR
     }
 
     protected Type type;

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.httpd.rpc.patch;
 
 import com.google.gerrit.common.data.ReviewerResult;
+import com.google.gerrit.common.data.ReviewResult;
 import com.google.gerrit.common.data.ApprovalSummary;
 import com.google.gerrit.common.data.ApprovalSummarySet;
 import com.google.gerrit.common.data.ApprovalTypes;
@@ -38,10 +39,10 @@ import com.google.gerrit.reviewdb.Patch.Key;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.account.AccountInfoCacheFactory;
+import com.google.gerrit.server.changedetail.DeleteDraftPatchSet;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.ReplicationQueue;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
-import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.patch.PublishComments;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
@@ -52,7 +53,6 @@ import com.google.gwtorm.client.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
-import java.io.IOException;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -66,6 +66,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
   private final AccountInfoCacheFactory.Factory accountInfoCacheFactory;
   private final AddReviewerHandler.Factory addReviewerHandlerFactory;
   private final ChangeControl.Factory changeControlFactory;
+  private final DeleteDraftPatchSet.Factory deleteDraftPatchSetFactory;
   private final RemoveReviewerHandler.Factory removeReviewerHandlerFactory;
   private final FunctionState.Factory functionStateFactory;
   private final PublishComments.Factory publishCommentsFactory;
@@ -83,6 +84,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
       final AddReviewerHandler.Factory addReviewerHandlerFactory,
       final RemoveReviewerHandler.Factory removeReviewerHandlerFactory,
       final ChangeControl.Factory changeControlFactory,
+      final DeleteDraftPatchSet.Factory deleteDraftPatchSetFactory,
       final FunctionState.Factory functionStateFactory,
       final PatchScriptFactory.Factory patchScriptFactoryFactory,
       final PublishComments.Factory publishCommentsFactory,
@@ -97,6 +99,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
     this.addReviewerHandlerFactory = addReviewerHandlerFactory;
     this.removeReviewerHandlerFactory = removeReviewerHandlerFactory;
     this.changeControlFactory = changeControlFactory;
+    this.deleteDraftPatchSetFactory = deleteDraftPatchSetFactory;
     this.functionStateFactory = functionStateFactory;
     this.patchScriptFactoryFactory = patchScriptFactoryFactory;
     this.publishCommentsFactory = publishCommentsFactory;
@@ -152,18 +155,14 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
       final AsyncCallback<VoidResult> callback) {
     run(callback, new Action<VoidResult>() {
       public VoidResult run(ReviewDb db) throws OrmException, Failure {
+        ReviewResult result = null;
         try {
-          final ChangeControl cc = changeControlFactory.validateFor(psid.getParentKey());
+          result = deleteDraftPatchSetFactory.create(psid).call();
-          if (!cc.isOwner()) {
-            throw new Failure(new NoSuchEntityException());
-          }
-          ChangeUtil.deleteDraftPatchSet(psid, gitManager, replication, patchSetInfoFactory, db);
         } catch (NoSuchChangeException e) {
-          throw new Failure(new NoSuchChangeException(psid.getParentKey()));
+          throw new Failure(new NoSuchChangeException(result.getChangeId()));
-        } catch (PatchSetInfoNotAvailableException e) {
+        }
-          throw new Failure(e);
+        if (result.getErrors().size() > 0) {
-        } catch (IOException e) {
+          throw new Failure(new NoSuchEntityException());
-          throw new Failure(e);
         }
         return VoidResult.INSTANCE;
       }

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

@@ -286,37 +286,7 @@ public class ChangeUtil {
     db.changes().delete(Collections.singleton(change));
   }
 
-  public static void deleteDraftPatchSet(final PatchSet.Id patchSetId,
+  public static void deleteOnlyDraftPatchSet(final PatchSet patch,
-      GitRepositoryManager gitManager,
-      final ReplicationQueue replication,
-      final PatchSetInfoFactory patchSetInfoFactory,
-      final ReviewDb db) throws NoSuchChangeException, OrmException,
-      PatchSetInfoNotAvailableException, IOException {
-    final Change.Id changeId = patchSetId.getParentKey();
-    final Change change = db.changes().get(changeId);
-    final PatchSet patch = db.patchSets().get(patchSetId);
-
-    deleteOnlyDraftPatchSet(patch, change, gitManager, replication, db);
-
-    List<PatchSet> restOfPatches = db.patchSets().byChange(changeId).toList();
-    if (restOfPatches.size() == 0) {
-      deleteDraftChange(patchSetId, gitManager, replication, db);
-    } else {
-      PatchSet.Id highestId = null;
-      for (PatchSet ps : restOfPatches) {
-        if (highestId == null || ps.getPatchSetId() > highestId.get()) {
-          highestId = ps.getId();
-        }
-      }
-      if (change.currentPatchSetId().equals(patchSetId)) {
-        change.removeLastPatchSetId();
-        change.setCurrentPatchSet(patchSetInfoFactory.get(db, change.currPatchSetId()));
-        db.changes().update(Collections.singleton(change));
-      }
-    }
-  }
-
-  private static void deleteOnlyDraftPatchSet(final PatchSet patch,
       final Change change, GitRepositoryManager gitManager,
       final ReplicationQueue replication, final ReviewDb db)
       throws NoSuchChangeException, OrmException, IOException {

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

@@ -0,0 +1,124 @@
+// Copyright (C) 2011 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+
+package com.google.gerrit.server.changedetail;
+
+import com.google.gerrit.common.data.ReviewResult;
+import com.google.gerrit.reviewdb.Change;
+import com.google.gerrit.reviewdb.PatchSet;
+import com.google.gerrit.reviewdb.ReviewDb;
+import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.ReplicationQueue;
+import com.google.gerrit.server.patch.PatchSetInfoFactory;
+import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
+import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.gwtorm.client.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.Callable;
+
+public class DeleteDraftPatchSet implements Callable<ReviewResult> {
+
+  public interface Factory {
+    DeleteDraftPatchSet create(PatchSet.Id patchSetId);
+  }
+
+  private final ChangeControl.Factory changeControlFactory;
+  private final ReviewDb db;
+  private final GitRepositoryManager gitManager;
+  private final ReplicationQueue replication;
+  private final PatchSetInfoFactory patchSetInfoFactory;
+
+  private final PatchSet.Id patchSetId;
+
+  @Inject
+  DeleteDraftPatchSet(ChangeControl.Factory changeControlFactory,
+      ReviewDb db, GitRepositoryManager gitManager,
+      ReplicationQueue replication, PatchSetInfoFactory patchSetInfoFactory,
+      @Assisted final PatchSet.Id patchSetId) {
+    this.changeControlFactory = changeControlFactory;
+    this.db = db;
+    this.gitManager = gitManager;
+    this.replication = replication;
+    this.patchSetInfoFactory = patchSetInfoFactory;
+
+    this.patchSetId = patchSetId;
+  }
+
+  @Override
+  public ReviewResult call() throws NoSuchChangeException, OrmException {
+    final ReviewResult result = new ReviewResult();
+
+    final Change.Id changeId = patchSetId.getParentKey();
+    result.setChangeId(changeId);
+    final ChangeControl control = changeControlFactory.validateFor(changeId);
+    final PatchSet patch = db.patchSets().get(patchSetId);
+    if (patch == null) {
+      throw new NoSuchChangeException(changeId);
+    }
+    if (!patch.isDraft()) {
+      result.addError(new ReviewResult.Error(
+          ReviewResult.Error.Type.NOT_A_DRAFT));
+      return result;
+    }
+
+    if (!control.canDelete(db)) {
+      result.addError(new ReviewResult.Error(
+          ReviewResult.Error.Type.DELETE_NOT_PERMITTED));
+      return result;
+    }
+    final Change change = control.getChange();
+
+    try {
+      ChangeUtil.deleteOnlyDraftPatchSet(patch, change, gitManager, replication, db);
+    } catch (IOException e) {
+      result.addError(new ReviewResult.Error(
+          ReviewResult.Error.Type.GIT_ERROR, e.getMessage()));
+    }
+
+    List<PatchSet> restOfPatches = db.patchSets().byChange(changeId).toList();
+    if (restOfPatches.size() == 0) {
+      try {
+        ChangeUtil.deleteDraftChange(patchSetId, gitManager, replication, db);
+      } catch (IOException e) {
+        result.addError(new ReviewResult.Error(
+            ReviewResult.Error.Type.GIT_ERROR, e.getMessage()));
+      }
+    } else {
+      PatchSet.Id highestId = null;
+      for (PatchSet ps : restOfPatches) {
+        if (highestId == null || ps.getPatchSetId() > highestId.get()) {
+          highestId = ps.getId();
+        }
+      }
+      if (change.currentPatchSetId().equals(patchSetId)) {
+        change.removeLastPatchSetId();
+        try {
+          change.setCurrentPatchSet(patchSetInfoFactory.get(db, change.currPatchSetId()));
+        } catch (PatchSetInfoNotAvailableException e) {
+          throw new NoSuchChangeException(changeId);
+        }
+        db.changes().update(Collections.singleton(change));
+      }
+    }
+    return result;
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java

@@ -28,6 +28,7 @@ import com.google.gerrit.server.account.PerformCreateGroup;
 import com.google.gerrit.server.account.PerformRenameGroup;
 import com.google.gerrit.server.account.VisibleGroups;
 import com.google.gerrit.server.changedetail.AbandonChange;
+import com.google.gerrit.server.changedetail.DeleteDraftPatchSet;
 import com.google.gerrit.server.changedetail.PublishDraft;
 import com.google.gerrit.server.changedetail.RestoreChange;
 import com.google.gerrit.server.changedetail.Submit;
@@ -88,6 +89,7 @@ public class GerritRequestModule extends FactoryModule {
     factory(AddReviewer.Factory.class);
     factory(AddReviewerSender.Factory.class);
     factory(CreateChangeSender.Factory.class);
+    factory(DeleteDraftPatchSet.Factory.class);
     factory(PublishComments.Factory.class);
     factory(PublishDraft.Factory.class);
     factory(ReplacePatchSetSender.Factory.class);

gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java

@@ -24,17 +24,13 @@ import com.google.gerrit.reviewdb.PatchSet;
 import com.google.gerrit.reviewdb.PatchSetApproval;
 import com.google.gerrit.reviewdb.RevId;
 import com.google.gerrit.reviewdb.ReviewDb;
-import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.changedetail.AbandonChange;
+import com.google.gerrit.server.changedetail.DeleteDraftPatchSet;
 import com.google.gerrit.server.changedetail.PublishDraft;
 import com.google.gerrit.server.changedetail.RestoreChange;
 import com.google.gerrit.server.changedetail.Submit;
-import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.git.ReplicationQueue;
 import com.google.gerrit.server.mail.EmailException;
-import com.google.gerrit.server.patch.PatchSetInfoFactory;
-import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.patch.PublishComments;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
@@ -123,6 +119,9 @@ public class ReviewCommand extends BaseCommand {
   @Inject
   private ChangeControl.Factory changeControlFactory;
 
+  @Inject
+  private DeleteDraftPatchSet.Factory deleteDraftPatchSetFactory;
+
   @Inject
   private AbandonChange.Factory abandonChangeFactory;
 
@@ -138,18 +137,9 @@ public class ReviewCommand extends BaseCommand {
   @Inject
   private RestoreChange.Factory restoreChangeFactory;
 
-  @Inject
-  private GitRepositoryManager gitManager;
-
-  @Inject
-  private ReplicationQueue replication;
-
   @Inject
   private Submit.Factory submitFactory;
 
-  @Inject
-  private PatchSetInfoFactory patchSetInfoFactory;
-
   private List<ApproveOption> optionList;
 
   @Override
@@ -233,16 +223,16 @@ public class ReviewCommand extends BaseCommand {
       publishCommentsFactory.create(patchSetId, changeComment, aps, forceMessage).call();
 
       if (abandonChange) {
-        ReviewResult result = abandonChangeFactory.create(
+        final ReviewResult result = abandonChangeFactory.create(
             patchSetId, changeComment).call();
         handleReviewResultErrors(result);
       } else if (restoreChange) {
-        ReviewResult result = restoreChangeFactory.create(
+        final ReviewResult result = restoreChangeFactory.create(
             patchSetId, changeComment).call();
         handleReviewResultErrors(result);
       }
       if (submitChange) {
-        ReviewResult result = submitFactory.create(patchSetId).call();
+        final ReviewResult result = submitFactory.create(patchSetId).call();
         handleReviewResultErrors(result);
       }
     } catch (InvalidChangeOperationException e) {
@@ -252,20 +242,12 @@ public class ReviewCommand extends BaseCommand {
     }
 
     if (publishPatchSet) {
-      ReviewResult result = publishDraftFactory.create(patchSetId).call();
+      final ReviewResult result = publishDraftFactory.create(patchSetId).call();
       handleReviewResultErrors(result);
     } else if (deleteDraftPatchSet) {
-      if (changeControl.canDelete(db)) {
+      final ReviewResult result =
-        try {
+          deleteDraftPatchSetFactory.create(patchSetId).call();
-          ChangeUtil.deleteDraftPatchSet(patchSetId, gitManager, replication, patchSetInfoFactory, db);
+      handleReviewResultErrors(result);
-        } catch (PatchSetInfoNotAvailableException e) {
-          throw error("Error retrieving draft patchset: " + patchSetId);
-        } catch (IOException e) {
-          throw error("Error deleting draft patchset: " + patchSetId);
-        }
-      } else {
-        throw error("Not permitted to delete draft patchset");
-      }
     }
   }
 
@@ -291,12 +273,18 @@ public class ReviewCommand extends BaseCommand {
         case PUBLISH_NOT_PERMITTED:
           errMsg += "not permitted to publish change";
           break;
+        case DELETE_NOT_PERMITTED:
+          errMsg += "not permitted to delete change/patch set";
+          break;
         case RULE_ERROR:
           errMsg += "rule error";
           break;
         case NOT_A_DRAFT:
           errMsg += "change is not a draft";
           break;
+        case GIT_ERROR:
+          errMsg += "error writing change to git repository";
+          break;
         default:
           errMsg += "failure in review";
       }