opendev/gerrit
Code Issues Proposed changes

Merge changes from topic "comment-validation"

Browse Source 
* 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
This commit is contained in:
Joerg Zieren
2019-06-17 07:01:43 +00:00
committed by Gerrit Code Review
parent 1066c58c28 e18c1dee2a
commit 800a351e30
18 changed files with 1034 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

48
java/com/google/gerrit/extensions/validators/CommentForValidation.java Normal file
View File

@@ -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);
}
}

32
java/com/google/gerrit/extensions/validators/CommentValidationFailure.java Normal file
View File

@@ -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();
}

34
java/com/google/gerrit/extensions/validators/CommentValidator.java Normal file
View File

@@ -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);
}

21
java/com/google/gerrit/server/PublishCommentUtil.java
View File

@@ -18,13 +18,18 @@ import static com.google.common.base.Preconditions.checkArgument;
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 @@ public class PublishCommentUtil {
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();
}
}

2
java/com/google/gerrit/server/config/GerritGlobalModule.java
View File

@@ -62,6 +62,7 @@ import com.google.gerrit.extensions.registration.DynamicItem;
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 @@ public class GerritGlobalModule extends FactoryModule {
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);

72
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -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;
@@ -1301,7 +1314,6 @@ class ReceiveCommits {
Optional<AuthException> err = checkRefPermission(cmd, RefPermission.DELETE);
if (!err.isPresent()) {
validRefOperation(cmd);
} else {
rejectProhibited(cmd, err.get());
}
@@ -1377,6 +1389,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();
@@ -1579,7 +1598,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) {
@@ -1976,7 +2003,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");
@@ -1984,10 +2011,12 @@ class ReceiveCommits {
}
/**
* 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 @@ 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;
}
@@ -2101,7 +2154,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 {
@@ -2218,7 +2271,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();
@@ -3197,7 +3251,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));
}

35
java/com/google/gerrit/server/mail/receive/MailProcessor.java
View File

@@ -18,6 +18,7 @@ import static java.util.stream.Collectors.toList;
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.DynamicMap;
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.ApprovalsUtil;
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.mail.send.InboundEmailRejectionSender;
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 @@ import java.util.Set;
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 @@ public class MailProcessor {
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 @@ public class MailProcessor {
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 @@ public class MailProcessor {
this.approvalsUtil = approvalsUtil;
this.accountCache = accountCache;
this.urlFormatter = urlFormatter;
this.commentValidators = commentValidators;
}
/**
@@ -259,6 +277,21 @@ public class MailProcessor {
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);

3
java/com/google/gerrit/server/mail/send/InboundEmailRejectionSender.java
View File

@@ -32,7 +32,8 @@ public class InboundEmailRejectionSender extends OutgoingEmail {
PARSING_ERROR,
INACTIVE_ACCOUNT,
UNKNOWN_ACCOUNT,
INTERNAL_EXCEPTION;
INTERNAL_EXCEPTION,
COMMENT_REJECTED
}
public interface Factory {

52
java/com/google/gerrit/server/restapi/change/PostReview.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.change;
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 static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
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.Response;
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.ChangePermission;
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.Optional;
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 @@ public class PostReview
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 @@ public class PostReview
@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 @@ public class PostReview
this.workInProgressOpFactory = workInProgressOpFactory;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
this.commentValidators = commentValidators;
this.strictLabels = gerritConfig.getBoolean("change", "strictLabels", false);
}
@@ -858,7 +871,7 @@ public class PostReview
@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 @@ public class PostReview
}
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 @@ public class PostReview
}
}
// This will be populated with Comment-s created from inputComments.
List<Comment> toPublish = new ArrayList<>();
Set<CommentSetEntry> existingComments =
@@ -954,11 +969,13 @@ public class PostReview
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 @@ public class PostReview
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 @@ public class PostReview
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 @@ public class PostReview
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);

11
java/com/google/gerrit/server/update/BatchUpdate.java
View File

@@ -34,6 +34,7 @@ import com.google.common.util.concurrent.ListenableFuture;
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 @@ public class BatchUpdate implements AutoCloseable {
|| 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 @@ public class BatchUpdate implements AutoCloseable {
private enum ChangeResult {
SKIPPED,
UPSERTED,
DELETED;
DELETED
}
private final GitRepositoryManager repoManager;
@@ -567,9 +572,7 @@ public class BatchUpdate implements AutoCloseable {
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);

47
java/com/google/gerrit/server/update/CommentsRejectedException.java Normal file
View File

@@ -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;
}
}

107
java/com/google/gerrit/testing/TestCommentHelper.java Normal file
View File

@@ -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;
}
}

288
javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java Normal file
View File

@@ -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);
}
}

8
javatests/com/google/gerrit/acceptance/server/git/receive/BUILD Normal file
View File

@@ -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 = [],
)

143
javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java Normal file
View File

@@ -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));
}
}

138
javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
View File

@@ -16,25 +16,61 @@ package com.google.gerrit.acceptance.server.mail;
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 @@ public class MailProcessorIT extends AbstractMailIT {
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;
}

5
resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
View File

@@ -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}

7
resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
View File

@@ -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}