From 321541e6d91d804455662d33097fcd8f3b274338 Mon Sep 17 00:00:00 2001
From: Dave Borowitz <dborowitz@google.com>
Date: Tue, 2 Oct 2018 12:55:28 -0700
Subject: [PATCH] PostReviewersOp: Don't pass ChangeResource to factory method

The resource was only used within the op bodies, where it is more
correct to read the latest state from the ChangeContext rather than the
resource which was read outside of the transaction.

Change-Id: I742ae8d53659eab0cec03a1e233655f47c0cc27c
---
 .../gerrit/server/change/PostReviewers.java   |  2 +-
 .../gerrit/server/change/PostReviewersOp.java | 20 +++++++++----------
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
index 0888fd4b78..61d6020dee 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
@@ -448,7 +448,7 @@ public class PostReviewers
       caller = rsrc.getUser().asIdentifiedUser();
       op =
           postReviewersOpFactory.create(
-              rsrc, this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
+              this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
       this.exactMatchFound = exactMatchFound;
     }
 
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewersOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewersOp.java
index aed6cd018b..1309194772 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewersOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewersOp.java
@@ -63,7 +63,6 @@ public class PostReviewersOp implements BatchUpdateOp {
 
   public interface Factory {
     PostReviewersOp create(
-        ChangeResource rsrc,
         Set<Account.Id> reviewers,
         Collection<Address> reviewersByEmail,
         ReviewerState state,
@@ -100,7 +99,6 @@ public class PostReviewersOp implements BatchUpdateOp {
   private final NotesMigration migration;
   private final Provider<IdentifiedUser> user;
   private final Provider<ReviewDb> dbProvider;
-  private final ChangeResource rsrc;
   private final Set<Account.Id> reviewers;
   private final Collection<Address> reviewersByEmail;
   private final ReviewerState state;
@@ -110,6 +108,7 @@ public class PostReviewersOp implements BatchUpdateOp {
   private List<PatchSetApproval> addedReviewers = new ArrayList<>();
   private Collection<Account.Id> addedCCs = new ArrayList<>();
   private Collection<Address> addedCCsByEmail = new ArrayList<>();
+  private Change change;
   private PatchSet patchSet;
   private Result opResult;
 
@@ -124,7 +123,6 @@ public class PostReviewersOp implements BatchUpdateOp {
       NotesMigration migration,
       Provider<IdentifiedUser> user,
       Provider<ReviewDb> dbProvider,
-      @Assisted ChangeResource rsrc,
       @Assisted Set<Account.Id> reviewers,
       @Assisted Collection<Address> reviewersByEmail,
       @Assisted ReviewerState state,
@@ -140,7 +138,6 @@ public class PostReviewersOp implements BatchUpdateOp {
     this.user = user;
     this.dbProvider = dbProvider;
 
-    this.rsrc = rsrc;
     this.reviewers = reviewers;
     this.reviewersByEmail = reviewersByEmail;
     this.state = state;
@@ -151,11 +148,12 @@ public class PostReviewersOp implements BatchUpdateOp {
   @Override
   public boolean updateChange(ChangeContext ctx)
       throws RestApiException, OrmException, IOException {
+    change = ctx.getChange();
     if (!reviewers.isEmpty()) {
       if (migration.readChanges() && state == CC) {
         addedCCs =
             approvalsUtil.addCcs(
-                ctx.getNotes(), ctx.getUpdate(ctx.getChange().currentPatchSetId()), reviewers);
+                ctx.getNotes(), ctx.getUpdate(change.currentPatchSetId()), reviewers);
         if (addedCCs.isEmpty()) {
           return false;
         }
@@ -166,9 +164,9 @@ public class PostReviewersOp implements BatchUpdateOp {
                 ctx.getNotes(),
                 ctx.getUpdate(ctx.getChange().currentPatchSetId()),
                 projectCache
-                    .checkedGet(rsrc.getProject())
-                    .getLabelTypes(rsrc.getChange().getDest(), ctx.getUser()),
-                rsrc.getChange(),
+                    .checkedGet(change.getProject())
+                    .getLabelTypes(change.getDest(), ctx.getUser()),
+                change,
                 reviewers);
         if (addedReviewers.isEmpty()) {
           return false;
@@ -181,7 +179,7 @@ public class PostReviewersOp implements BatchUpdateOp {
           .putReviewerByEmail(a, ReviewerStateInternal.fromReviewerState(state));
     }
 
-    patchSet = psUtil.current(dbProvider.get(), rsrc.getNotes());
+    patchSet = psUtil.current(dbProvider.get(), ctx.getNotes());
     return true;
   }
 
@@ -193,7 +191,7 @@ public class PostReviewersOp implements BatchUpdateOp {
             .setAddedCCs(ImmutableList.copyOf(addedCCs))
             .build();
     emailReviewers(
-        rsrc.getChange(),
+        change,
         Lists.transform(addedReviewers, r -> r.getAccountId()),
         addedCCs == null ? ImmutableList.of() : addedCCs,
         reviewersByEmail,
@@ -206,7 +204,7 @@ public class PostReviewersOp implements BatchUpdateOp {
               .stream()
               .map(r -> accountCache.get(r.getAccountId()).getAccount())
               .collect(toList());
-      reviewerAdded.fire(rsrc.getChange(), patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
+      reviewerAdded.fire(change, patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
     }
   }