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
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 98ed97a..09628ab 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -17,9 +17,9 @@
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.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 @@
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 @@
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 @@
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 @@
Optional<AuthException> err = checkRefPermission(cmd, RefPermission.DELETE);
if (!err.isPresent()) {
validRefOperation(cmd);
-
} else {
rejectProhibited(cmd, err.get());
}
@@ -1375,6 +1387,13 @@
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 @@
.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 @@
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 @@
}
/**
- * Add an update for an existing change. Returns true if it succeeded; rejects the command if it
- * failed.
+ * Update an existing change. If draft comments are to be published, these are validated and may
+ * 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 @@
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 @@
}
}
- 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 @@
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 @@
}
}
- for (String changeId : c.getFooterLines(CHANGE_ID)) {
+ for (String changeId : c.getFooterLines(FooterConstants.CHANGE_ID)) {
if (byKey == null) {
byKey = executeIndexQuery(() -> openChangesByKeyByBranch(branch));
}
diff --git a/javatests/com/google/gerrit/acceptance/server/git/receive/BUILD b/javatests/com/google/gerrit/acceptance/server/git/receive/BUILD
new file mode 100644
index 0000000..760e7f4
--- /dev/null
+++ b/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 = [],
+)
diff --git a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
new file mode 100644
index 0000000..cb5add3
--- /dev/null
+++ b/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));
+ }
+}
