Merge "Fix post submit diff size limit"
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(
