PostReview: Do not fail with ISE if reviewer in ReviewerInput is missing Before this change, calling PostReview failed with '500 Internal Server Error' when the user provided a ReviewerInput that didn't have the 'reviewer' field set. If bad user input is provided we usually reject the request with '400 Bad Request' but since PostReview can batch multiple operations (e.g. multiple reviewer updates) it always returns '200 OK' and errors are provided in the returned ReviewResult. Hence if a ReviewerInput without a reviewer is specified return a proper error message in the ReviewResult. Adding reviewers is also possible with PostReviewers. While we are at this update PostReviewers to make the error handling for missing reviewer user identifiers consistent and improve the error message: * Before this change, PostReviewers rejected a null reviewer with a '400 Bad Request' reposnse but an empty reviewer resulted in a '200 OK' response with an error in the returned ReviewerResult. Now in both cases we return a '200 OK' response with an error in the returned ReviewerResult. * The error message for when an empty reviewer is provided is improved from saying " is not a valid user identifier" to saying "reviewer user identifier is required". Bug: Google b/326096919 Release-Notes: Fixed internal server error when posting a review with a ReviewerInput that didn't set the 'reviewer' field. Change-Id: I0b523c58a97a7c48d5d7610ea24d167a5f8fa0b9 Signed-off-by: Edwin Kempin <ekempin@google.com>

diff --git a/java/com/google/gerrit/server/change/ReviewerModifier.java b/java/com/google/gerrit/server/change/ReviewerModifier.java
index d714215..4b1d06c 100644
--- a/java/com/google/gerrit/server/change/ReviewerModifier.java
+++ b/java/com/google/gerrit/server/change/ReviewerModifier.java

@@ -24,6 +24,7 @@
 import static java.util.Comparator.comparing;
 import static java.util.Objects.requireNonNull;
 
+import autovalue.shaded.com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -222,7 +223,10 @@
       throws IOException, PermissionBackendException, ConfigInvalidException {
     try (TraceContext.TraceTimer ignored =
         TraceContext.newTimer(getClass().getSimpleName() + "#prepare", Metadata.empty())) {
-      requireNonNull(input.reviewer);
+      if (Strings.nullToEmpty(input.reviewer).trim().isEmpty()) {
+        return fail(input, FailureType.NOT_FOUND, "reviewer user identifier is required");
+      }
+
       boolean confirmed = input.confirmed();
       boolean allowByEmail =
           projectCache

diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewers.java b/java/com/google/gerrit/server/restapi/change/PostReviewers.java
index b0e58c5..675610d 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewers.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewers.java

@@ -65,10 +65,6 @@
   public Response<ReviewerResult> apply(ChangeResource rsrc, ReviewerInput input)
       throws IOException, RestApiException, UpdateException, PermissionBackendException,
           ConfigInvalidException {
-    if (input.reviewer == null) {
-      throw new BadRequestException("missing reviewer field");
-    }
-
     ReviewerModification modification =
         reviewerModifier.prepare(rsrc.getNotes(), rsrc.getUser(), input, true);
     if (modification.op == null) {

diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 1b06b7b..3771bb9 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java

@@ -59,6 +59,7 @@
 import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
 import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
 import com.google.gerrit.extensions.api.changes.ReviewResult;
+import com.google.gerrit.extensions.api.changes.ReviewerInput;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.client.ReviewerState;
 import com.google.gerrit.extensions.client.Side;
@@ -825,6 +826,41 @@
   }
 
   @Test
+  public void rejectAddingReviewerIfReviewerUserIdentifierIsMissing() throws Exception {
+    PushOneCommit.Result r = createChange();
+
+    // Add reviewer with ReviewerInput where the 'reviewer' field is not set.
+    ReviewerInput reviewerInput = new ReviewerInput();
+    reviewerInput.state = ReviewerState.REVIEWER;
+
+    ReviewInput reviewInput = ReviewInput.create();
+    reviewInput.reviewers = ImmutableList.of(reviewerInput);
+
+    ReviewResult reviewResult = gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+    assertThat(reviewResult.error).isEqualTo("error adding reviewer");
+    assertThat(reviewResult.reviewers.keySet()).containsExactly(reviewerInput.reviewer);
+    assertThat(reviewResult.reviewers.get(reviewerInput.reviewer).error)
+        .isEqualTo("reviewer user identifier is required");
+
+    // Add reviewer with ReviewerInput where the 'reviewer' field is set to an empty string.
+    reviewerInput.reviewer = "";
+    reviewResult = gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+    assertThat(reviewResult.error).isEqualTo("error adding reviewer");
+    assertThat(reviewResult.reviewers.keySet()).containsExactly(reviewerInput.reviewer);
+    assertThat(reviewResult.reviewers.get(reviewerInput.reviewer).error)
+        .isEqualTo("reviewer user identifier is required");
+
+    // Add reviewer with ReviewerInput where the 'reviewer' field is set to an empty string after
+    // trimming it.
+    reviewerInput.reviewer = "   ";
+    reviewResult = gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+    assertThat(reviewResult.error).isEqualTo("error adding reviewer");
+    assertThat(reviewResult.reviewers.keySet()).containsExactly(reviewerInput.reviewer);
+    assertThat(reviewResult.reviewers.get(reviewerInput.reviewer).error)
+        .isEqualTo("reviewer user identifier is required");
+  }
+
+  @Test
   public void deletingReviewers() throws Exception {
     PushOneCommit.Result r = createChange();
 

diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
index 6cadf33..ca3a345 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java

@@ -323,11 +323,19 @@
   }
 
   @Test
-  public void rejectMissingEmail() throws Exception {
+  public void rejectIfReviewerUserIdentifierIsMissing() throws Exception {
     PushOneCommit.Result r = createChange();
 
-    ReviewerResult result = gApi.changes().id(r.getChangeId()).addReviewer("");
-    assertThat(result.error).isEqualTo(" is not a valid user identifier");
+    ReviewerResult result = gApi.changes().id(r.getChangeId()).addReviewer((String) null);
+    assertThat(result.error).isEqualTo("reviewer user identifier is required");
+    assertThat(result.reviewers).isNull();
+
+    result = gApi.changes().id(r.getChangeId()).addReviewer("");
+    assertThat(result.error).isEqualTo("reviewer user identifier is required");
+    assertThat(result.reviewers).isNull();
+
+    result = gApi.changes().id(r.getChangeId()).addReviewer("   ");
+    assertThat(result.error).isEqualTo("reviewer user identifier is required");
     assertThat(result.reviewers).isNull();
   }