Fix visibility filter for changes when paginationType NONE

The introduction of paginationType NONE in
change 379354 affected the API pagination.

In particular, if some results are skipped because of
of the visibility constraints, more changes need to
be asked from the index, until we have filled the
full limit the caller wants or if there are no more
changes [1].

[1]: https://gerrit-review.googlesource.com/c/gerrit/+/379354/8/java/com/google/gerrit/index/query/PaginatingSource.java#72

Bug: Issue 328958027
Release-Notes: Fix NONE paginationType filtering of changes
Change-Id: I3ccff7f8ba5a6d3903f9acbe3f5c439c55225ce2

5 files changed, 64 insertions, 38 deletions

diff --git a/java/com/google/gerrit/index/query/IndexedQuery.java b/java/com/google/gerrit/index/query/IndexedQuery.java
index df14e295cc..4bc8043ff7 100644
--- a/java/com/google/gerrit/index/query/IndexedQuery.java
+++ b/java/com/google/gerrit/index/query/IndexedQuery.java
@@ -87,6 +87,12 @@ public class IndexedQuery<I, T> extends Predicate<T> implements DataSource<T>, P
   }
 
   @Override
+  public ResultSet<T> restart(int start) {
+    opts = opts.withStart(start);
+    return search();
+  }
+
+  @Override
   public ResultSet<T> restart(int start, int pageSize) {
     opts = opts.withStart(start).withPageSize(pageSize);
     return search();
diff --git a/java/com/google/gerrit/index/query/Paginated.java b/java/com/google/gerrit/index/query/Paginated.java
index 552199093b..1065019c12 100644
--- a/java/com/google/gerrit/index/query/Paginated.java
+++ b/java/com/google/gerrit/index/query/Paginated.java
@@ -19,6 +19,8 @@ import com.google.gerrit.index.QueryOptions;
 public interface Paginated<T> {
   QueryOptions getOptions();
 
+  ResultSet<T> restart(int start);
+
   ResultSet<T> restart(int start, int pageSize);
 
   ResultSet<T> restart(Object searchAfter, int pageSize);
diff --git a/java/com/google/gerrit/index/query/PaginatingSource.java b/java/com/google/gerrit/index/query/PaginatingSource.java
index 4eceb319e7..8847005629 100644
--- a/java/com/google/gerrit/index/query/PaginatingSource.java
+++ b/java/com/google/gerrit/index/query/PaginatingSource.java
@@ -55,47 +55,66 @@ public class PaginatingSource<T> implements DataSource<T> {
           List<T> r = new ArrayList<>();
           T last = null;
           int pageResultSize = 0;
+          boolean skipped = false;
           for (T data : buffer(resultSet)) {
             if (!isMatchable() || match(data)) {
               r.add(data);
+            } else {
+              skipped = true;
             }
             last = data;
             pageResultSize++;
           }
 
-          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.
-            //
-            @SuppressWarnings("unchecked")
-            Paginated<T> p = (Paginated<T>) source;
-            QueryOptions opts = p.getOptions();
-            final int limit = opts.limit();
-            int pageSize = opts.pageSize();
-            int pageSizeMultiplier = opts.pageSizeMultiplier();
-            Object searchAfter = resultSet.searchAfter();
-            int nextStart = pageResultSize;
-            while (pageResultSize == pageSize && r.size() <= limit) { // get 1 more than the limit
-              pageSize = getNextPageSize(pageSize, pageSizeMultiplier);
-              ResultSet<T> next =
-                  indexConfig.paginationType().equals(PaginationType.SEARCH_AFTER)
-                      ? p.restart(searchAfter, pageSize)
-                      : p.restart(nextStart, pageSize);
-              pageResultSize = 0;
-              for (T data : buffer(next)) {
-                if (match(data)) {
-                  r.add(data);
+          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
+            if (!indexConfig.paginationType().equals(PaginationType.NONE)) {
+              // Restart source and continue if we have not filled the
+              // full limit the caller wants.
+              //
+              @SuppressWarnings("unchecked")
+              Paginated<T> p = (Paginated<T>) source;
+              QueryOptions opts = p.getOptions();
+              final int limit = opts.limit();
+              int pageSize = opts.pageSize();
+              int pageSizeMultiplier = opts.pageSizeMultiplier();
+              Object searchAfter = resultSet.searchAfter();
+              int nextStart = pageResultSize;
+              while (pageResultSize == pageSize && r.size() <= limit) { // get 1 more than the limit
+                pageSize = getNextPageSize(pageSize, pageSizeMultiplier);
+                ResultSet<T> next =
+                    indexConfig.paginationType().equals(PaginationType.SEARCH_AFTER)
+                        ? p.restart(searchAfter, pageSize)
+                        : p.restart(nextStart, pageSize);
+                pageResultSize = 0;
+                for (T data : buffer(next)) {
+                  if (match(data)) {
+                    r.add(data);
+                  }
+                  pageResultSize++;
+                }
+                nextStart += pageResultSize;
+                searchAfter = next.searchAfter();
+              }
+            } else {
+              @SuppressWarnings("unchecked")
+              Paginated<T> p = (Paginated<T>) source;
+              while (skipped && r.size() < p.getOptions().limit() + start) {
+                skipped = false;
+                ResultSet<T> next = p.restart(pageResultSize);
+
+                for (T data : buffer(next)) {
+                  if (match(data)) {
+                    r.add(data);
+                  } else {
+                    skipped = true;
+                  }
+                  pageResultSize++;
                 }
-                pageResultSize++;
               }
-              nextStart += pageResultSize;
-              searchAfter = next.searchAfter();
             }
           }
 
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index ca73fdcff4..3fc4de2038 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -464,11 +464,11 @@ public class LuceneChangeIndex implements ChangeIndex {
       IndexSearcher[] searchers = new IndexSearcher[indexes.size()];
       Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex = new HashMap<>();
       try {
-        int realPageSize = opts.start() + opts.pageSize();
-        if (Integer.MAX_VALUE - opts.pageSize() < opts.start()) {
-          realPageSize = Integer.MAX_VALUE;
+        int pageLimit = AbstractLuceneIndex.getLimitBasedOnPaginationType(opts, opts.pageSize());
+        int queryLimit = opts.start() + pageLimit;
+        if (Integer.MAX_VALUE - pageLimit < opts.start()) {
+          queryLimit = 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++) {
@@ -476,7 +476,7 @@ public class LuceneChangeIndex implements ChangeIndex {
           searchers[i] = subIndex.acquire();
           if (isSearchAfterPagination) {
             ScoreDoc searchAfter = getSearchAfter(subIndex);
-            int maxRemainingHits = realPageSize - searchAfterHitsCount;
+            int maxRemainingHits = queryLimit - searchAfterHitsCount;
             if (maxRemainingHits > 0) {
               TopFieldDocs subIndexHits =
                   searchers[i].searchAfter(
diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
index 4996ff95d7..603564c59e 100644
--- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
@@ -33,7 +33,6 @@ import com.google.inject.Injector;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.revwalk.RevCommit;
-import org.junit.Ignore;
 import org.junit.Test;
 
 public abstract class LuceneQueryChangesTest extends AbstractQueryChangesTest {
@@ -102,7 +101,7 @@ public abstract class LuceneQueryChangesTest extends AbstractQueryChangesTest {
     assertQuery(newQuery("project:repo").withNoLimit(), expected);
   }
 
-  @Ignore
+  @Test
   public void skipChangesNotVisible() throws Exception {
     // create 1 new change on a repo
     TestRepository<Repo> repo = createProject("repo");