From cf288d44ba848f66f013b173aa68920fa4049adb Mon Sep 17 00:00:00 2001 From: Joerg Zieren Date: Thu, 6 Jun 2019 13:23:02 +0200 Subject: [PATCH] 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 --- .../validators/CommentValidator.java | 4 +- .../gerrit/server/PublishCommentUtil.java | 21 ++ .../server/restapi/change/PostReview.java | 52 +++- .../gerrit/server/update/BatchUpdate.java | 11 +- .../update/CommentsRejectedException.java | 47 +++ .../gerrit/testing/TestCommentHelper.java | 107 +++++++ .../acceptance/api/change/PostReviewIT.java | 288 ++++++++++++++++++ 7 files changed, 520 insertions(+), 10 deletions(-) create mode 100644 java/com/google/gerrit/server/update/CommentsRejectedException.java create mode 100644 java/com/google/gerrit/testing/TestCommentHelper.java create mode 100644 javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java diff --git a/java/com/google/gerrit/extensions/validators/CommentValidator.java b/java/com/google/gerrit/extensions/validators/CommentValidator.java index cbcf1a9a8c..cfefdefc8f 100644 --- a/java/com/google/gerrit/extensions/validators/CommentValidator.java +++ b/java/com/google/gerrit/extensions/validators/CommentValidator.java @@ -25,9 +25,9 @@ import com.google.gerrit.extensions.annotations.ExtensionPoint; 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 validateComments( ImmutableList comments); diff --git a/java/com/google/gerrit/server/PublishCommentUtil.java b/java/com/google/gerrit/server/PublishCommentUtil.java index 8ae5bcd3db..6a1c859277 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.common.base.Preconditions.checkArgument; 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 @@ public class PublishCommentUtil { 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 findInvalidComments( + PluginSetContext commentValidators, + ImmutableList commentsForValidation) { + ImmutableList.Builder 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 ef38b051d7..50c15112b1 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 @@ package com.google.gerrit.server.restapi.change; 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 static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; 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.Response; 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.ChangePermission; 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.Optional; 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 @@ public class PostReview private final WorkInProgressOp.Factory workInProgressOpFactory; private final ProjectCache projectCache; private final PermissionBackend permissionBackend; + private final PluginSetContext commentValidators; private final boolean strictLabels; @Inject @@ -195,7 +206,8 @@ public class PostReview @GerritServerConfig Config gerritConfig, WorkInProgressOp.Factory workInProgressOpFactory, ProjectCache projectCache, - PermissionBackend permissionBackend) { + PermissionBackend permissionBackend, + PluginSetContext commentValidators) { super(retryHelper); this.changeResourceFactory = changeResourceFactory; this.changeDataFactory = changeDataFactory; @@ -215,6 +227,7 @@ public class PostReview this.workInProgressOpFactory = workInProgressOpFactory; this.projectCache = projectCache; this.permissionBackend = permissionBackend; + this.commentValidators = commentValidators; this.strictLabels = gerritConfig.getBoolean("change", "strictLabels", false); } @@ -858,7 +871,7 @@ public class PostReview @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 @@ public class PostReview } private boolean insertComments(ChangeContext ctx) - throws UnprocessableEntityException, PatchListNotAvailableException { + throws UnprocessableEntityException, PatchListNotAvailableException, + CommentsRejectedException { Map> inputComments = in.comments; if (inputComments == null) { inputComments = Collections.emptyMap(); @@ -910,6 +924,7 @@ public class PostReview } } + // This will be populated with Comment-s created from inputComments. List toPublish = new ArrayList<>(); Set existingComments = @@ -954,11 +969,13 @@ public class PostReview 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 @@ public class PostReview return !toPublish.isEmpty(); } + private void validateComments(Stream comments) throws CommentsRejectedException { + ImmutableList draftsForValidation = + comments + .map( + comment -> + CommentForValidation.create( + comment.lineNbr > 0 + ? CommentForValidation.CommentType.INLINE_COMMENT + : CommentType.FILE_COMMENT, + comment.message)) + .collect(toImmutableList()); + List 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 @@ public class PostReview 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 @@ public class PostReview buf.append(String.format("\n\n(%d comments)", comments.size())); } if (!msg.isEmpty()) { + List 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 2a958af731..6dee241c71 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.common.util.concurrent.ListenableFuture; 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 @@ public class BatchUpdate implements AutoCloseable { || 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 @@ public class BatchUpdate implements AutoCloseable { private enum ChangeResult { SKIPPED, UPSERTED, - DELETED; + DELETED } private final GitRepositoryManager repoManager; @@ -567,9 +572,7 @@ public class BatchUpdate implements AutoCloseable { 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 0000000000..6b0c04d77a --- /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 commentValidationFailures; + + public CommentsRejectedException(Collection 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 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 0000000000..b72cca7f01 --- /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 getPublishedComments(String changeId) throws Exception { + return gApi.changes().id(changeId).comments().values().stream() + .flatMap(Collection::stream) + .collect(toList()); + } + + public static C populate(C c, String path, String message) { + return populate(c, path, createLineRange(), message); + } + + private static 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 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 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 0000000000..ebed15cd22 --- /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> 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); + } +}