Run /abandon and /restore in a single database operation

On the backend behind gerrit-review each database call is expensive,
unless we do something crazy like wrap a block of database operations
into a single mock transaction with the beginTransaction/commit idiom.

Simplify both of these tasks by getting rid of the ugly helper
function from ChangeUtil and explicitly perform all database
operations before sending email to interested parties.

Change-Id: I63cbbc7ac47d20f202b042740abb204e040e7c65

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

@@ -48,7 +48,7 @@ public class ApprovalsUtil {
   private final ApprovalTypes approvalTypes;
 
   @Inject
-  ApprovalsUtil(ReviewDb db, ApprovalTypes approvalTypes) {
+  public ApprovalsUtil(ReviewDb db, ApprovalTypes approvalTypes) {
     this.db = db;
     this.approvalTypes = approvalTypes;
   }

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

@@ -30,7 +30,6 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeOp;
 import com.google.gerrit.server.mail.EmailException;
-import com.google.gerrit.server.mail.ReplyToChangeSender;
 import com.google.gerrit.server.mail.RevertedSender;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
@@ -56,8 +55,6 @@ import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.util.Base64;
 import org.eclipse.jgit.util.ChangeIdUtil;
 import org.eclipse.jgit.util.NB;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.sql.Timestamp;
@@ -70,8 +67,6 @@ import java.util.Set;
 import java.util.regex.Matcher;
 
 public class ChangeUtil {
-  private static final Logger log = LoggerFactory.getLogger(ChangeUtil.class);
-
   private static int uuidPrefix;
   private static int uuidSeq;
 
@@ -513,25 +508,6 @@ public class ChangeUtil {
     db.patchSets().delete(Collections.singleton(patch));
   }
 
-  public static <T extends ReplyToChangeSender> void updatedChange(
-      final ReviewDb db, final IdentifiedUser user, final Change change,
-      final ChangeMessage cmsg, ReplyToChangeSender.Factory<T> senderFactory)
-      throws OrmException {
-    db.changeMessages().insert(Collections.singleton(cmsg));
-
-    new ApprovalsUtil(db, null).syncChangeStatus(change);
-
-    // Email the reviewers
-    try {
-      final ReplyToChangeSender cm = senderFactory.create(change);
-      cm.setFrom(user.getAccountId());
-      cm.setChangeMessage(cmsg);
-      cm.send();
-    } catch (Exception e) {
-      log.error("Cannot email update for change " + change.getChangeId(), e);
-    }
-  }
-
   public static String sortKey(long lastUpdated, int id){
     // The encoding uses minutes since Wed Oct 1 00:00:00 2008 UTC.
     // We overrun approximately 4,085 years later, so ~6093.

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

@@ -23,18 +23,27 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
-import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.change.Abandon.Input;
 import com.google.gerrit.server.mail.AbandonedSender;
+import com.google.gerrit.server.mail.ReplyToChangeSender;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.server.AtomicUpdate;
+import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+
 public class Abandon implements RestModifyView<ChangeResource, Input> {
+  private static final Logger log = LoggerFactory.getLogger(Abandon.class);
+
   private final ChangeHooks hooks;
   private final AbandonedSender.Factory abandonedSenderFactory;
   private final Provider<ReviewDb> dbProvider;
@@ -66,6 +75,7 @@ public class Abandon implements RestModifyView<ChangeResource, Input> {
       throws BadRequestException, AuthException,
       ResourceConflictException, Exception {
     ChangeControl control = req.getControl();
+    IdentifiedUser caller = (IdentifiedUser) control.getCurrentUser();
     Change change = req.getChange();
     if (!control.canAbandon()) {
       throw new AuthException("abandon not permitted");
@@ -73,46 +83,68 @@ public class Abandon implements RestModifyView<ChangeResource, Input> {
       throw new ResourceConflictException("change is " + status(change));
     }
 
-    // Create a message to accompany the abandoned change
+    ChangeMessage message;
     ReviewDb db = dbProvider.get();
-    PatchSet.Id patchSetId = change.currentPatchSetId();
+    db.changes().beginTransaction(change.getId());
-    IdentifiedUser currentUser = (IdentifiedUser) control.getCurrentUser();
+    try {
-    String message = Strings.emptyToNull(input.message);
+      change = db.changes().atomicUpdate(
-    ChangeMessage cmsg = new ChangeMessage(
+        change.getId(),
-        new ChangeMessage.Key(change.getId(), ChangeUtil.messageUUID(db)),
+        new AtomicUpdate<Change>() {
-        currentUser.getAccountId(), patchSetId);
+          @Override
-    StringBuilder msg = new StringBuilder();
+          public Change update(Change change) {
-    msg.append(String.format("Patch Set %d: Abandoned", patchSetId.get()));
+            if (change.getStatus().isOpen()) {
-    if (message != null) {
+              change.setStatus(Change.Status.ABANDONED);
-      msg.append("\n\n");
+              ChangeUtil.updated(change);
-      msg.append(message);
+              return change;
-    }
+            }
-    cmsg.setMessage(msg.toString());
+            return null;
-
-    // Abandon the change
-    Change updatedChange = db.changes().atomicUpdate(
-      change.getId(),
-      new AtomicUpdate<Change>() {
-        @Override
-        public Change update(Change change) {
-          if (change.getStatus().isOpen()) {
-            change.setStatus(Change.Status.ABANDONED);
-            ChangeUtil.updated(change);
-            return change;
           }
-          return null;
+        });
-        }
+      if (change == null) {
-      });
+        throw new ResourceConflictException("change is "
-    if (updatedChange == null) {
+            + status(db.changes().get(req.getChange().getId())));
-      throw new ResourceConflictException("change is "
+      }
-          + status(db.changes().get(change.getId())));
+      message = newMessage(input, caller, change);
+      db.changeMessages().insert(Collections.singleton(message));
+      new ApprovalsUtil(db, null).syncChangeStatus(change);
+      db.commit();
+    } finally {
+      db.rollback();
     }
 
-    ChangeUtil.updatedChange(db, currentUser, updatedChange, cmsg,
+    try {
-                             abandonedSenderFactory);
+      ReplyToChangeSender cm = abandonedSenderFactory.create(change);
-    hooks.doChangeAbandonedHook(updatedChange, currentUser.getAccount(),
+      cm.setFrom(caller.getAccountId());
-                                message, db);
+      cm.setChangeMessage(message);
-    return json.format(change.getId());
+      cm.send();
+    } catch (Exception e) {
+      log.error("Cannot email update for change " + change.getChangeId(), e);
+    }
+    hooks.doChangeAbandonedHook(change,
+        caller.getAccount(),
+        Strings.emptyToNull(input.message),
+        db);
+    return json.format(change);
+  }
+
+  private ChangeMessage newMessage(Input input, IdentifiedUser caller,
+      Change change) throws OrmException {
+    StringBuilder msg = new StringBuilder();
+    msg.append("Abandoned");
+    if (!Strings.nullToEmpty(input.message).trim().isEmpty()) {
+      msg.append("\n\n");
+      msg.append(input.message.trim());
+    }
+
+    ChangeMessage message = new ChangeMessage(
+        new ChangeMessage.Key(
+            change.getId(),
+            ChangeUtil.messageUUID(dbProvider.get())),
+        caller.getAccountId(),
+        change.getLastUpdatedOn(),
+        change.currentPatchSetId());
+    message.setMessage(msg.toString());
+    return message;
   }
 
   private static String status(Change change) {

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

@@ -23,18 +23,27 @@ import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Change.Status;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
-import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.change.Restore.Input;
+import com.google.gerrit.server.mail.ReplyToChangeSender;
 import com.google.gerrit.server.mail.RestoredSender;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.server.AtomicUpdate;
+import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+
 public class Restore implements RestModifyView<ChangeResource, Input> {
+  private static final Logger log = LoggerFactory.getLogger(Restore.class);
+
   private final ChangeHooks hooks;
   private final RestoredSender.Factory restoredSenderFactory;
   private final Provider<ReviewDb> dbProvider;
@@ -65,6 +74,7 @@ public class Restore implements RestModifyView<ChangeResource, Input> {
   public Object apply(ChangeResource req, Input input)
       throws Exception {
     ChangeControl control = req.getControl();
+    IdentifiedUser caller = (IdentifiedUser) control.getCurrentUser();
     Change change = req.getChange();
     if (!control.canRestore()) {
       throw new AuthException("restore not permitted");
@@ -72,46 +82,68 @@ public class Restore implements RestModifyView<ChangeResource, Input> {
       throw new ResourceConflictException("change is " + status(change));
     }
 
-    // Create a message to accompany the restore change
+    ChangeMessage message;
     ReviewDb db = dbProvider.get();
-    PatchSet.Id patchSetId = change.currentPatchSetId();
+    db.changes().beginTransaction(change.getId());
-    IdentifiedUser currentUser = (IdentifiedUser) control.getCurrentUser();
+    try {
-    String message = Strings.emptyToNull(input.message);
+      change = db.changes().atomicUpdate(
-    final ChangeMessage cmsg = new ChangeMessage(
+        change.getId(),
-        new ChangeMessage.Key(change.getId(), ChangeUtil.messageUUID(db)),
+        new AtomicUpdate<Change>() {
-        currentUser.getAccountId(), patchSetId);
+          @Override
-    StringBuilder msg = new StringBuilder();
+          public Change update(Change change) {
-    msg.append(String.format("Patch Set %d: Restored", patchSetId.get()));
+            if (change.getStatus() == Change.Status.ABANDONED) {
-    if (message != null) {
+              change.setStatus(Change.Status.NEW);
-      msg.append("\n\n");
+              ChangeUtil.updated(change);
-      msg.append(message);
+              return change;
-    }
+            }
-    cmsg.setMessage(msg.toString());
+            return null;
-
-    // Restore the change
-    final Change updatedChange = db.changes().atomicUpdate(
-      change.getId(),
-      new AtomicUpdate<Change>() {
-        @Override
-        public Change update(Change change) {
-          if (change.getStatus() == Change.Status.ABANDONED) {
-            change.setStatus(Change.Status.NEW);
-            ChangeUtil.updated(change);
-            return change;
           }
-          return null;
+        });
-        }
+      if (change == null) {
-      });
+        throw new ResourceConflictException("change is "
-    if (updatedChange == null) {
+            + status(db.changes().get(req.getChange().getId())));
-      throw new ResourceConflictException("change is "
+      }
-          + status(db.changes().get(change.getId())));
+      message = newMessage(input, caller, change);
+      db.changeMessages().insert(Collections.singleton(message));
+      new ApprovalsUtil(db, null).syncChangeStatus(change);
+      db.commit();
+    } finally {
+      db.rollback();
     }
 
-    ChangeUtil.updatedChange(db, currentUser, updatedChange, cmsg,
+    try {
-                             restoredSenderFactory);
+      ReplyToChangeSender cm = restoredSenderFactory.create(change);
-    hooks.doChangeRestoredHook(updatedChange, currentUser.getAccount(),
+      cm.setFrom(caller.getAccountId());
-                                message, db);
+      cm.setChangeMessage(message);
-    return json.format(change.getId());
+      cm.send();
+    } catch (Exception e) {
+      log.error("Cannot email update for change " + change.getChangeId(), e);
+    }
+    hooks.doChangeRestoredHook(change,
+        caller.getAccount(),
+        Strings.emptyToNull(input.message),
+        dbProvider.get());
+    return json.format(change);
+  }
+
+  private ChangeMessage newMessage(Input input, IdentifiedUser caller,
+      Change change) throws OrmException {
+    StringBuilder msg = new StringBuilder();
+    msg.append("Restored");
+    if (!Strings.nullToEmpty(input.message).trim().isEmpty()) {
+      msg.append("\n\n");
+      msg.append(input.message.trim());
+    }
+
+    ChangeMessage message = new ChangeMessage(
+        new ChangeMessage.Key(
+            change.getId(),
+            ChangeUtil.messageUUID(dbProvider.get())),
+        caller.getAccountId(),
+        change.getLastUpdatedOn(),
+        change.currentPatchSetId());
+    message.setMessage(msg.toString());
+    return message;
   }
 
   private static String status(Change change) {