BatchUpdate: Key ChangeUpdates by patch set

Allow callers to create one update per patch set of the change,
looking up by patch set ID and creating as necessary. BatchUpdate will
take care of creating a BatchMetaDataUpdate and applying the updates
in order, oldest patch set to newest patch set.

Force callers to specify the patch set instead of implicitly using the
current patch set. There were some places we were forgetting to do
this, so it's good to have made it required.

We will eventually need to update multiple patch sets at once during
submit, where approvals may be copied between patch sets.

Change-Id: I35e9378d6f9b494db516f8d8c38c5b6e75c2f4c7

gerrit-reviewdb/BUCK

@@ -19,6 +19,7 @@ java_library(
   resources = glob(['src/main/resources/**/*']),
   deps = [
     '//gerrit-extension-api:api',
+    '//lib:guava',
     '//lib:gwtorm',
   ],
   visibility = ['PUBLIC'],

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java

@@ -0,0 +1,41 @@
+// Copyright (C) 2016 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.reviewdb.server;
+
+import com.google.common.base.Function;
+import com.google.common.collect.Ordering;
+import com.google.gwtorm.client.IntKey;
+
+/** Static utilities for ReviewDb types. */
+public class ReviewDbUtil {
+  private static final Function<IntKey<?>, Integer> INT_KEY_FUNCTION =
+      new Function<IntKey<?>, Integer>() {
+        @Override
+        public Integer apply(IntKey<?> in) {
+          return in.get();
+        }
+      };
+
+  private static final Ordering<? extends IntKey<?>> INT_KEY_ORDERING =
+      Ordering.natural().onResultOf(INT_KEY_FUNCTION);
+
+  @SuppressWarnings("unchecked")
+  public static <K extends IntKey<?>> Ordering<K> intKeyOrdering() {
+    return (Ordering<K>) INT_KEY_ORDERING;
+  }
+
+  private ReviewDbUtil() {
+  }
+}

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

@@ -119,14 +119,15 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
     public void updateChange(ChangeContext ctx) throws OrmException,
         ResourceConflictException {
       change = ctx.getChange();
-      ChangeUpdate update = ctx.getUpdate();
+      PatchSet.Id psId = change.currentPatchSetId();
+      ChangeUpdate update = ctx.getUpdate(psId);
       if (change == null || !change.getStatus().isOpen()) {
         throw new ResourceConflictException("change is " + status(change));
       } else if (change.getStatus() == Change.Status.DRAFT) {
         throw new ResourceConflictException(
             "draft changes cannot be abandoned");
       }
-      patchSet = ctx.getDb().patchSets().get(change.currentPatchSetId());
+      patchSet = ctx.getDb().patchSets().get(psId);
       change.setStatus(Change.Status.ABANDONED);
       change.setLastUpdatedOn(ctx.getWhen());
       ctx.getDb().changes().update(Collections.singleton(change));

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

@@ -242,10 +242,10 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
     change = ctx.getChange(); // Use defensive copy created by ChangeControl.
     ReviewDb db = ctx.getDb();
     ChangeControl ctl = ctx.getControl();
-    ChangeUpdate update = ctx.getUpdate();
     patchSetInfo = patchSetInfoFactory.get(
         ctx.getRevWalk(), commit, patchSet.getId());
     ctx.getChange().setCurrentPatchSet(patchSetInfo);
+    ChangeUpdate update = ctx.getUpdate(patchSet.getId());
 
     if (patchSet.getGroups() == null) {
       patchSet.setGroups(GroupCollector.getDefaultGroups(patchSet));

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

@@ -121,7 +121,7 @@ public class CreateDraftComment implements RestModifyView<RevisionResource, Draf
       setCommentRevId(
           comment, patchListCache, ctx.getChange(), ps);
       plcUtil.insertComments(
-          ctx.getDb(), ctx.getUpdate(), Collections.singleton(comment));
+          ctx.getDb(), ctx.getUpdate(psId), Collections.singleton(comment));
     }
   }
 }

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

@@ -97,7 +97,7 @@ public class DeleteDraftComment
       PatchLineComment c = maybeComment.get();
       setCommentRevId(c, patchListCache, ctx.getChange(), ps);
       plcUtil.deleteComments(
-          ctx.getDb(), ctx.getUpdate(), Collections.singleton(c));
+          ctx.getDb(), ctx.getUpdate(psId), Collections.singleton(c));
     }
   }
 }

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

