From 8f258d99726b80f5b890602eb5379543144bca81 Mon Sep 17 00:00:00 2001 From: Conley Owens Date: Thu, 7 Jun 2012 18:08:59 -0700 Subject: [PATCH] No longer throw InvalidChangeOp in changeUpdated InvalidChangeOperationExceptions are only thrown when we pass a null change to this method. It is better that we just don't pass the change and add a more useful error to the ReviewResult. We no longer perform a check to see if the patchset we have is the most current one. restore/abandon happens at the change level and the patchset is just fetched to append a message to the change. Change-Id: I3b3af1e3982fcd9111214a7c66a28476189c923b --- .../gerrit/common/data/ReviewResult.java | 3 +++ .../changedetail/AbandonChangeHandler.java | 6 ++---- .../changedetail/RestoreChangeHandler.java | 6 ++---- .../com/google/gerrit/server/ChangeUtil.java | 8 ++------ .../server/changedetail/AbandonChange.java | 20 +++++++++++-------- .../server/changedetail/RestoreChange.java | 19 +++++++++--------- .../gerrit/sshd/commands/ReviewCommand.java | 3 +++ 7 files changed, 34 insertions(+), 31 deletions(-) diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewResult.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewResult.java index 28cf49b4bc..c0bf81829e 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewResult.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewResult.java @@ -63,6 +63,9 @@ public class ReviewResult { /** Review operation invalid because change is closed. */ CHANGE_IS_CLOSED, + /** Review operation invalid because change is not abandoned. */ + CHANGE_NOT_ABANDONED, + /** Not permitted to publish this draft patch set */ PUBLISH_NOT_PERMITTED, diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChangeHandler.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChangeHandler.java index 1d4d3e2eb1..1713422a58 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChangeHandler.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChangeHandler.java @@ -22,7 +22,6 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.server.changedetail.AbandonChange; import com.google.gerrit.server.mail.EmailException; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; -import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -61,9 +60,8 @@ class AbandonChangeHandler extends Handler { @Override public ChangeDetail call() throws NoSuchChangeException, OrmException, - EmailException, NoSuchEntityException, InvalidChangeOperationException, - PatchSetInfoNotAvailableException, RepositoryNotFoundException, - IOException { + EmailException, NoSuchEntityException, PatchSetInfoNotAvailableException, + RepositoryNotFoundException, IOException { final ReviewResult result = abandonChangeFactory.create(patchSetId.getParentKey(), message).call(); if (result.getErrors().size() > 0) { diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RestoreChangeHandler.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RestoreChangeHandler.java index 5d7fe325d4..12d66fd79d 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RestoreChangeHandler.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RestoreChangeHandler.java @@ -22,7 +22,6 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.server.changedetail.RestoreChange; import com.google.gerrit.server.mail.EmailException; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; -import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -60,9 +59,8 @@ class RestoreChangeHandler extends Handler { @Override public ChangeDetail call() throws NoSuchChangeException, OrmException, - EmailException, NoSuchEntityException, InvalidChangeOperationException, - PatchSetInfoNotAvailableException, RepositoryNotFoundException, - IOException { + EmailException, NoSuchEntityException, PatchSetInfoNotAvailableException, + RepositoryNotFoundException, IOException { final ReviewResult result = restoreChangeFactory.create(patchSetId.getParentKey(), message).call(); if (result.getErrors().size() > 0) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index 4b984474b2..8d31593a57 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java @@ -585,12 +585,8 @@ public class ChangeUtil { public static void updatedChange( final ReviewDb db, final IdentifiedUser user, final Change change, - final ChangeMessage cmsg, ReplyToChangeSender.Factory senderFactory, - final String err) throws NoSuchChangeException, - InvalidChangeOperationException, EmailException, OrmException { - if (change == null) { - throw new InvalidChangeOperationException(err); - } + final ChangeMessage cmsg, ReplyToChangeSender.Factory senderFactory) + throws NoSuchChangeException, EmailException, OrmException { db.changeMessages().insert(Collections.singleton(cmsg)); new ApprovalsUtil(db, null).syncChangeStatus(change); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/AbandonChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/AbandonChange.java index 83fa671bb4..bc0ef61aa4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/AbandonChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/AbandonChange.java @@ -26,7 +26,6 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.mail.AbandonedSender; import com.google.gerrit.server.mail.EmailException; 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.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; @@ -69,8 +68,8 @@ public class AbandonChange implements Callable { } @Override - public ReviewResult call() throws EmailException, - InvalidChangeOperationException, NoSuchChangeException, OrmException { + public ReviewResult call() throws EmailException, NoSuchChangeException, + OrmException { final ReviewResult result = new ReviewResult(); result.setChangeId(changeId); @@ -102,8 +101,7 @@ public class AbandonChange implements Callable { new AtomicUpdate() { @Override public Change update(Change change) { - if (change.getStatus().isOpen() - && change.currentPatchSetId().equals(patchSetId)) { + if (change.getStatus().isOpen()) { change.setStatus(Change.Status.ABANDONED); ChangeUtil.updated(change); return change; @@ -112,9 +110,15 @@ public class AbandonChange implements Callable { } } }); - ChangeUtil.updatedChange( - db, currentUser, updatedChange, cmsg, abandonedSenderFactory, - "Change is no longer open or patchset is not latest"); + + if (updatedChange == null) { + result.addError(new ReviewResult.Error( + ReviewResult.Error.Type.CHANGE_IS_CLOSED)); + return result; + } + + ChangeUtil.updatedChange(db, currentUser, updatedChange, cmsg, + abandonedSenderFactory); hooks.doChangeAbandonedHook(updatedChange, currentUser.getAccount(), changeComment, db); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RestoreChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RestoreChange.java index 966efce4d4..859807cb0f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RestoreChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RestoreChange.java @@ -29,7 +29,6 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.mail.EmailException; import com.google.gerrit.server.mail.RestoredSender; 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.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; @@ -77,9 +76,8 @@ public class RestoreChange implements Callable { } @Override - public ReviewResult call() throws EmailException, - InvalidChangeOperationException, NoSuchChangeException, OrmException, - RepositoryNotFoundException, IOException { + public ReviewResult call() throws EmailException, NoSuchChangeException, + OrmException, RepositoryNotFoundException, IOException { final ReviewResult result = new ReviewResult(); result.setChangeId(changeId); @@ -121,8 +119,7 @@ public class RestoreChange implements Callable { new AtomicUpdate() { @Override public Change update(Change change) { - if (change.getStatus() == Change.Status.ABANDONED - && change.currentPatchSetId().equals(patchSetId)) { + if (change.getStatus() == Change.Status.ABANDONED) { change.setStatus(Change.Status.NEW); ChangeUtil.updated(change); return change; @@ -132,10 +129,14 @@ public class RestoreChange implements Callable { } }); - ChangeUtil.updatedChange( - db, currentUser, updatedChange, cmsg, restoredSenderFactory, - "Change is not abandoned or patchset is not latest"); + if (updatedChange == null) { + result.addError(new ReviewResult.Error( + ReviewResult.Error.Type.CHANGE_NOT_ABANDONED)); + return result; + } + ChangeUtil.updatedChange(db, currentUser, updatedChange, cmsg, + restoredSenderFactory); hooks.doChangeRestoreHook(updatedChange, currentUser.getAccount(), changeComment, db); diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java index f38e17ef86..236d9f15d2 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java @@ -248,6 +248,9 @@ public class ReviewCommand extends SshCommand { case CHANGE_IS_CLOSED: errMsg += "change is closed"; break; + case CHANGE_NOT_ABANDONED: + errMsg += "change is not abandoned"; + break; case PUBLISH_NOT_PERMITTED: errMsg += "not permitted to publish change"; break;