Return Bad Request on bad reviewers to PostReview

Now that posting a review can be used to add reviewers, it's possible
for the entire post to fail due to bad input (e.g., an unresolvable
account, an oversized group). This information is already returned in
the JSON response body, but we should use a status code other than
200 OK when this happens so the client doesn't erroneously assume that
the review was successfully posted.

Change-Id: I763b5c73878b7c70a95953759b371a182c8a8dad

Documentation/rest-api-changes.txt

@@ -2846,9 +2846,7 @@ with a review.
 Each element of the `reviewers` list is an instance of
 link:#reviewer-input[ReviewerInput]. The corresponding result of
 adding each reviewer will be returned in a list of
-link:#add-reviewer-result[AddReviewerResult]. If there are any
+link:#add-reviewer-result[AddReviewerResult].
-errors returned for reviewers, the entire review request will
-be rejected.
 
 .Response
 ----
@@ -2886,6 +2884,27 @@ be rejected.
   }
 ----
 
+If there are any errors returned for reviewers, the entire review request will
+be rejected with `400 Bad Request`.
+
+.Error Response
+----
+  HTTP/1.1 400 Bad Request
+  Content-Disposition: attachment
+  Content-Type: application/json; charset=UTF-8
+
+  )]}'
+  {
+    "reviewers": {
+      "MyProjectVerifiers": {
+        "input": "MyProjectVerifiers",
+        "error": "The group My Group has 15 members. Do you want to add them all as reviewers?",
+        "confirm": true
+      }
+    }
+  }
+----
+
 [[rebase-revision]]
 === Rebase Revision
 --

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java

@@ -17,6 +17,8 @@ package com.google.gerrit.acceptance.rest.change;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.gerrit.extensions.client.ReviewerState.CC;
 import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
+import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
+import static javax.servlet.http.HttpServletResponse.SC_OK;
 
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.PushOneCommit;
@@ -272,7 +274,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
       // In legacy mode, change owner should be the only reviewer.
       assertReviewers(c, REVIEWER, admin);
       assertReviewers(c, CC, user, observer);
-   }
+    }
 
     // Verify emails were sent to added reviewers.
     List<Message> messages = sender.getMessages();
@@ -329,7 +331,8 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
         .reviewer(user.email)
         .reviewer(observer.email, CC, false)
         .reviewer(largeGroup);
-    ReviewResult result = review(r.getChangeId(), r.getCommit().name(), input);
+    ReviewResult result = review(
+        r.getChangeId(), r.getCommit().name(), input, SC_BAD_REQUEST);
     assertThat(result.labels).isNull();
     assertThat(result.reviewers).isNotNull();
     assertThat(result.reviewers).hasSize(3);
@@ -353,7 +356,8 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
         .reviewer(user.email)
         .reviewer(observer.email, CC, false)
         .reviewer(mediumGroup);
-    result = review(r.getChangeId(), r.getCommit().name(), input);
+    result = review(r.getChangeId(), r.getCommit().name(), input,
+        SC_BAD_REQUEST);
     assertThat(result.labels).isNull();
     assertThat(result.reviewers).isNotNull();
     assertThat(result.reviewers).hasSize(3);
@@ -400,27 +404,46 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
 
   private AddReviewerResult addReviewer(String changeId, String reviewer)
       throws Exception {
+    return addReviewer(changeId, reviewer, SC_OK);
+  }
+
+  private AddReviewerResult addReviewer(
+      String changeId, String reviewer, int expectedStatus) throws Exception {
     AddReviewerInput in = new AddReviewerInput();
     in.reviewer = reviewer;
-    return addReviewer(changeId, in);
+    return addReviewer(changeId, in, expectedStatus);
   }
 
   private AddReviewerResult addReviewer(String changeId, AddReviewerInput in)
       throws Exception {
+    return addReviewer(changeId, in, SC_OK);
+  }
+
+  private AddReviewerResult addReviewer(String changeId, AddReviewerInput in,
+      int expectedStatus) throws Exception {
     RestResponse resp =
         adminRestSession.post("/changes/" + changeId + "/reviewers", in);
-    return readContentFromJson(resp, AddReviewerResult.class);
+    return readContentFromJson(
+        resp, expectedStatus, AddReviewerResult.class);
   }
 
-  private ReviewResult review(String changeId, String revisionId, ReviewInput in) throws Exception {
+  private ReviewResult review(
+      String changeId, String revisionId, ReviewInput in) throws Exception {
+    return review(changeId, revisionId, in, SC_OK);
+  }
+
+  private ReviewResult review(
+      String changeId, String revisionId, ReviewInput in, int expectedStatus)
+      throws Exception {
     RestResponse resp = adminRestSession.post(
         "/changes/" + changeId + "/revisions/" + revisionId + "/review", in);
-    return readContentFromJson(resp, ReviewResult.class);
+    return readContentFromJson(resp, expectedStatus, ReviewResult.class);
   }
 
-  private static <T> T readContentFromJson(RestResponse r, Class<T> clazz)
+  private static <T> T readContentFromJson(
+      RestResponse r, int expectedStatus, Class<T> clazz)
       throws Exception {
-    r.assertOK();
+    r.assertStatus(expectedStatus);
     JsonReader jsonReader = new JsonReader(r.getReader());
     jsonReader.setLenient(true);
     return newGson().fromJson(jsonReader, clazz);

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/Response.java

@@ -52,6 +52,11 @@ public abstract class Response<T> {
     return new Redirect(location);
   }
 
+  /** Arbitrary status code with wrapped result. */
+  public static <T> Response<T> withStatusCode(int statusCode, T value) {
+    return new Impl<>(statusCode, value);
+  }
+
   @SuppressWarnings({"unchecked", "rawtypes"})
   public static <T> T unwrap(T obj) {
     while (obj instanceof Response) {

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

@@ -20,6 +20,7 @@ import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId;
 import static com.google.gerrit.server.change.PutDraftComment.side;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.base.MoreObjects;
@@ -48,6 +49,7 @@ import com.google.gerrit.extensions.client.Side;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@@ -145,12 +147,12 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
   }
 
   @Override
-  public ReviewResult apply(RevisionResource revision, ReviewInput input)
+  public Response<ReviewResult> apply(RevisionResource revision, ReviewInput input)
       throws RestApiException, UpdateException, OrmException, IOException {
     return apply(revision, input, TimeUtil.nowTs());
   }
 
-  public ReviewResult apply(RevisionResource revision, ReviewInput input,
+  public Response<ReviewResult> apply(RevisionResource revision, ReviewInput input,
       Timestamp ts)
       throws RestApiException, UpdateException, OrmException, IOException {
     // Respect timestamp, but truncate at change created-on time.
@@ -197,7 +199,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
     ReviewResult output = new ReviewResult();
     output.reviewers = reviewerJsonResults;
     if (hasError || confirm) {
-      return output;
+      return Response.withStatusCode(SC_BAD_REQUEST, output);
     }
     output.labels = input.labels;
 
@@ -218,7 +220,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       }
     }
 
-    return output;
+    return Response.ok(output);
   }
 
   private RevisionResource onBehalfOf(RevisionResource rev, ReviewInput in)