Add comment validator for cumulative size
Even though we have validators for comment count and individual comment
size, storage systems fail before both are maxed out.
Change-Id: I93a6374baa9bad4d82f4e51f9593315c1ca68d48
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index d5af678..7dcd97e 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1221,7 +1221,15 @@
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.
+The default limit is 16kiB.
+
+[[change.cumulativeCommentSizeLimit]]change.cumulativeCommentSizeLimit::
++
+Maximum allowed size in characters of all comments (including robot comments)
+and change messages. Size computation is approximate and may be off by roughly
+1%. Common unit suffixes of 'k', 'm', or 'g' are supported.
++
+The default limit is 3MiB.
[[change.disablePrivateChanges]]change.disablePrivateChanges::
+
@@ -1348,7 +1356,7 @@
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.
+The default limit is 1MiB.
[[change.showAssigneeInChangesTable]]change.showAssigneeInChangesTable::
+
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 5c52db3..1f8075a 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -129,6 +129,7 @@
import com.google.gerrit.server.git.TransferConfig;
import com.google.gerrit.server.git.receive.ReceiveCommitsModule;
import com.google.gerrit.server.git.validators.CommentCountValidator;
+import com.google.gerrit.server.git.validators.CommentCumulativeSizeValidator;
import com.google.gerrit.server.git.validators.CommentSizeValidator;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.MergeValidationListener;
@@ -419,6 +420,9 @@
bind(CommentValidator.class)
.annotatedWith(Exports.named(CommentSizeValidator.class.getSimpleName()))
.to(CommentSizeValidator.class);
+ bind(CommentValidator.class)
+ .annotatedWith(Exports.named(CommentCumulativeSizeValidator.class.getSimpleName()))
+ .to(CommentCumulativeSizeValidator.class);
DynamicMap.mapOf(binder(), DynamicOptions.DynamicBean.class);
DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeOperatorFactory.class);
diff --git a/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java b/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java
new file mode 100644
index 0000000..d9a1420
--- /dev/null
+++ b/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java
@@ -0,0 +1,76 @@
+// 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.Comment;
+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 java.util.stream.Stream;
+import org.eclipse.jgit.lib.Config;
+
+/**
+ * Limits the total size of all comments and change messages to prevent space/time complexity
+ * issues. Note that autogenerated change messages are not subject to validation.
+ */
+public class CommentCumulativeSizeValidator implements CommentValidator {
+ private final int maxCumulativeSize;
+ private final ChangeNotes.Factory notesFactory;
+
+ @Inject
+ CommentCumulativeSizeValidator(
+ @GerritServerConfig Config serverConfig, ChangeNotes.Factory notesFactory) {
+ this.notesFactory = notesFactory;
+ maxCumulativeSize = serverConfig.getInt("change", "cumulativeCommentSizeLimit", 3 << 20);
+ }
+
+ @Override
+ public ImmutableList<CommentValidationFailure> validateComments(
+ CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) {
+ ChangeNotes notes =
+ notesFactory.createChecked(Project.nameKey(ctx.getProject()), Change.id(ctx.getChangeId()));
+ int existingCumulativeSize =
+ Stream.concat(
+ notes.getComments().values().stream(),
+ notes.getRobotComments().values().stream())
+ .mapToInt(Comment::getApproximateSize)
+ .sum()
+ + notes.getChangeMessages().stream().mapToInt(cm -> cm.getMessage().length()).sum();
+ int newCumulativeSize =
+ comments.stream().mapToInt(CommentForValidation::getApproximateSize).sum();
+ ImmutableList.Builder<CommentValidationFailure> failures = ImmutableList.builder();
+ if (!comments.isEmpty() && existingCumulativeSize + newCumulativeSize > maxCumulativeSize) {
+ // 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 cumulative size of comments and change messages:"
+ + " %d (existing) + %d (new) > %d",
+ existingCumulativeSize, newCumulativeSize, maxCumulativeSize)));
+ }
+ return failures.build();
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 9a71da6..be0cc04 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -352,6 +352,36 @@
assertThat(getRobotComments(r.getChangeId())).hasSize(1);
}
+ @Test
+ @GerritConfig(name = "change.cumulativeCommentSizeLimit", value = "7k")
+ public void validateCumulativeCommentSize() throws Exception {
+ PushOneCommit.Result r = createChange();
+ when(mockCommentValidator.validateComments(eq(contextFor(r)), any()))
+ .thenReturn(ImmutableList.of());
+
+ // Use large sizes because autogenerated messages already have O(100) bytes.
+ String commentText2000Bytes = new String(new char[2000]).replace("\0", "x");
+ String filePath = r.getChange().currentFilePaths().get(0);
+ ReviewInput reviewInput = new ReviewInput().message(commentText2000Bytes);
+ CommentInput commentInput = new CommentInput();
+ commentInput.line = 1;
+ commentInput.message = commentText2000Bytes;
+ commentInput.path = filePath;
+ reviewInput.comments = ImmutableMap.of(filePath, ImmutableList.of(commentInput));
+
+ // Use up ~4000 bytes.
+ gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+
+ // Hit the limit when trying that again.
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().review(reviewInput));
+ assertThat(exception)
+ .hasMessageThat()
+ .contains("Exceeding maximum cumulative size of comments");
+ }
+
private List<RobotCommentInfo> getRobotComments(String changeId) throws RestApiException {
return gApi.changes().id(changeId).robotComments().values().stream()
.flatMap(Collection::stream)
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 471172a..d68cada 100644
--- a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
@@ -205,4 +205,26 @@
assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).hasSize(1);
amendResult.assertMessage("exceeding maximum number of comments");
}
+
+ @Test
+ @GerritConfig(name = "change.cumulativeCommentSizeLimit", value = "500")
+ public void limitCumulativeCommentSize() 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);
+ String commentText400Bytes = new String(new char[400]).replace("\0", "x");
+ DraftInput draftInline =
+ testCommentHelper.newDraft(filePath, Side.REVISION, 1, commentText400Bytes);
+ testCommentHelper.addDraft(changeId, revId, draftInline);
+ amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo);
+ assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).hasSize(1);
+
+ draftInline = testCommentHelper.newDraft(filePath, Side.REVISION, 1, commentText400Bytes);
+ 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 cumulative size of comments");
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
index bdfe0e8..8cb2be4 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
@@ -389,6 +389,47 @@
assertThat(message.body()).contains("rejected one or more comments");
}
+ @Test
+ @GerritConfig(name = "change.cumulativeCommentSizeLimit", value = "7k")
+ public void limitCumulativeCommentSize() throws Exception {
+ // Use large sizes because autogenerated messages already have O(100) bytes.
+ String commentText2000Bytes = new String(new char[2000]).replace("\0", "x");
+ String changeId = createChangeWithReview();
+ CommentInput commentInput = new CommentInput();
+ commentInput.line = 1;
+ commentInput.message = commentText2000Bytes;
+ commentInput.path = FILE_NAME;
+ ReviewInput reviewInput = new ReviewInput().message(commentText2000Bytes);
+ reviewInput.comments = ImmutableMap.of(FILE_NAME, ImmutableList.of(commentInput));
+ // Use up ~4000 bytes.
+ 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")));
+
+ // Hit the limit when trying that again.
+ MailMessage.Builder mailMessage = messageBuilderWithDefaultFields();
+ String txt =
+ newPlaintextBody(
+ getChangeUrl(changeInfo) + "/1",
+ "change message: " + commentText2000Bytes,
+ "reply to comment: " + commentText2000Bytes,
+ null,
+ null);
+ mailMessage.textContent(txt + textFooterForChange(changeInfo._number, ts));
+
+ Collection<CommentInfo> commentsBefore = testCommentHelper.getPublishedComments(changeId);
+ mailProcessor.process(mailMessage.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;
}
