Update CommentValidator interface to include change context

Include the project and change number when calling to validate comments
so that validators can use that data to determine if they should run
validation for that comment.

Bug: Issue 12055
Change-Id: I99bb99350b08e9527e9e4b0da53eb8c8d14a3340
This commit is contained in:
Brian Egizi
2020-01-09 14:45:23 -08:00
parent aec070c97e
commit cbae09b1ba
10 changed files with 159 additions and 34 deletions

View File

@@ -0,0 +1,49 @@
// Copyright (C) 2020 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.extensions.validators;
import com.google.auto.value.AutoValue;
/**
* Holds a comment validators context in order to pass it to a validation plugin.
*
* <p>This is used to provided additional context around that comment that can be used by the
* validator to determine what validations should be run. For example, a comment validator may only
* want to validate a comment if it's on a change in the project foo.
*
* @see CommentValidator
*/
@AutoValue
public abstract class CommentValidationContext {
/** Returns the change id the comment is being added to. */
public abstract int getChangeId();
/** Returns the project the comment is being added to. */
public abstract String getProject();
public static Builder builder() {
return new AutoValue_CommentValidationContext.Builder();
}
@AutoValue.Builder
public abstract static class Builder {
public abstract Builder changeId(int value);
public abstract Builder project(String value);
public abstract CommentValidationContext build();
}
}

View File

@@ -30,5 +30,5 @@ public interface CommentValidator {
* @return An empty list if all comments are valid, or else a list of validation failures.
*/
ImmutableList<CommentValidationFailure> validateComments(
ImmutableList<CommentForValidation> comments);
CommentValidationContext ctx, ImmutableList<CommentForValidation> comments);
}

View File

@@ -25,6 +25,7 @@ import com.google.gerrit.entities.Comment.Status;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -118,16 +119,18 @@ public class PublishCommentUtil {
/**
* Helper to run the specified set of {@link CommentValidator}-s on the specified comments.
*
* @return See {@link CommentValidator#validateComments(ImmutableList)}.
* @return See {@link CommentValidator#validateComments(CommentValidationContext,ImmutableList)}.
*/
public static ImmutableList<CommentValidationFailure> findInvalidComments(
CommentValidationContext ctx,
PluginSetContext<CommentValidator> commentValidators,
ImmutableList<CommentForValidation> commentsForValidation) {
ImmutableList.Builder<CommentValidationFailure> commentValidationFailures =
new ImmutableList.Builder<>();
commentValidators.runEach(
validator ->
commentValidationFailures.addAll(validator.validateComments(commentsForValidation)));
commentValidationFailures.addAll(
validator.validateComments(ctx, commentsForValidation)));
return commentValidationFailures.build();
}
}

View File

