diff options
author | Diego Zambelli Sessona <diego.sessona@gmail.com> | 2023-07-10 14:22:29 +0100 |
---|---|---|
committer | Diego Zambelli Sessona <diego.sessona@gmail.com> | 2023-07-10 14:22:29 +0100 |
commit | a0da72341b8159c5341af711e6b8991ef1aa7875 (patch) | |
tree | ccaa2aacdbbb44153960809bd1ca8d6e68b5e71d | |
parent | 3bc24131ee6f62c89509f318665073715457dc05 (diff) |
Add configuration to disable indexes pagination
Add NONE option to disable index backend pagination,
allowing the user deciding between performance
and data consistency.
This option needs to be honoured by the indexing backend
and this change introduces the correct implementation
for Lucene.
We should also create the corresponding change on the
elasticsearch libModule for throwing an IAE to highlight
that the backend doesn't support NONE pagination type.
API pagination is not affected by this change and is
always preserved.
When having pagination, we could encounter some problems
in the result set, when moving from one page to the
next of the index results:
* duplicated entries (when new changes are created)
* missing entries (when changes are removed or set to
private)
* wrong entries status (when changes are switching
their state during the page switching)
Having duplicates or missing results could have side effects,
depending on the use-case of the consumer of the data.
For example, since indexes are used to populate some LoadingCache
(SearchingChangeCache [1]), inconsistent results returned by
the indexing backend may cause the cache loading function
to fail or keep an inconsistent state which doesn't reflect
the underlying data.
For this case, a workaround [2] has been put in place in master
and 3.8, however, it will not work for the missing
or inconsistent entries.
[1]: https://gerrit.googlesource.com/gerrit/+/refs/heads/stable-3.6/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java#157
[2]: https://gerrit-review.googlesource.com/c/gerrit/+/349995
Bug: Issue 287484350
Release-Notes: Introduce NONE pagination type and its support in Lucene indexing backend
Change-Id: I40f87895d9dac951ae30c5562b2ddbf28b34a41a
Co-Authored-With: Fabio Ponciroli <ponch78@gmail.com>
16 files changed, 324 insertions, 22 deletions
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index a6c7da6778..682239f35d 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -3253,6 +3253,8 @@ obtain the next set of results. Supported values are: + Index queries are repeated with a non-zero offset to obtain the next set of results. +_Note: Results may be inaccurate if the data-set is changing during the query +execution._ + * `SEARCH_AFTER` + @@ -3260,6 +3262,18 @@ Index queries are repeated using a search-after object. Index backends can provide their custom implementations for search-after. Note that, `SEARCH_AFTER` does not impact using offsets in Gerrit query APIs. +_Note: Depending on the index backend and its settings, results may be +inaccurate if the data-set is changing during the query execution._ ++ +* `NONE` ++ +Index queries are executed returning all results, without internal +pagination. +_Note: Since the entire set of indexing results is kept in memory +during the API execution, this option may lead to higher memory utilisation +and overall reduced performance. +Bear in mind that some indexing backends may not support unbounded queries; +therefore, the NONE option is unavailable._ + Defaults to `OFFSET`. @@ -3305,6 +3319,8 @@ likely help reduce the impacts of this. For example, if the limit of the previous query was 500 and pageSizeMultiplier is configured to 5, the next query will have a limit of 2500. + +_Note: ignored when paginationType is `NONE`_ ++ Defaults to 1 which effectively turns this feature off. [[index.maxPageSize]]index.maxPageSize:: @@ -3317,6 +3333,8 @@ For example, if the limit of previous query was 500, pageSizeMultiplier is configured to 5 and maxPageSize to 2000, the next query will have a limit of 2000 (instead of 2500). + +_Note: ignored when paginationType is `NONE`_ ++ Defaults to no limit. [[index.maxTerms]]index.maxTerms:: diff --git a/java/com/google/gerrit/index/PaginationType.java b/java/com/google/gerrit/index/PaginationType.java index e7e34fdc0a..f18f2899a5 100644 --- a/java/com/google/gerrit/index/PaginationType.java +++ b/java/com/google/gerrit/index/PaginationType.java @@ -25,5 +25,8 @@ public enum PaginationType { * <p>For example, Lucene implementation uses the last doc from the previous search as * search-after object and uses the IndexSearcher.searchAfter API to get the next set of results. */ - SEARCH_AFTER + SEARCH_AFTER, + + /** Index queries are executed returning all results, without internal pagination. */ + NONE } diff --git a/java/com/google/gerrit/index/query/PaginatingSource.java b/java/com/google/gerrit/index/query/PaginatingSource.java index b05c8f4846..03d749ac14 100644 --- a/java/com/google/gerrit/index/query/PaginatingSource.java +++ b/java/com/google/gerrit/index/query/PaginatingSource.java @@ -63,7 +63,13 @@ public class PaginatingSource<T> implements DataSource<T> { pageResultSize++; } - if (last != null && source instanceof Paginated) { + if (last != null + && source instanceof Paginated + // TODO: this fix is only for the stable branches and the real refactoring would be to + // restore the logic + // for the filtering in AndSource (L58 - 64) as per + // https://gerrit-review.googlesource.com/c/gerrit/+/345634/9 + && !indexConfig.paginationType().equals(PaginationType.NONE)) { // Restart source and continue if we have not filled the // full limit the caller wants. // diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java index 21d4c2e7f6..b83c1065bd 100644 --- a/java/com/google/gerrit/index/query/QueryProcessor.java +++ b/java/com/google/gerrit/index/query/QueryProcessor.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Ordering; import com.google.common.flogger.FluentLogger; +import com.google.common.primitives.Ints; import com.google.gerrit.common.Nullable; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.index.Index; @@ -265,7 +266,11 @@ public abstract class QueryProcessor<T> { start, initialPageSize, pageSizeMultiplier, - limit, + // Always bump limit by 1, even if this results in exceeding the permitted + // max for this user. The only way to see if there are more entities is to + // ask for one more result from the query. + // NOTE: This is consistent to the behaviour before the introduction of pagination.` + Ints.saturatedCast((long) limit + 1), getRequestedFields()); logger.atFine().log("Query options: %s", opts); Predicate<T> pred = rewriter.rewrite(q, opts); diff --git a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java index 4241828b8a..8ecdae86e6 100644 --- a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java +++ b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java @@ -16,6 +16,7 @@ package com.google.gerrit.index.testing; import static com.google.common.collect.ImmutableList.toImmutableList; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; @@ -49,6 +50,7 @@ import com.google.gerrit.server.query.change.ChangeData; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; import java.time.Instant; +import java.util.ArrayList; import java.util.Comparator; import java.util.HashMap; import java.util.List; @@ -74,6 +76,7 @@ public abstract class AbstractFakeIndex<K, V, D> implements Index<K, V> { private final String indexName; private final Map<K, D> indexedDocuments; private int queryCount; + private List<Integer> resultsSizes; AbstractFakeIndex(Schema<V> schema, SitePaths sitePaths, String indexName) { this.schema = schema; @@ -81,6 +84,7 @@ public abstract class AbstractFakeIndex<K, V, D> implements Index<K, V> { this.indexName = indexName; this.indexedDocuments = new HashMap<>(); this.queryCount = 0; + this.resultsSizes = new ArrayList<Integer>(); } @Override @@ -118,6 +122,16 @@ public abstract class AbstractFakeIndex<K, V, D> implements Index<K, V> { return queryCount; } + @VisibleForTesting + public void resetQueryCount() { + queryCount = 0; + } + + @VisibleForTesting + public List<Integer> getResultsSizes() { + return resultsSizes; + } + @Override public DataSource<V> getSource(Predicate<V> p, QueryOptions opts) { List<V> results; @@ -141,6 +155,7 @@ public abstract class AbstractFakeIndex<K, V, D> implements Index<K, V> { results = valueStream.skip(opts.start()).limit(opts.pageSize()).collect(toImmutableList()); } queryCount++; + resultsSizes.add(results.size()); } return new DataSource<>() { @Override diff --git a/java/com/google/gerrit/lucene/AbstractLuceneIndex.java b/java/com/google/gerrit/lucene/AbstractLuceneIndex.java index 586887b1e6..ca81966fea 100644 --- a/java/com/google/gerrit/lucene/AbstractLuceneIndex.java +++ b/java/com/google/gerrit/lucene/AbstractLuceneIndex.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; import com.google.common.collect.Sets; import com.google.common.flogger.FluentLogger; +import com.google.common.primitives.Ints; import com.google.common.util.concurrent.AbstractFuture; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -36,6 +37,7 @@ import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.index.FieldDef; import com.google.gerrit.index.FieldType; import com.google.gerrit.index.Index; +import com.google.gerrit.index.PaginationType; import com.google.gerrit.index.QueryOptions; import com.google.gerrit.index.Schema; import com.google.gerrit.index.Schema.Values; @@ -420,6 +422,10 @@ public abstract class AbstractLuceneIndex<K, V> implements Index<K, V> { return f.isStored() ? Field.Store.YES : Field.Store.NO; } + static int getLimitBasedOnPaginationType(QueryOptions opts, int pagesize) { + return PaginationType.NONE == opts.config().paginationType() ? opts.limit() : pagesize; + } + private final class NrtFuture extends AbstractFuture<Void> { private final long gen; @@ -539,7 +545,9 @@ public abstract class AbstractLuceneIndex<K, V> implements Index<K, V> { ScoreDoc scoreDoc = null; try { searcher = acquire(); - int realLimit = opts.start() + opts.pageSize(); + int realLimit = + Ints.saturatedCast( + (long) getLimitBasedOnPaginationType(opts, opts.pageSize()) + opts.start()); TopFieldDocs docs = opts.searchAfter() != null ? searcher.searchAfter( diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java index daa921ccde..f6f1eda59a 100644 --- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -430,6 +430,7 @@ public class LuceneChangeIndex implements ChangeIndex { if (Integer.MAX_VALUE - opts.pageSize() < opts.start()) { realPageSize = Integer.MAX_VALUE; } + int queryLimit = AbstractLuceneIndex.getLimitBasedOnPaginationType(opts, realPageSize); List<TopFieldDocs> hits = new ArrayList<>(); int searchAfterHitsCount = 0; for (int i = 0; i < indexes.size(); i++) { @@ -453,11 +454,10 @@ public class LuceneChangeIndex implements ChangeIndex { subIndex, Iterables.getLast(Arrays.asList(subIndexHits.scoreDocs), searchAfter)); } } else { - hits.add(searchers[i].search(query, realPageSize, sort)); + hits.add(searchers[i].search(query, queryLimit, sort)); } } - TopDocs docs = - TopDocs.merge(sort, realPageSize, hits.stream().toArray(TopFieldDocs[]::new)); + TopDocs docs = TopDocs.merge(sort, queryLimit, hits.stream().toArray(TopFieldDocs[]::new)); List<Document> result = new ArrayList<>(docs.scoreDocs.length); for (int i = opts.start(); i < docs.scoreDocs.length; i++) { diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 081fe02758..a820f86c2b 100644 --- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -95,6 +95,7 @@ import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.httpd.raw.IndexPreloadingUtil; import com.google.gerrit.index.FieldDef; import com.google.gerrit.index.IndexConfig; +import com.google.gerrit.index.PaginationType; import com.google.gerrit.index.Schema; import com.google.gerrit.index.query.IndexPredicate; import com.google.gerrit.index.query.Predicate; @@ -118,6 +119,7 @@ import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.ChangeTriplet; import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.change.PatchSetInserter; +import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.experiments.ExperimentFeaturesConstants; import com.google.gerrit.server.git.meta.MetaDataUpdate; @@ -179,6 +181,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { @Inject protected AccountManager accountManager; @Inject protected AllUsersName allUsersName; @Inject protected BatchUpdate.Factory updateFactory; + @Inject protected AllProjectsName allProjectsName; @Inject protected ChangeInserter.Factory changeFactory; @Inject protected Provider<ChangeQueryBuilder> queryBuilderProvider; @Inject protected GerritApi gApi; @@ -4635,4 +4638,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { protected Schema<ChangeData> getSchema() { return indexes.getSearchIndex().getSchema(); } + + PaginationType getCurrentPaginationType() { + return config.getEnum("index", null, "paginationType", PaginationType.OFFSET); + } } diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java index 4dde452dc9..df6ff5a15e 100644 --- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java +++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java @@ -33,4 +33,11 @@ public class FakeQueryChangesLatestIndexVersionTest extends FakeQueryChangesTest config.setString("index", null, "paginationType", "SEARCH_AFTER"); return config; } + + @ConfigSuite.Config + public static Config nonePaginationType() { + Config config = defaultConfig(); + config.setString("index", null, "paginationType", "NONE"); + return config; + } } diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java index 95896dc991..805c99abdb 100644 --- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java +++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java @@ -43,4 +43,11 @@ public class FakeQueryChangesPreviousIndexVersionTest extends FakeQueryChangesTe config.setString("index", null, "paginationType", "SEARCH_AFTER"); return config; } + + @ConfigSuite.Config + public static Config nonePaginationType() { + Config config = againstPreviousIndexVersion(); + config.setString("index", null, "paginationType", "NONE"); + return config; + } } diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java index 3968a33707..d20a00465c 100644 --- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java @@ -15,16 +15,24 @@ package com.google.gerrit.server.query.change; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block; import static com.google.gerrit.common.data.GlobalCapability.QUERY_LIMIT; +import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; +import static org.junit.Assume.assumeFalse; +import static org.junit.Assume.assumeTrue; import com.google.gerrit.acceptance.UseClockStep; import com.google.gerrit.entities.Permission; import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.index.PaginationType; +import com.google.gerrit.index.query.QueryParseException; import com.google.gerrit.index.testing.AbstractFakeIndex; import com.google.gerrit.server.config.AllProjectsName; +import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.index.change.ChangeIndexCollection; import com.google.gerrit.testing.InMemoryModule; import com.google.gerrit.testing.InMemoryRepositoryManager; @@ -32,6 +40,7 @@ import com.google.gerrit.testing.InMemoryRepositoryManager.Repo; import com.google.inject.Guice; import com.google.inject.Inject; import com.google.inject.Injector; +import java.util.List; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; import org.junit.Test; @@ -74,62 +83,167 @@ public abstract class FakeQueryChangesTest extends AbstractQueryChangesTest { AbstractFakeIndex idx = (AbstractFakeIndex) changeIndexCollection.getSearchIndex(); newQuery("status:new").withLimit(5).get(); - // Since the limit of the query (i.e. 5) is more than the total number of changes (i.e. 4), - // only 1 index search is expected. - assertThat(idx.getQueryCount()).isEqualTo(1); + assertThatSearchQueryWasNotPaginated(idx.getQueryCount()); } @Test @UseClockStep @SuppressWarnings("unchecked") public void noLimitQueryPaginates() throws Exception { + assumeFalse(PaginationType.NONE == getCurrentPaginationType()); + TestRepository<InMemoryRepositoryManager.Repo> testRepo = createProject("repo"); // create 4 changes insert(testRepo, newChange(testRepo)); insert(testRepo, newChange(testRepo)); insert(testRepo, newChange(testRepo)); insert(testRepo, newChange(testRepo)); - // Set queryLimit to 2 projectOperations .project(allProjects) .forUpdate() .add(allowCapability(QUERY_LIMIT).group(REGISTERED_USERS).range(0, 2)) .update(); - AbstractFakeIndex idx = (AbstractFakeIndex) changeIndexCollection.getSearchIndex(); - // 2 index searches are expected. The first index search will run with size 3 (i.e. // the configured query-limit+1), and then we will paginate to get the remaining // changes with the second index search. newQuery("status:new").withNoLimit().get(); - assertThat(idx.getQueryCount()).isEqualTo(2); + assertThatSearchQueryWasPaginated(idx.getQueryCount(), 2); + } + + @Test + @UseClockStep + public void noLimitQueryDoesNotPaginatesWithNonePaginationType() throws Exception { + assumeTrue(PaginationType.NONE == getCurrentPaginationType()); + AbstractFakeIndex idx = setupRepoWithFourChanges(); + newQuery("status:new").withNoLimit().get(); + assertThatSearchQueryWasNotPaginated(idx.getQueryCount()); + } + + @Test + @UseClockStep + public void invisibleChangesNotPaginatedWithNonePaginationType() throws Exception { + assumeTrue(PaginationType.NONE == getCurrentPaginationType()); + AbstractFakeIndex idx = setupRepoWithFourChanges(); + final int LIMIT = 3; + + projectOperations + .project(allProjectsName) + .forUpdate() + .removeAllAccessSections() + .add(allow(Permission.READ).ref("refs/*").group(SystemGroupBackend.REGISTERED_USERS)) + .update(); + + // Set queryLimit to 3 + projectOperations + .project(allProjects) + .forUpdate() + .add(allowCapability(QUERY_LIMIT).group(ANONYMOUS_USERS).range(0, LIMIT)) + .update(); + + requestContext.setContext(anonymousUserProvider::get); + List<ChangeInfo> result = newQuery("status:new").withLimit(LIMIT).get(); + assertThat(result.size()).isEqualTo(0); + assertThatSearchQueryWasNotPaginated(idx.getQueryCount()); + assertThat(idx.getResultsSizes().get(0)).isEqualTo(LIMIT + 1); + } + + @Test + @UseClockStep + public void invisibleChangesPaginatedWithPagination() throws Exception { + assumeFalse(PaginationType.NONE == getCurrentPaginationType()); + + AbstractFakeIndex idx = setupRepoWithFourChanges(); + final int LIMIT = 3; + + projectOperations + .project(allProjectsName) + .forUpdate() + .removeAllAccessSections() + .add(allow(Permission.READ).ref("refs/*").group(SystemGroupBackend.REGISTERED_USERS)) + .update(); + + projectOperations + .project(allProjects) + .forUpdate() + .add(allowCapability(QUERY_LIMIT).group(ANONYMOUS_USERS).range(0, LIMIT)) + .update(); + + requestContext.setContext(anonymousUserProvider::get); + List<ChangeInfo> result = newQuery("status:new").withLimit(LIMIT).get(); + assertThat(result.size()).isEqualTo(0); + assertThatSearchQueryWasPaginated(idx.getQueryCount(), 2); + assertThat(idx.getResultsSizes().get(0)).isEqualTo(LIMIT + 1); + assertThat(idx.getResultsSizes().get(1)).isEqualTo(0); // Second query size } @Test @UseClockStep @SuppressWarnings("unchecked") public void internalQueriesPaginate() throws Exception { + assumeFalse(PaginationType.NONE == getCurrentPaginationType()); + final int LIMIT = 2; + + TestRepository<Repo> testRepo = createProject("repo"); // create 4 changes - TestRepository<InMemoryRepositoryManager.Repo> testRepo = createProject("repo"); insert(testRepo, newChange(testRepo)); insert(testRepo, newChange(testRepo)); insert(testRepo, newChange(testRepo)); insert(testRepo, newChange(testRepo)); - // Set queryLimit to 2 projectOperations .project(allProjects) .forUpdate() - .add(allowCapability(QUERY_LIMIT).group(REGISTERED_USERS).range(0, 2)) + .add(allowCapability(QUERY_LIMIT).group(REGISTERED_USERS).range(0, LIMIT)) .update(); - AbstractFakeIndex idx = (AbstractFakeIndex) changeIndexCollection.getSearchIndex(); - // 2 index searches are expected. The first index search will run with size 3 (i.e. // the configured query-limit+1), and then we will paginate to get the remaining // changes with the second index search. queryProvider.get().query(queryBuilderProvider.get().parse("status:new")); - assertThat(idx.getQueryCount()).isEqualTo(2); + assertThat(idx.getQueryCount()).isEqualTo(LIMIT); + } + + @Test + @UseClockStep + @SuppressWarnings("unchecked") + public void internalQueriesDoNotPaginateWithNonePaginationType() throws Exception { + assumeTrue(PaginationType.NONE == getCurrentPaginationType()); + + AbstractFakeIndex idx = setupRepoWithFourChanges(); + // 1 index search is expected since we are not paginating. + executeQuery("status:new"); + assertThatSearchQueryWasNotPaginated(idx.getQueryCount()); + } + + private void executeQuery(String query) throws QueryParseException { + queryProvider.get().query(queryBuilderProvider.get().parse(query)); + } + + private void assertThatSearchQueryWasNotPaginated(int queryCount) { + assertThat(queryCount).isEqualTo(1); + } + + private void assertThatSearchQueryWasPaginated(int queryCount, int expectedPages) { + assertThat(queryCount).isEqualTo(expectedPages); + } + + private AbstractFakeIndex setupRepoWithFourChanges() throws Exception { + TestRepository<Repo> testRepo = createProject("repo"); + // create 4 changes + insert(testRepo, newChange(testRepo)); + insert(testRepo, newChange(testRepo)); + insert(testRepo, newChange(testRepo)); + insert(testRepo, newChange(testRepo)); + + // Set queryLimit to 2 + projectOperations + .project(allProjects) + .forUpdate() + .add(allowCapability(QUERY_LIMIT).group(REGISTERED_USERS).range(0, 2)) + .update(); + + return (AbstractFakeIndex) changeIndexCollection.getSearchIndex(); } } diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java index 45879433e6..bfd1bd6baa 100644 --- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java +++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java @@ -30,4 +30,11 @@ public class LuceneQueryChangesLatestIndexVersionTest extends LuceneQueryChanges config.setString("index", null, "paginationType", "SEARCH_AFTER"); return config; } + + @ConfigSuite.Config + public static Config nonePaginationType() { + Config config = defaultConfig(); + config.setString("index", null, "paginationType", "NONE"); + return config; + } } diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java index 178269730c..f93a2a7492 100644 --- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java +++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java @@ -40,4 +40,11 @@ public class LuceneQueryChangesPreviousIndexVersionTest extends LuceneQueryChang config.setString("index", null, "paginationType", "SEARCH_AFTER"); return config; } + + @ConfigSuite.Config + public static Config nonePaginationType() { + Config config = againstPreviousIndexVersion(); + config.setString("index", null, "paginationType", "NONE"); + return config; + } } diff --git a/javatests/com/google/gerrit/server/query/group/AbstractFakeQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/AbstractFakeQueryGroupsTest.java new file mode 100644 index 0000000000..bf224f063a --- /dev/null +++ b/javatests/com/google/gerrit/server/query/group/AbstractFakeQueryGroupsTest.java @@ -0,0 +1,80 @@ +// Copyright (C) 2023 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.query.group; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assume.assumeTrue; + +import com.google.gerrit.extensions.common.GroupInfo; +import com.google.gerrit.index.PaginationType; +import com.google.gerrit.index.testing.AbstractFakeIndex; +import com.google.gerrit.server.index.group.GroupIndexCollection; +import com.google.gerrit.testing.InMemoryModule; +import com.google.inject.Guice; +import com.google.inject.Inject; +import com.google.inject.Injector; +import java.util.ArrayList; +import java.util.List; +import org.eclipse.jgit.lib.Config; +import org.junit.Before; +import org.junit.Test; + +public abstract class AbstractFakeQueryGroupsTest extends AbstractQueryGroupsTest { + + @Inject private GroupIndexCollection groupIndexCollection; + + @Override + protected Injector createInjector() { + Config fakeConfig = new Config(config); + InMemoryModule.setDefaults(fakeConfig); + fakeConfig.setString("index", null, "type", "fake"); + return Guice.createInjector(new InMemoryModule(fakeConfig)); + } + + @Before + public void resetQueryCount() { + ((AbstractFakeIndex<?, ?, ?>) groupIndexCollection.getSearchIndex()).resetQueryCount(); + } + + @Test + public void internalQueriesDoNotPaginateWithNonePaginationType() throws Exception { + assumeTrue(PaginationType.NONE == getCurrentPaginationType()); + + final int GROUPS_CREATED_SIZE = 2; + List<GroupInfo> groupsCreated = new ArrayList<>(); + for (int i = 0; i < GROUPS_CREATED_SIZE; i++) { + groupsCreated.add(createGroupThatIsVisibleToAll(name("group-" + i))); + } + + List<GroupInfo> result = assertQuery(newQuery("is:visibletoall"), groupsCreated); + assertThat(result.size()).isEqualTo(GROUPS_CREATED_SIZE); + assertThat(result.get(result.size() - 1)._moreGroups).isNull(); + assertThatSearchQueryWasNotPaginated(); + } + + PaginationType getCurrentPaginationType() { + return config.getEnum("index", null, "paginationType", PaginationType.OFFSET); + } + + private void assertThatSearchQueryWasNotPaginated() { + assertThat(getQueryCount()).isEqualTo(1); + } + + private int getQueryCount() { + AbstractFakeIndex<?, ?, ?> idx = + (AbstractFakeIndex<?, ?, ?>) groupIndexCollection.getSearchIndex(); + return idx.getQueryCount(); + } +} diff --git a/javatests/com/google/gerrit/server/query/group/BUILD b/javatests/com/google/gerrit/server/query/group/BUILD index 0cc132d53e..0094bd6a93 100644 --- a/javatests/com/google/gerrit/server/query/group/BUILD +++ b/javatests/com/google/gerrit/server/query/group/BUILD @@ -1,7 +1,10 @@ load("@rules_java//java:defs.bzl", "java_library") load("//tools/bzl:junit.bzl", "junit_tests") -ABSTRACT_QUERY_TEST = ["AbstractQueryGroupsTest.java"] +ABSTRACT_QUERY_TEST = [ + "AbstractQueryGroupsTest.java", + "AbstractFakeQueryGroupsTest.java", +] java_library( name = "abstract_query_tests", @@ -13,6 +16,7 @@ java_library( "//java/com/google/gerrit/entities", "//java/com/google/gerrit/extensions:api", "//java/com/google/gerrit/index", + "//java/com/google/gerrit/index/testing", "//java/com/google/gerrit/lifecycle", "//java/com/google/gerrit/server", "//java/com/google/gerrit/server/schema", diff --git a/javatests/com/google/gerrit/server/query/group/FakeQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/FakeQueryGroupsTest.java index e4f228a084..d347716923 100644 --- a/javatests/com/google/gerrit/server/query/group/FakeQueryGroupsTest.java +++ b/javatests/com/google/gerrit/server/query/group/FakeQueryGroupsTest.java @@ -25,12 +25,26 @@ import java.util.List; import java.util.Map; import org.eclipse.jgit.lib.Config; -public class FakeQueryGroupsTest extends AbstractQueryGroupsTest { +public class FakeQueryGroupsTest extends AbstractFakeQueryGroupsTest { @ConfigSuite.Default public static Config defaultConfig() { return IndexConfig.createForFake(); } + @ConfigSuite.Config + public static Config searchAfterPaginationType() { + Config config = defaultConfig(); + config.setString("index", null, "paginationType", "SEARCH_AFTER"); + return config; + } + + @ConfigSuite.Config + public static Config nonePaginationType() { + Config config = defaultConfig(); + config.setString("index", null, "paginationType", "NONE"); + return config; + } + @ConfigSuite.Configs public static Map<String, Config> againstPreviousIndexVersion() { // the current schema version is already tested by the inherited default config suite |