Validate comments in ReceiveCommits using new interface

This commit adds logic to ReceiveCommits to validate all comments that
are published using the publish-comments option.

Tests cover acceptance, rejection and distinction between inline and
file comments.

Change-Id: Iecc18222753ac2e9e262eef135bae5a924322fef

java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -17,9 +17,9 @@ package com.google.gerrit.server.git.receive;
 import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.flogger.LazyArgs.lazy;
-import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
 import static com.google.gerrit.git.ObjectIds.abbreviateName;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES;
 import static com.google.gerrit.reviewdb.client.RefNames.isConfigRef;
@@ -81,19 +81,26 @@ import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 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.CommentValidationFailure;
+import com.google.gerrit.extensions.validators.CommentValidator;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
 import com.google.gerrit.reviewdb.client.BranchNameKey;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Comment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.CommentsUtil;
 import com.google.gerrit.server.CreateGroupPermissionSyncer;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.PublishCommentUtil;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.NotifyResolver;
@@ -301,6 +308,8 @@ class ReceiveCommits {
   private final ChangeNotes.Factory notesFactory;
   private final ChangeReportFormatter changeFormatter;
   private final CmdLineParser.Factory optionParserFactory;
+  private final CommentsUtil commentsUtil;
+  private final PluginSetContext<CommentValidator> commentValidators;
   private final BranchCommitValidator.Factory commitValidatorFactory;
   private final CreateGroupPermissionSyncer createGroupPermissionSyncer;
   private final CreateRefControl createRefControl;
@@ -378,11 +387,13 @@ class ReceiveCommits {
       ChangeNotes.Factory notesFactory,
       DynamicItem<ChangeReportFormatter> changeFormatterProvider,
       CmdLineParser.Factory optionParserFactory,
+      CommentsUtil commentsUtil,
       BranchCommitValidator.Factory commitValidatorFactory,
       CreateGroupPermissionSyncer createGroupPermissionSyncer,
       CreateRefControl createRefControl,
       DynamicMap<ProjectConfigEntry> pluginConfigEntries,
       PluginSetContext<ReceivePackInitializer> initializers,
+      PluginSetContext<CommentValidator> commentValidators,
       MergedByPushOp.Factory mergedByPushOpFactory,
       PatchSetInfoFactory patchSetInfoFactory,
       PatchSetUtil psUtil,
@@ -415,6 +426,8 @@ class ReceiveCommits {
     this.batchUpdateFactory = batchUpdateFactory;
     this.changeFormatter = changeFormatterProvider.get();
     this.changeInserterFactory = changeInserterFactory;
+    this.commentsUtil = commentsUtil;
+    this.commentValidators = commentValidators;
     this.commitValidatorFactory = commitValidatorFactory;
     this.createRefControl = createRefControl;
     this.createGroupPermissionSyncer = createGroupPermissionSyncer;
@@ -1299,7 +1312,6 @@ class ReceiveCommits {
     Optional<AuthException> err = checkRefPermission(cmd, RefPermission.DELETE);
     if (!err.isPresent()) {
       validRefOperation(cmd);
-
     } else {
       rejectProhibited(cmd, err.get());
     }
@@ -1375,6 +1387,13 @@ class ReceiveCommits {
     final ReceiveCommand cmd;
     final LabelTypes labelTypes;
     private final boolean defaultPublishComments;
+    /**
+     * Result of running {@link CommentValidator}-s on drafts that are published with the commit
+     * (which happens iff {@code --publish-comments} is set). Remains {@code true} if none are
+     * installed.
+     */
+    private boolean commentsValid = true;
+
     BranchNameKey dest;
     PermissionBackend.ForRef perm;
     Set<String> reviewer = Sets.newLinkedHashSet();
@@ -1577,7 +1596,15 @@ class ReceiveCommits {
           .collect(toImmutableSet());
     }
 
+    void setCommentsValid(boolean commentsValid) {
+      this.commentsValid = commentsValid;
+    }
+
     boolean shouldPublishComments() {
+      if (!commentsValid) {
+        // Validation messages of type WARNING have already been added, now withhold the comments.
+        return false;
+      }
       if (publishComments) {
         return true;
       } else if (noPublishComments) {
@@ -1974,7 +2001,7 @@ class ReceiveCommits {
       messages.addAll(validationResult.messages());
       if (validationResult.isValid()) {
         logger.atFine().log("Replacing change %s", changeEnt.getId());
-        requestReplace(cmd, true, changeEnt, newCommit);
+        requestReplaceAndValidateComments(cmd, true, changeEnt, newCommit);
       }
     } catch (IOException e) {
       reject(cmd, "I/O exception validating commit");
@@ -1982,10 +2009,12 @@ class ReceiveCommits {
   }
 
   /**
-   * Add an update for an existing change. Returns true if it succeeded; rejects the command if it
+   * Update an existing change. If draft comments are to be published, these are validated and may
-   * failed.
+   * be withheld.
+   *
+   * @return True if the command succeeded, false if it was rejected.
    */
-  private boolean requestReplace(
+  private boolean requestReplaceAndValidateComments(
       ReceiveCommand cmd, boolean checkMergedInto, Change change, RevCommit newCommit) {
     if (change.isClosed()) {
       reject(
@@ -2000,6 +2029,30 @@ class ReceiveCommits {
       reject(cmd, "duplicate request");
       return false;
     }
+
+    if (magicBranch != null && magicBranch.shouldPublishComments()) {
+      List<Comment> drafts =
+          commentsUtil.draftByChangeAuthor(notesFactory.createChecked(change), user.getAccountId());
+      ImmutableList<CommentForValidation> draftsForValidation =
+          drafts.stream()
+              .map(
+                  comment ->
+                      CommentForValidation.create(
+                          comment.lineNbr > 0
+                              ? CommentType.INLINE_COMMENT
+                              : CommentType.FILE_COMMENT,
+                          comment.message))
+              .collect(toImmutableList());
+      List<CommentValidationFailure> commentValidationFailures =
+          PublishCommentUtil.findInvalidComments(commentValidators, draftsForValidation);
+      magicBranch.setCommentsValid(commentValidationFailures.isEmpty());
+      commentValidationFailures.forEach(
+          failure ->
+              addMessage(
+                  "Comment validation failure: " + failure.getMessage(),
+                  ValidationMessage.Type.WARNING));
+    }
+
     replaceByChange.put(req.ontoChange, req);
     return true;
   }
@@ -2099,7 +2152,7 @@ class ReceiveCommits {
           }
         }
 
-        List<String> idList = c.getFooterLines(CHANGE_ID);
+        List<String> idList = c.getFooterLines(FooterConstants.CHANGE_ID);
         if (!idList.isEmpty()) {
           pending.put(c, lookupByChangeKey(c, Change.key(idList.get(idList.size() - 1).trim())));
         } else {
@@ -2216,7 +2269,8 @@ class ReceiveCommits {
               continue;
             }
           }
-          if (requestReplace(magicBranch.cmd, false, changes.get(0).change(), p.commit)) {
+          if (requestReplaceAndValidateComments(
+              magicBranch.cmd, false, changes.get(0).change(), p.commit)) {
             continue;
           }
           return Collections.emptyList();
@@ -3195,7 +3249,7 @@ class ReceiveCommits {
                   }
                 }
 
-                for (String changeId : c.getFooterLines(CHANGE_ID)) {
+                for (String changeId : c.getFooterLines(FooterConstants.CHANGE_ID)) {
                   if (byKey == null) {
                     byKey = executeIndexQuery(() -> openChangesByKeyByBranch(branch));
                   }

javatests/com/google/gerrit/acceptance/server/git/receive/BUILD

@@ -0,0 +1,8 @@
+load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests")
+
+acceptance_tests(
+    srcs = glob(["*IT.java"]),
+    group = "receive",
+    labels = ["server"],
+    deps = [],
+)

javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java

@@ -0,0 +1,143 @@
+// 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.server.git.receive;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.PushOneCommit.Result;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.extensions.api.changes.DraftInput;
+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.CommentValidator;
+import com.google.gerrit.testing.TestCommentHelper;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import org.easymock.Capture;
+import org.easymock.EasyMock;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Tests for comment validation when publishing drafts via the {@code --publish-comments} option.
+ */
+public class ReceiveCommitsCommentValidationIT 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 validateComments_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 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();
+    Result amendResult = amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
+    amendResult.assertOkStatus();
+    amendResult.assertNotMessage("Comment validation failure:");
+    assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).hasSize(1);
+  }
+
+  @Test
+  public void validateComments_commentRejected() throws Exception {
+    CommentForValidation commentForValidation =
+        CommentForValidation.create(CommentType.FILE_COMMENT, 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);
+    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();
+    Result amendResult = amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
+    amendResult.assertOkStatus();
+    amendResult.assertMessage("Comment validation failure:");
+    assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).isEmpty();
+  }
+
+  @Test
+  public void validateComments_inlineVsFileComments_allOK() throws Exception {
+    EasyMock.expect(mockCommentValidator.validateComments(EasyMock.capture(capture)))
+        .andReturn(ImmutableList.of());
+    EasyMock.replay(mockCommentValidator);
+    PushOneCommit.Result result = createChange();
+    String changeId = result.getChangeId();
+    String revId = result.getCommit().getName();
+    DraftInput draftFile = testCommentHelper.newDraft(COMMENT_TEXT);
+    testCommentHelper.addDraft(changeId, revId, draftFile);
+    DraftInput draftInline =
+        testCommentHelper.newDraft(
+            result.getChange().currentFilePaths().get(0), Side.REVISION, 1, COMMENT_TEXT);
+    testCommentHelper.addDraft(changeId, revId, draftInline);
+    assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).isEmpty();
+    amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
+    assertThat(testCommentHelper.getPublishedComments(result.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));
+  }
+}