Add error messages for abandon and restore when in bad state

The abandon and restore routines can fail if the change
is in the wrong state (abandoned already, or open still).
These failure lead to internal NPEs.  Instead, create a
new exception and give a reason for the failure to the user.

Change-Id: Id7861d75e535c439c12329f7e891797c5b1f6eca

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChange.java

@@ -27,6 +27,7 @@ import com.google.gerrit.server.mail.AbandonedSender;
 import com.google.gerrit.server.mail.EmailException;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.client.OrmException;
 import com.google.inject.Inject;
@@ -71,7 +72,8 @@ class AbandonChange extends Handler<ChangeDetail> {
 
   @Override
   public ChangeDetail call() throws NoSuchChangeException, OrmException,
-      EmailException, NoSuchEntityException, PatchSetInfoNotAvailableException {
+      EmailException, NoSuchEntityException, InvalidChangeOperationException,
+      PatchSetInfoNotAvailableException {
 
     final Change.Id changeId = patchSetId.getParentKey();
     final ChangeControl control = changeControlFactory.validateFor(changeId);

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RestoreChange.java

@@ -25,6 +25,7 @@ import com.google.gerrit.server.mail.AbandonedSender;
 import com.google.gerrit.server.mail.EmailException;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.client.OrmException;
 import com.google.inject.Inject;
@@ -69,7 +70,8 @@ class RestoreChange extends Handler<ChangeDetail> {
 
   @Override
   public ChangeDetail call() throws NoSuchChangeException, OrmException,
-      EmailException, NoSuchEntityException, PatchSetInfoNotAvailableException {
+      EmailException, NoSuchEntityException, InvalidChangeOperationException,
+      PatchSetInfoNotAvailableException {
 
     final Change.Id changeId = patchSetId.getParentKey();
     final ChangeControl control = changeControlFactory.validateFor(changeId);

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

@@ -34,6 +34,7 @@ import com.google.gerrit.server.git.MergeQueue;
 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.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.mail.AbandonedSender;
 import com.google.gerrit.server.mail.EmailException;
@@ -212,7 +213,7 @@ public class ChangeUtil {
       final IdentifiedUser user, final String message, final ReviewDb db,
       final AbandonedSender.Factory abandonedSenderFactory,
       final ChangeHookRunner hooks) throws NoSuchChangeException,
-      EmailException, OrmException {
+      InvalidChangeOperationException, EmailException, OrmException {
     final Change.Id changeId = patchSetId.getParentKey();
     final PatchSet patch = db.patchSets().get(patchSetId);
     if (patch == null) {
@@ -245,23 +246,26 @@ public class ChangeUtil {
       }
     });
 
-    if (updatedChange != null) {
-      db.changeMessages().insert(Collections.singleton(cmsg));
-
-      final List<PatchSetApproval> approvals =
-          db.patchSetApprovals().byChange(changeId).toList();
-      for (PatchSetApproval a : approvals) {
-        a.cache(updatedChange);
-      }
-      db.patchSetApprovals().update(approvals);
-
-      // Email the reviewers
-      final AbandonedSender cm = abandonedSenderFactory.create(updatedChange);
-      cm.setFrom(user.getAccountId());
-      cm.setChangeMessage(cmsg);
-      cm.send();
+    if (updatedChange == null) {
+      throw new InvalidChangeOperationException(
+          "Change is no longer open or patchset is not latest");
     }
 
+    db.changeMessages().insert(Collections.singleton(cmsg));
+
+    final List<PatchSetApproval> approvals =
+        db.patchSetApprovals().byChange(changeId).toList();
+    for (PatchSetApproval a : approvals) {
+      a.cache(updatedChange);
+    }
+    db.patchSetApprovals().update(approvals);
+
+    // Email the reviewers
+    final AbandonedSender cm = abandonedSenderFactory.create(updatedChange);
+    cm.setFrom(user.getAccountId());
+    cm.setChangeMessage(cmsg);
+    cm.send();
+
     hooks.doChangeAbandonedHook(updatedChange, user.getAccount(), message);
   }
 
@@ -371,7 +375,7 @@ public class ChangeUtil {
       final IdentifiedUser user, final String message, final ReviewDb db,
       final AbandonedSender.Factory abandonedSenderFactory,
       final ChangeHookRunner hooks) throws NoSuchChangeException,
-      EmailException, OrmException {
+      InvalidChangeOperationException, EmailException, OrmException {
     final Change.Id changeId = patchSetId.getParentKey();
     final PatchSet patch = db.patchSets().get(patchSetId);
     if (patch == null) {
@@ -404,23 +408,26 @@ public class ChangeUtil {
       }
     });
 
-    if (updatedChange != null) {
-      db.changeMessages().insert(Collections.singleton(cmsg));
-
-      final List<PatchSetApproval> approvals =
-          db.patchSetApprovals().byChange(changeId).toList();
-      for (PatchSetApproval a : approvals) {
-        a.cache(updatedChange);
-      }
-      db.patchSetApprovals().update(approvals);
-
-      // Email the reviewers
-      final AbandonedSender cm = abandonedSenderFactory.create(updatedChange);
-      cm.setFrom(user.getAccountId());
-      cm.setChangeMessage(cmsg);
-      cm.send();
+    if (updatedChange == null) {
+      throw new InvalidChangeOperationException(
+          "Change is not abandoned or patchset is not latest");
     }
 
+    db.changeMessages().insert(Collections.singleton(cmsg));
+
+    final List<PatchSetApproval> approvals =
+        db.patchSetApprovals().byChange(changeId).toList();
+    for (PatchSetApproval a : approvals) {
+      a.cache(updatedChange);
+    }
+    db.patchSetApprovals().update(approvals);
+
+    // Email the reviewers
+    final AbandonedSender cm = abandonedSenderFactory.create(updatedChange);
+    cm.setFrom(user.getAccountId());
+    cm.setChangeMessage(cmsg);
+    cm.send();
+
     hooks.doChangeRestoreHook(updatedChange, user.getAccount(), message);
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/project/InvalidChangeOperationException.java

@@ -0,0 +1,26 @@
+// 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.project;
+
+import com.google.gerrit.reviewdb.Change;
+
+/** Indicates the change operation is not currently valid. */
+public class InvalidChangeOperationException extends Exception {
+  private static final long serialVersionUID = 1L;
+
+  public InvalidChangeOperationException(String msg) {
+    super(msg);
+  }
+}

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

@@ -34,6 +34,7 @@ import com.google.gerrit.server.mail.EmailException;
 import com.google.gerrit.server.patch.PublishComments;
 import com.google.gerrit.server.project.CanSubmitResult;
 import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectControl;
 import com.google.gerrit.server.workflow.FunctionState;
@@ -202,9 +203,8 @@ public class ReviewCommand extends BaseCommand {
     });
   }
 
-  private void approveOne(final PatchSet.Id patchSetId)
-      throws NoSuchChangeException, UnloggedFailure, OrmException,
-             EmailException {
+  private void approveOne(final PatchSet.Id patchSetId) throws
+      NoSuchChangeException, UnloggedFailure, OrmException, EmailException {
 
     final Change.Id changeId = patchSetId.getParentKey();
     ChangeControl changeControl = changeControlFactory.validateFor(changeId);
@@ -224,25 +224,29 @@ public class ReviewCommand extends BaseCommand {
 
     publishCommentsFactory.create(patchSetId, changeComment, aps).call();
 
-    if (abandonChange) {
-      if (changeControl.canAbandon()) {
-        ChangeUtil.abandon(patchSetId, currentUser, changeComment, db,
-          abandonedSenderFactory, hooks);
-      } else {
-        throw error("Not permitted to abandon change");
+    try {
+      if (abandonChange) {
+        if (changeControl.canAbandon()) {
+          ChangeUtil.abandon(patchSetId, currentUser, changeComment, db,
+              abandonedSenderFactory, hooks);
+        } else {
+          throw error("Not permitted to abandon change");
+        }
       }
-    }
 
-    if (restoreChange) {
-      if (changeControl.canRestore()) {
-        ChangeUtil.restore(patchSetId, currentUser, changeComment, db,
-          abandonedSenderFactory, hooks);
-      } else {
-        throw error("Not permitted to restore change");
-      }
-      if (submitChange) {
-        changeControl = changeControlFactory.validateFor(changeId);
+      if (restoreChange) {
+        if (changeControl.canRestore()) {
+          ChangeUtil.restore(patchSetId, currentUser, changeComment, db,
+              abandonedSenderFactory, hooks);
+        } else {
+          throw error("Not permitted to restore change");
+        }
+        if (submitChange) {
+          changeControl = changeControlFactory.validateFor(changeId);
+        }
       }
+    } catch (InvalidChangeOperationException e) {
+      throw error(e.getMessage());
     }
 
     if (submitChange) {