opendev/gerrit
Code Issues Proposed changes

Limit the number of comments per change (to 5000 by default)

Browse Source 
Also refactor the robot comment size check to be in line with the size
check for human comments.

Change-Id: If24adc10f5f16cd8894bf246078f0e6c992cdafa
This commit is contained in:
Joerg Zieren 2020-01-02 10:18:14 +01:00
parent b91db6bcdd
commit 7cac5d14a9
23 changed files with 555 additions and 266 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
Documentation/config-gerrit.txt
View File

@ -1195,6 +1195,15 @@ performance improvements by reducing the number of refs in the repo.
+
Default is true.
[[change.commentSizeLimit]]change.commentSizeLimit::
+
Maximum allowed size in characters of a regular (non-robot) comment. Comments
which exceed this size will be rejected. Size computation is approximate and may
be off by roughly 1%. Common unit suffixes of 'k', 'm', or 'g' are supported.
The value must be positive.
+
The default limit is 16kB.
[[change.disablePrivateChanges]]change.disablePrivateChanges::
+
If set to true, users are not allowed to create private changes.
@ -1212,6 +1221,13 @@ in change tables and user dashboards.
+
By default 500.
[[change.maxComments]]change.maxComments::
+
Maximum number of comments (regular plus robot) allowed per change. Additional
comments are rejected.
+
By default 5,000.
[[change.maxUpdates]]change.maxUpdates::
+
Maximum number of updates to a change. Counts only updates to the main NoteDb
@ -1266,11 +1282,10 @@ and score (Shortcut: a)".
[[change.robotCommentSizeLimit]]change.robotCommentSizeLimit::
+
Maximum allowed size of a robot comment that will be accepted. Robot comments
which exceed the indicated size will be rejected on addition. The specified
value is interpreted as the maximum size in bytes of the JSON representation of
the robot comment. Common unit suffixes of 'k', 'm', or 'g' are supported.
Zero or negative values allow robot comments of unlimited size.
Maximum allowed size in characters of a robot comment. Robot comments which
exceed this size will be rejected on addition. Size computation is approximate
and may be off by roughly 1%. Common unit suffixes of 'k', 'm', or 'g' are
supported. Zero or negative values allow robot comments of unlimited size.
+
The default limit is 1024kB.

35
java/com/google/gerrit/entities/Comment.java
View File

