Avoid passing ChangeResource through PostReviewers

The end goal is to make PostReviewers usable from contexts where a
ChangeResource is not available, such as ReceiveCommits. After some
massaging, it turns out that the ChangeResource was only used for the
following reasons, all of which can be avoided:

* As a holder of change + user, which saves a bit of boilerplate in
  passing around to internal methods. This isn't actually necessary, and
  this refactoring is in general moving away from tying the
  PostReviewersOp functionality to the REST API.
* To use the ChangeNotes within the body of PostReviewersOp; this usage
  was recently removed.

Change-Id: Id9d1ed998677f056aaa9f81488b3c12f82e09e6a

java/com/google/gerrit/server/restapi/change/PostReview.java

@@ -279,7 +279,8 @@ public class PostReview
         reviewerInput.notify = NotifyHandling.NONE;
 
         PostReviewers.Addition result =
-            postReviewers.prepareApplication(revision.getChangeResource(), reviewerInput, true);
+            postReviewers.prepareApplication(
+                revision.getNotes(), revision.getUser(), reviewerInput, true);
         reviewerJsonResults.put(reviewerInput.reviewer, result.result);
         if (result.result.error != null) {
           hasError = true;
@@ -379,7 +380,8 @@ public class PostReview
       bu.execute();
 
       for (PostReviewers.Addition reviewerResult : reviewerResults) {
-        reviewerResult.gatherResults();
+        // TODO(dborowitz): Should this be re-read to take updates into account?
+        reviewerResult.gatherResults(revision.getNotes());
       }
 
       boolean readyForReview =

java/com/google/gerrit/server/restapi/change/PostReviewers.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.restapi.change;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.extensions.client.ReviewerState.CC;
 import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
 
@@ -155,7 +156,7 @@ public class PostReviewers
       throw new BadRequestException("missing reviewer field");
     }
 
-    Addition addition = prepareApplication(rsrc, input, true);
+    Addition addition = prepareApplication(rsrc.getNotes(), rsrc.getUser(), input, true);
     if (addition.op == null) {
       return addition.result;
     }
@@ -165,14 +166,31 @@ public class PostReviewers
       Change.Id id = rsrc.getChange().getId();
       bu.addOp(id, addition.op);
       bu.execute();
-      addition.gatherResults();
+      // TODO(dborowitz): Should this be re-read to take updates into account?
+      addition.gatherResults(rsrc.getNotes());
     }
     return addition.result;
   }
 
+  /**
+   * Prepare application of a single {@link AddReviewerInput}.
+   *
+   * @param notes change notes.
+   * @param user user performing the reviewer addition.
+   * @param input input describing user or group to add as a reviewer.
+   * @param allowGroup whether to allow
+   * @return handle describing the addition operation. If the {@code op} field is present, this
+   *     operation may be added to a {@code BatchUpdate}. Otherwise, the {@code error} field
+   *     contains information about an error that occurred
+   * @throws OrmException
+   * @throws IOException
+   * @throws PermissionBackendException
+   * @throws ConfigInvalidException
+   */
   public Addition prepareApplication(
-      ChangeResource rsrc, AddReviewerInput input, boolean allowGroup)
+      ChangeNotes notes, CurrentUser user, AddReviewerInput input, boolean allowGroup)
       throws OrmException, IOException, PermissionBackendException, ConfigInvalidException {
+    Branch.NameKey dest = notes.getChange().getDest();
     String reviewer = input.reviewer;
     ReviewerState state = input.state();
     NotifyHandling notify = input.notify;
@@ -185,17 +203,26 @@ public class PostReviewers
     boolean confirmed = input.confirmed();
     boolean allowByEmail =
         projectCache
-            .checkedGet(rsrc.getProject())
+            .checkedGet(dest.getParentKey())
             .is(BooleanProjectConfig.ENABLE_REVIEWER_BY_EMAIL);
 
     Addition byAccountId =
-        addByAccountId(reviewer, rsrc, state, notify, accountsToNotify, allowGroup, allowByEmail);
+        addByAccountId(
+            reviewer, dest, user, state, notify, accountsToNotify, allowGroup, allowByEmail);
 
     Addition wholeGroup = null;
     if (byAccountId == null || !byAccountId.exactMatchFound) {
       wholeGroup =
           addWholeGroup(
-              reviewer, rsrc, state, notify, accountsToNotify, confirmed, allowGroup, allowByEmail);
+              reviewer,
+              dest,
+              user,
+              state,
+              notify,
+              accountsToNotify,
+              confirmed,
+              allowGroup,
+              allowByEmail);
       if (wholeGroup != null && wholeGroup.exactMatchFound) {
         return wholeGroup;
       }
@@ -208,13 +235,13 @@ public class PostReviewers
       return wholeGroup;
     }
 
