Merge "Enforce size limits for post submit diffs"
diff --git a/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java b/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java
index d507531..6e640f3 100644
--- a/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java
+++ b/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java
@@ -23,6 +23,7 @@
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;
@@ -34,6 +35,8 @@
* issues. Note that autogenerated change messages are not subject to validation.
*/
public class CommentCumulativeSizeValidator implements CommentValidator {
+ public static final int DEFAULT_CUMULATIVE_COMMENT_SIZE_LIMIT = 3 << 20;
+
private final int maxCumulativeSize;
private final ChangeNotes.Factory notesFactory;
@@ -41,7 +44,9 @@
CommentCumulativeSizeValidator(
@GerritServerConfig Config serverConfig, ChangeNotes.Factory notesFactory) {
this.notesFactory = notesFactory;
- maxCumulativeSize = serverConfig.getInt("change", "cumulativeCommentSizeLimit", 3 << 20);
+ maxCumulativeSize =
+ serverConfig.getInt(
+ "change", "cumulativeCommentSizeLimit", DEFAULT_CUMULATIVE_COMMENT_SIZE_LIMIT);
}
@Override
@@ -55,7 +60,13 @@
notes.getRobotComments().values().stream())
.mapToInt(Comment::getApproximateSize)
.sum()
- + notes.getChangeMessages().stream().mapToInt(cm -> cm.getMessage().length()).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();
int newCumulativeSize =
comments.stream().mapToInt(CommentForValidation::getApproximateSize).sum();
ImmutableList.Builder<CommentValidationFailure> failures = ImmutableList.builder();
diff --git a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
index 63e0b7a..18d532b 100644
--- a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
+++ b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
@@ -30,7 +30,9 @@
import com.google.gerrit.prettify.common.SparseFileContent;
import com.google.gerrit.prettify.common.SparseFileContent.Accessor;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.LargeObjectException;
+import com.google.gerrit.server.git.validators.CommentCumulativeSizeValidator;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.InvalidChangeOperationException;
@@ -41,6 +43,7 @@
import java.util.List;
import java.util.stream.Collectors;
import org.eclipse.jgit.diff.Edit;
+import org.eclipse.jgit.lib.Config;
/**
* This class is used on submit to compute the diff between the latest approved patch-set, and the
@@ -58,15 +61,22 @@
private final ProjectCache projectCache;
private final PatchScriptFactory.Factory patchScriptFactoryFactory;
private final PatchListCache patchListCache;
+ private final int maxCumulativeSize;
@Inject
SubmitWithStickyApprovalDiff(
ProjectCache projectCache,
PatchScriptFactory.Factory patchScriptFactoryFactory,
- PatchListCache patchListCache) {
+ PatchListCache patchListCache,
+ @GerritServerConfig Config serverConfig) {
this.projectCache = projectCache;
this.patchScriptFactoryFactory = patchScriptFactoryFactory;
this.patchListCache = patchListCache;
+ maxCumulativeSize =
+ serverConfig.getInt(
+ "change",
+ "cumulativeCommentSizeLimit",
+ CommentCumulativeSizeValidator.DEFAULT_CUMULATIVE_COMMENT_SIZE_LIMIT);
}
public String apply(ChangeNotes notes, CurrentUser currentUser)
@@ -117,6 +127,16 @@
getDiffForFile(
notes, currentPatchset.id(), latestApprovedPatchsetId, patchListEntry, currentUser));
}
+ if (diff.length() > maxCumulativeSize) {
+ // The diff length is not counted as part of the limit (for technical reasons, since we'd
+ // have to call CommentCumulativeSizeValidator), but it's best not to post an extra large
+ // change message here.
+ return String.format(
+ "\n\n%d is the latest approved patch-set.\nThe change was submitted "
+ + "with many unreviewed changes (the diff is too large to show). Please review the "
+ + "diff.",
+ latestApprovedPatchsetId.get());
+ }
return diff.toString();
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
index eba2634..5ca7310 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
@@ -21,8 +21,10 @@
import static com.google.gerrit.server.project.testing.TestLabels.value;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.entities.Change;
@@ -30,6 +32,7 @@
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.server.patch.filediff.Edit;
import com.google.gerrit.server.project.testing.TestLabels;
import com.google.inject.Inject;
@@ -144,6 +147,56 @@
}
@Test
+ @GerritConfig(name = "change.cumulativeCommentSizeLimit", value = "1k")
+ public void autoGeneratedPostSubmitDiffIsNotPartOfTheCommentSizeLimit() 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[800]).replace("\0", "a");
+ changeOperations.change(changeId).newPatchset().file("file").content(content).create();
+
+ // 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");
+
+ // unrelated comment and change message posting works fine, since the post submit diff is not
+ // 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");
+ 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);
+ }
+
+ @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");
+
+ changeOperations.change(changeId).newPatchset().file("file").content(content).create();
+
+ // Post submit diff is over the cumulativeCommentSizeLimit, so we shorten the message.
+ 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 many unreviewed changes (the diff is too large to show). Please review the "
+ + "diff.");
+ }
+
+ @Test
public void diffChangeMessageOnSubmitWithStickyVote_addedFile() throws Exception {
Change.Id changeId = changeOperations.newChange().project(project).create();
gApi.changes().id(changeId.get()).current().review(ReviewInput.approve());