@@ -93,6 +93,7 @@ import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentForValidation.CommentType;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.ApprovalsUtil;
@@ -2014,8 +2015,13 @@ class ReceiveCommits {
: CommentType.FILE_COMMENT,
comment.message))
.collect(toImmutableList());
CommentValidationContext ctx =
CommentValidationContext.builder()
.changeId(change.getChangeId())
.project(change.getProject().get())
.build();
ImmutableList<CommentValidationFailure> commentValidationFailures =
PublishCommentUtil.findInvalidComments(commentValidators, draftsForValidation);
PublishCommentUtil.findInvalidComments(ctx, commentValidators, draftsForValidation);
magicBranch.setCommentsValid(commentValidationFailures.isEmpty());
commentValidationFailures.forEach(
failure ->
@@ -3337,7 +3343,8 @@ class ReceiveCommits {
}
logger.atFine().log(
"Auto-closing %d changes with existing patch sets and %d with new patch sets",
"Auto-closing %d changes with existing patch sets and %d with new patch"
+ " sets",
existingPatchSets, newPatchSets);
bu.execute();
} catch (IOException | StorageException | PermissionBackendException e) {

View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.git.validators;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -33,7 +34,7 @@ public class CommentLimitsValidator implements CommentValidator {
@Override
public ImmutableList<CommentValidationFailure> validateComments(
ImmutableList<CommentForValidation> comments) {
CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) {
return comments.stream()
.filter(c -> c.getText().length() > maxCommentLength)
.map(

View File

@@ -34,6 +34,7 @@ import com.google.gerrit.extensions.registration.Extension;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.mail.HtmlParser;
@@ -287,8 +288,14 @@ public class MailProcessor {
MAIL_COMMENT_TYPE_TO_VALIDATION_TYPE.get(comment.getType()),
comment.getMessage()))
.collect(ImmutableList.toImmutableList());
CommentValidationContext commentValidationCtx =
CommentValidationContext.builder()
.changeId(cd.change().getChangeId())
.project(cd.change().getProject().get())
.build();
ImmutableList<CommentValidationFailure> commentValidationFailures =
PublishCommentUtil.findInvalidComments(commentValidators, parsedCommentsForValidation);
PublishCommentUtil.findInvalidComments(
commentValidationCtx, commentValidators, parsedCommentsForValidation);
if (!commentValidationFailures.isEmpty()) {
sendRejectionEmail(message, InboundEmailRejectionSender.Error.COMMENT_REJECTED);
return;

View File

@@ -77,6 +77,7 @@ import com.google.gerrit.extensions.restapi.RestModifyView;
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.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.json.OutputFormat;
@@ -999,16 +1000,22 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
CommentValidationContext commentValidationCtx =
CommentValidationContext.builder()
.changeId(ctx.getChange().getChangeId())
.project(ctx.getChange().getProject().get())
.build();
switch (in.drafts) {
case PUBLISH:
case PUBLISH_ALL_REVISIONS:
validateComments(Streams.concat(drafts.values().stream(), toPublish.stream()));
validateComments(
commentValidationCtx, Streams.concat(drafts.values().stream(), toPublish.stream()));
publishCommentUtil.publish(ctx, ctx.getUpdate(psId), drafts.values(), in.tag);
comments.addAll(drafts.values());
break;
case KEEP:
default:
validateComments(toPublish.stream());
validateComments(commentValidationCtx, toPublish.stream());
break;
}
ChangeUpdate changeUpdate = ctx.getUpdate(psId);
@@ -1017,7 +1024,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
return !toPublish.isEmpty();
}
private void validateComments(Stream<Comment> comments) throws CommentsRejectedException {
private void validateComments(CommentValidationContext ctx, Stream<Comment> comments)
throws CommentsRejectedException {
ImmutableList<CommentForValidation> draftsForValidation =
comments
.map(
@@ -1029,7 +1037,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
comment.message))
.collect(toImmutableList());
ImmutableList<CommentValidationFailure> draftValidationFailures =
PublishCommentUtil.findInvalidComments(commentValidators, draftsForValidation);
PublishCommentUtil.findInvalidComments(ctx, commentValidators, draftsForValidation);
if (!draftValidationFailures.isEmpty()) {
throw new CommentsRejectedException(draftValidationFailures);
}
@@ -1415,8 +1423,14 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
buf.append(String.format("\n\n(%d comments)", comments.size()));
}
if (!msg.isEmpty()) {
CommentValidationContext commentValidationCtx =
CommentValidationContext.builder()
.changeId(ctx.getChange().getChangeId())
.project(ctx.getChange().getProject().get())
.build();
ImmutableList<CommentValidationFailure> messageValidationFailure =
PublishCommentUtil.findInvalidComments(
commentValidationCtx,
commentValidators,
ImmutableList.of(
CommentForValidation.create(

View File

@@ -16,6 +16,7 @@ package com.google.gerrit.acceptance.api.change;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -37,6 +38,7 @@ 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.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.restapi.change.PostReview;
import com.google.gerrit.server.update.CommentsRejectedException;
@@ -80,14 +82,17 @@ public class PostReviewIT extends AbstractDaemonTest {
@Test
public void validateCommentsInInput_commentOK() throws Exception {
PushOneCommit.Result r = createChange();
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(
CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT))))
.thenReturn(ImmutableList.of());
PushOneCommit.Result r = createChange();
ReviewInput input = new ReviewInput();
CommentInput comment = newComment(r.getChange().currentFilePaths().get(0));
comment.updated = new Timestamp(0);
@@ -101,14 +106,17 @@ public class PostReviewIT extends AbstractDaemonTest {
@Test
public void validateCommentsInInput_commentRejected() throws Exception {
PushOneCommit.Result r = createChange();
CommentForValidation commentForValidation =
CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT);
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT))))
.thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
PushOneCommit.Result r = createChange();
ReviewInput input = new ReviewInput();
CommentInput comment = newComment(r.getChange().currentFilePaths().get(0));
comment.updated = new Timestamp(0);
@@ -151,14 +159,17 @@ public class PostReviewIT extends AbstractDaemonTest {
@Test
public void validateDrafts_draftOK() throws Exception {
PushOneCommit.Result r = createChange();
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(
CommentForValidation.CommentType.INLINE_COMMENT, COMMENT_TEXT))))
.thenReturn(ImmutableList.of());
PushOneCommit.Result r = createChange();
DraftInput draft =
testCommentHelper.newDraft(
r.getChange().currentFilePaths().get(0), Side.REVISION, 1, COMMENT_TEXT);
@@ -174,14 +185,18 @@ public class PostReviewIT extends AbstractDaemonTest {
@Test
public void validateDrafts_draftRejected() throws Exception {
PushOneCommit.Result r = createChange();
CommentForValidation commentForValidation =
CommentForValidation.create(CommentType.INLINE_COMMENT, COMMENT_TEXT);
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(
CommentForValidation.CommentType.INLINE_COMMENT, COMMENT_TEXT))))
.thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
PushOneCommit.Result r = createChange();
DraftInput draft =
testCommentHelper.newDraft(
@@ -218,7 +233,8 @@ public class PostReviewIT extends AbstractDaemonTest {
testCommentHelper.addDraft(r.getChangeId(), r.getCommit().getName(), draftFile);
assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
when(mockCommentValidator.validateComments(capture.capture())).thenReturn(ImmutableList.of());
when(mockCommentValidator.validateComments(any(), capture.capture()))
.thenReturn(ImmutableList.of());
ReviewInput input = new ReviewInput();
input.drafts = DraftHandling.PUBLISH;
@@ -236,11 +252,15 @@ public class PostReviewIT extends AbstractDaemonTest {
@Test
public void validateCommentsInChangeMessage_messageOK() throws Exception {
PushOneCommit.Result r = createChange();
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT))))
.thenReturn(ImmutableList.of());
PushOneCommit.Result r = createChange();
ReviewInput input = new ReviewInput().message(COMMENT_TEXT);
int numMessages = gApi.changes().id(r.getChangeId()).get().messages.size();
@@ -253,13 +273,17 @@ public class PostReviewIT extends AbstractDaemonTest {
@Test
public void validateCommentsInChangeMessage_messageRejected() throws Exception {
PushOneCommit.Result r = createChange();
CommentForValidation commentForValidation =
CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT);
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT))))
.thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
PushOneCommit.Result r = createChange();
ReviewInput input = new ReviewInput().message(COMMENT_TEXT);
assertThat(gApi.changes().id(r.getChangeId()).get().messages)

