commit 321541e6d91d804455662d33097fcd8f3b274338
author Dave Borowitz <dborowitz@google.com> Tue Oct 02 12:55:28 2018 -0700
committer David Pursehouse <dpursehouse@collab.net> Wed Oct 03 08:53:41 2018 +0900
tree 8ded772137da3f57d6e6d9083d86d81ca5adc78d
parent 487e8875df980979735de10e0265f65e616af331

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/src/main/java/com/google/gerrit/server/change/PostReviewers.java

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 0888fd4..61d6020 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 @@
       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;
     }
 

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

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 aed6cd0..1309194 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 interface Factory {
     PostReviewersOp create(
-        ChangeResource rsrc,
         Set<Account.Id> reviewers,
         Collection<Address> reviewersByEmail,
         ReviewerState state,
@@ -100,7 +99,6 @@
   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 @@
   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 @@
       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 @@
     this.user = user;
     this.dbProvider = dbProvider;
 
-    this.rsrc = rsrc;
     this.reviewers = reviewers;
     this.reviewersByEmail = reviewersByEmail;
     this.state = state;
@@ -151,11 +148,12 @@
   @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 @@
                 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 @@
           .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 @@
             .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 @@
               .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());
     }
   }