BatchUpdate: Create ChangeControl inside transaction

Previously, we were passing in ChangeControls to addOp, which
contained their own defensive copy of the Change object. Callers
already have that ChangeControl instance available, so they might be
tempted to read/write the Change within it inside the body of a
BatchUpdate.Op, which doesn't do what they expect.

Instead, just pass in the Change.Id, and hoist the user setup into the
BatchUpdate creation itself. (This has the side effect of preventing a
single batch update from performing updates on behalf of multiple
users, which is ok.) Push the ChangeControl creation into the
BatchUpdate internals, and use the change that was read in the
transaction rather than depending on a change previously read not in
the transaction and passed in by the caller (via ChangeControl).

This improves consistency in another important way: the Change
instance accessed via ctx.getChangeUpdate().getChange() (aka
ctx.getChangeUpdate().getChangeNotes().getChange()) was read in the
same transaction as the change accessed via ctx.getChange(). (They are
currently still two different instances due to the aforementioned
defensive copy, but that is a quirk we will deal with as we continue
working on notedb update semantics.)

Change-Id: Ifab569aece17a4bcf18b180f2b90b2d5daa3c4ac

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

@@ -93,9 +93,10 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
       final String msgTxt, final Account account)
       throws RestApiException, UpdateException {
     Op op = new Op(msgTxt, account);
+    Change c = control.getChange();
     try (BatchUpdate u = batchUpdateFactory.create(dbProvider.get(),
-        control.getChange().getProject(), TimeUtil.nowTs())) {
+        c.getProject(), control.getCurrentUser(), TimeUtil.nowTs())) {
-      u.addOp(control, op).execute();
+      u.addOp(c.getId(), op).execute();
     }
     return op.change;
   }