@@ -111,8 +111,7 @@ public class DeleteVote implements RestModifyView<VoteResource, Input> {
                 .append("\n");
             psa = a;
             a.setValue((short)0);
-            ctx.getUpdate().setPatchSetId(psId);
+            ctx.getUpdate(psId).removeApprovalFor(a.getAccountId(), label);
-            ctx.getUpdate().removeApprovalFor(a.getAccountId(), label);
             break;
           }
         } else {
@@ -133,7 +132,7 @@ public class DeleteVote implements RestModifyView<VoteResource, Input> {
                 ctx.getWhen(),
                 change.currentPatchSetId());
         changeMessage.setMessage(msg.toString());
-        cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(),
+        cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId),
             changeMessage);
       }
     }

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

@@ -209,7 +209,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
     ChangeControl ctl = ctx.getControl();
 
     change = ctx.getChange();
-    ChangeUpdate update = ctx.getUpdate();
+    ChangeUpdate update = ctx.getUpdate(psId);
 
     if (!change.getStatus().isOpen() && !allowClosed) {
       throw new InvalidChangeOperationException(String.format(
@@ -222,8 +222,6 @@ public class PatchSetInserter extends BatchUpdate.Op {
     patchSet.setRevision(new RevId(commit.name()));
     patchSet.setDraft(draft);
 
-    update.setPatchSetId(patchSet.getId());
-
     if (groups != null) {
       patchSet.setGroups(groups);
     } else {
@@ -251,7 +249,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
     db.changes().update(Collections.singleton(change));
     approvalCopier.copy(db, ctl, patchSet);
     if (changeMessage != null) {
-      cmUtil.addChangeMessage(db, ctx.getUpdate(), changeMessage);
+      cmUtil.addChangeMessage(db, update, changeMessage);
     }
   }
 

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

@@ -358,7 +358,6 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         change.setLastUpdatedOn(ctx.getWhen());
       }
       ps = ctx.getDb().patchSets().get(psId);
-      ctx.getUpdate().setPatchSetId(psId);
       boolean dirty = false;
       dirty |= insertComments(ctx);
       dirty |= updateLabels(ctx);
@@ -466,8 +465,11 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
           }
           break;
       }
-      plcUtil.deleteComments(ctx.getDb(), ctx.getUpdate(), del);
+      ChangeUpdate u = ctx.getUpdate(psId);
-      plcUtil.upsertComments(ctx.getDb(), ctx.getUpdate(), ups);
+      // TODO(dborowitz): Currently doesn't work for PUBLISH_ALL_REVISIONS with
+      // notedb.
+      plcUtil.deleteComments(ctx.getDb(), u, del);
+      plcUtil.upsertComments(ctx.getDb(), u, ups);
       comments.addAll(ups);
       return !del.isEmpty() || !ups.isEmpty();
     }
@@ -513,7 +515,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       List<PatchSetApproval> ups = Lists.newArrayList();
       Map<String, PatchSetApproval> current = scanLabels(ctx, del);
 
-      ChangeUpdate update = ctx.getUpdate();
+      ChangeUpdate update = ctx.getUpdate(psId);
       LabelTypes labelTypes = ctx.getControl().getLabelTypes();
       for (Map.Entry<String, Short> ent : labels.entrySet()) {
         String name = ent.getKey();
@@ -644,7 +646,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
           "Patch Set %d:%s",
           psId.get(),
           buf.toString()));
-      cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(), message);
+      cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), message);
       return true;
     }
 

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