-    return addByEmail(reviewer, rsrc, state, notify, accountsToNotify);
+    return addByEmail(reviewer, notes, user, state, notify, accountsToNotify);
   }
 
   Addition ccCurrentUser(CurrentUser user, RevisionResource revision) {
     return new Addition(
         user.getUserName().orElse(null),
-        revision.getChangeResource(),
+        revision.getUser(),
         ImmutableSet.of(user.getAccountId()),
         null,
         CC,
@@ -226,7 +253,8 @@ public class PostReviewers
   @Nullable
   private Addition addByAccountId(
       String reviewer,
-      ChangeResource rsrc,
+      Branch.NameKey dest,
+      CurrentUser user,
       ReviewerState state,
       NotifyHandling notify,
       ListMultimap<RecipientType, Account.Id> accountsToNotify,
@@ -251,10 +279,10 @@ public class PostReviewers
       return null;
     }
 
-    if (isValidReviewer(rsrc.getChange().getDest(), reviewerUser.getAccount())) {
+    if (isValidReviewer(dest, reviewerUser.getAccount())) {
       return new Addition(
           reviewer,
-          rsrc,
+          user,
           ImmutableSet.of(reviewerUser.getAccountId()),
           null,
           state,
@@ -275,7 +303,8 @@ public class PostReviewers
   @Nullable
   private Addition addWholeGroup(
       String reviewer,
-      ChangeResource rsrc,
+      Branch.NameKey dest,
+      CurrentUser user,
       ReviewerState state,
       NotifyHandling notify,
       ListMultimap<RecipientType, Account.Id> accountsToNotify,
@@ -307,7 +336,7 @@ public class PostReviewers
     Set<Account.Id> reviewers = new HashSet<>();
     Set<Account> members;
     try {
-      members = groupMembers.listAccounts(group.getGroupUUID(), rsrc.getProject());
+      members = groupMembers.listAccounts(group.getGroupUUID(), dest.getParentKey());
     } catch (NoSuchProjectException e) {
       return fail(reviewer, e.getMessage());
     }
@@ -333,18 +362,19 @@ public class PostReviewers
     }
 
     for (Account member : members) {
-      if (isValidReviewer(rsrc.getChange().getDest(), member)) {
+      if (isValidReviewer(dest, member)) {
         reviewers.add(member.getId());
       }
     }
 
-    return new Addition(reviewer, rsrc, reviewers, null, state, notify, accountsToNotify, true);
+    return new Addition(reviewer, user, reviewers, null, state, notify, accountsToNotify, true);
   }
 
   @Nullable
   private Addition addByEmail(
       String reviewer,
-      ChangeResource rsrc,
+      ChangeNotes notes,
+      CurrentUser user,
       ReviewerState state,
       NotifyHandling notify,
       ListMultimap<RecipientType, Account.Id> accountsToNotify)
@@ -352,8 +382,8 @@ public class PostReviewers
     try {
       permissionBackend
           .user(anonymousProvider.get())
-          .change(rsrc.getNotes())
           .database(dbProvider)
+          .change(notes)
           .check(ChangePermission.READ);
     } catch (AuthException e) {
       return fail(
@@ -370,7 +400,7 @@ public class PostReviewers
       return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInvalid, reviewer));
     }
     return new Addition(
-        reviewer, rsrc, null, ImmutableList.of(adr), state, notify, accountsToNotify, true);
+        reviewer, user, null, ImmutableList.of(adr), state, notify, accountsToNotify, true);
   }
 
   private boolean isValidReviewer(Branch.NameKey branch, Account member)
@@ -379,9 +409,10 @@ public class PostReviewers
       return false;
     }
 
-    // Does not account for draft status as a user might want to let a
-    // reviewer see a draft.
     try {
+      // Check ref permission instead of change permission, since change permissions take into
+      // account the private bit, whereas adding a user as a reviewer is explicitly allowing them to
+      // see private changes.
       permissionBackend
           .absentUser(member.getId())
           .database(dbProvider)
@@ -405,13 +436,12 @@ public class PostReviewers
   }
 
   public class Addition {
-    final AddReviewerResult result;
+    public final AddReviewerResult result;
-    final PostReviewersOp op;
+    @Nullable public final PostReviewersOp op;
     final Set<Account.Id> reviewers;
     final Collection<Address> reviewersByEmail;
     final ReviewerState state;
-    final ChangeNotes notes;
+    @Nullable final IdentifiedUser caller;
-    final IdentifiedUser caller;
     final boolean exactMatchFound;
 
     Addition(String reviewer) {
@@ -420,14 +450,13 @@ public class PostReviewers
       reviewers = ImmutableSet.of();
       reviewersByEmail = ImmutableSet.of();
       state = REVIEWER;
-      notes = null;
       caller = null;
       exactMatchFound = false;
     }
 
-    protected Addition(
+    Addition(
         String reviewer,
-        ChangeResource rsrc,
+        CurrentUser caller,
         @Nullable Set<Account.Id> reviewers,
         @Nullable Collection<Address> reviewersByEmail,
         ReviewerState state,
@@ -442,20 +471,16 @@ public class PostReviewers
       this.reviewers = reviewers == null ? ImmutableSet.of() : reviewers;
       this.reviewersByEmail = reviewersByEmail == null ? ImmutableList.of() : reviewersByEmail;
       this.state = state;
-      notes = rsrc.getNotes();
+      this.caller = caller.asIdentifiedUser();
-      caller = rsrc.getUser().asIdentifiedUser();
       op =
           postReviewersOpFactory.create(
               this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
       this.exactMatchFound = exactMatchFound;
     }
 
-    void gatherResults() throws OrmException, PermissionBackendException {
+    void gatherResults(ChangeNotes notes) throws OrmException, PermissionBackendException {
-      if (notes == null || caller == null) {
+      checkState(op != null, "addition did not result in an update op");
-        // When notes or caller is missing this is likely just carrying an error message
+      checkState(op.getResult() != null, "op did not return a result");
-        // in the contained AddReviewerResult.
-        return;
-      }
 
       ChangeData cd = changeDataFactory.create(dbProvider.get(), notes);
       // Generate result details and fill AccountLoader. This occurs outside

java/com/google/gerrit/server/restapi/change/PutAssignee.java

@@ -123,7 +123,7 @@ public class PutAssignee extends RetryingRestModifyView<ChangeResource, Assignee
     reviewerInput.state = ReviewerState.CC;
     reviewerInput.confirmed = true;
     reviewerInput.notify = NotifyHandling.NONE;
-    return postReviewers.prepareApplication(rsrc, reviewerInput, false);
+    return postReviewers.prepareApplication(rsrc.getNotes(), rsrc.getUser(), reviewerInput, false);
   }
 
   @Override