Limit the number of comments per change (to 5000 by default)
Also refactor the robot comment size check to be in line with the size
check for human comments.
Change-Id: If24adc10f5f16cd8894bf246078f0e6c992cdafa
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index fc4b00a..0dfe13d 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1195,6 +1195,15 @@
+
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 @@
+
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 @@
[[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.
diff --git a/java/com/google/gerrit/entities/Comment.java b/java/com/google/gerrit/entities/Comment.java
index 55d739a..9c58fef 100644
--- a/java/com/google/gerrit/entities/Comment.java
+++ b/java/com/google/gerrit/entities/Comment.java
@@ -29,6 +29,8 @@
*
* <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 static class Key {
+ public static final class Key {
public String uuid;
public String filename;
public int patchSetId;
@@ -97,7 +99,7 @@
}
}
- public static class Identity {
+ public static final class Identity {
int id;
public Identity(Account.Id id) {
@@ -147,7 +149,7 @@
* </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 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 @@
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)) {
diff --git a/java/com/google/gerrit/entities/FixReplacement.java b/java/com/google/gerrit/entities/FixReplacement.java
index 046300e..fbbf746 100644
--- a/java/com/google/gerrit/entities/FixReplacement.java
+++ b/java/com/google/gerrit/entities/FixReplacement.java
@@ -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 @@
+ '\''
+ '}';
}
+
+ /** Returns this instance's approximate size in bytes for the purpose of applying size limits. */
+ int getApproximateSize() {
+ return path.length() + replacement.length();
+ }
}
diff --git a/java/com/google/gerrit/entities/FixSuggestion.java b/java/com/google/gerrit/entities/FixSuggestion.java
index ac4e720..c88afe7 100644
--- a/java/com/google/gerrit/entities/FixSuggestion.java
+++ b/java/com/google/gerrit/entities/FixSuggestion.java
@@ -16,10 +16,10 @@
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 @@
+ 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);
+ }
}
diff --git a/java/com/google/gerrit/entities/RobotComment.java b/java/com/google/gerrit/entities/RobotComment.java
index a7951ad..4a17566 100644
--- a/java/com/google/gerrit/entities/RobotComment.java
+++ b/java/com/google/gerrit/entities/RobotComment.java
@@ -19,7 +19,7 @@
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;
@@ -41,6 +41,22 @@
}
@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()
.add("robotId", robotId)
diff --git a/java/com/google/gerrit/extensions/validators/CommentForValidation.java b/java/com/google/gerrit/extensions/validators/CommentForValidation.java
index 51ae5ae..9b91dff 100644
--- a/java/com/google/gerrit/extensions/validators/CommentForValidation.java
+++ b/java/com/google/gerrit/extensions/validators/CommentForValidation.java
@@ -17,13 +17,21 @@
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 @@
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);
}
diff --git a/java/com/google/gerrit/extensions/validators/CommentValidationContext.java b/java/com/google/gerrit/extensions/validators/CommentValidationContext.java
index 1cb00e3..db08058 100644
--- a/java/com/google/gerrit/extensions/validators/CommentValidationContext.java
+++ b/java/com/google/gerrit/extensions/validators/CommentValidationContext.java
@@ -34,16 +34,7 @@
/** 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);
}
}
diff --git a/java/com/google/gerrit/extensions/validators/CommentValidator.java b/java/com/google/gerrit/extensions/validators/CommentValidator.java
index ba73e46..2dfd6e2 100644
--- a/java/com/google/gerrit/extensions/validators/CommentValidator.java
+++ b/java/com/google/gerrit/extensions/validators/CommentValidator.java
@@ -25,7 +25,10 @@
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.
*/
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 7df3902..012e4cd 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -128,7 +128,8 @@
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 @@
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);
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index d8aa054..4b6a4cd 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -92,6 +92,7 @@
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.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 @@
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 @@
.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 @@
.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(
diff --git a/java/com/google/gerrit/server/git/validators/CommentCountValidator.java b/java/com/google/gerrit/server/git/validators/CommentCountValidator.java
new file mode 100644
index 0000000..2c939c8
--- /dev/null
+++ b/java/com/google/gerrit/server/git/validators/CommentCountValidator.java
@@ -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();
+ }
+}
diff --git a/java/com/google/gerrit/server/git/validators/CommentLimitsValidator.java b/java/com/google/gerrit/server/git/validators/CommentLimitsValidator.java
deleted file mode 100644
index 3a8bcac..0000000
--- a/java/com/google/gerrit/server/git/validators/CommentLimitsValidator.java
+++ /dev/null
@@ -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());
- }
-}
diff --git a/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java b/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java
new file mode 100644
index 0000000..58b0cb1
--- /dev/null
+++ b/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java
@@ -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());
+ }
+}
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index e79696a..9c3dd02 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -19,6 +19,7 @@
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 @@
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 @@
.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);
diff --git a/java/com/google/gerrit/server/notedb/LimitExceededException.java b/java/com/google/gerrit/server/notedb/LimitExceededException.java
index 69f9241..0a57d96 100644
--- a/java/com/google/gerrit/server/notedb/LimitExceededException.java
+++ b/java/com/google/gerrit/server/notedb/LimitExceededException.java
@@ -23,12 +23,13 @@
* <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);
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 324069d..19f126a 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -80,7 +80,6 @@
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.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.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 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
}
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 @@
.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 @@
}
}
- 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 @@
}
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);
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 524a05e..707076c 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -16,7 +16,10 @@
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 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.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 @@
@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 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 @@
@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 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 @@
@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 @@
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 @@
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 @@
@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 @@
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());
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
index c35ded6..6b92c16 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
@@ -173,9 +173,8 @@
@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 @@
@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 @@
@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 @@
@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 @@
@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);
diff --git a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
index ccfe783..eb1f907 100644
--- a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
@@ -31,7 +31,6 @@
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 @@
@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 @@
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 @@
@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 @@
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 @@
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");
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java b/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
index 768c269..3b0e27b 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
@@ -33,7 +33,9 @@
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 @@
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 @@
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 @@
* @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 @@
+ "> \n";
}
- protected static String textFooterForChange(int changeNumber, String timestamp) {
+ static String textFooterForChange(int changeNumber, String timestamp) {
return "Gerrit-Change-Number: "
+ changeNumber
+ "\n"
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
index 2409f52..502194a 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
@@ -21,9 +21,14 @@
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 @@
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!")));
}
}
diff --git a/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy b/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
index e88c424..16c5c8d 100644
--- a/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
+++ b/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
@@ -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}
diff --git a/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy b/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
index e17508d..8762e10 100644
--- a/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
+++ b/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
@@ -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}