View File

@@ -32,6 +32,7 @@ import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentForValidation.CommentType;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.testing.TestCommentHelper;
import com.google.inject.Inject;
@@ -53,6 +54,7 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
private static final String COMMENT_TEXT = "The comment text";
@Captor private ArgumentCaptor<ImmutableList<CommentForValidation>> capture;
@Captor private ArgumentCaptor<CommentValidationContext> captureCtx;
@Override
public Module createModule() {
@@ -76,14 +78,18 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
@Test
public void validateComments_commentOK() throws Exception {
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
String revId = result.getCommit().getName();
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(result.getChange().getId().get())
.project(result.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(
CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT))))
.thenReturn(ImmutableList.of());
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
String revId = result.getCommit().getName();
DraftInput comment = testCommentHelper.newDraft(COMMENT_TEXT);
testCommentHelper.addDraft(changeId, revId, comment);
assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).isEmpty();
@@ -97,14 +103,18 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
public void validateComments_commentRejected() throws Exception {
CommentForValidation commentForValidation =
CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT);
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
String revId = result.getCommit().getName();
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(result.getChange().getId().get())
.project(result.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(
CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT))))
.thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
String revId = result.getCommit().getName();
DraftInput comment = testCommentHelper.newDraft(COMMENT_TEXT);
testCommentHelper.addDraft(changeId, revId, comment);
assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).isEmpty();
@@ -116,7 +126,8 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
@Test
public void validateComments_inlineVsFileComments_allOK() throws Exception {
when(mockCommentValidator.validateComments(capture.capture())).thenReturn(ImmutableList.of());
when(mockCommentValidator.validateComments(captureCtx.capture(), capture.capture()))
.thenReturn(ImmutableList.of());
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
String revId = result.getCommit().getName();
@@ -132,6 +143,9 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
assertThat(capture.getAllValues()).hasSize(1);
assertThat(captureCtx.getValue().getProject()).isEqualTo(result.getChange().project().get());
assertThat(captureCtx.getValue().getChangeId()).isEqualTo(result.getChange().getId().get());
assertThat(capture.getAllValues().get(0))
.containsExactly(
CommentForValidation.create(
@@ -143,7 +157,7 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
@Test
@GerritConfig(name = "change.maxCommentLength", value = "" + MAX_COMMENT_LENGTH)
public void validateComments_enforceLimits_commentTooLarge() throws Exception {
when(mockCommentValidator.validateComments(any())).thenReturn(ImmutableList.of());
when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of());
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
int commentLength = MAX_COMMENT_LENGTH + 1;

View File

@@ -29,6 +29,7 @@ import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.mail.MailMessage;
import com.google.gerrit.mail.MailProcessingUtil;
@@ -70,7 +71,7 @@ public class MailProcessorIT extends AbstractMailIT {
@BeforeClass
public static void setUpMock() {
// Let the mock comment validator accept all comments during test setup.
when(mockCommentValidator.validateComments(any())).thenReturn(ImmutableList.of());
when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of());
}
@Before
@@ -274,7 +275,8 @@ public class MailProcessorIT extends AbstractMailIT {
MailProcessingUtil.rfcDateformatter.format(
ZonedDateTime.ofInstant(comments.get(0).updated.toInstant(), ZoneId.of("UTC")));
setupFailValidation(CommentForValidation.CommentType.CHANGE_MESSAGE);
setupFailValidation(
CommentForValidation.CommentType.CHANGE_MESSAGE, changeInfo.project, changeInfo._number);
MailMessage.Builder b = messageBuilderWithDefaultFields();
String txt = newPlaintextBody(getChangeUrl(changeInfo) + "/1", COMMENT_TEXT, null, null, null);
@@ -298,7 +300,8 @@ public class MailProcessorIT extends AbstractMailIT {
MailProcessingUtil.rfcDateformatter.format(
ZonedDateTime.ofInstant(comments.get(0).updated.toInstant(), ZoneId.of("UTC")));
setupFailValidation(CommentForValidation.CommentType.INLINE_COMMENT);
setupFailValidation(
CommentForValidation.CommentType.INLINE_COMMENT, changeInfo.project, changeInfo._number);
MailMessage.Builder b = messageBuilderWithDefaultFields();
String txt = newPlaintextBody(getChangeUrl(changeInfo) + "/1", null, COMMENT_TEXT, null, null);
@@ -322,7 +325,8 @@ public class MailProcessorIT extends AbstractMailIT {
MailProcessingUtil.rfcDateformatter.format(
ZonedDateTime.ofInstant(comments.get(0).updated.toInstant(), ZoneId.of("UTC")));
setupFailValidation(CommentForValidation.CommentType.FILE_COMMENT);
setupFailValidation(
CommentForValidation.CommentType.FILE_COMMENT, changeInfo.project, changeInfo._number);
MailMessage.Builder b = messageBuilderWithDefaultFields();
String txt = newPlaintextBody(getChangeUrl(changeInfo) + "/1", null, null, COMMENT_TEXT, null);
@@ -341,10 +345,12 @@ public class MailProcessorIT extends AbstractMailIT {
return canonicalWebUrl.get() + "c/" + changeInfo.project + "/+/" + changeInfo._number;
}
private void setupFailValidation(CommentForValidation.CommentType type) {
private void setupFailValidation(
CommentForValidation.CommentType type, String failProject, int failChange) {
CommentForValidation commentForValidation = CommentForValidation.create(type, COMMENT_TEXT);
when(mockCommentValidator.validateComments(
CommentValidationContext.builder().changeId(failChange).project(failProject).build(),
ImmutableList.of(CommentForValidation.create(type, COMMENT_TEXT))))
.thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
}