@ -29,6 +29,8 @@ import org.eclipse.jgit.lib.ObjectId;
*
* <p>Changing fields in this class changes the storage format of inline comments in NoteDb and may
* require a corresponding data migration (adding new optional fields is generally okay).
*
* <p>Consider updating {@link #getApproximateSize()} when adding/changing fields.
*/
public class Comment {
public enum Status {
@ -56,7 +58,7 @@ public class Comment {
}
}
public static class Key {
public static final class Key {
public String uuid;
public String filename;
public int patchSetId;
@ -97,7 +99,7 @@ public class Comment {
}
}
public static class Identity {
public static final class Identity {
int id;
public Identity(Account.Id id) {
@ -147,7 +149,7 @@ public class Comment {
* </ul>
* </ul>
*/
public static class Range implements Comparable<Range> {
public static final class Range implements Comparable<Range> {
private static final Comparator<Range> RANGE_COMPARATOR =
Comparator.<Range>comparingInt(range -> range.startLine)
.thenComparingInt(range -> range.startChar)
@ -220,10 +222,12 @@ public class Comment {
public Range range;
public String tag;
// Hex commit SHA1 of the commit of the patchset to which this comment applies. Other classes call
// this "commitId", but this class uses the old ReviewDb term "revId", and this field name is
// serialized into JSON in NoteDb, so it can't easily be changed. Callers do not access this field
// directly, and instead use the public getter/setter that wraps an ObjectId.
/**
* Hex commit SHA1 of the commit of the patchset to which this comment applies. Other classes call
* this "commitId", but this class uses the old ReviewDb term "revId", and this field name is
* serialized into JSON in NoteDb, so it can't easily be changed. Callers do not access this field
* directly, and instead use the public getter/setter that wraps an ObjectId.
*/
private String revId;
public String serverId;
@ -293,6 +297,23 @@ public class Comment {
return realAuthor != null ? realAuthor : author;
}
/**
* Returns the comment's approximate size. This is used to enforce size limits and should
* therefore include all unbounded fields (e.g. String-s).
*/
public int getApproximateSize() {
return nullableLength(message, parentUuid, tag, revId, serverId)
+ (key != null ? nullableLength(key.filename, key.uuid) : 0);
}
static int nullableLength(String... strings) {
int length = 0;
for (String s : strings) {
length += s == null ? 0 : s.length();
}
return length;
}
@Override
public boolean equals(Object o) {
if (!(o instanceof Comment)) {

13
java/com/google/gerrit/entities/FixReplacement.java
View File

@ -14,10 +14,10 @@
package com.google.gerrit.entities;
public class FixReplacement {
public String path;
public Comment.Range range;
public String replacement;
public final class FixReplacement {
public final String path;
public final Comment.Range range;
public final String replacement;
public FixReplacement(String path, Comment.Range range, String replacement) {
this.path = path;
@ -38,4 +38,9 @@ public class FixReplacement {
+ '\''
+ '}';
}
/** Returns this instance's approximate size in bytes for the purpose of applying size limits. */
int getApproximateSize() {
return path.length() + replacement.length();
}
}

15
java/com/google/gerrit/entities/FixSuggestion.java
View File

@ -16,10 +16,10 @@ package com.google.gerrit.entities;
import java.util.List;
public class FixSuggestion {
public String fixId;
public String description;
public List<FixReplacement> replacements;
public final class FixSuggestion {
public final String fixId;
public final String description;
public final List<FixReplacement> replacements;
public FixSuggestion(String fixId, String description, List<FixReplacement> replacements) {
this.fixId = fixId;
@ -40,4 +40,11 @@ public class FixSuggestion {
+ replacements
+ '}';
}
/** Returns this instance's approximate size in bytes for the purpose of applying size limits. */
int getApproximateSize() {
return fixId.length()
+ description.length()
+ replacements.stream().map(FixReplacement::getApproximateSize).reduce(0, Integer::sum);
}
}

18
java/com/google/gerrit/entities/RobotComment.java
View File

@ -19,7 +19,7 @@ import java.util.List;
import java.util.Map;
import java.util.Objects;
public class RobotComment extends Comment {
public final class RobotComment extends Comment {
public String robotId;
public String robotRunId;
public String url;
@ -40,6 +40,22 @@ public class RobotComment extends Comment {
this.robotRunId = robotRunId;
}
@Override
public int getApproximateSize() {
int approximateSize = super.getApproximateSize() + nullableLength(robotId, robotRunId, url);
approximateSize +=
properties != null
? properties.entrySet().stream()
.map(entry -> nullableLength(entry.getKey(), entry.getValue()))
.reduce(0, Integer::sum)
: 0;
approximateSize +=
fixSuggestions != null
? fixSuggestions.stream().map(FixSuggestion::getApproximateSize).reduce(0, Integer::sum)
: 0;
return approximateSize;
}
@Override
public String toString() {
return toStringHelper()

27
java/com/google/gerrit/extensions/validators/CommentForValidation.java
View File

@ -17,13 +17,21 @@ 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.
* Holds a comment's text and some metadata in order to pass it to a validation plugin.
*
* @see CommentValidator
*/
@AutoValue
public abstract class CommentForValidation {
/** The creator of the comment. */
public enum CommentSource {
/** A regular user comment. */
HUMAN,
/** A robot comment. */
ROBOT
}
/** The type of comment. */
public enum CommentType {
/** A regular (inline) comment. */
@ -34,14 +42,27 @@ public abstract class CommentForValidation {
CHANGE_MESSAGE
}
public static CommentForValidation create(CommentType type, String text) {
return new AutoValue_CommentForValidation(type, text);
public static CommentForValidation create(
CommentSource source, CommentType type, String text, long size) {
return new AutoValue_CommentForValidation(source, type, text, size);
}
public abstract CommentSource getSource();
public abstract CommentType getType();
/**
* Returns the comment text. Note that especially for robot comments the total size may be
* significantly larger and should be determined by using {@link #getApproximateSize()}.
*/
public abstract String getText();
/**
* Returns this instance's approximate size in bytes for the purpose of applying size limits. For
* robot comments this may be significantly larger than the size of the comment text.
*/
public abstract long getApproximateSize();
public CommentValidationFailure failValidation(String message) {
return CommentValidationFailure.create(this, message);
}

13
java/com/google/gerrit/extensions/validators/CommentValidationContext.java
View File

@ -34,16 +34,7 @@ public abstract class CommentValidationContext {
/** Returns the project the comment is being added to. */
public abstract String getProject();
public static Builder builder() {
return new AutoValue_CommentValidationContext.Builder();
}
@AutoValue.Builder
public abstract static class Builder {
public abstract Builder changeId(int value);
public abstract Builder project(String value);
public abstract CommentValidationContext build();
public static CommentValidationContext create(int changeId, String project) {
return new AutoValue_CommentValidationContext(changeId, project);
}
}

5
java/com/google/gerrit/extensions/validators/CommentValidator.java
View File

@ -25,7 +25,10 @@ import com.google.gerrit.extensions.annotations.ExtensionPoint;
public interface CommentValidator {
/**
* Validate the specified comments.
* Validate the specified comments. This method will be called with all new comments that need to
* be validated. This allows validators to statelessly count the new comments. Note that the
* method may be called separately for texts that are not comments, but similar in nature and also
* subject to size limits, such as a change message.
*
* @return An empty list if all comments are valid, or else a list of validation failures.
*/

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

@ -128,7 +128,8 @@ import com.google.gerrit.server.git.ReceivePackInitializer;
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.TransferConfig;
import com.google.gerrit.server.git.receive.ReceiveCommitsModule;
import com.google.gerrit.server.git.validators.CommentLimitsValidator;
import com.google.gerrit.server.git.validators.CommentCountValidator;
import com.google.gerrit.server.git.validators.CommentSizeValidator;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.MergeValidationListener;
import com.google.gerrit.server.git.validators.MergeValidators;
@ -410,8 +411,11 @@ public class GerritGlobalModule extends FactoryModule {
DynamicSet.setOf(binder(), UploadValidationListener.class);
bind(CommentValidator.class)
.annotatedWith(Exports.named(CommentLimitsValidator.class.getSimpleName()))
.to(CommentLimitsValidator.class);
.annotatedWith(Exports.named(CommentCountValidator.class.getSimpleName()))
.to(CommentCountValidator.class);
bind(CommentValidator.class)
.annotatedWith(Exports.named(CommentSizeValidator.class.getSimpleName()))
.to(CommentSizeValidator.class);
DynamicMap.mapOf(binder(), DynamicOptions.DynamicBean.class);
DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeOperatorFactory.class);

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

@ -92,6 +92,7 @@ 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.CommentSource;
import com.google.gerrit.extensions.validators.CommentForValidation.CommentType;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
@ -126,6 +127,8 @@ import com.google.gerrit.server.git.MultiProgressMonitor.Task;
import com.google.gerrit.server.git.ReceivePackInitializer;
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.ValidationError;
import com.google.gerrit.server.git.validators.CommentCountValidator;
import com.google.gerrit.server.git.validators.CommentSizeValidator;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.git.validators.RefOperationValidationException;
import com.google.gerrit.server.git.validators.RefOperationValidators;
@ -1441,11 +1444,18 @@ class ReceiveCommits {
final ReceiveCommand cmd;
final LabelTypes labelTypes;
/**
* 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.
* Draft comments are published with the commit iff {@code --publish-comments} is set. All
* drafts are withheld (overriding the option) if at least one of the following conditions are
* met:
*
* <ul>
* <li>Installed {@link CommentValidator} plugins reject one or more draft comments.
* <li>One or more comments exceed the maximum comment size (see {@link
* CommentSizeValidator}).
* <li>The maximum number of comments would be exceeded (see {@link CommentCountValidator}).
* </ul>
*/
private boolean commentsValid = true;
private boolean withholdComments = false;
BranchNameKey dest;
PermissionBackend.ForRef perm;
@ -1642,18 +1652,19 @@ class ReceiveCommits {
.collect(toImmutableSet());
}
void setCommentsValid(boolean commentsValid) {
this.commentsValid = commentsValid;
void setWithholdComments(boolean withholdComments) {
this.withholdComments = withholdComments;
}
boolean shouldPublishComments() {
if (!commentsValid) {
if (withholdComments) {
// Validation messages of type WARNING have already been added, now withhold the comments.
return false;
}
if (publishComments) {
return true;
} else if (noPublishComments) {
}
if (noPublishComments) {
return false;
}
return defaultPublishComments;
@ -2010,19 +2021,18 @@ class ReceiveCommits {
.map(
comment ->
CommentForValidation.create(
CommentSource.HUMAN,
comment.lineNbr > 0
? CommentType.INLINE_COMMENT
: CommentType.FILE_COMMENT,
comment.message))
comment.message,
comment.message.length()))
.collect(toImmutableList());
CommentValidationContext ctx =
CommentValidationContext.builder()
.changeId(change.getChangeId())
.project(change.getProject().get())
.build();
CommentValidationContext.create(change.getChangeId(), change.getProject().get());
ImmutableList<CommentValidationFailure> commentValidationFailures =
PublishCommentUtil.findInvalidComments(ctx, commentValidators, draftsForValidation);
magicBranch.setCommentsValid(commentValidationFailures.isEmpty());
magicBranch.setWithholdComments(!commentValidationFailures.isEmpty());
commentValidationFailures.forEach(
failure ->
addMessage(

61
java/com/google/gerrit/server/git/validators/CommentCountValidator.java Normal file
View File

@ -0,0 +1,61 @@
// Copyright (C) 2020 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.git.validators;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.inject.Inject;
import org.eclipse.jgit.lib.Config;
/** Limits number of comments to prevent space/time complexity issues. */
public class CommentCountValidator implements CommentValidator {
private final int maxComments;
private final ChangeNotes.Factory notesFactory;
@Inject
CommentCountValidator(@GerritServerConfig Config serverConfig, ChangeNotes.Factory notesFactory) {
this.notesFactory = notesFactory;
maxComments = serverConfig.getInt("change", "maxComments", 5_000);
}
@Override
public ImmutableList<CommentValidationFailure> validateComments(
CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) {
ImmutableList.Builder<CommentValidationFailure> failures = ImmutableList.builder();
ChangeNotes notes =
notesFactory.createChecked(Project.nameKey(ctx.getProject()), Change.id(ctx.getChangeId()));
int numExistingComments = notes.getComments().size() + notes.getRobotComments().size();
if (!comments.isEmpty() && numExistingComments + comments.size() > maxComments) {
// This warning really applies to the set of all comments, but we need to pick one to attach
// the message to.
CommentForValidation commentForFailureMessage = Iterables.getLast(comments);
failures.add(
commentForFailureMessage.failValidation(
String.format(
"Exceeding maximum number of comments: %d (existing) + %d (new) > %d",
numExistingComments, comments.size(), maxComments)));
}
return failures.build();
}
}

47
java/com/google/gerrit/server/git/validators/CommentLimitsValidator.java
View File

@ -1,47 +0,0 @@
// Copyright (C) 2020 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.git.validators;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.Inject;
import org.eclipse.jgit.lib.Config;
/** Limits the size of comments to prevent large comments from causing issues. */
public class CommentLimitsValidator implements CommentValidator {
private final int maxCommentLength;
@Inject
CommentLimitsValidator(@GerritServerConfig Config serverConfig) {
maxCommentLength = serverConfig.getInt("change", null, "maxCommentLength", 16 << 10);
}
@Override
public ImmutableList<CommentValidationFailure> validateComments(
CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) {
return comments.stream()
.filter(c -> c.getText().length() > maxCommentLength)
.map(
c ->
c.failValidation(
String.format(
"Comment too large (%d > %d)", c.getText().length(), maxCommentLength)))
.collect(ImmutableList.toImmutableList());
}
}

71
java/com/google/gerrit/server/git/validators/CommentSizeValidator.java Normal file
View File

@ -0,0 +1,71 @@
// Copyright (C) 2020 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.git.validators;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.Inject;
import org.eclipse.jgit.lib.Config;
/** Limits the size of comments to prevent space/time complexity issues. */
public class CommentSizeValidator implements CommentValidator {
private final int commentSizeLimit;
private final int robotCommentSizeLimit;
@Inject
CommentSizeValidator(@GerritServerConfig Config serverConfig) {
commentSizeLimit = serverConfig.getInt("change", "commentSizeLimit", 16 << 10);
robotCommentSizeLimit = serverConfig.getInt("change", "robotCommentSizeLimit", 1 << 20);
}
@Override
public ImmutableList<CommentValidationFailure> validateComments(
CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) {
return comments.stream()
.filter(this::exceedsSizeLimit)
.map(c -> c.failValidation(buildErrorMessage(c)))
.collect(ImmutableList.toImmutableList());
}
private boolean exceedsSizeLimit(CommentForValidation comment) {
switch (comment.getSource()) {
case HUMAN:
return comment.getApproximateSize() > commentSizeLimit;
case ROBOT:
return robotCommentSizeLimit > 0 && comment.getApproximateSize() > robotCommentSizeLimit;
}
throw new RuntimeException(
"Unknown comment source (should not have compiled): " + comment.getSource());
}
private String buildErrorMessage(CommentForValidation comment) {
switch (comment.getSource()) {
case HUMAN:
return String.format(
"Comment size exceeds limit (%d > %d)", comment.getApproximateSize(), commentSizeLimit);
case ROBOT:
return String.format(
"Size %d (bytes) of robot comment is greater than limit %d (bytes)",
comment.getApproximateSize(), robotCommentSizeLimit);
}
throw new RuntimeException(
"Unknown comment source (should not have compiled): " + comment.getSource());
}
}

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

@ -19,6 +19,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.collect.Iterables;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
@ -243,7 +244,7 @@ public class MailProcessor {
sendRejectionEmail(message, InboundEmailRejectionSender.Error.INTERNAL_EXCEPTION);
return;
}
ChangeData cd = changeDataList.get(0);
ChangeData cd = Iterables.getOnlyElement(changeDataList);
if (existingMessageIds(cd).contains(message.id())) {
logger.atInfo().log("Message %s was already processed. Will delete message.", message.id());
return;
@ -285,14 +286,14 @@ public class MailProcessor {
.map(
comment ->
CommentForValidation.create(
CommentForValidation.CommentSource.HUMAN,
MAIL_COMMENT_TYPE_TO_VALIDATION_TYPE.get(comment.getType()),
comment.getMessage()))
comment.getMessage(),
comment.getMessage().length()))
.collect(ImmutableList.toImmutableList());
CommentValidationContext commentValidationCtx =
CommentValidationContext.builder()
.changeId(cd.change().getChangeId())
.project(cd.change().getProject().get())
.build();
CommentValidationContext.create(
cd.change().getChangeId(), cd.change().getProject().get());
ImmutableList<CommentValidationFailure> commentValidationFailures =
PublishCommentUtil.findInvalidComments(
commentValidationCtx, commentValidators, parsedCommentsForValidation);

3
java/com/google/gerrit/server/notedb/LimitExceededException.java
View File

@ -23,12 +23,13 @@ import com.google.gerrit.exceptions.StorageException;
* <ul>
* <li>The number of NoteDb updates per change.
* <li>The number of patch sets per change.
* <li>The number of files per change.
* </ul>
*/
public class LimitExceededException extends StorageException {
private static final long serialVersionUID = 1L;
LimitExceededException(String message) {
public LimitExceededException(String message) {
super(message);
}
}

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

@ -80,7 +80,6 @@ import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentValidationContext;
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.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
@ -125,11 +124,9 @@ import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.LabelVote;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gson.Gson;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collection;
@ -141,7 +138,6 @@ import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.OptionalInt;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
@ -159,9 +155,6 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
public static final String START_REVIEW_MESSAGE = "This change is ready for review.";
private static final Gson GSON = OutputFormat.JSON_COMPACT.newGson();
private static final int DEFAULT_ROBOT_COMMENT_SIZE_LIMIT_IN_BYTES = 1024 * 1024;
private final BatchUpdate.Factory updateFactory;
private final ChangeResource.Factory changeResourceFactory;
private final ChangeData.Factory changeDataFactory;
@ -177,7 +170,6 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
private final ReviewerAdder reviewerAdder;
private final AddReviewersEmail addReviewersEmail;
private final NotifyResolver notifyResolver;
private final Config gerritConfig;
private final WorkInProgressOp.Factory workInProgressOpFactory;
private final ProjectCache projectCache;
private final PermissionBackend permissionBackend;
@ -221,7 +213,6 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
this.reviewerAdder = reviewerAdder;
this.addReviewersEmail = addReviewersEmail;
this.notifyResolver = notifyResolver;
this.gerritConfig = gerritConfig;
this.workInProgressOpFactory = workInProgressOpFactory;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
@ -671,41 +662,15 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
for (Map.Entry<String, List<RobotCommentInput>> e : in.entrySet()) {
String commentPath = e.getKey();
for (RobotCommentInput c : e.getValue()) {
ensureSizeOfJsonInputIsWithinBounds(c);
ensureRobotIdIsSet(c.robotId, commentPath);
ensureRobotRunIdIsSet(c.robotRunId, commentPath);
ensureFixSuggestionsAreAddable(c.fixSuggestions, commentPath);
// Size is validated later, in CommentLimitsValidator.
}
}
checkComments(revision, in);
}
private void ensureSizeOfJsonInputIsWithinBounds(RobotCommentInput robotCommentInput)
throws BadRequestException {
OptionalInt robotCommentSizeLimit = getRobotCommentSizeLimit();
if (robotCommentSizeLimit.isPresent()) {
int sizeLimit = robotCommentSizeLimit.getAsInt();
byte[] robotCommentBytes = GSON.toJson(robotCommentInput).getBytes(StandardCharsets.UTF_8);
int robotCommentSize = robotCommentBytes.length;
if (robotCommentSize > sizeLimit) {
throw new BadRequestException(
String.format(
"Size %d (bytes) of robot comment is greater than limit %d (bytes)",
robotCommentSize, sizeLimit));
}
}
}
private OptionalInt getRobotCommentSizeLimit() {
int robotCommentSizeLimit =
gerritConfig.getInt(
"change", "robotCommentSizeLimit", DEFAULT_ROBOT_COMMENT_SIZE_LIMIT_IN_BYTES);
if (robotCommentSizeLimit <= 0) {
return OptionalInt.empty();
}
return OptionalInt.of(robotCommentSizeLimit);
}
private static void ensureRobotIdIsSet(String robotId, String commentPath)
throws BadRequestException {
if (robotId == null) {
@ -909,8 +874,10 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
user = ctx.getIdentifiedUser();
notes = ctx.getNotes();
ps = psUtil.get(ctx.getNotes(), psId);
boolean dirty = insertComments(ctx);
dirty |= insertRobotComments(ctx);
List<RobotComment> newRobotComments =
in.robotComments == null ? ImmutableList.of() : getNewRobotComments(ctx);
boolean dirty = insertComments(ctx, newRobotComments);
dirty |= insertRobotComments(ctx, newRobotComments);
dirty |= updateLabels(projectState, ctx);
dirty |= insertMessage(ctx);
return dirty;
@ -937,7 +904,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
ctx.getWhen());
}
private boolean insertComments(ChangeContext ctx)
private boolean insertComments(ChangeContext ctx, List<RobotComment> newRobotComments)
throws UnprocessableEntityException, PatchListNotAvailableException,
CommentsRejectedException {
Map<String, List<CommentInput>> inputComments = in.comments;
@ -1001,21 +968,21 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
CommentValidationContext commentValidationCtx =
CommentValidationContext.builder()
.changeId(ctx.getChange().getChangeId())
.project(ctx.getChange().getProject().get())
.build();
CommentValidationContext.create(
ctx.getChange().getChangeId(), ctx.getChange().getProject().get());
switch (in.drafts) {
case PUBLISH:
case PUBLISH_ALL_REVISIONS:
validateComments(
commentValidationCtx, Streams.concat(drafts.values().stream(), toPublish.stream()));
commentValidationCtx,
Streams.concat(
drafts.values().stream(), toPublish.stream(), newRobotComments.stream()));
publishCommentUtil.publish(ctx, ctx.getUpdate(psId), drafts.values(), in.tag);
comments.addAll(drafts.values());
break;
case KEEP:
default:
validateComments(commentValidationCtx, toPublish.stream());
validateComments(
commentValidationCtx, Stream.concat(toPublish.stream(), newRobotComments.stream()));
break;
}
ChangeUpdate changeUpdate = ctx.getUpdate(psId);
@ -1031,10 +998,14 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
.map(
comment ->
CommentForValidation.create(
comment instanceof RobotComment
? CommentForValidation.CommentSource.ROBOT
: CommentForValidation.CommentSource.HUMAN,
comment.lineNbr > 0
? CommentForValidation.CommentType.INLINE_COMMENT
: CommentForValidation.CommentType.FILE_COMMENT,
comment.message))
comment.message,
comment.getApproximateSize()))
.collect(toImmutableList());
ImmutableList<CommentValidationFailure> draftValidationFailures =
PublishCommentUtil.findInvalidComments(ctx, commentValidators, draftsForValidation);
@ -1043,12 +1014,10 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
private boolean insertRobotComments(ChangeContext ctx) throws PatchListNotAvailableException {
private boolean insertRobotComments(ChangeContext ctx, List<RobotComment> newRobotComments) {
if (in.robotComments == null) {
return false;
}
List<RobotComment> newRobotComments = getNewRobotComments(ctx);
commentsUtil.putRobotComments(ctx.getUpdate(psId), newRobotComments);
comments.addAll(newRobotComments);
return !newRobotComments.isEmpty();
@ -1424,17 +1393,18 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
if (!msg.isEmpty()) {
CommentValidationContext commentValidationCtx =
CommentValidationContext.builder()
.changeId(ctx.getChange().getChangeId())
.project(ctx.getChange().getProject().get())
.build();
CommentValidationContext.create(
ctx.getChange().getChangeId(), ctx.getChange().getProject().get());
ImmutableList<CommentValidationFailure> messageValidationFailure =
PublishCommentUtil.findInvalidComments(
commentValidationCtx,
commentValidators,
ImmutableList.of(
CommentForValidation.create(
CommentForValidation.CommentType.CHANGE_MESSAGE, msg)));
CommentForValidation.CommentSource.HUMAN,
CommentForValidation.CommentType.CHANGE_MESSAGE,
msg,
msg.length())));
if (!messageValidationFailure.isEmpty()) {
throw new CommentsRejectedException(messageValidationFailure);
}

179
javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
View File

@ -16,7 +16,10 @@ package com.google.gerrit.acceptance.api.change;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.util.stream.Collectors.toList;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@ -25,19 +28,23 @@ import static org.mockito.MockitoAnnotations.initMocks;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.truth.Correspondence;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.config.GerritConfig;
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.api.changes.ReviewInput.RobotCommentInput;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.RobotCommentInfo;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.validators.CommentForValidation;
import com.google.gerrit.extensions.validators.CommentForValidation.CommentType;
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.restapi.change.PostReview;
@ -46,9 +53,12 @@ import com.google.gerrit.testing.TestCommentHelper;
import com.google.inject.Inject;
import com.google.inject.Module;
import java.sql.Timestamp;
import java.util.Collection;
import java.util.List;
import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
import org.mockito.ArgumentMatcher;
import org.mockito.Captor;
/** Tests for comment validation in {@link PostReview}. */
@ -57,8 +67,43 @@ public class PostReviewIT extends AbstractDaemonTest {
@Inject private TestCommentHelper testCommentHelper;
private static final String COMMENT_TEXT = "The comment text";
private static final CommentForValidation FILE_COMMENT_FOR_VALIDATION =
CommentForValidation.create(
CommentForValidation.CommentSource.HUMAN,
CommentForValidation.CommentType.FILE_COMMENT,
COMMENT_TEXT,
COMMENT_TEXT.length());
private static final CommentForValidation INLINE_COMMENT_FOR_VALIDATION =
CommentForValidation.create(
CommentForValidation.CommentSource.HUMAN,
CommentForValidation.CommentType.INLINE_COMMENT,
COMMENT_TEXT,
COMMENT_TEXT.length());
private static final CommentForValidation CHANGE_MESSAGE_FOR_VALIDATION =
CommentForValidation.create(
CommentForValidation.CommentSource.HUMAN,
CommentForValidation.CommentType.CHANGE_MESSAGE,
COMMENT_TEXT,
COMMENT_TEXT.length());
@Captor private ArgumentCaptor<ImmutableList<CommentForValidation>> capture;
@Captor private ArgumentCaptor<ImmutableList<CommentForValidation>> captor;
private static class SingleCommentMatcher
implements ArgumentMatcher<ImmutableList<CommentForValidation>> {
final CommentForValidation left;
SingleCommentMatcher(CommentForValidation left) {
this.left = left;
}
@Override
public boolean matches(ImmutableList<CommentForValidation> rightList) {
CommentForValidation right = Iterables.getOnlyElement(rightList);
return left.getSource() == right.getSource()
&& left.getType() == right.getType()
&& left.getText().equals(right.getText());
}
}
@Override
public Module createModule() {
@ -84,13 +129,7 @@ public class PostReviewIT extends AbstractDaemonTest {
public void validateCommentsInInput_commentOK() throws Exception {
PushOneCommit.Result r = createChange();
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(
CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT))))
eq(contextFor(r)), argThat(new SingleCommentMatcher(FILE_COMMENT_FOR_VALIDATION))))
.thenReturn(ImmutableList.of());
ReviewInput input = new ReviewInput();
@ -107,15 +146,9 @@ public class PostReviewIT extends AbstractDaemonTest {
@Test
public void validateCommentsInInput_commentRejected() throws Exception {
PushOneCommit.Result r = createChange();
CommentForValidation commentForValidation =
CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT);
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT))))
.thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
eq(contextFor(r)), argThat(new SingleCommentMatcher(FILE_COMMENT_FOR_VALIDATION))))
.thenReturn(ImmutableList.of(FILE_COMMENT_FOR_VALIDATION.failValidation("Oh no!")));
ReviewInput input = new ReviewInput();
CommentInput comment = newComment(r.getChange().currentFilePaths().get(0));
@ -161,19 +194,13 @@ public class PostReviewIT extends AbstractDaemonTest {
public void validateDrafts_draftOK() throws Exception {
PushOneCommit.Result r = createChange();
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(
CommentForValidation.CommentType.INLINE_COMMENT, COMMENT_TEXT))))
eq(contextFor(r)), argThat(new SingleCommentMatcher((INLINE_COMMENT_FOR_VALIDATION)))))
.thenReturn(ImmutableList.of());
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();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().getName()).createDraft(draft);
assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
ReviewInput input = new ReviewInput();
@ -186,17 +213,9 @@ public class PostReviewIT extends AbstractDaemonTest {
@Test
public void validateDrafts_draftRejected() throws Exception {
PushOneCommit.Result r = createChange();
CommentForValidation commentForValidation =
CommentForValidation.create(CommentType.INLINE_COMMENT, COMMENT_TEXT);
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(
CommentForValidation.CommentType.INLINE_COMMENT, COMMENT_TEXT))))
.thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
eq(contextFor(r)), argThat(new SingleCommentMatcher(INLINE_COMMENT_FOR_VALIDATION))))
.thenReturn(ImmutableList.of(INLINE_COMMENT_FOR_VALIDATION.failValidation("Oh no!")));
DraftInput draft =
testCommentHelper.newDraft(
@ -233,7 +252,7 @@ public class PostReviewIT extends AbstractDaemonTest {
testCommentHelper.addDraft(r.getChangeId(), r.getCommit().getName(), draftFile);
assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty();
when(mockCommentValidator.validateComments(any(), capture.capture()))
when(mockCommentValidator.validateComments(any(), captor.capture()))
.thenReturn(ImmutableList.of());
ReviewInput input = new ReviewInput();
@ -241,25 +260,33 @@ public class PostReviewIT extends AbstractDaemonTest {
gApi.changes().id(r.getChangeId()).current().review(input);
assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).hasSize(2);
assertThat(capture.getAllValues()).hasSize(1);
assertThat(capture.getValue())
assertThat(captor.getAllValues()).hasSize(1);
assertThat(captor.getValue())
.comparingElementsUsing(
Correspondence.<CommentForValidation, CommentForValidation>from(
(a, b) ->
a.getSource() == b.getSource()
&& a.getType() == b.getType()
&& a.getText().equals(b.getText()),
"matches (ignoring size approximation)"))
.containsExactly(
CommentForValidation.create(
CommentForValidation.CommentType.INLINE_COMMENT, draftInline.message),
CommentForValidation.CommentSource.HUMAN,
CommentForValidation.CommentType.INLINE_COMMENT,
draftInline.message,
draftInline.message.length()),
CommentForValidation.create(
CommentForValidation.CommentType.FILE_COMMENT, draftFile.message));
CommentForValidation.CommentSource.HUMAN,
CommentForValidation.CommentType.FILE_COMMENT,
draftFile.message,
draftFile.message.length()));
}
@Test
public void validateCommentsInChangeMessage_messageOK() throws Exception {
PushOneCommit.Result r = createChange();
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT))))
contextFor(r), ImmutableList.of(CHANGE_MESSAGE_FOR_VALIDATION)))
.thenReturn(ImmutableList.of());
ReviewInput input = new ReviewInput().message(COMMENT_TEXT);
@ -274,16 +301,9 @@ public class PostReviewIT extends AbstractDaemonTest {
@Test
public void validateCommentsInChangeMessage_messageRejected() throws Exception {
PushOneCommit.Result r = createChange();
CommentForValidation commentForValidation =
CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT);
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(r.getChange().getId().get())
.project(r.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT))))
.thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
contextFor(r), ImmutableList.of(CHANGE_MESSAGE_FOR_VALIDATION)))
.thenReturn(ImmutableList.of(CHANGE_MESSAGE_FOR_VALIDATION.failValidation("Oh no!")));
ReviewInput input = new ReviewInput().message(COMMENT_TEXT);
assertThat(gApi.changes().id(r.getChangeId()).get().messages)
@ -308,7 +328,56 @@ public class PostReviewIT extends AbstractDaemonTest {
assertThat(message.message).doesNotContain(COMMENT_TEXT);
}
@Test
@GerritConfig(name = "change.maxComments", value = "4")
public void restrictNumberOfComments() throws Exception {
when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of());
PushOneCommit.Result r = createChange();
String filePath = r.getChange().currentFilePaths().get(0);
CommentInput commentInput = new CommentInput();
commentInput.line = 1;
commentInput.message = "foo";
commentInput.path = filePath;
RobotCommentInput robotCommentInput =
TestCommentHelper.createRobotCommentInputWithMandatoryFields(filePath);
ReviewInput reviewInput = new ReviewInput();
reviewInput.comments = ImmutableMap.of(filePath, ImmutableList.of(commentInput));
reviewInput.robotComments = ImmutableMap.of(filePath, ImmutableList.of(robotCommentInput));
gApi.changes().id(r.getChangeId()).current().review(reviewInput);
// reviewInput still has both a user and a robot comment (and deduplication is false). We also
// create a draft so that in total there would be 5 comments. The limit is set to 4, so this
// verifies that all three channels are considered.
DraftInput draftInline = testCommentHelper.newDraft(filePath, Side.REVISION, 1, "a draft");
testCommentHelper.addDraft(r.getChangeId(), r.getPatchSetId().getId(), draftInline);
reviewInput.drafts = DraftHandling.PUBLISH;
reviewInput.omitDuplicateComments = false;
BadRequestException exception =
assertThrows(
BadRequestException.class,
() -> gApi.changes().id(r.getChangeId()).current().review(reviewInput));
assertThat(exception)
.hasMessageThat()
.contains("Exceeding maximum number of comments: 2 (existing) + 3 (new) > 4");
assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).hasSize(1);
assertThat(getRobotComments(r.getChangeId())).hasSize(1);
}
private List<RobotCommentInfo> getRobotComments(String changeId) throws RestApiException {
return gApi.changes().id(changeId).robotComments().values().stream()
.flatMap(Collection::stream)
.collect(toList());
}
private static CommentInput newComment(String path) {
return TestCommentHelper.populate(new CommentInput(), path, PostReviewIT.COMMENT_TEXT);
}
private static CommentValidationContext contextFor(PushOneCommit.Result result) {
return CommentValidationContext.create(
result.getChange().getId().get(), result.getChange().project().get());
}
}

