Validate comments and message in PostReview using new interface
This commit adds logic to PostReview to validate all comments that get
published as well as the change message through the new
CommentValidator.
Tests cover both validation success and failure for these cases:
- Publishing draft comments
- Publishing comments directly
- Adding a change message
Surrounding logic is refactored where needed.
Change-Id: I8ed6f721137766fc3d597d7b8484747e151814de
diff --git a/java/com/google/gerrit/extensions/validators/CommentValidator.java b/java/com/google/gerrit/extensions/validators/CommentValidator.java
index cbcf1a9..cfefdef 100644
--- a/java/com/google/gerrit/extensions/validators/CommentValidator.java
+++ b/java/com/google/gerrit/extensions/validators/CommentValidator.java
@@ -25,9 +25,9 @@
public interface CommentValidator {
/**
- * Validate the specified commits.
+ * Validate the specified comments.
*
- * @return An empty list if all commits are valid, or else a list of validation failures.
+ * @return An empty list if all comments are valid, or else a list of validation failures.
*/
ImmutableList<CommentValidationFailure> validateComments(
ImmutableList<CommentForValidation> comments);
diff --git a/java/com/google/gerrit/server/PublishCommentUtil.java b/java/com/google/gerrit/server/PublishCommentUtil.java
index 8ae5bcd..6a1c859 100644
--- a/java/com/google/gerrit/server/PublishCommentUtil.java
+++ b/java/com/google/gerrit/server/PublishCommentUtil.java
@@ -18,13 +18,18 @@
import static com.google.gerrit.reviewdb.client.PatchLineComment.Status.PUBLISHED;
import static java.util.stream.Collectors.toSet;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.validators.CommentForValidation;
+import com.google.gerrit.extensions.validators.CommentValidationFailure;
+import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.reviewdb.client.Comment;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
+import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.update.ChangeContext;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -77,4 +82,20 @@
private static PatchSet.Id psId(ChangeNotes notes, Comment c) {
return PatchSet.id(notes.getChangeId(), c.key.patchSetId);
}
+
+ /**
+ * Helper to run the specified set of {@link CommentValidator}-s on the specified comments.
+ *
+ * @return See {@link CommentValidator#validateComments(ImmutableList)}.
+ */
+ public static ImmutableList<CommentValidationFailure> findInvalidComments(
+ PluginSetContext<CommentValidator> commentValidators,
+ ImmutableList<CommentForValidation> commentsForValidation) {
+ ImmutableList.Builder<CommentValidationFailure> commentValidationFailures =
+ new ImmutableList.Builder<>();
+ commentValidators.runEach(
+ listener ->
+ commentValidationFailures.addAll(listener.validateComments(commentsForValidation)));
+ return commentValidationFailures.build();
+ }
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index ef38b05..50c1511 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -16,6 +16,7 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.server.CommentsUtil.setCommentCommitId;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
@@ -29,9 +30,11 @@
import com.google.auto.value.AutoValue;
import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Ordering;
+import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
import com.google.common.hash.HashCode;
import com.google.common.hash.Hashing;
@@ -61,6 +64,10 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.restapi.Url;
+import com.google.gerrit.extensions.validators.CommentForValidation;
+import com.google.gerrit.extensions.validators.CommentForValidation.CommentType;
+import com.google.gerrit.extensions.validators.CommentValidationFailure;
+import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.json.OutputFormat;
import com.google.gerrit.mail.Address;
import com.google.gerrit.reviewdb.client.Account;
@@ -106,12 +113,14 @@
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
+import com.google.gerrit.server.update.CommentsRejectedException;
import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
@@ -137,6 +146,7 @@
import java.util.OptionalInt;
import java.util.Set;
import java.util.stream.Collectors;
+import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
@@ -173,6 +183,7 @@
private final WorkInProgressOp.Factory workInProgressOpFactory;
private final ProjectCache projectCache;
private final PermissionBackend permissionBackend;
+ private final PluginSetContext<CommentValidator> commentValidators;
private final boolean strictLabels;
@Inject
@@ -195,7 +206,8 @@
@GerritServerConfig Config gerritConfig,
WorkInProgressOp.Factory workInProgressOpFactory,
ProjectCache projectCache,
- PermissionBackend permissionBackend) {
+ PermissionBackend permissionBackend,
+ PluginSetContext<CommentValidator> commentValidators) {
super(retryHelper);
this.changeResourceFactory = changeResourceFactory;
this.changeDataFactory = changeDataFactory;
@@ -215,6 +227,7 @@
this.workInProgressOpFactory = workInProgressOpFactory;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
+ this.commentValidators = commentValidators;
this.strictLabels = gerritConfig.getBoolean("change", "strictLabels", false);
}
@@ -858,7 +871,7 @@
@Override
public boolean updateChange(ChangeContext ctx)
throws ResourceConflictException, UnprocessableEntityException, IOException,
- PatchListNotAvailableException {
+ PatchListNotAvailableException, CommentsRejectedException {
user = ctx.getIdentifiedUser();
notes = ctx.getNotes();
ps = psUtil.get(ctx.getNotes(), psId);
@@ -891,7 +904,8 @@
}
private boolean insertComments(ChangeContext ctx)
- throws UnprocessableEntityException, PatchListNotAvailableException {
+ throws UnprocessableEntityException, PatchListNotAvailableException,
+ CommentsRejectedException {
Map<String, List<CommentInput>> inputComments = in.comments;
if (inputComments == null) {
inputComments = Collections.emptyMap();
@@ -910,6 +924,7 @@
}
}
+ // This will be populated with Comment-s created from inputComments.
List<Comment> toPublish = new ArrayList<>();
Set<CommentSetEntry> existingComments =
@@ -954,11 +969,13 @@
switch (in.drafts) {
case PUBLISH:
case PUBLISH_ALL_REVISIONS:
+ validateComments(Streams.concat(drafts.values().stream(), toPublish.stream()));
publishCommentUtil.publish(ctx, psId, drafts.values(), in.tag);
comments.addAll(drafts.values());
break;
case KEEP:
default:
+ validateComments(toPublish.stream());
break;
}
ChangeUpdate changeUpdate = ctx.getUpdate(psId);
@@ -967,6 +984,24 @@
return !toPublish.isEmpty();
}
+ private void validateComments(Stream<Comment> comments) throws CommentsRejectedException {
+ ImmutableList<CommentForValidation> draftsForValidation =
+ comments
+ .map(
+ comment ->
+ CommentForValidation.create(
+ comment.lineNbr > 0
+ ? CommentForValidation.CommentType.INLINE_COMMENT
+ : CommentType.FILE_COMMENT,
+ comment.message))
+ .collect(toImmutableList());
+ List<CommentValidationFailure> draftValidationFailures =
+ PublishCommentUtil.findInvalidComments(commentValidators, draftsForValidation);
+ if (!draftValidationFailures.isEmpty()) {
+ throw new CommentsRejectedException(draftValidationFailures);
+ }
+ }
+
private boolean insertRobotComments(ChangeContext ctx) throws PatchListNotAvailableException {
if (in.robotComments == null) {
return false;
@@ -1337,7 +1372,7 @@
return current;
}
- private boolean insertMessage(ChangeContext ctx) {
+ private boolean insertMessage(ChangeContext ctx) throws CommentsRejectedException {
String msg = Strings.nullToEmpty(in.message).trim();
StringBuilder buf = new StringBuilder();
@@ -1350,6 +1385,15 @@
buf.append(String.format("\n\n(%d comments)", comments.size()));
}
if (!msg.isEmpty()) {
+ List<CommentValidationFailure> messageValidationFailure =
+ PublishCommentUtil.findInvalidComments(
+ commentValidators,
+ ImmutableList.of(
+ CommentForValidation.create(
+ CommentForValidation.CommentType.CHANGE_MESSAGE, msg)));
+ if (!messageValidationFailure.isEmpty()) {
+ throw new CommentsRejectedException(messageValidationFailure);
+ }
buf.append("\n\n").append(msg);
} else if (in.ready) {
buf.append("\n\n" + START_REVIEW_MESSAGE);
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 2a958af..6dee241 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -34,6 +34,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -187,6 +188,10 @@
|| e instanceof NoSuchRefException
|| e instanceof NoSuchProjectException) {
throw new ResourceNotFoundException(e.getMessage(), e);
+ } else if (e instanceof CommentsRejectedException) {
+ // SC_BAD_REQUEST is not ideal because it's not a syntactic error, but there is no better
+ // status code and it's isolated in monitoring.
+ throw new BadRequestException(e.getMessage(), e);
}
Throwables.throwIfUnchecked(e);
@@ -290,7 +295,7 @@
private enum ChangeResult {
SKIPPED,
UPSERTED,
- DELETED;
+ DELETED
}
private final GitRepositoryManager repoManager;
@@ -567,9 +572,7 @@
handle.setResult(id, ChangeResult.SKIPPED);
continue;
}
- for (ChangeUpdate u : ctx.updates.values()) {
- handle.manager.add(u);
- }
+ ctx.updates.values().forEach(handle.manager::add);
if (ctx.deleted) {
logDebug("Change %s was deleted", id);
handle.manager.deleteChange(id);
diff --git a/java/com/google/gerrit/server/update/CommentsRejectedException.java b/java/com/google/gerrit/server/update/CommentsRejectedException.java
new file mode 100644
index 0000000..6b0c04d
--- /dev/null
+++ b/java/com/google/gerrit/server/update/CommentsRejectedException.java
@@ -0,0 +1,47 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.update;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.extensions.validators.CommentValidationFailure;
+import java.util.Collection;
+import java.util.stream.Collectors;
+
+/** Thrown when comment validation rejected a comment, preventing it from being published. */
+public class CommentsRejectedException extends Exception {
+ private static final long serialVersionUID = 1L;
+
+ private final ImmutableList<CommentValidationFailure> commentValidationFailures;
+
+ public CommentsRejectedException(Collection<CommentValidationFailure> commentValidationFailures) {
+ this.commentValidationFailures = ImmutableList.copyOf(commentValidationFailures);
+ }
+
+ @Override
+ public String getMessage() {
+ return "One or more comments were rejected in validation: "
+ + commentValidationFailures.stream()
+ .map(CommentValidationFailure::getMessage)
+ .collect(Collectors.joining("; "));
+ }
+
+ /**
+ * Returns the validation failures that caused this exception. By contract this list is never
+ * empty.
+ */
+ public ImmutableList<CommentValidationFailure> getCommentValidationFailures() {
+ return commentValidationFailures;
+ }
+}
diff --git a/java/com/google/gerrit/testing/TestCommentHelper.java b/java/com/google/gerrit/testing/TestCommentHelper.java
new file mode 100644
index 0000000..b72cca7
--- /dev/null
+++ b/java/com/google/gerrit/testing/TestCommentHelper.java
@@ -0,0 +1,107 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.testing;
+
+import static java.util.stream.Collectors.toList;
+
+import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.extensions.api.changes.DraftInput;
+import com.google.gerrit.extensions.client.Comment;
+import com.google.gerrit.extensions.client.Comment.Range;
+import com.google.gerrit.extensions.client.Side;
+import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.inject.Inject;
+import java.util.Collection;
+
+/** Test helper for dealing with comments/drafts. */
+public class TestCommentHelper {
+ private final GerritApi gApi;
+
+ @Inject
+ public TestCommentHelper(GerritApi gerritApi) {
+ gApi = gerritApi;
+ }
+
+ public DraftInput newDraft(String message) {
+ return populate(new DraftInput(), "file", message);
+ }
+
+ public DraftInput newDraft(String path, Side side, int line, String message) {
+ DraftInput d = new DraftInput();
+ return populate(d, path, side, line, message);
+ }
+
+ public void addDraft(String changeId, String revId, DraftInput in) throws Exception {
+ gApi.changes().id(changeId).revision(revId).createDraft(in).get();
+ }
+
+ public Collection<CommentInfo> getPublishedComments(String changeId) throws Exception {
+ return gApi.changes().id(changeId).comments().values().stream()
+ .flatMap(Collection::stream)
+ .collect(toList());
+ }
+
+ public static <C extends Comment> C populate(C c, String path, String message) {
+ return populate(c, path, createLineRange(), message);
+ }
+
+ private static <C extends Comment> C populate(C c, String path, Range range, String message) {
+ int line = range.startLine;
+ c.path = path;
+ c.side = Side.REVISION;
+ c.parent = null;
+ c.line = line != 0 ? line : null;
+ c.message = message;
+ c.unresolved = false;
+ if (line != 0) c.range = range;
+ return c;
+ }
+
+ private static <C extends Comment> C populate(
+ C c, String path, Side side, Range range, String message) {
+ int line = range.startLine;
+ c.path = path;
+ c.side = side;
+ c.parent = null;
+ c.line = line != 0 ? line : null;
+ c.message = message;
+ c.unresolved = false;
+ if (line != 0) c.range = range;
+ return c;
+ }
+
+ private static <C extends Comment> C populate(
+ C c, String path, Side side, int line, String message) {
+ return populate(c, path, side, createLineRange(line), message);
+ }
+
+ private static Range createLineRange() {
+ Range range = new Range();
+ range.startLine = 0;
+ range.startCharacter = 1;
+ range.endLine = 0;
+ range.endCharacter = 5;
+ return range;
+ }
+
+ private static Range createLineRange(int line) {
+ Range range = new Range();
+ range.startLine = line;
+ range.startCharacter = 1;
+ range.endLine = line;
+ range.endCharacter = 5;
+ return range;
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
new file mode 100644
index 0000000..ebed15c
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -0,0 +1,288 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.api.change;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.extensions.api.changes.DraftInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
+import com.google.gerrit.extensions.client.Side;
+import com.google.gerrit.extensions.common.ChangeMessageInfo;
+import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.validators.CommentForValidation;
+import com.google.gerrit.extensions.validators.CommentForValidation.CommentType;
+import com.google.gerrit.extensions.validators.CommentValidator;
+import com.google.gerrit.server.restapi.change.PostReview;
+import com.google.gerrit.server.update.CommentsRejectedException;
+import com.google.gerrit.testing.TestCommentHelper;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import java.sql.Timestamp;
+import org.easymock.Capture;
+import org.easymock.EasyMock;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests for comment validation in {@link PostReview}. */
+public class PostReviewIT extends AbstractDaemonTest {
+ @Inject private CommentValidator mockCommentValidator;
+ @Inject private TestCommentHelper testCommentHelper;
+
+ private static final String COMMENT_TEXT = "The comment text";
+
+ private Capture<ImmutableList<CommentForValidation>> capture = new Capture<>();
+
+ @Override
+ public Module createModule() {
+ return new FactoryModule() {
+ @Override
+ public void configure() {
+ CommentValidator mockCommentValidator = EasyMock.createMock(CommentValidator.class);
+ bind(CommentValidator.class)
+ .annotatedWith(Exports.named(mockCommentValidator.getClass()))
+ .toInstance(mockCommentValidator);
+ bind(CommentValidator.class).toInstance(mockCommentValidator);
+ }
+ };
+ }
+
+ @Before
+ public void resetMock() {
+ EasyMock.reset(mockCommentValidator);
+ }
+
+ @After
+ public void verifyMock() {
+ EasyMock.verify(mockCommentValidator);
+ }
+
+ @Test
+ public void validateCommentsInInput_commentOK() throws Exception {
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(
+ CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of());
+ EasyMock.replay(mockCommentValidator);
+
+ PushOneCommit.Result r = createChange();
+
+ ReviewInput input = new ReviewInput();
+ CommentInput comment = newComment(r.getChange().currentFilePaths().get(0));
+ comment.updated = new Timestamp(0);
+ input.comments = ImmutableMap.of(comment.path, ImmutableList.of(comment));
+
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+ gApi.changes().id(r.getChangeId()).current().review(input);
+
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).hasSize(1);
+ }
+
+ @Test
+ public void validateCommentsInInput_commentRejected() throws Exception {
+ CommentForValidation commentForValidation =
+ CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT);
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
+ EasyMock.replay(mockCommentValidator);
+
+ PushOneCommit.Result r = createChange();
+
+ ReviewInput input = new ReviewInput();
+ CommentInput comment = newComment(r.getChange().currentFilePaths().get(0));
+ comment.updated = new Timestamp(0);
+ input.comments = ImmutableMap.of(comment.path, ImmutableList.of(comment));
+
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+ BadRequestException badRequestException =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().review(input));
+ assertThat(badRequestException.getCause()).isInstanceOf(CommentsRejectedException.class);
+ assertThat(
+ Iterables.getOnlyElement(
+ ((CommentsRejectedException) badRequestException.getCause())
+ .getCommentValidationFailures())
+ .getComment()
+ .getText())
+ .isEqualTo(COMMENT_TEXT);
+ assertThat(badRequestException.getCause()).hasMessageThat().contains("Oh no!");
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+ }
+
+ @Test
+ public void validateDrafts_draftOK() throws Exception {
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(
+ CommentForValidation.CommentType.INLINE_COMMENT, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of());
+ EasyMock.replay(mockCommentValidator);
+
+ PushOneCommit.Result r = createChange();
+
+ DraftInput draft =
+ testCommentHelper.newDraft(
+ r.getChange().currentFilePaths().get(0), Side.REVISION, 1, COMMENT_TEXT);
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().getName()).createDraft(draft).get();
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+
+ ReviewInput input = new ReviewInput();
+ input.drafts = DraftHandling.PUBLISH;
+
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).hasSize(1);
+ }
+
+ @Test
+ public void validateDrafts_draftRejected() throws Exception {
+ CommentForValidation commentForValidation =
+ CommentForValidation.create(CommentType.INLINE_COMMENT, COMMENT_TEXT);
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(
+ CommentForValidation.CommentType.INLINE_COMMENT, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
+ EasyMock.replay(mockCommentValidator);
+ PushOneCommit.Result r = createChange();
+
+ DraftInput draft =
+ testCommentHelper.newDraft(
+ r.getChange().currentFilePaths().get(0), Side.REVISION, 1, COMMENT_TEXT);
+ testCommentHelper.addDraft(r.getChangeId(), r.getCommit().getName(), draft);
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+
+ ReviewInput input = new ReviewInput();
+ input.drafts = DraftHandling.PUBLISH;
+ BadRequestException badRequestException =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().review(input));
+ assertThat(badRequestException.getCause()).isInstanceOf(CommentsRejectedException.class);
+ assertThat(
+ Iterables.getOnlyElement(
+ ((CommentsRejectedException) badRequestException.getCause())
+ .getCommentValidationFailures())
+ .getComment()
+ .getText())
+ .isEqualTo(draft.message);
+ assertThat(badRequestException.getCause()).hasMessageThat().contains("Oh no!");
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+ }
+
+ @Test
+ public void validateDrafts_inlineVsFileComments_allOK() throws Exception {
+ PushOneCommit.Result r = createChange();
+ DraftInput draftInline =
+ testCommentHelper.newDraft(
+ r.getChange().currentFilePaths().get(0), Side.REVISION, 1, COMMENT_TEXT);
+ testCommentHelper.addDraft(r.getChangeId(), r.getCommit().getName(), draftInline);
+ DraftInput draftFile = testCommentHelper.newDraft(COMMENT_TEXT);
+ testCommentHelper.addDraft(r.getChangeId(), r.getCommit().getName(), draftFile);
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+
+ EasyMock.expect(mockCommentValidator.validateComments(EasyMock.capture(capture)))
+ .andReturn(ImmutableList.of());
+ EasyMock.replay(mockCommentValidator);
+
+ ReviewInput input = new ReviewInput();
+ input.drafts = DraftHandling.PUBLISH;
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).hasSize(2);
+
+ assertThat(capture.getValues()).hasSize(1);
+ assertThat(capture.getValue())
+ .containsExactly(
+ CommentForValidation.create(
+ CommentForValidation.CommentType.INLINE_COMMENT, draftInline.message),
+ CommentForValidation.create(
+ CommentForValidation.CommentType.FILE_COMMENT, draftFile.message));
+ }
+
+ @Test
+ public void validateCommentsInChangeMessage_messageOK() throws Exception {
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of());
+ EasyMock.replay(mockCommentValidator);
+ PushOneCommit.Result r = createChange();
+
+ ReviewInput input = new ReviewInput().message(COMMENT_TEXT);
+ int numMessages = gApi.changes().id(r.getChangeId()).get().messages.size();
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ assertThat(gApi.changes().id(r.getChangeId()).get().messages).hasSize(numMessages + 1);
+ ChangeMessageInfo message =
+ Iterables.getLast(gApi.changes().id(r.getChangeId()).get().messages);
+ assertThat(message.message).contains(COMMENT_TEXT);
+ }
+
+ @Test
+ public void validateCommentsInChangeMessage_messageRejected() throws Exception {
+ CommentForValidation commentForValidation =
+ CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT);
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
+ EasyMock.replay(mockCommentValidator);
+ PushOneCommit.Result r = createChange();
+
+ ReviewInput input = new ReviewInput().message(COMMENT_TEXT);
+ assertThat(gApi.changes().id(r.getChangeId()).get().messages)
+ .hasSize(1); // From the initial commit.
+ BadRequestException badRequestException =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().review(input));
+ assertThat(badRequestException.getCause()).isInstanceOf(CommentsRejectedException.class);
+ assertThat(
+ Iterables.getOnlyElement(
+ ((CommentsRejectedException) badRequestException.getCause())
+ .getCommentValidationFailures())
+ .getComment()
+ .getText())
+ .isEqualTo(COMMENT_TEXT);
+ assertThat(badRequestException.getCause()).hasMessageThat().contains("Oh no!");
+ assertThat(gApi.changes().id(r.getChangeId()).get().messages)
+ .hasSize(1); // Unchanged from before.
+ ChangeMessageInfo message =
+ Iterables.getLast(gApi.changes().id(r.getChangeId()).get().messages);
+ assertThat(message.message).doesNotContain(COMMENT_TEXT);
+ }
+
+ private static CommentInput newComment(String path) {
+ return TestCommentHelper.populate(new CommentInput(), path, PostReviewIT.COMMENT_TEXT);
+ }
+}