@@ -182,7 +182,7 @@ public class PublishDraftPatchSet implements RestModifyView<RevisionResource, In
 
     private void saveChange(ChangeContext ctx) throws OrmException {
       change = ctx.getChange();
-      ChangeUpdate update = ctx.getUpdate();
+      ChangeUpdate update = ctx.getUpdate(psId);
       wasDraftChange = change.getStatus() == Change.Status.DRAFT;
       if (wasDraftChange) {
         change.setStatus(Change.Status.NEW);
@@ -219,7 +219,7 @@ public class PublishDraftPatchSet implements RestModifyView<RevisionResource, In
       recipients =
           getRecipientsFromFooters(accountResolver, patchSet, footerLines);
       recipients.remove(ctx.getUser().getAccountId());
-      approvalsUtil.addReviewers(ctx.getDb(), ctx.getUpdate(), labelTypes,
+      approvalsUtil.addReviewers(ctx.getDb(), ctx.getUpdate(psId), labelTypes,
           change, patchSet, patchSetInfo, recipients.getReviewers(),
           oldReviewers);
     }

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

@@ -35,6 +35,7 @@ import com.google.gerrit.server.PatchLineCommentsUtil;
 import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.git.UpdateException;
+import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -116,6 +117,7 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
       comment = maybeComment.get();
 
       PatchSet.Id psId = comment.getKey().getParentKey().getParentKey();
+      ChangeUpdate update = ctx.getUpdate(psId);
       PatchSet ps = ctx.getDb().patchSets().get(psId);
       if (ps == null) {
         throw new ResourceNotFoundException("patch set not found: " + psId);
@@ -126,7 +128,7 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
         // Delete then recreate the comment instead of an update.
 
         plcUtil.deleteComments(
-            ctx.getDb(), ctx.getUpdate(), Collections.singleton(comment));
+            ctx.getDb(), update, Collections.singleton(comment));
         comment = new PatchLineComment(
             new PatchLineComment.Key(
                 new Patch.Key(psId, in.path),
@@ -135,14 +137,14 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
             ctx.getUser().getAccountId(),
             comment.getParentUuid(), ctx.getWhen());
         setCommentRevId(comment, patchListCache, ctx.getChange(), ps);
-        plcUtil.insertComments(ctx.getDb(), ctx.getUpdate(),
+        plcUtil.insertComments(ctx.getDb(), update,
             Collections.singleton(update(comment, in)));
       } else {
         if (comment.getRevId() == null) {
           setCommentRevId(
               comment, patchListCache, ctx.getChange(), ps);
         }
-        plcUtil.updateComments(ctx.getDb(), ctx.getUpdate(),
+        plcUtil.updateComments(ctx.getDb(), update,
             Collections.singleton(update(comment, in)));
       }
     }

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

@@ -34,6 +34,7 @@ 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.project.ChangeControl;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -101,6 +102,7 @@ public class PutTopic implements RestModifyView<ChangeResource, Input>,
     @Override
     public void updateChange(ChangeContext ctx) throws OrmException {
       change = ctx.getChange();
+      ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
       newTopicName = Strings.nullToEmpty(input.topic);
       oldTopicName = Strings.nullToEmpty(change.getTopic());
       if (oldTopicName.equals(newTopicName)) {
@@ -116,7 +118,7 @@ public class PutTopic implements RestModifyView<ChangeResource, Input>,
             oldTopicName, newTopicName);
       }
       change.setTopic(Strings.emptyToNull(newTopicName));
-      ctx.getUpdate().setTopic(change.getTopic());
+      update.setTopic(change.getTopic());
       ChangeUtil.updated(change);
       ctx.getDb().changes().update(Collections.singleton(change));
 
@@ -127,7 +129,7 @@ public class PutTopic implements RestModifyView<ChangeResource, Input>,
           caller.getAccountId(), ctx.getWhen(),
           change.currentPatchSetId());
       cmsg.setMessage(summary);
-      cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(), cmsg);
+      cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
     }
 
     @Override

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

