Convert PostReview to use BatchUpdate

This was fairly straightforward: instance state from PostReview moved
into its Op implementation, and we no longer have to manage
transactions, batch updates, and reindexing.

Change-Id: Id9d5c86401a9f1445cc515cfa7843528cffab95c

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java

@@ -150,7 +150,7 @@ class RevisionApiImpl implements RevisionApi {
   public void review(ReviewInput in) throws RestApiException {
     try {
       review.get().apply(revision, in);
-    } catch (OrmException | IOException e) {
+    } catch (OrmException | UpdateException e) {
       throw new RestApiException("Cannot post review", e);
     }
   }

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

@@ -14,10 +14,10 @@
 
 package com.google.gerrit.server.change;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId;
 
-import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
@@ -36,6 +36,7 @@ import com.google.gerrit.extensions.client.Side;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.extensions.restapi.Url;
@@ -44,6 +45,7 @@ import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.CommentRange;
 import com.google.gerrit.reviewdb.client.Patch;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
+import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
@@ -52,7 +54,10 @@ import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.PatchLineCommentsUtil;
 import com.google.gerrit.server.account.AccountsCollection;
-import com.google.gerrit.server.index.ChangeIndexer;
+import com.google.gerrit.server.git.BatchUpdate;
+import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
+import com.google.gerrit.server.git.BatchUpdate.Context;
+import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.project.ChangeControl;
@@ -61,18 +66,21 @@ import com.google.gerrit.server.util.LabelVote;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.google.inject.Singleton;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.io.IOException;
 import java.sql.Timestamp;
+import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+@Singleton
 public class PostReview implements RestModifyView<RevisionResource, ReviewInput> {
   private static final Logger log = LoggerFactory.getLogger(PostReview.class);
 
@@ -81,47 +89,37 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
   }
 
   private final Provider<ReviewDb> db;
+  private final BatchUpdate.Factory batchUpdateFactory;
   private final ChangesCollection changes;
   private final ChangeData.Factory changeDataFactory;
-  private final ChangeUpdate.Factory updateFactory;
   private final ApprovalsUtil approvalsUtil;
   private final ChangeMessagesUtil cmUtil;
   private final PatchLineCommentsUtil plcUtil;
   private final PatchListCache patchListCache;
-  private final ChangeIndexer indexer;
   private final AccountsCollection accounts;
   private final EmailReviewComments.Factory email;
   private final ChangeHooks hooks;
 
-  private Change change;
-  private ChangeMessage message;
-  private Timestamp timestamp;
-  private List<PatchLineComment> comments = Lists.newArrayList();
-  private List<String> labelDelta = Lists.newArrayList();
-  private Map<String, Short> categories = Maps.newHashMap();
-
   @Inject
   PostReview(Provider<ReviewDb> db,
+      BatchUpdate.Factory batchUpdateFactory,
       ChangesCollection changes,
       ChangeData.Factory changeDataFactory,
-      ChangeUpdate.Factory updateFactory,
       ApprovalsUtil approvalsUtil,
       ChangeMessagesUtil cmUtil,
       PatchLineCommentsUtil plcUtil,
       PatchListCache patchListCache,
-      ChangeIndexer indexer,
       AccountsCollection accounts,
       EmailReviewComments.Factory email,
       ChangeHooks hooks) {
     this.db = db;
+    this.batchUpdateFactory = batchUpdateFactory;
     this.changes = changes;
     this.changeDataFactory = changeDataFactory;
-    this.updateFactory = updateFactory;
     this.plcUtil = plcUtil;
     this.patchListCache = patchListCache;
     this.approvalsUtil = approvalsUtil;
     this.cmUtil = cmUtil;
-    this.indexer = indexer;
     this.accounts = accounts;
     this.email = email;
     this.hooks = hooks;
@@ -129,16 +127,12 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
 
   @Override
   public Output apply(RevisionResource revision, ReviewInput input)
-      throws AuthException, BadRequestException, ResourceConflictException,
+      throws RestApiException, UpdateException, OrmException {
-      UnprocessableEntityException, OrmException, IOException {
     return apply(revision, input, TimeUtil.nowTs());
   }
 
   public Output apply(RevisionResource revision, ReviewInput input,
-      Timestamp ts) throws AuthException, BadRequestException,
+      Timestamp ts) throws RestApiException, UpdateException, OrmException {
-      ResourceConflictException, UnprocessableEntityException, OrmException,
-      IOException {
-    timestamp = ts;
     if (revision.getEdit().isPresent()) {
       throw new ResourceConflictException("cannot post review on edit");
     }
@@ -156,46 +150,15 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       input.notify = NotifyHandling.NONE;
     }
 
-    db.get().changes().beginTransaction(revision.getChange().getId());
+    try (BatchUpdate bu = batchUpdateFactory.create(db.get(),
-    boolean dirty = false;
+          revision.getChange().getProject(), revision.getUser(), ts)) {
-    try {
+      bu.addOp(
-      change = db.get().changes().get(revision.getChange().getId());
+          revision.getChange().getId(),
-      if (change.getLastUpdatedOn().before(timestamp)) {
+          new Op(revision.getPatchSet().getId(), input));
-        change.setLastUpdatedOn(timestamp);
+      bu.execute();
     }
-
-      ChangeUpdate update = updateFactory.create(revision.getControl(), timestamp);
-      update.setPatchSetId(revision.getPatchSet().getId());
-      dirty |= insertComments(revision, update, input.comments, input.drafts);
-      dirty |= updateLabels(revision, update, input.labels);
-      dirty |= insertMessage(revision, input.message, update);
-      if (dirty) {
-        db.get().changes().update(Collections.singleton(change));
-        db.get().commit();
-      }
-      update.commit();
-    } finally {
-      db.get().rollback();
-    }
-
-    if (dirty) {
-      indexer.index(db.get(), change);
-    }
-    if (message != null && input.notify.compareTo(NotifyHandling.NONE) > 0) {
-      email.create(
-          input.notify,
-          change,
-          revision.getPatchSet(),
-          revision.getAccountId(),
-          message,
-          comments).sendAsync();
-    }
-
     Output output = new Output();
     output.labels = input.labels;
-    if (message != null) {
-      fireCommentAddedHook(revision);
-    }
     return output;
   }
 
@@ -336,26 +299,82 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
     }
   }
 
-  private boolean insertComments(RevisionResource rsrc,
+  private class Op extends BatchUpdate.Op {
-      ChangeUpdate update, Map<String, List<CommentInput>> in, DraftHandling draftsHandling)
+    private final PatchSet.Id psId;
-      throws OrmException {
+    private final ReviewInput in;
-    if (in == null) {
+
-      in = Collections.emptyMap();
+    private IdentifiedUser user;
+    private Change change;
+    private PatchSet ps;
+    private ChangeMessage message;
+    private List<PatchLineComment> comments = new ArrayList<>();
+    private List<String> labelDelta = new ArrayList<>();
+    private Map<String, Short> categories = new HashMap<>();
+
+    private Op(PatchSet.Id psId, ReviewInput in) {
+      this.psId = psId;
+      this.in = in;
+    }
+
+    @Override
+    public void updateChange(ChangeContext ctx) throws OrmException {
+      user = (IdentifiedUser) ctx.getUser();
+      change = ctx.getChange();
+      if (change.getLastUpdatedOn().before(ctx.getWhen())) {
+        change.setLastUpdatedOn(ctx.getWhen());
+      }
+      ps = ctx.getDb().patchSets().get(psId);
+      ctx.getChangeUpdate().setPatchSetId(psId);
+      boolean dirty = false;
+      dirty |= insertComments(ctx);
+      dirty |= updateLabels(ctx);
+      dirty |= insertMessage(ctx);
+      if (dirty) {
+        ctx.getDb().changes().update(Collections.singleton(change));
+      }
+    }
+
+    @Override
+    public void postUpdate(Context ctx) {
+      if (message == null) {
+        return;
+      }
+      if (in.notify.compareTo(NotifyHandling.NONE) > 0) {
+        email.create(
+            in.notify,
+            change,
+            ps,
+            user.getAccountId(),
+            message,
+            comments).sendAsync();
+      }
+      try {
+        hooks.doCommentAddedHook(change, user.getAccount(), ps,
+            message.getMessage(), categories, ctx.getDb());
+      } catch (OrmException e) {
+        log.warn("ChangeHook.doCommentAddedHook delivery failed", e);
+      }
+    }
+
+    private boolean insertComments(ChangeContext ctx) throws OrmException {
+      Map<String, List<CommentInput>> map = in.comments;
+      if (map == null) {
+        map = Collections.emptyMap();
       }
 
       Map<String, PatchLineComment> drafts = Collections.emptyMap();
-    if (!in.isEmpty() || draftsHandling != DraftHandling.KEEP) {
+      if (!map.isEmpty() || in.drafts != DraftHandling.KEEP) {
-      if (draftsHandling == DraftHandling.PUBLISH_ALL_REVISIONS) {
+        if (in.drafts == DraftHandling.PUBLISH_ALL_REVISIONS) {
-        drafts = changeDrafts(rsrc);
+          drafts = changeDrafts(ctx);
         } else {
-        drafts = patchSetDrafts(rsrc);
+          drafts = patchSetDrafts(ctx);
         }
       }
 
       List<PatchLineComment> del = Lists.newArrayList();
       List<PatchLineComment> ups = Lists.newArrayList();
 
-    for (Map.Entry<String, List<CommentInput>> ent : in.entrySet()) {
+      for (Map.Entry<String, List<CommentInput>> ent : map.entrySet()) {
         String path = ent.getKey();
         for (CommentInput c : ent.getValue()) {
           String parent = Url.decode(c.inReplyTo);
@@ -363,18 +382,18 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
           if (e == null) {
             e = new PatchLineComment(
                 new PatchLineComment.Key(
-                  new Patch.Key(rsrc.getPatchSet().getId(), path),
+                    new Patch.Key(psId, path),
-                  ChangeUtil.messageUUID(db.get())),
+                    ChangeUtil.messageUUID(ctx.getDb())),
                 c.line != null ? c.line : 0,
-              rsrc.getAccountId(),
+                user.getAccountId(),
-              parent, timestamp);
+                parent, ctx.getWhen());
           } else if (parent != null) {
             e.setParentUuid(parent);
           }
           e.setStatus(PatchLineComment.Status.PUBLISHED);
-        e.setWrittenOn(timestamp);
+          e.setWrittenOn(ctx.getWhen());
           e.setSide(c.side == Side.PARENT ? (short) 0 : (short) 1);
-        setCommentRevId(e, patchListCache, rsrc.getChange(), rsrc.getPatchSet());
+          setCommentRevId(e, patchListCache, ctx.getChange(), ps);
           e.setMessage(c.message);
           if (c.range != null) {
             e.setRange(new CommentRange(
@@ -388,7 +407,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         }
       }
 
-    switch (MoreObjects.firstNonNull(draftsHandling, DraftHandling.DELETE)) {
+      switch (firstNonNull(in.drafts, DraftHandling.DELETE)) {
         case KEEP:
         default:
           break;
@@ -399,53 +418,54 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         case PUBLISH_ALL_REVISIONS:
           for (PatchLineComment e : drafts.values()) {
             e.setStatus(PatchLineComment.Status.PUBLISHED);
-          e.setWrittenOn(timestamp);
+            e.setWrittenOn(ctx.getWhen());
-          setCommentRevId(e, patchListCache, rsrc.getChange(), rsrc.getPatchSet());
+            setCommentRevId(e, patchListCache, ctx.getChange(), ps);
             ups.add(e);
           }
           break;
       }
-    plcUtil.deleteComments(db.get(), update, del);
+      plcUtil.deleteComments(ctx.getDb(), ctx.getChangeUpdate(), del);
-    plcUtil.upsertComments(db.get(), update, ups);
+      plcUtil.upsertComments(ctx.getDb(), ctx.getChangeUpdate(), ups);
       comments.addAll(ups);
       return !del.isEmpty() || !ups.isEmpty();
     }
 
-  private Map<String, PatchLineComment> changeDrafts(RevisionResource rsrc)
+    private Map<String, PatchLineComment> changeDrafts(ChangeContext ctx)
         throws OrmException {
       Map<String, PatchLineComment> drafts = Maps.newHashMap();
       for (PatchLineComment c : plcUtil.draftByChangeAuthor(
-        db.get(), rsrc.getNotes(), rsrc.getAccountId())) {
+          ctx.getDb(), ctx.getChangeNotes(), user.getAccountId())) {
         drafts.put(c.getKey().get(), c);
       }
       return drafts;
     }
 
-  private Map<String, PatchLineComment> patchSetDrafts(RevisionResource rsrc)
+    private Map<String, PatchLineComment> patchSetDrafts(ChangeContext ctx)
         throws OrmException {
       Map<String, PatchLineComment> drafts = Maps.newHashMap();
-    for (PatchLineComment c : plcUtil.draftByPatchSetAuthor(db.get(),
+      for (PatchLineComment c : plcUtil.draftByPatchSetAuthor(ctx.getDb(),
-        rsrc.getPatchSet().getId(), rsrc.getAccountId(), rsrc.getNotes())) {
+          psId, user.getAccountId(), ctx.getChangeNotes())) {
         drafts.put(c.getKey().get(), c);
       }
       return drafts;
     }
 
-  private boolean updateLabels(RevisionResource rsrc, ChangeUpdate update,
+    private boolean updateLabels(ChangeContext ctx) throws OrmException {
-      Map<String, Short> labels) throws OrmException {
+      Map<String, Short> labels = in.labels;
       if (labels == null) {
         labels = Collections.emptyMap();
       }
 
       List<PatchSetApproval> del = Lists.newArrayList();
       List<PatchSetApproval> ups = Lists.newArrayList();
-    Map<String, PatchSetApproval> current = scanLabels(rsrc, del);
+      Map<String, PatchSetApproval> current = scanLabels(ctx, del);
 
-    LabelTypes labelTypes = rsrc.getControl().getLabelTypes();
+      ChangeUpdate update = ctx.getChangeUpdate();
+      LabelTypes labelTypes = ctx.getChangeControl().getLabelTypes();
       for (Map.Entry<String, Short> ent : labels.entrySet()) {
         String name = ent.getKey();
         LabelType lt = checkNotNull(labelTypes.byLabel(name), name);
-      if (change.getStatus().isClosed()) {
+        if (ctx.getChange().getStatus().isClosed()) {
           // TODO Allow updating some labels even when closed.
           continue;
         }
@@ -463,7 +483,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
           }
         } else if (c != null && c.getValue() != ent.getValue()) {
           c.setValue(ent.getValue());
-        c.setGranted(timestamp);
+          c.setGranted(ctx.getWhen());
           ups.add(c);
           addLabelDelta(normName, c.getValue());
           categories.put(normName, c.getValue());
@@ -472,11 +492,11 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
           current.put(normName, c);
         } else if (c == null) {
           c = new PatchSetApproval(new PatchSetApproval.Key(
-                rsrc.getPatchSet().getId(),
+                  psId,
-                rsrc.getAccountId(),
+                  user.getAccountId(),
                   lt.getLabelId()),
               ent.getValue(), TimeUtil.nowTs());
-        c.setGranted(timestamp);
+          c.setGranted(ctx.getWhen());
           ups.add(c);
           addLabelDelta(normName, c.getValue());
           categories.put(normName, c.getValue());
@@ -484,13 +504,13 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         }
       }
 
-    forceCallerAsReviewer(rsrc, current, ups, del);
+      forceCallerAsReviewer(ctx, current, ups, del);
-    db.get().patchSetApprovals().delete(del);
+      ctx.getDb().patchSetApprovals().delete(del);
-    db.get().patchSetApprovals().upsert(ups);
+      ctx.getDb().patchSetApprovals().upsert(ups);
       return !del.isEmpty() || !ups.isEmpty();
     }
 
-  private void forceCallerAsReviewer(RevisionResource rsrc,
+    private void forceCallerAsReviewer(ChangeContext ctx,
         Map<String, PatchSetApproval> current, List<PatchSetApproval> ups,
         List<PatchSetApproval> del) {
       if (current.isEmpty() && ups.isEmpty()) {
@@ -499,33 +519,32 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
           // If no existing label is being set to 0, hack in the caller
           // as a reviewer by picking the first server-wide LabelType.
           PatchSetApproval c = new PatchSetApproval(new PatchSetApproval.Key(
-            rsrc.getPatchSet().getId(),
+              psId,
-            rsrc.getAccountId(),
+              user.getAccountId(),
-            rsrc.getControl().getLabelTypes().getLabelTypes().get(0)
+              ctx.getChangeControl().getLabelTypes().getLabelTypes().get(0)
                   .getLabelId()),
               (short) 0, TimeUtil.nowTs());
-        c.setGranted(timestamp);
+          c.setGranted(ctx.getWhen());
           ups.add(c);
         } else {
           // Pick a random label that is about to be deleted and keep it.
           Iterator<PatchSetApproval> i = del.iterator();
           PatchSetApproval c = i.next();
           c.setValue((short) 0);
-        c.setGranted(timestamp);
+          c.setGranted(ctx.getWhen());
           i.remove();
           ups.add(c);
         }
       }
     }
 
-  private Map<String, PatchSetApproval> scanLabels(RevisionResource rsrc,
+    private Map<String, PatchSetApproval> scanLabels(ChangeContext ctx,
         List<PatchSetApproval> del) throws OrmException {
-    LabelTypes labelTypes = rsrc.getControl().getLabelTypes();
+      LabelTypes labelTypes = ctx.getChangeControl().getLabelTypes();
       Map<String, PatchSetApproval> current = Maps.newHashMap();
 
       for (PatchSetApproval a : approvalsUtil.byPatchSetUser(
-        db.get(), rsrc.getControl(), rsrc.getPatchSet().getId(),
+          ctx.getDb(), ctx.getChangeControl(), psId, user.getAccountId())) {
-        rsrc.getAccountId())) {
         if (a.isSubmit()) {
           continue;
         }
@@ -540,13 +559,9 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       return current;
     }
 
-  private void addLabelDelta(String name, short value) {
+    private boolean insertMessage(ChangeContext ctx)
-    labelDelta.add(LabelVote.create(name, value).format());
+        throws OrmException {
-  }
+      String msg = Strings.nullToEmpty(in.message).trim();
-
-  private boolean insertMessage(RevisionResource rsrc, String msg,
-      ChangeUpdate update) throws OrmException {
-    msg = Strings.nullToEmpty(msg).trim();
 
       StringBuilder buf = new StringBuilder();
       for (String d : labelDelta) {
@@ -565,28 +580,21 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       }
 
       message = new ChangeMessage(
-        new ChangeMessage.Key(change.getId(), ChangeUtil.messageUUID(db.get())),
+          new ChangeMessage.Key(
-        rsrc.getAccountId(),
+            psId.getParentKey(), ChangeUtil.messageUUID(ctx.getDb())),
-        timestamp,
+          user.getAccountId(),
-        rsrc.getPatchSet().getId());
+          ctx.getWhen(),
+          psId);
       message.setMessage(String.format(
           "Patch Set %d:%s",
-        rsrc.getPatchSet().getPatchSetId(),
+          psId.get(),
           buf.toString()));
-    cmUtil.addChangeMessage(db.get(), update, message);
+      cmUtil.addChangeMessage(ctx.getDb(), ctx.getChangeUpdate(), message);
       return true;
     }
 
-  private void fireCommentAddedHook(RevisionResource rsrc) {
+    private void addLabelDelta(String name, short value) {
-    IdentifiedUser user = (IdentifiedUser) rsrc.getControl().getCurrentUser();
+      labelDelta.add(LabelVote.create(name, value).format());
-    try {
-      hooks.doCommentAddedHook(change,
-          user.getAccount(),
-          rsrc.getPatchSet(),
-          message.getMessage(),
-          categories, db.get());
-    } catch (OrmException e) {
-      log.warn("ChangeHook.doCommentAddedHook delivery failed", e);
     }
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java

@@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.index.ChangeIndexer;
+import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.inject.assistedinject.Assisted;
@@ -133,6 +134,10 @@ public class BatchUpdate implements AutoCloseable {
       return update;
     }
 
+    public ChangeNotes getChangeNotes() {
+      return update.getChangeNotes();
+    }
+
     public ChangeControl getChangeControl() {
       return ctl;
     }