Merge "PostReview: Do not fail with ISE if reviewer in ReviewerInput is missing"
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();
}
