Fix post submit diff size limit
The change index has a limit for the cumulative size of comments for
each change. At Google, the limit is 5MB. The cumulative comment size
that is default and is set at Google is 3MB, which provides a buffer of
2MB for things that don't count against the limit. Post Submit Diff is
an example for something that doesn't count against the limit.
In the 2 follow-up changes, we will count autogenerated messages and the
post submit diff as part of the limit.
Hence, we fix the maximum size of this post submit diff to be a tenth of
the cumulative size limit that is set (by default that would be 300KB).
That way, there is no danger of surpassing the "real" maximum which is
5MB.
While at it, we also adjust/fix some of the tests about those limits.
Additionally:
1. We do not filter change messages on CommentCumulativeSizeValidator.
2. We check on post submit diff that we do not exceed the limit (and not
just "not exceed 10% of the limit").
Change-Id: Ic67e06dbc2f6330519837e1644a3a2fab2110c59
diff --git a/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java b/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java
index 6e640f3..b887323 100644
--- a/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java
+++ b/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java
@@ -23,7 +23,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.server.ChangeMessagesUtil;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.inject.Inject;
@@ -32,7 +31,8 @@
/**
* 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.
+ * issues. Note that autogenerated change messages are not subject to validation. However, we still
+ * count autogenerated messages for the limit (which will be notified on a further comment).
*/
public class CommentCumulativeSizeValidator implements CommentValidator {
public static final int DEFAULT_CUMULATIVE_COMMENT_SIZE_LIMIT = 3 << 20;
@@ -60,17 +60,11 @@
notes.getRobotComments().values().stream())
.mapToInt(Comment::getApproximateSize)
.sum()
- + notes.getChangeMessages().stream()
- // Auto-generated change messages are not counted for the limit. This method is not
- // called when those change messages are created, but we should also skip them when
- // counting the size for unrelated messages.
- .filter(cm -> !ChangeMessagesUtil.isAutogenerated(cm.getTag()))
- .mapToInt(cm -> cm.getMessage().length())
- .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) {
+ if (!comments.isEmpty() && !isEnoughSpace(notes, 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);
@@ -84,4 +78,19 @@
}
return failures.build();
}
+
+ /**
+ * Returns {@code true} if there is available space and the new size that we wish to add is less
+ * than the maximum allowed size. {@code false} otherwise (if there is not enough space).
+ */
+ public static boolean isEnoughSpace(ChangeNotes notes, int addedBytes, int maxCumulativeSize) {
+ int existingCumulativeSize =
+ Stream.concat(
+ notes.getHumanComments().values().stream(),
+ notes.getRobotComments().values().stream())
+ .mapToInt(Comment::getApproximateSize)
+ .sum()
+ + notes.getChangeMessages().stream().mapToInt(cm -> cm.getMessage().length()).sum();
+ return existingCumulativeSize + addedBytes < maxCumulativeSize;
+ }
}
diff --git a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
index 572d73d..c918e3d 100644
--- a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
+++ b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
@@ -70,6 +70,7 @@
*/
public class SubmitWithStickyApprovalDiff {
private static final int HEAP_EST_SIZE = 32 * 1024;
+ private static final int DEFAULT_POST_SUBMIT_SIZE_LIMIT = 300 * 1024; // 300 KiB
private final DiffOperations diffOperations;
private final ProjectCache projectCache;
@@ -88,6 +89,15 @@
this.projectCache = projectCache;
this.patchScriptFactoryFactory = patchScriptFactoryFactory;
this.repositoryManager = repositoryManager;
+ // (November 2021) We define the max cumulative comment size to 300 KIB since it's a reasonable
+ // size that is large enough for all purposes but not too large to choke the change index by
+ // exceeding the cumulative comment size limit (new comments are not allowed once the limit
+ // is reached). At Google, the change index limit is 5MB, while the cumulative size limit is
+ // set at 3MB. In this example, we can reach at most 3.3MB hence we ensure not to exceed the
+ // limit of 5MB.
+ // The reason we exclude the post submit diff from the cumulative comment size limit is
+ // just because change messages not currently being validated. Change messages are still
+ // counted towards the limit, though.
maxCumulativeSize =
serverConfig.getInt(
"change",
@@ -129,7 +139,9 @@
diff.append("The change was submitted with unreviewed changes in the following files:\n\n");
TemporaryBuffer.Heap buffer =
- new TemporaryBuffer.Heap(Math.min(HEAP_EST_SIZE, maxCumulativeSize), maxCumulativeSize);
+ new TemporaryBuffer.Heap(
+ Math.min(HEAP_EST_SIZE, DEFAULT_POST_SUBMIT_SIZE_LIMIT),
+ DEFAULT_POST_SUBMIT_SIZE_LIMIT);
try (Repository repository = repositoryManager.openRepository(notes.getProjectName());
DiffFormatter formatter = new DiffFormatter(buffer)) {
formatter.setRepository(repository);
@@ -150,6 +162,12 @@
throw e;
}
}
+ if (formatterResult != null) {
+ int addedBytes = formatterResult.stream().mapToInt(String::length).sum();
+ if (!CommentCumulativeSizeValidator.isEnoughSpace(notes, addedBytes, maxCumulativeSize)) {
+ isDiffTooLarge = true;
+ }
+ }
for (FileDiffOutput fileDiff : modifiedFilesList) {
diff.append(
getDiffForFile(
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
index 5124d11..29fdc15 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
@@ -19,6 +19,7 @@
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder;
import static com.google.gerrit.server.project.testing.TestLabels.value;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static org.eclipse.jgit.lib.Constants.HEAD;
import com.google.common.collect.ImmutableList;
@@ -35,6 +36,7 @@
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.server.project.testing.TestLabels;
import com.google.inject.Inject;
import java.util.HashSet;
@@ -345,8 +347,8 @@
}
@Test
- @GerritConfig(name = "change.cumulativeCommentSizeLimit", value = "1k")
- public void autoGeneratedPostSubmitDiffIsNotPartOfTheCommentSizeLimit() throws Exception {
+ @GerritConfig(name = "change.cumulativeCommentSizeLimit", value = "10k")
+ public void autoGeneratedPostSubmitDiffIsPartOfTheCommentSizeLimit() throws Exception {
Change.Id changeId =
changeOperations.newChange().project(project).file("file").content("content").create();
gApi.changes().id(changeId.get()).current().review(ReviewInput.approve());
@@ -356,35 +358,69 @@
// Post a submit diff that is almost the cumulativeCommentSizeLimit
gApi.changes().id(changeId.get()).current().submit();
assertThat(Iterables.getLast(gApi.changes().id(changeId.get()).messages()).message)
- .doesNotContain("many unreviewed changes");
+ .doesNotContain("The diff is too large to show. Please review the diff");
- // unrelated comment and change message posting works fine, since the post submit diff is not
+ // unrelated comment and change message posting doesn't work, since the post submit diff is
// counted towards the cumulativeCommentSizeLimit for unrelated follow-up comments.
- // 800 + 400 + 400 > 1k, but 400 + 400 < 1k, hence these comments are accepted (the original
- // 800 is not counted).
- String message = new String(new char[400]).replace("\0", "a");
+ // 800 + 9500 > 10k.
+ String message = new String(new char[9500]).replace("\0", "a");
ReviewInput reviewInput = new ReviewInput().message(message);
CommentInput commentInput = new CommentInput();
commentInput.line = 1;
- commentInput.message = message;
commentInput.path = "file";
reviewInput.comments = ImmutableMap.of("file", ImmutableList.of(commentInput));
- gApi.changes().id(changeId.get()).current().review(reviewInput);
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(changeId.get()).current().review(reviewInput));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains("Exceeding maximum cumulative size of comments and change messages");
}
@Test
- @GerritConfig(name = "change.cumulativeCommentSizeLimit", value = "1k")
public void postSubmitDiffCannotBeTooBig() throws Exception {
Change.Id changeId =
changeOperations.newChange().project(project).file("file").content("content").create();
gApi.changes().id(changeId.get()).current().review(ReviewInput.approve());
- String content = new String(new char[1100]).replace("\0", "a");
+ // max post submit diff size is 300k
+ String content = new String(new char[320000]).replace("\0", "a");
changeOperations.change(changeId).newPatchset().file("file").content(content).create();
- // Post submit diff is over the cumulativeCommentSizeLimit, so we shorten the message.
+ // Post submit diff is over the postSubmitDiffSizeLimit (300k).
+ gApi.changes().id(changeId.get()).current().submit();
+ assertThat(Iterables.getLast(gApi.changes().id(changeId.get()).messages()).message)
+ .isEqualTo(
+ "Change has been successfully merged\n\n1 is the latest approved patch-set.\nThe "
+ + "change was submitted with unreviewed changes in the following "
+ + "files:\n\n```\nThe name of the file: file\nInsertions: 1, Deletions: 1.\n\nThe"
+ + " diff is too large to show. Please review the diff.\n```\n");
+ }
+
+ @Test
+ @GerritConfig(name = "change.cumulativeCommentSizeLimit", value = "10k")
+ public void postSubmitDiffCannotBeTooBigWithLargeComments() throws Exception {
+ Change.Id changeId =
+ changeOperations.newChange().project(project).file("file").content("content").create();
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.approve());
+
+ // unrelated comment taking up most of the space, making post submit diff shorter.
+ String message = new String(new char[9700]).replace("\0", "a");
+ ReviewInput reviewInput = new ReviewInput().message(message);
+ CommentInput commentInput = new CommentInput();
+ commentInput.line = 1;
+ commentInput.path = "file";
+ reviewInput.comments = ImmutableMap.of("file", ImmutableList.of(commentInput));
+ gApi.changes().id(changeId.get()).current().review(reviewInput);
+
+ String content = new String(new char[500]).replace("\0", "a");
+ changeOperations.change(changeId).newPatchset().file("file").content(content).create();
+
+ // Post submit diff is over the cumulativeCommentSizeLimit, since the comment took most of
+ // the space (even though the post submit diff is not limited).
gApi.changes().id(changeId.get()).current().submit();
assertThat(Iterables.getLast(gApi.changes().id(changeId.get()).messages()).message)
.isEqualTo(