@@ -110,11 +110,12 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
         ResourceConflictException {
       caller = ctx.getUser().asIdentifiedUser();
       change = ctx.getChange();
-      ChangeUpdate update = ctx.getUpdate();
+      PatchSet.Id psId = change.currentPatchSetId();
+      ChangeUpdate update = ctx.getUpdate(psId);
       if (change == null || change.getStatus() != Status.ABANDONED) {
         throw new ResourceConflictException("change is " + status(change));
       }
-      patchSet = ctx.getDb().patchSets().get(change.currentPatchSetId());
+      patchSet = ctx.getDb().patchSets().get(psId);
       change.setStatus(Status.NEW);
       change.setLastUpdatedOn(ctx.getWhen());
       ctx.getDb().changes().update(Collections.singleton(change));

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

@@ -83,7 +83,8 @@ public class SetHashtagsOp extends BatchUpdate.Op {
     if (!ctx.getControl().canEditHashtags()) {
       throw new AuthException("Editing hashtags not permitted");
     }
-    ChangeUpdate update = ctx.getUpdate();
+    change = ctx.getChange();
+    ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
     ChangeNotes notes = update.getChangeNotes().load();
 
     Set<String> existingHashtags = notes.getHashtags();
@@ -110,7 +111,6 @@ public class SetHashtagsOp extends BatchUpdate.Op {
       update.setHashtags(updated);
     }
 
-    change = update.getChange();
     updatedHashtags = ImmutableSortedSet.copyOf(updated);
   }
 

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

@@ -25,11 +25,14 @@ import com.google.common.collect.ListMultimap;
 import com.google.common.util.concurrent.CheckedFuture;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate;
 import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -54,6 +57,7 @@ import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.TimeZone;
+import java.util.TreeMap;
 
 /**
  * Context for a set of updates that should be applied for a site.
@@ -159,20 +163,27 @@ public class BatchUpdate implements AutoCloseable {
 
   public class ChangeContext extends Context {
     private final ChangeControl ctl;
-    private final ChangeUpdate update;
+    private final Map<PatchSet.Id, ChangeUpdate> updates;
+
     private boolean deleted;
 
     private ChangeContext(ChangeControl ctl) {
       this.ctl = ctl;
-      this.update = changeUpdateFactory.create(ctl, when);
+      updates = new TreeMap<>(ReviewDbUtil.intKeyOrdering());
     }
 
-    public ChangeUpdate getUpdate() {
+    public ChangeUpdate getUpdate(PatchSet.Id psId) {
-      return update;
+      ChangeUpdate u = updates.get(psId);
+      if (u == null) {
+        u = changeUpdateFactory.create(ctl, when);
+        u.setPatchSetId(psId);
+        updates.put(psId, u);
+      }
+      return u;
     }
 
     public ChangeNotes getNotes() {
-      return update.getChangeNotes();
+      return ctl.getNotes();
     }
 
     public ChangeControl getControl() {
@@ -180,7 +191,7 @@ public class BatchUpdate implements AutoCloseable {
     }
 
     public Change getChange() {
-      return update.getChange();
+      return ctl.getChange();
     }
 
     public void markDeleted() {
@@ -523,7 +534,18 @@ public class BatchUpdate implements AutoCloseable {
         } finally {
           db.rollback();
         }
-        ctx.getUpdate().commit();
+
+        BatchMetaDataUpdate bmdu = null;
+        for (ChangeUpdate u : ctx.updates.values()) {
+          if (bmdu == null) {
+            bmdu = u.openUpdate();
+          }
+          u.writeCommit(bmdu);
+        }
+        if (bmdu != null) {
+          bmdu.commit();
+        }
+
         if (ctx.deleted) {
           indexFutures.add(indexer.deleteAsync(id));
         } else {

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

@@ -1807,7 +1807,7 @@ public class ReceiveCommits {
                 new BatchUpdate.Op() {
                   @Override
                   public void updateChange(ChangeContext ctx) throws Exception {
-                    ctx.getUpdate().setTopic(magicBranch.topic);
+                    ctx.getUpdate(ps.getId()).setTopic(magicBranch.topic);
                   }
                 });
           }

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java

@@ -167,7 +167,6 @@ public class CherryPick extends SubmitStrategy {
         // Merge conflict; don't update change.
         return;
       }
-      ctx.getUpdate().setPatchSetId(psId);
       PatchSet ps = new PatchSet(psId);
       ps.setCreatedOn(ctx.getWhen());
       ps.setUploader(args.caller.getAccountId());
@@ -183,7 +182,7 @@ public class CherryPick extends SubmitStrategy {
       for (PatchSetApproval a : args.approvalsUtil.byPatchSet(
           args.db, toMerge.getControl(), toMerge.getPatchsetId())) {
         approvals.add(new PatchSetApproval(ps.getId(), a));
-        ctx.getUpdate().putApproval(a.getLabel(), a.getValue());
+        ctx.getUpdate(psId).putApproval(a.getLabel(), a.getValue());
       }
       args.db.patchSetApprovals().insert(approvals);
 

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java

@@ -386,9 +386,6 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     BatchMetaDataUpdate batch = openUpdate();
     try {
       writeCommit(batch);
-      if (draftUpdate != null) {
-        draftUpdate.commit();
-      }
       RevCommit c = batch.commit();
       return c;
     } catch (OrmException e) {
@@ -409,6 +406,9 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       }
     }
     batch.write(this, builder);
+    if (draftUpdate != null) {
+      draftUpdate.commit();
+    }
   }
 
   @Override