@@ -116,7 +117,7 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
     @Override
     public void updateChange(ChangeContext ctx) throws OrmException,
         ResourceConflictException {
-      change = ctx.readChange();
+      change = ctx.getChange();
       if (change == null || !change.getStatus().isOpen()) {
         throw new ResourceConflictException("change is " + status(change));
       } else if (change.getStatus() == Change.Status.DRAFT) {

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

@@ -215,8 +215,9 @@ public class CherryPickChange {
     PatchSet current = db.get().patchSets().get(change.currentPatchSetId());
 
     try (BatchUpdate bu = batchUpdateFactory.create(
-        db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) {
+        db.get(), change.getDest().getParentKey(), identifiedUser,
-      bu.addOp(changeControl, inserter
+        TimeUtil.nowTs())) {
+      bu.addOp(change.getId(), inserter
           .setMessage("Uploaded patch set " + newPatchSetId.get() + ".")
           .setDraft(current.isDraft())
           .setUploader(identifiedUser.getAccountId())

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

@@ -466,8 +466,10 @@ public class ConsistencyChecker {
       PatchSetInserter inserter =
           patchSetInserterFactory.create(repo, rw, ctl, commit);
       try (BatchUpdate bu = updateFactory.create(
-          db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) {
+          db.get(), change.getDest().getParentKey(), user.get(),
-        bu.addOp(ctl, inserter.setValidatePolicy(CommitValidators.Policy.NONE)
+          TimeUtil.nowTs())) {
+        bu.addOp(change.getId(), inserter
+            .setValidatePolicy(CommitValidators.Policy.NONE)
             .setRunHooks(false)
             .setSendMail(false)
             .setAllowClosed(true)

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

@@ -220,7 +220,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
   @Override
   public void updateChange(ChangeContext ctx) throws OrmException,
       InvalidChangeOperationException {
-    change = ctx.readChange();
+    change = ctx.getChange();
     Change.Id id = change.getId();
     final PatchSet.Id currentPatchSetId = change.currentPatchSetId();
     if (!change.getStatus().isOpen() && !allowClosed) {

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

@@ -78,8 +78,8 @@ public class PutTopic implements RestModifyView<ChangeResource, Input>,
 
     Op op = new Op(ctl, input != null ? input : new Input());
     try (BatchUpdate u = batchUpdateFactory.create(dbProvider.get(),
-        req.getChange().getProject(), TimeUtil.nowTs())) {
+        req.getChange().getProject(), ctl.getCurrentUser(), TimeUtil.nowTs())) {
-      u.addOp(ctl, op);
+      u.addOp(req.getChange().getId(), op);
       u.execute();
     }
     return Strings.isNullOrEmpty(op.newTopicName)
@@ -102,7 +102,7 @@ public class PutTopic implements RestModifyView<ChangeResource, Input>,
 
     @Override
     public void updateChange(ChangeContext ctx) throws OrmException {
-      change = ctx.readChange();
+      change = ctx.getChange();
       String newTopicName = Strings.nullToEmpty(input.topic);
       String oldTopicName = Strings.nullToEmpty(change.getTopic());
       if (oldTopicName.equals(newTopicName)) {

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

@@ -288,8 +288,8 @@ public class RebaseChange {
         .setRunHooks(runHooks);
 
     try (BatchUpdate bu = updateFactory.create(
-        db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) {
+        db.get(), change.getProject(), uploader, TimeUtil.nowTs())) {
-      bu.addOp(changeControl, patchSetInserter.setMessage(
+      bu.addOp(change.getId(), patchSetInserter.setMessage(
           "Patch Set " + patchSetInserter.getPatchSetId().get()
           + ": Patch Set " + patchSetId.get() + " was rebased"));
       bu.execute();

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

@@ -88,8 +88,8 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
 
     Op op = new Op(input);
     try (BatchUpdate u = batchUpdateFactory.create(dbProvider.get(),
-        req.getChange().getProject(), TimeUtil.nowTs())) {
+        req.getChange().getProject(), ctl.getCurrentUser(), TimeUtil.nowTs())) {
-      u.addOp(ctl, op).execute();
+      u.addOp(req.getChange().getId(), op).execute();
     }
     return json.create(ChangeJson.NO_OPTIONS).format(op.change);
   }
@@ -110,7 +110,7 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
     public void updateChange(ChangeContext ctx) throws OrmException,
         ResourceConflictException {
       caller = (IdentifiedUser) ctx.getUser();
-      change = ctx.readChange();
+      change = ctx.getChange();
       if (change == null || change.getStatus() != Status.ABANDONED) {
         throw new ResourceConflictException("change is " + status(change));
       }

gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java

@@ -239,8 +239,9 @@ public class ChangeEditUtil {
     }
 
     try (BatchUpdate bu = updateFactory.create(
-        db.get(), change.getDest().getParentKey(), TimeUtil.nowTs())) {
+        db.get(), change.getProject(), ctl.getCurrentUser(),
-      bu.addOp(ctl, inserter
+        TimeUtil.nowTs())) {
+      bu.addOp(change.getId(), inserter
         .setDraft(change.getStatus() == Status.DRAFT ||
             basePatchSet.isDraft())
         .setMessage(message.toString()));

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

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.git;
 
-import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
@@ -31,7 +30,6 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.project.ChangeControl;
-import com.google.gwtorm.server.OrmException;
 import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
 
@@ -46,7 +44,6 @@ import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -75,7 +72,7 @@ import java.util.Map;
 public class BatchUpdate implements AutoCloseable {
   public interface Factory {
     public BatchUpdate create(ReviewDb db, Project.NameKey project,
-        Timestamp when);
+        CurrentUser user, Timestamp when);
   }
 
   public class Context {
@@ -128,8 +125,8 @@ public class BatchUpdate implements AutoCloseable {
       return update;
     }
 
-    public Change readChange() throws OrmException {
+    public Change getChange() {
-      return db.changes().get(update.getChange().getId());
+      return update.getChange();
     }
 
     public CurrentUser getUser() {
@@ -155,14 +152,15 @@ public class BatchUpdate implements AutoCloseable {
   private final ReviewDb db;
   private final GitRepositoryManager repoManager;
   private final ChangeIndexer indexer;
+  private final ChangeControl.GenericFactory changeControlFactory;
   private final ChangeUpdate.Factory changeUpdateFactory;
   private final GitReferenceUpdated gitRefUpdated;
 
   private final Project.NameKey project;
+  private final CurrentUser user;
   private final Timestamp when;
 
   private final ListMultimap<Change.Id, Op> ops = ArrayListMultimap.create();
-  private final Map<Change.Id, ChangeControl> changeControls = new HashMap<>();
   private final List<CheckedFuture<?, IOException>> indexFutures =
       new ArrayList<>();
 
@@ -175,17 +173,21 @@ public class BatchUpdate implements AutoCloseable {
   @AssistedInject
   BatchUpdate(GitRepositoryManager repoManager,
       ChangeIndexer indexer,
+      ChangeControl.GenericFactory changeControlFactory,
       ChangeUpdate.Factory changeUpdateFactory,
       GitReferenceUpdated gitRefUpdated,
       @Assisted ReviewDb db,
       @Assisted Project.NameKey project,
+      @Assisted CurrentUser user,
       @Assisted Timestamp when) {
     this.db = db;
     this.repoManager = repoManager;
     this.indexer = indexer;
+    this.changeControlFactory = changeControlFactory;
     this.changeUpdateFactory = changeUpdateFactory;
     this.gitRefUpdated = gitRefUpdated;
     this.project = project;
+    this.user = user;
     this.when = when;
   }
 
@@ -232,14 +234,8 @@ public class BatchUpdate implements AutoCloseable {
     return inserter;
   }
 
-  public BatchUpdate addOp(ChangeControl ctl, Op op) {
+  public BatchUpdate addOp(Change.Id id, Op op) {
-    Change.Id id = ctl.getChange().getId();
-    ChangeControl old = changeControls.get(id);
-    // TODO(dborowitz): Not sure this is guaranteed in general.
-    checkArgument(old == null || old == ctl,
-        "mismatched ChangeControls for change %s", id);
     ops.put(id, op);
-    changeControls.put(id, ctl);
     return this;
   }
 
@@ -303,9 +299,10 @@ public class BatchUpdate implements AutoCloseable {
     try {
       for (Map.Entry<Change.Id, Collection<Op>> e : ops.asMap().entrySet()) {
         Change.Id id = e.getKey();
-        ChangeUpdate update =
-            changeUpdateFactory.create(changeControls.get(id), when);
         db.changes().beginTransaction(id);
+        Change change = db.changes().get(id);
+        ChangeControl ctl = changeControlFactory.controlFor(change, user);
+        ChangeUpdate update = changeUpdateFactory.create(ctl, when);
         try {
           for (Op op : e.getValue()) {
             op.updateChange(new ChangeContext(update));

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

@@ -72,14 +72,15 @@ public class CherryPick extends SubmitStrategy {
     try (BatchUpdate u = args.newBatchUpdate(TimeUtil.nowTs())) {
       while (!sorted.isEmpty()) {
         CodeReviewCommit n = sorted.remove(0);
+        Change.Id cid = n.change().getId();
         if (first && branchTip == null) {
-          u.addOp(n.getControl(), new CherryPickUnbornRootOp(mergeTip, n));
+          u.addOp(cid, new CherryPickUnbornRootOp(mergeTip, n));
         } else if (n.getParentCount() == 0) {
-          u.addOp(n.getControl(), new CherryPickRootOp(n));
+          u.addOp(cid, new CherryPickRootOp(n));
         } else if (n.getParentCount() == 1) {
-          u.addOp(n.getControl(), new CherryPickOneOp(mergeTip, n));
+          u.addOp(cid, new CherryPickOneOp(mergeTip, n));
         } else {
-          u.addOp(n.getControl(), new CherryPickMultipleParentsOp(mergeTip, n));
+          u.addOp(cid, new CherryPickMultipleParentsOp(mergeTip, n));
         }
         first = false;
       }

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

@@ -100,7 +100,8 @@ public abstract class SubmitStrategy {
     }
 
     BatchUpdate newBatchUpdate(Timestamp when) {
-      return batchUpdateFactory.create(db, destBranch.getParentKey(), when)
+      return batchUpdateFactory
+          .create(db, destBranch.getParentKey(), caller, when)
           .setRepository(repo, rw, inserter);
     }
   }

gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java

@@ -1336,8 +1336,8 @@ public abstract class AbstractQueryChangesTest {
         .setRunHooks(false)
         .setValidatePolicy(CommitValidators.Policy.NONE);
     try (BatchUpdate bu = updateFactory.create(
-        db, c.getDest().getParentKey(), TimeUtil.nowTs())) {
+        db, c.getDest().getParentKey(), user, TimeUtil.nowTs())) {
-      bu.addOp(ctl, inserter);
+      bu.addOp(c.getId(), inserter);
       bu.execute();
     }