Merge "Update CommentValidator interface to include change context"

This commit is contained in:
Patrick Hiesel
2020-01-16 08:49:38 +00:00
committed by Gerrit Code Review
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!")));
}