Add a default index query limit
Users and scripts tend to provide no limit with their index queries.
Normally this is not a problem, because users often don't get granted
any extra query limit permissions. So falling back to the permitted
limit of say 500 is totally fine.
However, if users or bots are granted a higher permitted limit, then
this is also used as a default/fallback. With this change we are
introducing a new index config option to specify a `defaultLimit`,
which is applied, if the user does not provide a limit. That does not
affect what is *permitted*!
Release-Notes: skip
Google-Bug-Id: b/289166403
Change-Id: Iad3d4bd452ec6f885aeb505a62f32d17e7aa08b2
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 4d8965a..1ab2c44 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -3397,6 +3397,15 @@
+
Defaults to `OFFSET`.
+[[index.defaultLimit]]index.defaultLimit::
++
+Default limit, if the user does not provide a limit. If this is not set or set
+to 0, then index queries are executed with the maximum permitted limit for the
+user, which may be really high and cause too much load on the index. Thus
+setting this default limit to something smaller like 100 allows you to control
+the load, while not taking away any permission from the user. If the user
+provides a limit themselves, then `defaultLimit` is ignored.
+
[[index.maxLimit]]index.maxLimit::
+
Maximum limit to allow for search queries. Requesting results above this
diff --git a/java/com/google/gerrit/index/IndexConfig.java b/java/com/google/gerrit/index/IndexConfig.java
index c21f32e..2141bf2 100644
--- a/java/com/google/gerrit/index/IndexConfig.java
+++ b/java/com/google/gerrit/index/IndexConfig.java
@@ -38,6 +38,7 @@
public static Builder fromConfig(Config cfg) {
Builder b = builder();
+ setIfPresent(cfg, "defaultLimit", b::defaultLimit);
setIfPresent(cfg, "maxLimit", b::maxLimit);
setIfPresent(cfg, "maxPages", b::maxPages);
setIfPresent(cfg, "maxTerms", b::maxTerms);
@@ -67,6 +68,7 @@
public static Builder builder() {
return new AutoValue_IndexConfig.Builder()
+ .defaultLimit(Integer.MAX_VALUE)
.maxLimit(Integer.MAX_VALUE)
.maxPages(Integer.MAX_VALUE)
.maxTerms(DEFAULT_MAX_TERMS)
@@ -79,6 +81,10 @@
@AutoValue.Builder
public abstract static class Builder {
+ public abstract Builder defaultLimit(int defaultLimit);
+
+ public abstract int defaultLimit();
+
public abstract Builder maxLimit(int maxLimit);
public abstract int maxLimit();
@@ -107,6 +113,7 @@
public IndexConfig build() {
IndexConfig cfg = autoBuild();
+ checkLimit(cfg.defaultLimit(), "defaultLimit");
checkLimit(cfg.maxLimit(), "maxLimit");
checkLimit(cfg.maxPages(), "maxPages");
checkLimit(cfg.maxTerms(), "maxTerms");
@@ -121,6 +128,12 @@
}
/**
+ * Returns default limit for index queries, if the user does not provide one. If this is not set,
+ * then the max permitted limit for each user is used, which might be much higher than intended.
+ */
+ public abstract int defaultLimit();
+
+ /**
* Returns maximum limit supported by the underlying index, or limited for performance reasons.
*/
public abstract int maxLimit();
diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java
index 5440766..f61a45d 100644
--- a/java/com/google/gerrit/index/query/QueryProcessor.java
+++ b/java/com/google/gerrit/index/query/QueryProcessor.java
@@ -415,6 +415,8 @@
possibleLimits.add(getPermittedLimit());
if (userProvidedLimit > 0) {
possibleLimits.add(userProvidedLimit);
+ } else if (indexConfig.defaultLimit() > 0) {
+ possibleLimits.add(indexConfig.defaultLimit());
}
if (limitField != null) {
Integer limitFromPredicate = LimitPredicate.getLimit(limitField, p);
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 7fe7635..00861f499 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -2643,11 +2643,25 @@
@Test
public void queryChangesLimit() throws Exception {
- createChange();
- PushOneCommit.Result r2 = createChange();
- List<ChangeInfo> results = gApi.changes().query().withLimit(1).get();
- assertThat(results).hasSize(1);
- assertThat(Iterables.getOnlyElement(results).changeId).isEqualTo(r2.getChangeId());
+ for (int i = 0; i < 3; i++) {
+ createChange();
+ }
+ List<ChangeInfo> resultsLimited = gApi.changes().query().withLimit(1).get();
+ List<ChangeInfo> resultsUnlimited = gApi.changes().query().get();
+ assertThat(resultsLimited).hasSize(1);
+ assertThat(resultsUnlimited.size()).isAtLeast(3);
+ }
+
+ @Test
+ @GerritConfig(name = "index.defaultLimit", value = "2")
+ public void queryChangesLimitDefault() throws Exception {
+ for (int i = 0; i < 3; i++) {
+ createChange();
+ }
+ List<ChangeInfo> resultsLimited = gApi.changes().query().withLimit(1).get();
+ List<ChangeInfo> resultsUnlimited = gApi.changes().query().get();
+ assertThat(resultsLimited).hasSize(1);
+ assertThat(resultsUnlimited).hasSize(2);
}
@Test
