Update Set Review API to behave in line with RFC 7231

This CL updates the implementation of PostReview to replace
all instances of raising UnprocessableEntityException
(HTTP 422) with failed Additions instead. This matches the
docs for Set Reviewer, which no longer mention 422 being a
valid response code. This also falls in line with
RFC 7231, which promotes HTTP 400s with detailed bodies
over 422s.

It also updates the documentation to be clearer about
how these API functions work.

Bug: issue 6016
Change-Id: I1adad98107ba021132293ec47c6a6da01787556e

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.api.changes;
 import com.google.gerrit.common.errors.EmailException;
 import com.google.gerrit.extensions.api.changes.AbandonInput;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
+import com.google.gerrit.extensions.api.changes.AddReviewerResult;
 import com.google.gerrit.extensions.api.changes.AssigneeInput;
 import com.google.gerrit.extensions.api.changes.ChangeApi;
 import com.google.gerrit.extensions.api.changes.ChangeEditApi;
@@ -406,16 +407,16 @@ class ChangeApiImpl implements ChangeApi {
   }
 
   @Override
-  public void addReviewer(String reviewer) throws RestApiException {
+  public AddReviewerResult addReviewer(String reviewer) throws RestApiException {
     AddReviewerInput in = new AddReviewerInput();
     in.reviewer = reviewer;
-    addReviewer(in);
+    return addReviewer(in);
   }
 
   @Override
-  public void addReviewer(AddReviewerInput in) throws RestApiException {
+  public AddReviewerResult addReviewer(AddReviewerInput in) throws RestApiException {
     try {
-      postReviewers.apply(change, in);
+      return postReviewers.apply(change, in);
     } catch (OrmException | IOException | UpdateException | PermissionBackendException e) {
       throw new RestApiException("Cannot add change reviewer", e);
     }

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

@@ -23,6 +23,10 @@ public class ChangeMessages extends TranslationBundle {
   }
 
   public String revertChangeDefaultMessage;
+
+  public String reviewerCantSeeChange;
+  public String reviewerInactive;
+  public String reviewerInvalid;
   public String reviewerNotFoundUser;
   public String reviewerNotFoundUserOrGroup;
 

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

@@ -168,20 +168,33 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
 
   public Addition prepareApplication(
       ChangeResource rsrc, AddReviewerInput input, boolean allowGroup)
-      throws OrmException, RestApiException, IOException, PermissionBackendException {
+      throws OrmException, IOException, PermissionBackendException {
+    String reviewer = input.reviewer;
+    ReviewerState state = input.state();
+    NotifyHandling notify = input.notify;
+    ListMultimap<RecipientType, Account.Id> accountsToNotify = null;
+    try {
+      accountsToNotify = notifyUtil.resolveAccounts(input.notifyDetails);
+    } catch (BadRequestException e) {
+      return fail(reviewer, e.getMessage());
+    }
+    boolean confirmed = input.confirmed();
     boolean allowByEmail = projectCache.checkedGet(rsrc.getProject()).isEnableReviewerByEmail();
 
-    Addition byAccountId = addByAccountId(rsrc, input, allowGroup, allowByEmail);
+    Addition byAccountId =
+        addByAccountId(reviewer, rsrc, state, notify, accountsToNotify, allowGroup, allowByEmail);
     if (byAccountId != null) {
       return byAccountId;
     }
 
-    Addition wholeGroup = addWholeGroup(rsrc, input, allowGroup, allowByEmail);
+    Addition wholeGroup =
+        addWholeGroup(
+            reviewer, rsrc, state, notify, accountsToNotify, confirmed, allowGroup, allowByEmail);
     if (wholeGroup != null) {
       return wholeGroup;
     }
 
-    return addByEmail(rsrc, input);
+    return addByEmail(reviewer, rsrc, state, notify, accountsToNotify);
   }
 
   Addition ccCurrentUser(CurrentUser user, RevisionResource revision) {
@@ -197,117 +210,72 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
 
   @Nullable
   private Addition addByAccountId(
-      ChangeResource rsrc, AddReviewerInput input, boolean allowGroup, boolean allowByEmail)
-      throws OrmException, RestApiException, PermissionBackendException {
-    Account.Id accountId = null;
-    try {
-      accountId = accounts.parse(input.reviewer).getAccountId();
-    } catch (UnprocessableEntityException e) {
-      if (!allowGroup && !allowByEmail) {
-        throw new UnprocessableEntityException(
-            MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, input.reviewer));
-      }
-    }
-    if (accountId != null) {
-      return putAccount(
-          input.reviewer,
-          reviewerFactory.create(rsrc, accountId),
-          input.state(),
-          input.notify,
-          notifyUtil.resolveAccounts(input.notifyDetails));
-    }
-    return null;
-  }
-
-  @Nullable
-  private Addition addWholeGroup(
-      ChangeResource rsrc, AddReviewerInput input, boolean allowGroup, boolean allowByEmail)
-      throws OrmException, RestApiException, IOException, PermissionBackendException {
-    if (!allowGroup) {
-      return null;
-    }
-
-    try {
-      return putGroup(rsrc, input);
-    } catch (UnprocessableEntityException e) {
-      if (!allowByEmail) {
-        throw new UnprocessableEntityException(
-            MessageFormat.format(ChangeMessages.get().reviewerNotFoundUserOrGroup, input.reviewer));
-      }
-    }
-    return null;
-  }
-
-  @Nullable
-  private Addition addByEmail(ChangeResource rsrc, AddReviewerInput input)
-      throws OrmException, RestApiException {
-    return putAccountByEmail(
-        input.reviewer,
-        rsrc,
-        input.state(),
-        input.notify,
-        notifyUtil.resolveAccounts(input.notifyDetails));
-  }
-
-  private Addition putAccount(
-      String reviewer,
-      ReviewerResource rsrc,
-      ReviewerState state,
-      NotifyHandling notify,
-      ListMultimap<RecipientType, Account.Id> accountsToNotify)
-      throws UnprocessableEntityException, PermissionBackendException {
-    Account member = rsrc.getReviewerUser().getAccount();
-    PermissionBackend.ForRef perm =
-        permissionBackend.user(rsrc.getReviewerUser()).ref(rsrc.getChange().getDest());
-    if (isValidReviewer(member, perm)) {
-      return new Addition(
-          reviewer,
-          rsrc.getChangeResource(),
-          ImmutableSet.of(member.getId()),
-          null,
-          state,
-          notify,
-          accountsToNotify);
-    }
-    if (member.isActive()) {
-      throw new UnprocessableEntityException(String.format("Change not visible to %s", reviewer));
-    }
-    throw new UnprocessableEntityException(String.format("Account of %s is inactive.", reviewer));
-  }
-
-  private Addition putAccountByEmail(
       String reviewer,
       ChangeResource rsrc,
       ReviewerState state,
       NotifyHandling notify,
-      ListMultimap<RecipientType, Account.Id> accountsToNotify)
-      throws UnprocessableEntityException, OrmException, BadRequestException {
-    if (!rsrc.getControl().forUser(anonymousProvider.get()).isVisible(dbProvider.get())) {
-      throw new BadRequestException("change is not publicly visible");
-    }
-    if (!migration.readChanges()) {
-      throw new BadRequestException("feature only supported in NoteDb");
-    }
-    Address adr;
+      ListMultimap<RecipientType, Account.Id> accountsToNotify,
+      boolean allowGroup,
+      boolean allowByEmail)
+      throws OrmException, PermissionBackendException {
+    Account.Id accountId = null;
     try {
-      adr = Address.parse(reviewer);
-    } catch (IllegalArgumentException e) {
-      throw new UnprocessableEntityException(String.format("email invalid %s", reviewer));
+      accountId = accounts.parse(reviewer).getAccountId();
+    } catch (UnprocessableEntityException | AuthException e) {
+      // AuthException won't occur since the user is authenticated at this point.
+      if (!allowGroup && !allowByEmail) {
+        // Only return failure if we aren't going to try other interpretations.
+        return fail(
+            reviewer, MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, reviewer));
+      }
+      return null;
     }
-    if (!validator.isValid(adr.getEmail())) {
-      throw new UnprocessableEntityException(String.format("email invalid %s", reviewer));
+
+    ReviewerResource rrsrc = reviewerFactory.create(rsrc, accountId);
+    Account member = rrsrc.getReviewerUser().getAccount();
+    PermissionBackend.ForRef perm =
+        permissionBackend.user(rrsrc.getReviewerUser()).ref(rrsrc.getChange().getDest());
+    if (isValidReviewer(member, perm)) {
+      return new Addition(
+          reviewer, rsrc, ImmutableSet.of(member.getId()), null, state, notify, accountsToNotify);
     }
-    return new Addition(
-        reviewer, rsrc, null, ImmutableList.of(adr), state, notify, accountsToNotify);
+    if (!member.isActive()) {
+      return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInactive, reviewer));
+    }
+    return fail(
+        reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer));
   }
 
-  private Addition putGroup(ChangeResource rsrc, AddReviewerInput input)
-      throws RestApiException, OrmException, IOException, PermissionBackendException {
-    GroupDescription.Basic group = groupsCollection.parseInternal(input.reviewer);
+  @Nullable
+  private Addition addWholeGroup(
+      String reviewer,
+      ChangeResource rsrc,
+      ReviewerState state,
+      NotifyHandling notify,
+      ListMultimap<RecipientType, Account.Id> accountsToNotify,
+      boolean confirmed,
+      boolean allowGroup,
+      boolean allowByEmail)
+      throws OrmException, IOException, PermissionBackendException {
+    if (!allowGroup) {
+      return null;
+    }
+
+    GroupDescription.Basic group = null;
+    try {
+      group = groupsCollection.parseInternal(reviewer);
+    } catch (UnprocessableEntityException e) {
+      if (!allowByEmail) {
+        return fail(
+            reviewer,
+            MessageFormat.format(ChangeMessages.get().reviewerNotFoundUserOrGroup, reviewer));
+      }
+      return null;
+    }
+
     if (!isLegalReviewerGroup(group.getGroupUUID())) {
       return fail(
-          input.reviewer,
-          MessageFormat.format(ChangeMessages.get().groupIsNotAllowed, group.getName()));
+          reviewer, MessageFormat.format(ChangeMessages.get().groupIsNotAllowed, group.getName()));
     }
 
     Set<Account.Id> reviewers = new HashSet<>();
@@ -319,9 +287,11 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
               .create(control.getUser())
               .listAccounts(group.getGroupUUID(), control.getProject().getNameKey());
     } catch (NoSuchGroupException e) {
-      throw new UnprocessableEntityException(e.getMessage());
+      return fail(
+          reviewer,
+          MessageFormat.format(ChangeMessages.get().reviewerNotFoundUserOrGroup, group.getName()));
     } catch (NoSuchProjectException e) {
-      throw new BadRequestException(e.getMessage());
+      return fail(reviewer, e.getMessage());
     }
 
     // if maxAllowed is set to 0, it is allowed to add any number of
@@ -329,18 +299,16 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
     int maxAllowed = cfg.getInt("addreviewer", "maxAllowed", DEFAULT_MAX_REVIEWERS);
     if (maxAllowed > 0 && members.size() > maxAllowed) {
       return fail(
-          input.reviewer,
+          reviewer,
           MessageFormat.format(ChangeMessages.get().groupHasTooManyMembers, group.getName()));
     }
 
     // if maxWithoutCheck is set to 0, we never ask for confirmation
     int maxWithoutConfirmation =
         cfg.getInt("addreviewer", "maxWithoutConfirmation", DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK);
-    if (!input.confirmed()
-        && maxWithoutConfirmation > 0
-        && members.size() > maxWithoutConfirmation) {
+    if (!confirmed && maxWithoutConfirmation > 0 && members.size() > maxWithoutConfirmation) {
       return fail(
-          input.reviewer,
+          reviewer,
           true,
           MessageFormat.format(
               ChangeMessages.get().groupManyMembersConfirmation, group.getName(), members.size()));
@@ -354,14 +322,32 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
       }
     }
 
+    return new Addition(reviewer, rsrc, reviewers, null, state, notify, accountsToNotify);
+  }
+
+  @Nullable
+  private Addition addByEmail(
+      String reviewer,
+      ChangeResource rsrc,
+      ReviewerState state,
+      NotifyHandling notify,
+      ListMultimap<RecipientType, Account.Id> accountsToNotify)
+      throws OrmException {
+    if (!rsrc.getControl().forUser(anonymousProvider.get()).isVisible(dbProvider.get())) {
+      return fail(
+          reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer));
+    }
+    if (!migration.readChanges()) {
+      // addByEmail depends on NoteDb.
+      return fail(
+          reviewer, MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, reviewer));
+    }
+    Address adr = Address.tryParse(reviewer);
+    if (adr == null || !validator.isValid(adr.getEmail())) {
+      return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInvalid, reviewer));
+    }
     return new Addition(
-        input.reviewer,
-        rsrc,
-        reviewers,
-        null,
-        input.state(),
-        input.notify,
-        notifyUtil.resolveAccounts(input.notifyDetails));
+        reviewer, rsrc, null, ImmutableList.of(adr), state, notify, accountsToNotify);
   }
 
   private boolean isValidReviewer(Account member, PermissionBackend.ForRef perm)

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

@@ -108,7 +108,7 @@ public class PutAssignee
   }
 
   private Addition addAssigneeAsCC(ChangeResource rsrc, String assignee)
-      throws OrmException, RestApiException, IOException, PermissionBackendException {
+      throws OrmException, IOException, PermissionBackendException {
     AddReviewerInput reviewerInput = new AddReviewerInput();
     reviewerInput.reviewer = assignee;
     reviewerInput.state = ReviewerState.CC;