IndexConfig: Add maxPages option With an index system like Lucene, which only supports "pagination" by skipping results, a search with a start offset actually has to iterate through the skipped results. This means that paginating through a long list of results is a quadratic operation. Allow administrators to limit the amount of work done by such pagination requests, by setting a limit on the number of pages of results. Larger result sets may thus require larger limits (possibly requiring extra permission) to paginate through. Change-Id: Ie62f506e3b479992358f70a3c6d685749f43f0da
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 797178b..8119d4a 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt
@@ -2213,6 +2213,16 @@ + Defaults to no limit. +[[index.maxPages]]index.maxPages:: ++ +Maximum number of pages of search results to allow, as index +implementations may have to scan through large numbers of skipped +results when searching with an offset. Requesting results starting past +this threshold times the requested limit will result in an error. Set to +0 for no limit. ++ +Defaults to no limit. + ==== Lucene configuration Open and closed changes are indexed in separate indexes named
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexConfig.java index 5a22bc7..08f4748 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexConfig.java
@@ -30,21 +30,30 @@ @AutoValue public abstract class IndexConfig { public static IndexConfig createDefault() { - return create(0); + return create(0, 0); } public static IndexConfig fromConfig(Config cfg) { - return create(cfg.getInt("index", null, "maxLimit", 0)); + return create( + cfg.getInt("index", null, "maxLimit", 0), + cfg.getInt("index", null, "maxPages", 0)); } - public static IndexConfig create(int maxLimit) { - if (maxLimit == 0) { - maxLimit = Integer.MAX_VALUE; - } else { - checkArgument(maxLimit > 0, "maxLimit must be positive: %s", maxLimit); + public static IndexConfig create(int maxLimit, int maxPages) { + return new AutoValue_IndexConfig( + checkLimit(maxLimit, "maxLimit"), + checkLimit(maxPages, "maxPages")); + } + + private static int checkLimit(int limit, String name) { + if (limit == 0) { + return Integer.MAX_VALUE; } - return new AutoValue_IndexConfig(maxLimit); + checkArgument(limit > 0, "%s must be positive: %s", name, limit); + return limit; } public abstract int maxLimit(); + + public abstract int maxPages(); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java index 51d971d..6068dd0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java
@@ -132,6 +132,13 @@ if (limit == getBackendSupportedLimit()) { limit--; } + + int page = (start / limit) + 1; + if (page > indexConfig.maxPages()) { + throw new QueryParseException( + "Cannot go beyond page " + indexConfig.maxPages() + "of results"); + } + Predicate<ChangeData> s = queryRewriter.rewrite(q, start, limit + 1); if (!(s instanceof ChangeDataSource)) { q = Predicate.and(open(), q);
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 9ad896e..85e06e8 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -84,9 +84,19 @@ @Ignore @RunWith(ConfigSuite.class) public abstract class AbstractQueryChangesTest { + @ConfigSuite.Default + public static Config defaultConfig() { + return updateConfig(new Config()); + } + @ConfigSuite.Config public static Config noteDbEnabled() { - return NotesMigration.allEnabledConfig(); + return updateConfig(NotesMigration.allEnabledConfig()); + } + + private static Config updateConfig(Config cfg) { + cfg.setInt("index", null, "maxPages", 10); + return cfg; } @ConfigSuite.Parameter public Config config; @@ -557,6 +567,19 @@ } @Test + public void maxPages() throws Exception { + TestRepository<InMemoryRepository> repo = createProject("repo"); + Change change = newChange(repo, null, null, null, null).insert(); + + QueryRequest query = newQuery("status:new").withLimit(10); + assertQuery(query, change); + assertQuery(query.withStart(1)); + assertQuery(query.withStart(99)); + assertBadQuery(query.withStart(100)); + assertQuery(query.withLimit(100).withStart(100)); + } + + @Test public void updateOrder() throws Exception { clockStepMs = MILLISECONDS.convert(2, MINUTES); TestRepository<InMemoryRepository> repo = createProject("repo"); @@ -1073,8 +1096,12 @@ } protected void assertBadQuery(Object query) throws Exception { + assertBadQuery(newQuery(query)); + } + + protected void assertBadQuery(QueryRequest query) throws Exception { try { - newQuery(query).get(); + query.get(); fail("expected BadRequestException for query: " + query); } catch (BadRequestException e) { // Expected.