Enforce a limit on the number of patch sets per change
The default limit is 1500. There are currently only 8 changes above that
limit.
This change also replaces the existing TooManyUpdatesException and
introduces a common LimitExceededException class.
Change-Id: I2527234dae2f9e0bfc0ed7977d1d1ce60b882923
diff --git a/java/com/google/gerrit/server/notedb/AllUsersAsyncUpdate.java b/java/com/google/gerrit/server/notedb/AllUsersAsyncUpdate.java
index 5d909d0..030cfb2 100644
--- a/java/com/google/gerrit/server/notedb/AllUsersAsyncUpdate.java
+++ b/java/com/google/gerrit/server/notedb/AllUsersAsyncUpdate.java
@@ -91,7 +91,7 @@
executor.submit(
() -> {
try (OpenRepo allUsersRepo = OpenRepo.open(repoManager, allUsersName)) {
- allUsersRepo.addUpdates(draftUpdates);
+ allUsersRepo.addUpdatesNoLimits(draftUpdates);
allUsersRepo.flush();
BatchRefUpdate bru = allUsersRepo.repo.getRefDatabase().newBatchUpdate();
bru.setPushCertificate(pushCert);
diff --git a/java/com/google/gerrit/server/notedb/LimitExceededException.java b/java/com/google/gerrit/server/notedb/LimitExceededException.java
new file mode 100644
index 0000000..9e0969b
--- /dev/null
+++ b/java/com/google/gerrit/server/notedb/LimitExceededException.java
@@ -0,0 +1,32 @@
+// Copyright (C) 2019 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.notedb;
+
+import com.google.gerrit.exceptions.StorageException;
+
+/**
+ * A write operation was rejected because a limit would be exceeded. Limits are currently imposed
+ * on:
+ *
+ * <ul>
+ * <li>The number of NoteDb updates per change.
+ * <li>The number of patch sets per change.
+ * </ul>
+ */
+public class LimitExceededException extends StorageException {
+ LimitExceededException(String message) {
+ super(message);
+ }
+}
diff --git a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
index 886e02b..1b92c0e 100644
--- a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
+++ b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
@@ -64,6 +64,10 @@
* {@link #stage()}.
*/
public class NoteDbUpdateManager implements AutoCloseable {
+ private static final int MAX_UPDATES_DEFAULT = 1000;
+ /** Limits the number of patch sets that can be created. Can be overridden in the config. */
+ private static final int MAX_PATCH_SETS_DEFAULT = 1500;
+
public interface Factory {
NoteDbUpdateManager create(Project.NameKey projectName);
}
@@ -74,6 +78,7 @@
private final NoteDbMetrics metrics;
private final Project.NameKey projectName;
private final int maxUpdates;
+ private final int maxPatchSets;
private final ListMultimap<String, ChangeUpdate> changeUpdates;
private final ListMultimap<String, ChangeDraftUpdate> draftUpdates;
private final ListMultimap<String, RobotCommentUpdate> robotCommentUpdates;
@@ -103,7 +108,8 @@
this.metrics = metrics;
this.updateAllUsersAsync = updateAllUsersAsync;
this.projectName = projectName;
- maxUpdates = cfg.getInt("change", null, "maxUpdates", 1000);
+ maxUpdates = cfg.getInt("change", null, "maxUpdates", MAX_UPDATES_DEFAULT);
+ maxPatchSets = cfg.getInt("change", null, "maxPatchSets", MAX_PATCH_SETS_DEFAULT);
changeUpdates = MultimapBuilder.hashKeys().arrayListValues().build();
draftUpdates = MultimapBuilder.hashKeys().arrayListValues().build();
robotCommentUpdates = MultimapBuilder.hashKeys().arrayListValues().build();
@@ -351,17 +357,17 @@
}
private void addCommands() throws IOException {
- changeRepo.addUpdates(changeUpdates, Optional.of(maxUpdates));
+ changeRepo.addUpdates(changeUpdates, Optional.of(maxUpdates), Optional.of(maxPatchSets));
if (!draftUpdates.isEmpty()) {
boolean publishOnly = draftUpdates.values().stream().allMatch(ChangeDraftUpdate::canRunAsync);
if (publishOnly) {
updateAllUsersAsync.setDraftUpdates(draftUpdates);
} else {
- allUsersRepo.addUpdates(draftUpdates);
+ allUsersRepo.addUpdatesNoLimits(draftUpdates);
}
}
if (!robotCommentUpdates.isEmpty()) {
- changeRepo.addUpdates(robotCommentUpdates);
+ changeRepo.addUpdatesNoLimits(robotCommentUpdates);
}
if (!rewriters.isEmpty()) {
addRewrites(rewriters, changeRepo);
@@ -375,17 +381,16 @@
private void doDelete(Change.Id id) throws IOException {
String metaRef = RefNames.changeMetaRef(id);
Optional<ObjectId> old = changeRepo.cmds.get(metaRef);
- if (old.isPresent()) {
- changeRepo.cmds.add(new ReceiveCommand(old.get(), ObjectId.zeroId(), metaRef));
- }
+ old.ifPresent(
+ objectId -> changeRepo.cmds.add(new ReceiveCommand(objectId, ObjectId.zeroId(), metaRef)));
// Just scan repo for ref names, but get "old" values from cmds.
for (Ref r :
allUsersRepo.repo.getRefDatabase().getRefsByPrefix(RefNames.refsDraftCommentsPrefix(id))) {
old = allUsersRepo.cmds.get(r.getName());
- if (old.isPresent()) {
- allUsersRepo.cmds.add(new ReceiveCommand(old.get(), ObjectId.zeroId(), r.getName()));
- }
+ old.ifPresent(
+ objectId ->
+ allUsersRepo.cmds.add(new ReceiveCommand(objectId, ObjectId.zeroId(), r.getName())));
}
}
diff --git a/java/com/google/gerrit/server/notedb/OpenRepo.java b/java/com/google/gerrit/server/notedb/OpenRepo.java
index de88684..a60309a 100644
--- a/java/com/google/gerrit/server/notedb/OpenRepo.java
+++ b/java/com/google/gerrit/server/notedb/OpenRepo.java
@@ -43,6 +43,18 @@
* objects that are jointly closed when invoking {@link #close}.
*/
class OpenRepo implements AutoCloseable {
+ private static final Integer UNLIMITED_UPDATES = null;
+ private static final Integer UNLIMITED_PATCH_SETS = null;
+
+ final Repository repo;
+ final RevWalk rw;
+ final ChainedReceiveCommands cmds;
+ final ObjectInserter tempIns;
+
+ private final InMemoryInserter inMemIns;
+ @Nullable private final ObjectInserter finalIns;
+ private final boolean close;
+
/** Returns a {@link OpenRepo} wrapping around an open {@link Repository}. */
static OpenRepo open(GitRepositoryManager repoManager, Project.NameKey project)
throws IOException {
@@ -60,15 +72,6 @@
}
}
- final Repository repo;
- final RevWalk rw;
- final ChainedReceiveCommands cmds;
- final ObjectInserter tempIns;
-
- private final InMemoryInserter inMemIns;
- @Nullable private final ObjectInserter finalIns;
- private final boolean close;
-
OpenRepo(
Repository repo,
RevWalk rw,
@@ -125,12 +128,15 @@
return updates.iterator().next().allowWriteToNewRef();
}
- <U extends AbstractChangeUpdate> void addUpdates(ListMultimap<String, U> all) throws IOException {
- addUpdates(all, Optional.empty());
+ <U extends AbstractChangeUpdate> void addUpdatesNoLimits(ListMultimap<String, U> all)
+ throws IOException {
+ addUpdates(
+ all, Optional.empty() /* unlimited updates */, Optional.empty() /* unlimited patch sets */);
}
<U extends AbstractChangeUpdate> void addUpdates(
- ListMultimap<String, U> all, Optional<Integer> maxUpdates) throws IOException {
+ ListMultimap<String, U> all, Optional<Integer> maxUpdates, Optional<Integer> maxPatchSets)
+ throws IOException {
for (Map.Entry<String, Collection<U>> e : all.asMap().entrySet()) {
String refName = e.getKey();
Collection<U> updates = e.getValue();
@@ -142,29 +148,43 @@
continue;
}
- int updateCount;
+ int updateCount = 0;
U first = updates.iterator().next();
if (maxUpdates.isPresent()) {
checkState(first.getNotes() != null, "expected ChangeNotes on %s", first);
updateCount = first.getNotes().getUpdateCount();
- } else {
- updateCount = 0;
}
ObjectId curr = old;
- for (U u : updates) {
- if (u.isRootOnly() && !old.equals(ObjectId.zeroId())) {
+ for (U update : updates) {
+ if (maxPatchSets.isPresent() && update.psId != null) {
+ // Patch set IDs are assigned consecutively. Patch sets may have been deleted, but the ID
+ // is still a good estimate and an upper bound.
+ if (update.psId.get() > maxPatchSets.get()) {
+ throw new LimitExceededException(
+ String.format(
+ "Change %d may not exceed %d patch sets. To continue working on this change, "
+ + "recreate it with a new Change-Id, then abandon this one.",
+ update.getId().get(), maxPatchSets.get()));
+ }
+ }
+ if (update.isRootOnly() && !old.equals(ObjectId.zeroId())) {
throw new StorageException("Given ChangeUpdate is only allowed on initial commit");
}
- ObjectId next = u.apply(rw, tempIns, curr);
+ ObjectId next = update.apply(rw, tempIns, curr);
if (next == null) {
continue;
}
if (maxUpdates.isPresent()
&& !Objects.equals(next, curr)
&& ++updateCount > maxUpdates.get()
- && !u.bypassMaxUpdates()) {
- throw new TooManyUpdatesException(u.getId(), maxUpdates.get());
+ && !update.bypassMaxUpdates()) {
+ throw new LimitExceededException(
+ String.format(
+ "Change %s may not exceed %d updates. It may still be abandoned or submitted. To"
+ + " continue working on this change, recreate it with a new Change-Id, then"
+ + " abandon this one.",
+ update.getId(), maxUpdates.get()));
}
curr = next;
}
diff --git a/java/com/google/gerrit/server/notedb/TooManyUpdatesException.java b/java/com/google/gerrit/server/notedb/TooManyUpdatesException.java
deleted file mode 100644
index 9c6faaf..0000000
--- a/java/com/google/gerrit/server/notedb/TooManyUpdatesException.java
+++ /dev/null
@@ -1,41 +0,0 @@
-// Copyright (C) 2019 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.notedb;
-
-import com.google.common.annotations.VisibleForTesting;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.exceptions.StorageException;
-
-/**
- * Exception indicating that the change has received too many updates. Further actions apart from
- * {@code abandon} or {@code submit} are blocked.
- */
-public class TooManyUpdatesException extends StorageException {
- @VisibleForTesting
- public static String message(Change.Id id, int maxUpdates) {
- return "Change "
- + id
- + " may not exceed "
- + maxUpdates
- + " updates. It may still be abandoned or submitted. To continue working on this "
- + "change, recreate it with a new Change-Id, then abandon this one.";
- }
-
- private static final long serialVersionUID = 1L;
-
- TooManyUpdatesException(Change.Id id, int maxUpdates) {
- super(message(id, maxUpdates));
- }
-}
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index e240f6a..1e86d52 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -52,8 +52,8 @@
import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.LimitExceededException;
import com.google.gerrit.server.notedb.NoteDbUpdateManager;
-import com.google.gerrit.server.notedb.TooManyUpdatesException;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException;
@@ -181,7 +181,7 @@
private static void wrapAndThrowException(Exception e) throws UpdateException, RestApiException {
// Convert common non-REST exception types with user-visible messages to corresponding REST
// exception types.
- if (e instanceof InvalidChangeOperationException || e instanceof TooManyUpdatesException) {
+ if (e instanceof InvalidChangeOperationException || e instanceof LimitExceededException) {
throw new ResourceConflictException(e.getMessage(), e);
} else if (e instanceof NoSuchChangeException
|| e instanceof NoSuchRefException
diff --git a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
index a2f800a..5860c48 100644
--- a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
+++ b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
@@ -35,7 +35,6 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.Sequences;
-import com.google.gerrit.server.notedb.TooManyUpdatesException;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gerrit.testing.InMemoryTestEnvironment;
import com.google.inject.Inject;
@@ -51,6 +50,7 @@
public class BatchUpdateTest {
private static final int MAX_UPDATES = 4;
+ private static final int MAX_PATCH_SETS = 3;
@Rule
public InMemoryTestEnvironment testEnvironment =
@@ -58,6 +58,7 @@
() -> {
Config cfg = new Config();
cfg.setInt("change", null, "maxUpdates", MAX_UPDATES);
+ cfg.setInt("change", null, "maxPatchSets", MAX_PATCH_SETS);
return cfg;
});
@@ -106,11 +107,10 @@
ObjectId oldMetaId = getMetaId(id);
try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.nowTs())) {
bu.addOp(id, new AddMessageOp("Excessive update"));
- ResourceConflictException thrown =
- assertThrows(ResourceConflictException.class, () -> bu.execute());
+ ResourceConflictException thrown = assertThrows(ResourceConflictException.class, bu::execute);
assertThat(thrown)
.hasMessageThat()
- .isEqualTo(TooManyUpdatesException.message(id, MAX_UPDATES));
+ .contains("Change " + id + " may not exceed " + MAX_UPDATES);
}
assertThat(getUpdateCount(id)).isEqualTo(MAX_UPDATES);
assertThat(getMetaId(id)).isEqualTo(oldMetaId);
@@ -118,17 +118,16 @@
@Test
public void cannotExceedMaxUpdatesCountingMultipleChangeUpdatesInSingleBatch() throws Exception {
- Change.Id id = createChangeWithTwoPatchSets(MAX_UPDATES - 1);
+ Change.Id id = createChangeWithPatchSets(2);
ObjectId oldMetaId = getMetaId(id);
try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.nowTs())) {
bu.addOp(id, new AddMessageOp("Update on PS1", PatchSet.id(id, 1)));
bu.addOp(id, new AddMessageOp("Update on PS2", PatchSet.id(id, 2)));
- ResourceConflictException thrown =
- assertThrows(ResourceConflictException.class, () -> bu.execute());
+ ResourceConflictException thrown = assertThrows(ResourceConflictException.class, bu::execute);
assertThat(thrown)
.hasMessageThat()
- .isEqualTo(TooManyUpdatesException.message(id, MAX_UPDATES));
+ .contains("Change " + id + " may not exceed " + MAX_UPDATES);
}
assertThat(getUpdateCount(id)).isEqualTo(MAX_UPDATES - 1);
assertThat(getMetaId(id)).isEqualTo(oldMetaId);
@@ -187,7 +186,7 @@
@Test
public void exceedingMaxUpdatesAllowedWithSubmitAfterOtherOp() throws Exception {
- Change.Id id = createChangeWithTwoPatchSets(MAX_UPDATES - 1);
+ Change.Id id = createChangeWithPatchSets(2);
ObjectId oldMetaId = getMetaId(id);
try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.nowTs())) {
bu.addOp(id, new AddMessageOp("Message on PS1", PatchSet.id(id, 1)));
@@ -222,6 +221,28 @@
assertThat(getMetaId(id)).isNotEqualTo(oldMetaId);
}
+ @Test
+ public void limitPatchSetCount_exceed() throws Exception {
+ Change.Id changeId = createChangeWithPatchSets(MAX_PATCH_SETS);
+ ObjectId oldMetaId = getMetaId(changeId);
+ ChangeNotes notes = changeNotesFactory.create(project, changeId);
+ try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.nowTs())) {
+ ObjectId commitId =
+ repo.amend(notes.getCurrentPatchSet().commitId()).message("kaboom").create();
+ bu.addOp(
+ changeId,
+ patchSetInserterFactory
+ .create(notes, PatchSet.id(changeId, MAX_PATCH_SETS + 1), commitId)
+ .setMessage("kaboom"));
+ ResourceConflictException thrown = assertThrows(ResourceConflictException.class, bu::execute);
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains("Change " + changeId + " may not exceed " + MAX_PATCH_SETS + " patch sets");
+ }
+ assertThat(changeNotesFactory.create(project, changeId).getPatchSets()).hasSize(MAX_PATCH_SETS);
+ assertThat(getMetaId(changeId)).isEqualTo(oldMetaId);
+ }
+
private Change.Id createChangeWithUpdates(int totalUpdates) throws Exception {
checkArgument(totalUpdates > 0);
checkArgument(totalUpdates <= MAX_UPDATES);
@@ -243,21 +264,22 @@
return id;
}
- private Change.Id createChangeWithTwoPatchSets(int totalUpdates) throws Exception {
- Change.Id id = createChangeWithUpdates(totalUpdates - 1);
+ private Change.Id createChangeWithPatchSets(int patchSets) throws Exception {
+ checkArgument(patchSets >= 2);
+ Change.Id id = createChangeWithUpdates(MAX_UPDATES - 2);
ChangeNotes notes = changeNotesFactory.create(project, id);
-
- try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.nowTs())) {
- ObjectId commitId = repo.amend(notes.getCurrentPatchSet().commitId()).message("PS2").create();
- bu.addOp(
- id,
- patchSetInserterFactory
- .create(notes, PatchSet.id(id, 2), commitId)
- .setMessage("Add PS2"));
- bu.execute();
+ for (int i = 2; i <= patchSets; ++i) {
+ try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.nowTs())) {
+ ObjectId commitId =
+ repo.amend(notes.getCurrentPatchSet().commitId()).message("PS" + i).create();
+ bu.addOp(
+ id,
+ patchSetInserterFactory
+ .create(notes, PatchSet.id(id, i), commitId)
+ .setMessage("Add PS" + i));
+ bu.execute();
+ }
}
-
- assertThat(getUpdateCount(id)).isEqualTo(totalUpdates);
return id;
}
@@ -291,7 +313,7 @@
}
}
- private int getUpdateCount(Change.Id changeId) throws Exception {
+ private int getUpdateCount(Change.Id changeId) {
return changeNotesFactory.create(project, changeId).getUpdateCount();
}
