Validate comments in MailProcessor using new interface
Adds logic to MailProcessor to validate all comments received via email.
Test case covers acceptance (existing tests) and rejection (new tests)
as well as distinction between inline/file/change message comments.
Change-Id: Iad889893cd3007c18aa36103fe1bd96491fdb8d8
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index 3655369..9db004e 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -18,6 +18,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.Side;
@@ -26,6 +27,9 @@
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.CommentValidationFailure;
+import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.mail.HtmlParser;
import com.google.gerrit.mail.MailComment;
import com.google.gerrit.mail.MailHeaderParser;
@@ -43,6 +47,7 @@
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.PublishCommentUtil;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.Emails;
@@ -54,6 +59,7 @@
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.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.update.BatchUpdate;
@@ -83,6 +89,15 @@
public class MailProcessor {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static final ImmutableMap<MailComment.CommentType, CommentForValidation.CommentType>
+ MAIL_COMMENT_TYPE_TO_VALIDATION_TYPE =
+ ImmutableMap.of(
+ MailComment.CommentType.CHANGE_MESSAGE,
+ CommentForValidation.CommentType.CHANGE_MESSAGE,
+ MailComment.CommentType.FILE_COMMENT, CommentForValidation.CommentType.FILE_COMMENT,
+ MailComment.CommentType.INLINE_COMMENT,
+ CommentForValidation.CommentType.INLINE_COMMENT);
+
private final Emails emails;
private final InboundEmailRejectionSender.Factory emailRejectionSender;
private final RetryHelper retryHelper;
@@ -98,6 +113,7 @@
private final ApprovalsUtil approvalsUtil;
private final AccountCache accountCache;
private final DynamicItem<UrlFormatter> urlFormatter;
+ private final PluginSetContext<CommentValidator> commentValidators;
@Inject
public MailProcessor(
@@ -115,7 +131,8 @@
ApprovalsUtil approvalsUtil,
CommentAdded commentAdded,
AccountCache accountCache,
- DynamicItem<UrlFormatter> urlFormatter) {
+ DynamicItem<UrlFormatter> urlFormatter,
+ PluginSetContext<CommentValidator> commentValidators) {
this.emails = emails;
this.emailRejectionSender = emailRejectionSender;
this.retryHelper = retryHelper;
@@ -131,6 +148,7 @@
this.approvalsUtil = approvalsUtil;
this.accountCache = accountCache;
this.urlFormatter = urlFormatter;
+ this.commentValidators = commentValidators;
}
/**
@@ -259,6 +277,21 @@
return;
}
+ ImmutableList<CommentForValidation> parsedCommentsForValidation =
+ parsedComments.stream()
+ .map(
+ comment ->
+ CommentForValidation.create(
+ MAIL_COMMENT_TYPE_TO_VALIDATION_TYPE.get(comment.getType()),
+ comment.getMessage()))
+ .collect(ImmutableList.toImmutableList());
+ List<CommentValidationFailure> commentValidationFailures =
+ PublishCommentUtil.findInvalidComments(commentValidators, parsedCommentsForValidation);
+ if (!commentValidationFailures.isEmpty()) {
+ sendRejectionEmail(message, InboundEmailRejectionSender.Error.COMMENT_REJECTED);
+ return;
+ }
+
Op o = new Op(PatchSet.id(cd.getId(), metadata.patchSet), parsedComments, message.id());
BatchUpdate batchUpdate = buf.create(project, ctx.getUser(), TimeUtil.nowTs());
batchUpdate.addOp(cd.getId(), o);
diff --git a/java/com/google/gerrit/server/mail/send/InboundEmailRejectionSender.java b/java/com/google/gerrit/server/mail/send/InboundEmailRejectionSender.java
index b5d384d..f0b3c66 100644
--- a/java/com/google/gerrit/server/mail/send/InboundEmailRejectionSender.java
+++ b/java/com/google/gerrit/server/mail/send/InboundEmailRejectionSender.java
@@ -32,7 +32,8 @@
PARSING_ERROR,
INACTIVE_ACCOUNT,
UNKNOWN_ACCOUNT,
- INTERNAL_EXCEPTION;
+ INTERNAL_EXCEPTION,
+ COMMENT_REJECTED
}
public interface Factory {
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
index f917fd8..6e14635 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
@@ -16,25 +16,61 @@
import static com.google.common.truth.Truth.assertThat;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.common.ChangeInfo;
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.CommentValidator;
import com.google.gerrit.mail.MailMessage;
import com.google.gerrit.mail.MailProcessingUtil;
import com.google.gerrit.server.mail.receive.MailProcessor;
import com.google.gerrit.testing.FakeEmailSender.Message;
+import com.google.gerrit.testing.TestCommentHelper;
import com.google.inject.Inject;
+import com.google.inject.Module;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.util.Collection;
import java.util.List;
+import org.easymock.EasyMock;
+import org.junit.Before;
import org.junit.Test;
public class MailProcessorIT extends AbstractMailIT {
@Inject private MailProcessor mailProcessor;
@Inject private AccountOperations accountOperations;
+ @Inject private CommentValidator mockCommentValidator;
+ @Inject private TestCommentHelper testCommentHelper;
+
+ private static final String COMMENT_TEXT = "The comment text";
+
+ @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 setUp() {
+ // Let the mock comment validator accept all comments during test setup.
+ EasyMock.reset(mockCommentValidator);
+ EasyMock.expect(mockCommentValidator.validateComments(EasyMock.anyObject()))
+ .andReturn(ImmutableList.of());
+ EasyMock.replay(mockCommentValidator);
+ }
@Test
public void parseAndPersistChangeMessage() throws Exception {
@@ -223,6 +259,108 @@
assertThat(message.headers()).containsKey("Subject");
}
+ @Test
+ public void validateChangeMessage_rejected() throws Exception {
+ String changeId = createChangeWithReview();
+ ChangeInfo changeInfo = gApi.changes().id(changeId).get();
+ List<CommentInfo> comments = gApi.changes().id(changeId).current().commentsAsList();
+ String ts =
+ MailProcessingUtil.rfcDateformatter.format(
+ ZonedDateTime.ofInstant(comments.get(0).updated.toInstant(), ZoneId.of("UTC")));
+
+ EasyMock.reset(mockCommentValidator);
+ CommentForValidation commentForValidation =
+ CommentForValidation.create(CommentForValidation.CommentType.CHANGE_MESSAGE, COMMENT_TEXT);
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(
+ CommentForValidation.CommentType.CHANGE_MESSAGE, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
+ EasyMock.replay(mockCommentValidator);
+
+ MailMessage.Builder b = messageBuilderWithDefaultFields();
+ String txt = newPlaintextBody(getChangeUrl(changeInfo) + "/1", COMMENT_TEXT, null, null, null);
+ b.textContent(txt + textFooterForChange(changeInfo._number, ts));
+
+ Collection<CommentInfo> commentsBefore = testCommentHelper.getPublishedComments(changeId);
+ mailProcessor.process(b.build());
+ assertThat(testCommentHelper.getPublishedComments(changeId)).isEqualTo(commentsBefore);
+
+ assertNotifyTo(user);
+ Message message = sender.nextMessage();
+ assertThat(message.body()).contains("rejected one or more comments");
+ EasyMock.verify(mockCommentValidator);
+ }
+
+ @Test
+ public void validateInlineComment_rejected() throws Exception {
+ String changeId = createChangeWithReview();
+ ChangeInfo changeInfo = gApi.changes().id(changeId).get();
+ List<CommentInfo> comments = gApi.changes().id(changeId).current().commentsAsList();
+ String ts =
+ MailProcessingUtil.rfcDateformatter.format(
+ ZonedDateTime.ofInstant(comments.get(0).updated.toInstant(), ZoneId.of("UTC")));
+
+ EasyMock.reset(mockCommentValidator);
+ CommentForValidation commentForValidation =
+ CommentForValidation.create(CommentForValidation.CommentType.CHANGE_MESSAGE, 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);
+
+ MailMessage.Builder b = messageBuilderWithDefaultFields();
+ String txt = newPlaintextBody(getChangeUrl(changeInfo) + "/1", null, COMMENT_TEXT, null, null);
+ b.textContent(txt + textFooterForChange(changeInfo._number, ts));
+
+ Collection<CommentInfo> commentsBefore = testCommentHelper.getPublishedComments(changeId);
+ mailProcessor.process(b.build());
+ assertThat(testCommentHelper.getPublishedComments(changeId)).isEqualTo(commentsBefore);
+
+ assertNotifyTo(user);
+ Message message = sender.nextMessage();
+ assertThat(message.body()).contains("rejected one or more comments");
+ EasyMock.verify(mockCommentValidator);
+ }
+
+ @Test
+ public void validateFileComment_rejected() throws Exception {
+ String changeId = createChangeWithReview();
+ ChangeInfo changeInfo = gApi.changes().id(changeId).get();
+ List<CommentInfo> comments = gApi.changes().id(changeId).current().commentsAsList();
+ String ts =
+ MailProcessingUtil.rfcDateformatter.format(
+ ZonedDateTime.ofInstant(comments.get(0).updated.toInstant(), ZoneId.of("UTC")));
+
+ EasyMock.reset(mockCommentValidator);
+ CommentForValidation commentForValidation =
+ CommentForValidation.create(CommentForValidation.CommentType.CHANGE_MESSAGE, COMMENT_TEXT);
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(
+ CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
+ EasyMock.replay(mockCommentValidator);
+
+ MailMessage.Builder b = messageBuilderWithDefaultFields();
+ String txt = newPlaintextBody(getChangeUrl(changeInfo) + "/1", null, null, COMMENT_TEXT, null);
+ b.textContent(txt + textFooterForChange(changeInfo._number, ts));
+
+ Collection<CommentInfo> commentsBefore = testCommentHelper.getPublishedComments(changeId);
+ mailProcessor.process(b.build());
+ assertThat(testCommentHelper.getPublishedComments(changeId)).isEqualTo(commentsBefore);
+
+ assertNotifyTo(user);
+ Message message = sender.nextMessage();
+ assertThat(message.body()).contains("rejected one or more comments");
+ EasyMock.verify(mockCommentValidator);
+ }
+
private String getChangeUrl(ChangeInfo changeInfo) {
return canonicalWebUrl.get() + "c/" + changeInfo.project + "/+/" + changeInfo._number;
}
diff --git a/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy b/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
index e997776..e88c424 100644
--- a/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
+++ b/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
@@ -62,3 +62,8 @@
This might be caused by an ongoing maintenance or a data corruption.
{call .InboundEmailRejectionFooter /}
{/template}
+
+{template .InboundEmailRejection_COMMENT_REJECTED kind="text"}
+ Gerrit Code Review rejected one or more comments because they did not pass validation.
+ {call .InboundEmailRejectionFooter /}
+{/template}
diff --git a/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy b/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
index f879270..e17508d 100644
--- a/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
+++ b/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
@@ -78,3 +78,10 @@
<p>
{call .InboundEmailRejectionFooterHtml /}
{/template}
+
+{template .InboundEmailRejectionHtml_COMMENT_REJECTED}
+ <p>
+ Gerrit Code Review rejected one or more comments because they did not pass validation.
+ </p>
+ {call .InboundEmailRejectionFooterHtml /}
+{/template}