21
javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
View File

@ -173,9 +173,8 @@ public class RobotCommentsIT extends AbstractDaemonTest {
@Test
public void hugeRobotCommentIsRejected() {
int defaultSizeLimit = 1024 * 1024;
int sizeOfRest = 451;
fixReplacementInfo.replacement = getStringFor(defaultSizeLimit - sizeOfRest + 1);
int defaultSizeLimit = 1 << 20;
fixReplacementInfo.replacement = getStringFor(defaultSizeLimit + 1);
BadRequestException thrown =
assertThrows(
@ -186,9 +185,9 @@ public class RobotCommentsIT extends AbstractDaemonTest {
@Test
public void reasonablyLargeRobotCommentIsAccepted() throws Exception {
int defaultSizeLimit = 1024 * 1024;
int sizeOfRest = 451;
fixReplacementInfo.replacement = getStringFor(defaultSizeLimit - sizeOfRest);
int defaultSizeLimit = 1 << 20;
// Allow for a few hundred bytes in other fields.
fixReplacementInfo.replacement = getStringFor(defaultSizeLimit - 666);
testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput);
@ -199,7 +198,7 @@ public class RobotCommentsIT extends AbstractDaemonTest {
@Test
@GerritConfig(name = "change.robotCommentSizeLimit", value = "10k")
public void maximumAllowedSizeOfRobotCommentCanBeAdjusted() {
int sizeLimit = 10 * 1024;
int sizeLimit = 10 << 20;
fixReplacementInfo.replacement = getStringFor(sizeLimit);
BadRequestException thrown =
@ -212,8 +211,8 @@ public class RobotCommentsIT extends AbstractDaemonTest {
@Test
@GerritConfig(name = "change.robotCommentSizeLimit", value = "0")
public void zeroForMaximumAllowedSizeOfRobotCommentRemovesRestriction() throws Exception {
int defaultSizeLimit = 1024 * 1024;
fixReplacementInfo.replacement = getStringFor(defaultSizeLimit);
int defaultSizeLimit = 1 << 20;
fixReplacementInfo.replacement = getStringFor(2 * defaultSizeLimit);
testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput);
@ -225,8 +224,8 @@ public class RobotCommentsIT extends AbstractDaemonTest {
@GerritConfig(name = "change.robotCommentSizeLimit", value = "-1")
public void negativeValueForMaximumAllowedSizeOfRobotCommentRemovesRestriction()
throws Exception {
int defaultSizeLimit = 1024 * 1024;
fixReplacementInfo.replacement = getStringFor(defaultSizeLimit);
int defaultSizeLimit = 1 << 20;
fixReplacementInfo.replacement = getStringFor(2 * defaultSizeLimit);
testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput);

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

@ -31,7 +31,6 @@ 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.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.testing.TestCommentHelper;
@ -49,9 +48,15 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
@Inject private CommentValidator mockCommentValidator;
@Inject private TestCommentHelper testCommentHelper;
private static final int MAX_COMMENT_LENGTH = 666;
private static final int COMMENT_SIZE_LIMIT = 666;
private static final String COMMENT_TEXT = "The comment text";
private static final CommentForValidation COMMENT_FOR_VALIDATION =
CommentForValidation.create(
CommentForValidation.CommentSource.HUMAN,
CommentForValidation.CommentType.FILE_COMMENT,
COMMENT_TEXT,
COMMENT_TEXT.length());
@Captor private ArgumentCaptor<ImmutableList<CommentForValidation>> capture;
@Captor private ArgumentCaptor<CommentValidationContext> captureCtx;
@ -82,13 +87,9 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
String changeId = result.getChangeId();
String revId = result.getCommit().getName();
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(result.getChange().getId().get())
.project(result.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(
CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT))))
CommentValidationContext.create(
result.getChange().getId().get(), result.getChange().project().get()),
ImmutableList.of(COMMENT_FOR_VALIDATION)))
.thenReturn(ImmutableList.of());
DraftInput comment = testCommentHelper.newDraft(COMMENT_TEXT);
testCommentHelper.addDraft(changeId, revId, comment);
@ -101,20 +102,14 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
@Test
public void validateComments_commentRejected() throws Exception {
CommentForValidation commentForValidation =
CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT);
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
String revId = result.getCommit().getName();
when(mockCommentValidator.validateComments(
CommentValidationContext.builder()
.changeId(result.getChange().getId().get())
.project(result.getChange().project().get())
.build(),
ImmutableList.of(
CommentForValidation.create(
CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT))))
.thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
CommentValidationContext.create(
result.getChange().getId().get(), result.getChange().project().get()),
ImmutableList.of(COMMENT_FOR_VALIDATION)))
.thenReturn(ImmutableList.of(COMMENT_FOR_VALIDATION.failValidation("Oh no!")));
DraftInput comment = testCommentHelper.newDraft(COMMENT_TEXT);
testCommentHelper.addDraft(changeId, revId, comment);
assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).isEmpty();
@ -149,18 +144,24 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
assertThat(capture.getAllValues().get(0))
.containsExactly(
CommentForValidation.create(
CommentForValidation.CommentType.INLINE_COMMENT, draftInline.message),
CommentForValidation.CommentSource.HUMAN,
CommentForValidation.CommentType.INLINE_COMMENT,
draftInline.message,
draftInline.message.length()),
CommentForValidation.create(
CommentForValidation.CommentType.FILE_COMMENT, draftFile.message));
CommentForValidation.CommentSource.HUMAN,
CommentForValidation.CommentType.FILE_COMMENT,
draftFile.message,
draftFile.message.length()));
}
@Test
@GerritConfig(name = "change.maxCommentLength", value = "" + MAX_COMMENT_LENGTH)
@GerritConfig(name = "change.commentSizeLimit", value = "" + COMMENT_SIZE_LIMIT)
public void validateComments_enforceLimits_commentTooLarge() throws Exception {
when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of());
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
int commentLength = MAX_COMMENT_LENGTH + 1;
int commentLength = COMMENT_SIZE_LIMIT + 1;
DraftInput comment =
testCommentHelper.newDraft(new String(new char[commentLength]).replace("\0", "x"));
testCommentHelper.addDraft(changeId, result.getCommit().getName(), comment);
@ -169,7 +170,33 @@ public class ReceiveCommitsCommentValidationIT extends AbstractDaemonTest {
Result amendResult = amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
amendResult.assertOkStatus();
amendResult.assertMessage(
String.format("Comment too large (%d > %d)", commentLength, MAX_COMMENT_LENGTH));
String.format("Comment size exceeds limit (%d > %d)", commentLength, COMMENT_SIZE_LIMIT));
assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).isEmpty();
}
@Test
@GerritConfig(name = "change.maxComments", value = "3")
public void countComments_limitNumberOfComments() throws Exception {
when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of());
PushOneCommit.Result result = createChange();
String changeId = result.getChangeId();
String revId = result.getCommit().getName();
String filePath = result.getChange().currentFilePaths().get(0);
DraftInput draftInline = testCommentHelper.newDraft(filePath, Side.REVISION, 1, COMMENT_TEXT);
testCommentHelper.addDraft(changeId, revId, draftInline);
amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).hasSize(1);
for (int i = 1; i < 3; ++i) {
testCommentHelper.addRobotComment(
changeId,
TestCommentHelper.createRobotCommentInput(result.getChange().currentFilePaths().get(0)));
}
draftInline = testCommentHelper.newDraft(filePath, Side.REVISION, 1, COMMENT_TEXT);
testCommentHelper.addDraft(changeId, revId, draftInline);
Result amendResult = amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).hasSize(1);
amendResult.assertMessage("exceeding maximum number of comments");
}
}

