Merge changes from topic "comment-validation"
* changes:
Validate comments in MailProcessor using new interface
Validate comments in ReceiveCommits using new interface
Validate comments and message in PostReview using new interface
Add extension point for plugins to validate comments before publishing
diff --git a/java/com/google/gerrit/extensions/validators/CommentForValidation.java b/java/com/google/gerrit/extensions/validators/CommentForValidation.java
new file mode 100644
index 0000000..51ae5ae
--- /dev/null
+++ b/java/com/google/gerrit/extensions/validators/CommentForValidation.java
@@ -0,0 +1,48 @@
+// 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.extensions.validators;
+
+import com.google.auto.value.AutoValue;
+
+/**
+ * Holds a comment's text and {@link CommentType} in order to pass it to a validation plugin.
+ *
+ * @see CommentValidator
+ */
+@AutoValue
+public abstract class CommentForValidation {
+
+ /** The type of comment. */
+ public enum CommentType {
+ /** A regular (inline) comment. */
+ INLINE_COMMENT,
+ /** A file comment. */
+ FILE_COMMENT,
+ /** A change message. */
+ CHANGE_MESSAGE
+ }
+
+ public static CommentForValidation create(CommentType type, String text) {
+ return new AutoValue_CommentForValidation(type, text);
+ }
+
+ public abstract CommentType getType();
+
+ public abstract String getText();
+
+ public CommentValidationFailure failValidation(String message) {
+ return CommentValidationFailure.create(this, message);
+ }
+}
diff --git a/java/com/google/gerrit/extensions/validators/CommentValidationFailure.java b/java/com/google/gerrit/extensions/validators/CommentValidationFailure.java
new file mode 100644
index 0000000..1a832760
--- /dev/null
+++ b/java/com/google/gerrit/extensions/validators/CommentValidationFailure.java
@@ -0,0 +1,32 @@
+// 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.extensions.validators;
+
+import com.google.auto.value.AutoValue;
+
+/** A comment or review message was rejected by a {@link CommentValidator}. */
+@AutoValue
+public abstract class CommentValidationFailure {
+ static CommentValidationFailure create(
+ CommentForValidation commentForValidation, String message) {
+ return new AutoValue_CommentValidationFailure(commentForValidation, message);
+ }
+
+ /** Returns the offending comment. */
+ public abstract CommentForValidation getComment();
+
+ /** A friendly message set by the {@link CommentValidator}. */
+ public abstract String getMessage();
+}
diff --git a/java/com/google/gerrit/extensions/validators/CommentValidator.java b/java/com/google/gerrit/extensions/validators/CommentValidator.java
new file mode 100644
index 0000000..cfefdef
--- /dev/null
+++ b/java/com/google/gerrit/extensions/validators/CommentValidator.java
@@ -0,0 +1,34 @@
+// 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.extensions.validators;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.extensions.annotations.ExtensionPoint;
+
+/**
+ * Validates review comments and messages. Rejecting any comment/message will prevent all comments
+ * from being published.
+ */
+@ExtensionPoint
+public interface CommentValidator {
+
+ /**
+ * Validate the specified comments.
+ *
+ * @return An empty list if all comments are valid, or else a list of validation failures.
+ */
+ ImmutableList<CommentValidationFailure> validateComments(
+ ImmutableList<CommentForValidation> comments);
+}
diff --git a/java/com/google/gerrit/server/PublishCommentUtil.java b/java/com/google/gerrit/server/PublishCommentUtil.java
index 8ae5bcd..6a1c859 100644
--- a/java/com/google/gerrit/server/PublishCommentUtil.java
+++ b/java/com/google/gerrit/server/PublishCommentUtil.java
@@ -18,13 +18,18 @@
import static com.google.gerrit.reviewdb.client.PatchLineComment.Status.PUBLISHED;
import static java.util.stream.Collectors.toSet;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.exceptions.StorageException;
+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.reviewdb.client.Comment;
import com.google.gerrit.reviewdb.client.PatchSet;
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.update.ChangeContext;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -77,4 +82,20 @@
private static PatchSet.Id psId(ChangeNotes notes, Comment c) {
return PatchSet.id(notes.getChangeId(), c.key.patchSetId);
}
+
+ /**
+ * Helper to run the specified set of {@link CommentValidator}-s on the specified comments.
+ *
+ * @return See {@link CommentValidator#validateComments(ImmutableList)}.
+ */
+ public static ImmutableList<CommentValidationFailure> findInvalidComments(
+ PluginSetContext<CommentValidator> commentValidators,
+ ImmutableList<CommentForValidation> commentsForValidation) {
+ ImmutableList.Builder<CommentValidationFailure> commentValidationFailures =
+ new ImmutableList.Builder<>();
+ commentValidators.runEach(
+ listener ->
+ commentValidationFailures.addAll(listener.validateComments(commentsForValidation)));
+ return commentValidationFailures.build();
+ }
}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 14747e5..478cf8f 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -62,6 +62,7 @@
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.systemstatus.MessageOfTheDay;
+import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.extensions.webui.BranchWebLink;
import com.google.gerrit.extensions.webui.DiffWebLink;
import com.google.gerrit.extensions.webui.FileHistoryWebLink;
@@ -341,6 +342,7 @@
DynamicSet.bind(binder(), EventListener.class).to(EventsMetrics.class);
DynamicSet.setOf(binder(), UserScopedEventListener.class);
DynamicSet.setOf(binder(), CommitValidationListener.class);
+ DynamicSet.setOf(binder(), CommentValidator.class);
DynamicSet.setOf(binder(), ChangeMessageModifier.class);
DynamicSet.setOf(binder(), RefOperationValidationListener.class);
DynamicSet.setOf(binder(), OnSubmitValidationListener.class);
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index f69e364..b238867 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;
@@ -1301,7 +1314,6 @@
Optional<AuthException> err = checkRefPermission(cmd, RefPermission.DELETE);
if (!err.isPresent()) {
validRefOperation(cmd);
-
} else {
rejectProhibited(cmd, err.get());
}
@@ -1377,6 +1389,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();
@@ -1579,7 +1598,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) {
@@ -1976,7 +2003,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");
@@ -1984,10 +2011,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(
@@ -2002,6 +2031,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;
}
@@ -2101,7 +2154,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 {
@@ -2218,7 +2271,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();
@@ -3197,7 +3251,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/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/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index ef38b05..50c1511 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -16,6 +16,7 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.server.CommentsUtil.setCommentCommitId;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
@@ -29,9 +30,11 @@
import com.google.auto.value.AutoValue;
import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Ordering;
+import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
import com.google.common.hash.HashCode;
import com.google.common.hash.Hashing;
@@ -61,6 +64,10 @@
import com.google.gerrit.extensions.restapi.RestApiException;
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.CommentForValidation.CommentType;
+import com.google.gerrit.extensions.validators.CommentValidationFailure;
+import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.json.OutputFormat;
import com.google.gerrit.mail.Address;
import com.google.gerrit.reviewdb.client.Account;
@@ -106,12 +113,14 @@
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
+import com.google.gerrit.server.update.CommentsRejectedException;
import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
@@ -137,6 +146,7 @@
import java.util.OptionalInt;
import java.util.Set;
import java.util.stream.Collectors;
+import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
@@ -173,6 +183,7 @@
private final WorkInProgressOp.Factory workInProgressOpFactory;
private final ProjectCache projectCache;
private final PermissionBackend permissionBackend;
+ private final PluginSetContext<CommentValidator> commentValidators;
private final boolean strictLabels;
@Inject
@@ -195,7 +206,8 @@
@GerritServerConfig Config gerritConfig,
WorkInProgressOp.Factory workInProgressOpFactory,
ProjectCache projectCache,
- PermissionBackend permissionBackend) {
+ PermissionBackend permissionBackend,
+ PluginSetContext<CommentValidator> commentValidators) {
super(retryHelper);
this.changeResourceFactory = changeResourceFactory;
this.changeDataFactory = changeDataFactory;
@@ -215,6 +227,7 @@
this.workInProgressOpFactory = workInProgressOpFactory;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
+ this.commentValidators = commentValidators;
this.strictLabels = gerritConfig.getBoolean("change", "strictLabels", false);
}
@@ -858,7 +871,7 @@
@Override
public boolean updateChange(ChangeContext ctx)
throws ResourceConflictException, UnprocessableEntityException, IOException,
- PatchListNotAvailableException {
+ PatchListNotAvailableException, CommentsRejectedException {
user = ctx.getIdentifiedUser();
notes = ctx.getNotes();
ps = psUtil.get(ctx.getNotes(), psId);
@@ -891,7 +904,8 @@
}
private boolean insertComments(ChangeContext ctx)
- throws UnprocessableEntityException, PatchListNotAvailableException {
+ throws UnprocessableEntityException, PatchListNotAvailableException,
+ CommentsRejectedException {
Map<String, List<CommentInput>> inputComments = in.comments;
if (inputComments == null) {
inputComments = Collections.emptyMap();
@@ -910,6 +924,7 @@
}
}
+ // This will be populated with Comment-s created from inputComments.
List<Comment> toPublish = new ArrayList<>();
Set<CommentSetEntry> existingComments =
@@ -954,11 +969,13 @@
switch (in.drafts) {
case PUBLISH:
case PUBLISH_ALL_REVISIONS:
+ validateComments(Streams.concat(drafts.values().stream(), toPublish.stream()));
publishCommentUtil.publish(ctx, psId, drafts.values(), in.tag);
comments.addAll(drafts.values());
break;
case KEEP:
default:
+ validateComments(toPublish.stream());
break;
}
ChangeUpdate changeUpdate = ctx.getUpdate(psId);
@@ -967,6 +984,24 @@
return !toPublish.isEmpty();
}
+ private void validateComments(Stream<Comment> comments) throws CommentsRejectedException {
+ ImmutableList<CommentForValidation> draftsForValidation =
+ comments
+ .map(
+ comment ->
+ CommentForValidation.create(
+ comment.lineNbr > 0
+ ? CommentForValidation.CommentType.INLINE_COMMENT
+ : CommentType.FILE_COMMENT,
+ comment.message))
+ .collect(toImmutableList());
+ List<CommentValidationFailure> draftValidationFailures =
+ PublishCommentUtil.findInvalidComments(commentValidators, draftsForValidation);
+ if (!draftValidationFailures.isEmpty()) {
+ throw new CommentsRejectedException(draftValidationFailures);
+ }
+ }
+
private boolean insertRobotComments(ChangeContext ctx) throws PatchListNotAvailableException {
if (in.robotComments == null) {
return false;
@@ -1337,7 +1372,7 @@
return current;
}
- private boolean insertMessage(ChangeContext ctx) {
+ private boolean insertMessage(ChangeContext ctx) throws CommentsRejectedException {
String msg = Strings.nullToEmpty(in.message).trim();
StringBuilder buf = new StringBuilder();
@@ -1350,6 +1385,15 @@
buf.append(String.format("\n\n(%d comments)", comments.size()));
}
if (!msg.isEmpty()) {
+ List<CommentValidationFailure> messageValidationFailure =
+ PublishCommentUtil.findInvalidComments(
+ commentValidators,
+ ImmutableList.of(
+ CommentForValidation.create(
+ CommentForValidation.CommentType.CHANGE_MESSAGE, msg)));
+ if (!messageValidationFailure.isEmpty()) {
+ throw new CommentsRejectedException(messageValidationFailure);
+ }
buf.append("\n\n").append(msg);
} else if (in.ready) {
buf.append("\n\n" + START_REVIEW_MESSAGE);
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 2a958af..6dee241 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -34,6 +34,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -187,6 +188,10 @@
|| e instanceof NoSuchRefException
|| e instanceof NoSuchProjectException) {
throw new ResourceNotFoundException(e.getMessage(), e);
+ } else if (e instanceof CommentsRejectedException) {
+ // SC_BAD_REQUEST is not ideal because it's not a syntactic error, but there is no better
+ // status code and it's isolated in monitoring.
+ throw new BadRequestException(e.getMessage(), e);
}
Throwables.throwIfUnchecked(e);
@@ -290,7 +295,7 @@
private enum ChangeResult {
SKIPPED,
UPSERTED,
- DELETED;
+ DELETED
}
private final GitRepositoryManager repoManager;
@@ -567,9 +572,7 @@
handle.setResult(id, ChangeResult.SKIPPED);
continue;
}
- for (ChangeUpdate u : ctx.updates.values()) {
- handle.manager.add(u);
- }
+ ctx.updates.values().forEach(handle.manager::add);
if (ctx.deleted) {
logDebug("Change %s was deleted", id);
handle.manager.deleteChange(id);
diff --git a/java/com/google/gerrit/server/update/CommentsRejectedException.java b/java/com/google/gerrit/server/update/CommentsRejectedException.java
new file mode 100644
index 0000000..6b0c04d
--- /dev/null
+++ b/java/com/google/gerrit/server/update/CommentsRejectedException.java
@@ -0,0 +1,47 @@
+// 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.server.update;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.extensions.validators.CommentValidationFailure;
+import java.util.Collection;
+import java.util.stream.Collectors;
+
+/** Thrown when comment validation rejected a comment, preventing it from being published. */
+public class CommentsRejectedException extends Exception {
+ private static final long serialVersionUID = 1L;
+
+ private final ImmutableList<CommentValidationFailure> commentValidationFailures;
+
+ public CommentsRejectedException(Collection<CommentValidationFailure> commentValidationFailures) {
+ this.commentValidationFailures = ImmutableList.copyOf(commentValidationFailures);
+ }
+
+ @Override
+ public String getMessage() {
+ return "One or more comments were rejected in validation: "
+ + commentValidationFailures.stream()
+ .map(CommentValidationFailure::getMessage)
+ .collect(Collectors.joining("; "));
+ }
+
+ /**
+ * Returns the validation failures that caused this exception. By contract this list is never
+ * empty.
+ */
+ public ImmutableList<CommentValidationFailure> getCommentValidationFailures() {
+ return commentValidationFailures;
+ }
+}
diff --git a/java/com/google/gerrit/testing/TestCommentHelper.java b/java/com/google/gerrit/testing/TestCommentHelper.java
new file mode 100644
index 0000000..b72cca7
--- /dev/null
+++ b/java/com/google/gerrit/testing/TestCommentHelper.java
@@ -0,0 +1,107 @@
+// 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.testing;
+
+import static java.util.stream.Collectors.toList;
+
+import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.extensions.api.changes.DraftInput;
+import com.google.gerrit.extensions.client.Comment;
+import com.google.gerrit.extensions.client.Comment.Range;
+import com.google.gerrit.extensions.client.Side;
+import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.inject.Inject;
+import java.util.Collection;
+
+/** Test helper for dealing with comments/drafts. */
+public class TestCommentHelper {
+ private final GerritApi gApi;
+
+ @Inject
+ public TestCommentHelper(GerritApi gerritApi) {
+ gApi = gerritApi;
+ }
+
+ public DraftInput newDraft(String message) {
+ return populate(new DraftInput(), "file", message);
+ }
+
+ public DraftInput newDraft(String path, Side side, int line, String message) {
+ DraftInput d = new DraftInput();
+ return populate(d, path, side, line, message);
+ }
+
+ public void addDraft(String changeId, String revId, DraftInput in) throws Exception {
+ gApi.changes().id(changeId).revision(revId).createDraft(in).get();
+ }
+
+ public Collection<CommentInfo> getPublishedComments(String changeId) throws Exception {
+ return gApi.changes().id(changeId).comments().values().stream()
+ .flatMap(Collection::stream)
+ .collect(toList());
+ }
+
+ public static <C extends Comment> C populate(C c, String path, String message) {
+ return populate(c, path, createLineRange(), message);
+ }
+
+ private static <C extends Comment> C populate(C c, String path, Range range, String message) {
+ int line = range.startLine;
+ c.path = path;
+ c.side = Side.REVISION;
+ c.parent = null;
+ c.line = line != 0 ? line : null;
+ c.message = message;
+ c.unresolved = false;
+ if (line != 0) c.range = range;
+ return c;
+ }
+
+ private static <C extends Comment> C populate(
+ C c, String path, Side side, Range range, String message) {
+ int line = range.startLine;
+ c.path = path;
+ c.side = side;
+ c.parent = null;
+ c.line = line != 0 ? line : null;
+ c.message = message;
+ c.unresolved = false;
+ if (line != 0) c.range = range;
+ return c;
+ }
+
+ private static <C extends Comment> C populate(
+ C c, String path, Side side, int line, String message) {
+ return populate(c, path, side, createLineRange(line), message);
+ }
+
+ private static Range createLineRange() {
+ Range range = new Range();
+ range.startLine = 0;
+ range.startCharacter = 1;
+ range.endLine = 0;
+ range.endCharacter = 5;
+ return range;
+ }
+
+ private static Range createLineRange(int line) {
+ Range range = new Range();
+ range.startLine = line;
+ range.startCharacter = 1;
+ range.endLine = line;
+ range.endCharacter = 5;
+ return range;
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
new file mode 100644
index 0000000..ebed15c
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -0,0 +1,288 @@
+// 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.api.change;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.extensions.api.changes.DraftInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
+import com.google.gerrit.extensions.client.Side;
+import com.google.gerrit.extensions.common.ChangeMessageInfo;
+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.CommentValidator;
+import com.google.gerrit.server.restapi.change.PostReview;
+import com.google.gerrit.server.update.CommentsRejectedException;
+import com.google.gerrit.testing.TestCommentHelper;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import java.sql.Timestamp;
+import org.easymock.Capture;
+import org.easymock.EasyMock;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests for comment validation in {@link PostReview}. */
+public class PostReviewIT 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 validateCommentsInInput_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 r = createChange();
+
+ ReviewInput input = new ReviewInput();
+ CommentInput comment = newComment(r.getChange().currentFilePaths().get(0));
+ comment.updated = new Timestamp(0);
+ input.comments = ImmutableMap.of(comment.path, ImmutableList.of(comment));
+
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+ gApi.changes().id(r.getChangeId()).current().review(input);
+
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).hasSize(1);
+ }
+
+ @Test
+ public void validateCommentsInInput_commentRejected() throws Exception {
+ CommentForValidation commentForValidation =
+ CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT);
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
+ EasyMock.replay(mockCommentValidator);
+
+ PushOneCommit.Result r = createChange();
+
+ ReviewInput input = new ReviewInput();
+ CommentInput comment = newComment(r.getChange().currentFilePaths().get(0));
+ comment.updated = new Timestamp(0);
+ input.comments = ImmutableMap.of(comment.path, ImmutableList.of(comment));
+
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+ BadRequestException badRequestException =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().review(input));
+ assertThat(badRequestException.getCause()).isInstanceOf(CommentsRejectedException.class);
+ assertThat(
+ Iterables.getOnlyElement(
+ ((CommentsRejectedException) badRequestException.getCause())
+ .getCommentValidationFailures())
+ .getComment()
+ .getText())
+ .isEqualTo(COMMENT_TEXT);
+ assertThat(badRequestException.getCause()).hasMessageThat().contains("Oh no!");
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+ }
+
+ @Test
+ public void validateDrafts_draftOK() throws Exception {
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(
+ CommentForValidation.CommentType.INLINE_COMMENT, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of());
+ EasyMock.replay(mockCommentValidator);
+
+ PushOneCommit.Result r = createChange();
+
+ DraftInput draft =
+ testCommentHelper.newDraft(
+ r.getChange().currentFilePaths().get(0), Side.REVISION, 1, COMMENT_TEXT);
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().getName()).createDraft(draft).get();
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+
+ ReviewInput input = new ReviewInput();
+ input.drafts = DraftHandling.PUBLISH;
+
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).hasSize(1);
+ }
+
+ @Test
+ public void validateDrafts_draftRejected() throws Exception {
+ CommentForValidation commentForValidation =
+ CommentForValidation.create(CommentType.INLINE_COMMENT, 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);
+ PushOneCommit.Result r = createChange();
+
+ DraftInput draft =
+ testCommentHelper.newDraft(
+ r.getChange().currentFilePaths().get(0), Side.REVISION, 1, COMMENT_TEXT);
+ testCommentHelper.addDraft(r.getChangeId(), r.getCommit().getName(), draft);
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+
+ ReviewInput input = new ReviewInput();
+ input.drafts = DraftHandling.PUBLISH;
+ BadRequestException badRequestException =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().review(input));
+ assertThat(badRequestException.getCause()).isInstanceOf(CommentsRejectedException.class);
+ assertThat(
+ Iterables.getOnlyElement(
+ ((CommentsRejectedException) badRequestException.getCause())
+ .getCommentValidationFailures())
+ .getComment()
+ .getText())
+ .isEqualTo(draft.message);
+ assertThat(badRequestException.getCause()).hasMessageThat().contains("Oh no!");
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+ }
+
+ @Test
+ public void validateDrafts_inlineVsFileComments_allOK() throws Exception {
+ PushOneCommit.Result r = createChange();
+ DraftInput draftInline =
+ testCommentHelper.newDraft(
+ r.getChange().currentFilePaths().get(0), Side.REVISION, 1, COMMENT_TEXT);
+ testCommentHelper.addDraft(r.getChangeId(), r.getCommit().getName(), draftInline);
+ DraftInput draftFile = testCommentHelper.newDraft(COMMENT_TEXT);
+ testCommentHelper.addDraft(r.getChangeId(), r.getCommit().getName(), draftFile);
+ assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
+
+ EasyMock.expect(mockCommentValidator.validateComments(EasyMock.capture(capture)))
+ .andReturn(ImmutableList.of());
+ EasyMock.replay(mockCommentValidator);
+
+ ReviewInput input = new ReviewInput();
+ input.drafts = DraftHandling.PUBLISH;
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ assertThat(testCommentHelper.getPublishedComments(r.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));
+ }
+
+ @Test
+ public void validateCommentsInChangeMessage_messageOK() throws Exception {
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of());
+ EasyMock.replay(mockCommentValidator);
+ PushOneCommit.Result r = createChange();
+
+ ReviewInput input = new ReviewInput().message(COMMENT_TEXT);
+ int numMessages = gApi.changes().id(r.getChangeId()).get().messages.size();
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ assertThat(gApi.changes().id(r.getChangeId()).get().messages).hasSize(numMessages + 1);
+ ChangeMessageInfo message =
+ Iterables.getLast(gApi.changes().id(r.getChangeId()).get().messages);
+ assertThat(message.message).contains(COMMENT_TEXT);
+ }
+
+ @Test
+ public void validateCommentsInChangeMessage_messageRejected() throws Exception {
+ CommentForValidation commentForValidation =
+ CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT);
+ EasyMock.expect(
+ mockCommentValidator.validateComments(
+ ImmutableList.of(
+ CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT))))
+ .andReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
+ EasyMock.replay(mockCommentValidator);
+ PushOneCommit.Result r = createChange();
+
+ ReviewInput input = new ReviewInput().message(COMMENT_TEXT);
+ assertThat(gApi.changes().id(r.getChangeId()).get().messages)
+ .hasSize(1); // From the initial commit.
+ BadRequestException badRequestException =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().review(input));
+ assertThat(badRequestException.getCause()).isInstanceOf(CommentsRejectedException.class);
+ assertThat(
+ Iterables.getOnlyElement(
+ ((CommentsRejectedException) badRequestException.getCause())
+ .getCommentValidationFailures())
+ .getComment()
+ .getText())
+ .isEqualTo(COMMENT_TEXT);
+ assertThat(badRequestException.getCause()).hasMessageThat().contains("Oh no!");
+ assertThat(gApi.changes().id(r.getChangeId()).get().messages)
+ .hasSize(1); // Unchanged from before.
+ ChangeMessageInfo message =
+ Iterables.getLast(gApi.changes().id(r.getChangeId()).get().messages);
+ assertThat(message.message).doesNotContain(COMMENT_TEXT);
+ }
+
+ private static CommentInput newComment(String path) {
+ return TestCommentHelper.populate(new CommentInput(), path, PostReviewIT.COMMENT_TEXT);
+ }
+}
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));
+ }
+}
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}