19
javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
View File

@ -33,7 +33,9 @@ import org.junit.Ignore;
public class AbstractMailIT extends AbstractDaemonTest {
@Inject private RequestScopeOperations requestScopeOperations;
protected MailMessage.Builder messageBuilderWithDefaultFields() {
static final String FILE_NAME = "gerrit-server/test.txt";
MailMessage.Builder messageBuilderWithDefaultFields() {
MailMessage.Builder b = MailMessage.builder();
b.id("some id");
b.from(user.getEmailAddress());
@ -43,16 +45,15 @@ public class AbstractMailIT extends AbstractDaemonTest {
return b;
}
protected String createChangeWithReview() throws Exception {
String createChangeWithReview() throws Exception {
return createChangeWithReview(admin);
}
protected String createChangeWithReview(TestAccount reviewer) throws Exception {
String createChangeWithReview(TestAccount reviewer) throws Exception {
// Create change
String file = "gerrit-server/test.txt";
String contents = "contents \nlorem \nipsum \nlorem";
PushOneCommit push =
pushFactory.create(admin.newIdent(), testRepo, "first subject", file, contents);
pushFactory.create(admin.newIdent(), testRepo, "first subject", FILE_NAME, contents);
PushOneCommit.Result r = push.to("refs/for/master");
String changeId = r.getChangeId();
@ -61,8 +62,8 @@ public class AbstractMailIT extends AbstractDaemonTest {
ReviewInput input = new ReviewInput();
input.message = "I have two comments";
input.comments = new HashMap<>();
CommentInput c1 = newComment(file, Side.REVISION, 0, "comment on file");
CommentInput c2 = newComment(file, Side.REVISION, 2, "inline comment");
CommentInput c1 = newComment(FILE_NAME, Side.REVISION, 0, "comment on file");
CommentInput c2 = newComment(FILE_NAME, Side.REVISION, 2, "inline comment");
input.comments.put(c1.path, ImmutableList.of(c1, c2));
revision(r).review(input);
return changeId;
@ -94,7 +95,7 @@ public class AbstractMailIT extends AbstractDaemonTest {
* @param fc1 Comment in reply to a comment of file 1.
* @return A string with all inline comments and the original quoted email.
*/
protected static String newPlaintextBody(
static String newPlaintextBody(
String changeURL, String changeMessage, String c1, String f1, String fc1) {
return (changeMessage == null ? "" : changeMessage + "\n")
+ "> Foo Bar has posted comments on this change. ( \n"
@ -137,7 +138,7 @@ public class AbstractMailIT extends AbstractDaemonTest {
+ "> \n";
}
protected static String textFooterForChange(int changeNumber, String timestamp) {
static String textFooterForChange(int changeNumber, String timestamp) {
return "Gerrit-Change-Number: "
+ changeNumber
+ "\n"

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

@ -21,9 +21,14 @@ import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.extensions.annotations.Exports;
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.RobotCommentInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.CommentInfo;
@ -341,17 +346,53 @@ public class MailProcessorIT extends AbstractMailIT {
assertThat(message.body()).contains("rejected one or more comments");
}
@Test
@GerritConfig(name = "change.maxComments", value = "4")
public void limitNumberOfComments() throws Exception {
String changeId = createChangeWithReview();
CommentInput commentInput = new CommentInput();
commentInput.line = 1;
commentInput.message = "foo";
commentInput.path = FILE_NAME;
RobotCommentInput robotCommentInput =
TestCommentHelper.createRobotCommentInputWithMandatoryFields(FILE_NAME);
ReviewInput reviewInput = new ReviewInput();
reviewInput.comments = ImmutableMap.of(FILE_NAME, ImmutableList.of(commentInput));
reviewInput.robotComments = ImmutableMap.of(FILE_NAME, ImmutableList.of(robotCommentInput));
gApi.changes().id(changeId).current().review(reviewInput);
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")));
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");
}
private String getChangeUrl(ChangeInfo changeInfo) {
return canonicalWebUrl.get() + "c/" + changeInfo.project + "/+/" + changeInfo._number;
}
private void setupFailValidation(
CommentForValidation.CommentType type, String failProject, int failChange) {
CommentForValidation commentForValidation = CommentForValidation.create(type, COMMENT_TEXT);
CommentForValidation commentForValidation =
CommentForValidation.create(
CommentForValidation.CommentSource.HUMAN, type, COMMENT_TEXT, COMMENT_TEXT.length());
when(mockCommentValidator.validateComments(
CommentValidationContext.builder().changeId(failChange).project(failProject).build(),
ImmutableList.of(CommentForValidation.create(type, COMMENT_TEXT))))
CommentValidationContext.create(failChange, failProject),
ImmutableList.of(commentForValidation)))
.thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!")));
}
}

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

@ -64,6 +64,7 @@
{/template}
{template .InboundEmailRejection_COMMENT_REJECTED kind="text"}
Gerrit Code Review rejected one or more comments because they did not pass validation.
Gerrit Code Review rejected one or more comments because they did not pass validation, or
because the maximum number of comments per change would be exceeded.
{call .InboundEmailRejectionFooter /}
{/template}

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

@ -81,7 +81,8 @@
{template .InboundEmailRejectionHtml_COMMENT_REJECTED}
<p>
Gerrit Code Review rejected one or more comments because they did not pass validation.
Gerrit Code Review rejected one or more comments because they did not pass validation, or
because the maximum number of comments per change would be exceeded.
</p>
{call .InboundEmailRejectionFooterHtml /}
{/template}