Use PaginatingSource only when pagination enabled

PaginatingSource was used also to handle non-paginated
queries, now the filtering has been restored in
AndSource, so that indices queries can directly use
it when pagination.type is set to NONE.

As stated in change 377794 this was not yet done
for the stable branches to limit the moving parts [1].

[1]: https://gerrit-review.googlesource.com/c/gerrit/+/377794/comment/8e9f0669_19585b73/

Bug: Issue 287484350
Release-Notes: skip
Change-Id: Iae3bd95b0d8ca751f9c71f989cdf9b1a71964bc1

7 files changed, 178 insertions, 67 deletions

diff --git a/java/com/google/gerrit/index/query/AndSource.java b/java/com/google/gerrit/index/query/AndSource.java
index 3adf881310..6de07123b7 100644
--- a/java/com/google/gerrit/index/query/AndSource.java
+++ b/java/com/google/gerrit/index/query/AndSource.java
@@ -17,11 +17,12 @@ package com.google.gerrit.index.query;
 import static com.google.common.base.Preconditions.checkArgument;
 
 import com.google.gerrit.index.IndexConfig;
+import com.google.gerrit.index.PaginationType;
 import java.util.Collection;
 import java.util.List;
 
 public class AndSource<T> extends AndPredicate<T> implements DataSource<T> {
-  protected final DataSource<T> source;
+  protected final FilteredSource<T> filteredSource;
 
   private final int start;
   private final int cardinality;
@@ -58,18 +59,18 @@ public class AndSource<T> extends AndPredicate<T> implements DataSource<T> {
     if (selectedSource == null) {
       throw new IllegalArgumentException("No DataSource Found");
     }
-    this.source = toPaginatingSource(selectedSource);
+    this.filteredSource = toDataSource(selectedSource);
     this.cardinality = c;
   }
 
   @Override
   public ResultSet<T> read() {
-    return source.read();
+    return filteredSource.read();
   }
 
   @Override
   public ResultSet<FieldBundle> readRaw() {
-    return source.readRaw();
+    return filteredSource.readRaw();
   }
 
   @Override
@@ -91,17 +92,44 @@ public class AndSource<T> extends AndPredicate<T> implements DataSource<T> {
   }
 
   @SuppressWarnings("unchecked")
-  private PaginatingSource<T> toPaginatingSource(Predicate<T> pred) {
-    return new PaginatingSource<>((DataSource<T>) pred, start, indexConfig) {
-      @Override
-      protected boolean match(T object) {
-        return AndSource.this.match(object);
-      }
+  private FilteredSource<T> toDataSource(Predicate<T> pred) {
+    if (indexConfig.paginationType().equals(PaginationType.NONE)) {
+      return new DatasourceWithoutPagination((DataSource<T>) pred, start, indexConfig);
+    }
+    return new DatasourceWithPagination((DataSource<T>) pred, start, indexConfig);
+  }
 
-      @Override
-      protected boolean isMatchable() {
-        return AndSource.this.isMatchable();
-      }
-    };
+  private class DatasourceWithoutPagination extends FilteredSource<T> {
+
+    public DatasourceWithoutPagination(DataSource<T> source, int start, IndexConfig indexConfig) {
+      super(source, start, indexConfig);
+    }
+
+    @Override
+    protected boolean match(T object) {
+      return AndSource.this.match(object);
+    }
+
+    @Override
+    protected boolean isMatchable() {
+      return AndSource.this.isMatchable();
+    }
+  }
+
+  private class DatasourceWithPagination extends PaginatingSource<T> {
+
+    public DatasourceWithPagination(DataSource<T> source, int start, IndexConfig indexConfig) {
+      super(source, start, indexConfig);
+    }
+
+    @Override
+    protected boolean match(T object) {
+      return AndSource.this.match(object);
+    }
+
+    @Override
+    protected boolean isMatchable() {
+      return AndSource.this.isMatchable();
+    }
   }
 }
diff --git a/java/com/google/gerrit/index/query/FilteredSource.java b/java/com/google/gerrit/index/query/FilteredSource.java
new file mode 100644
index 0000000000..fb31eb6693
--- /dev/null
+++ b/java/com/google/gerrit/index/query/FilteredSource.java
@@ -0,0 +1,94 @@
+// 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.index.query;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.common.collect.FluentIterable;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.index.IndexConfig;
+import java.util.ArrayList;
+import java.util.List;
+
+public class FilteredSource<T> implements DataSource<T> {
+
+  protected final DataSource<T> source;
+  protected final int start;
+  protected final int cardinality;
+  protected final IndexConfig indexConfig;
+  private static final int PARTITION_SIZE = 50;
+
+  public FilteredSource(DataSource<T> source, int start, IndexConfig indexConfig) {
+    checkArgument(start >= 0, "negative start: %s", start);
+    this.source = source;
+    this.start = start;
+    this.cardinality = source.getCardinality();
+    this.indexConfig = indexConfig;
+  }
+
+  @Override
+  public ResultSet<T> read() {
+    if (source == null) {
+      throw new StorageException("No DataSource defined.");
+    }
+    // ResultSets are lazy. Calling #read here first and then dealing with ResultSets only when
+    // requested allows the index to run asynchronous queries.
+    ResultSet<T> resultSet = source.read();
+    return new LazyResultSet<>(
+        () -> {
+          List<T> r = new ArrayList<>();
+          for (T data : buffer(resultSet)) {
+            if (!isMatchable() || match(data)) {
+              r.add(data);
+            }
+          }
+          if (start >= r.size()) {
+            return ImmutableList.of();
+          } else if (start > 0) {
+            return ImmutableList.copyOf(r.subList(start, r.size()));
+          }
+          return ImmutableList.copyOf(r);
+        });
+  }
+
+  @Override
+  public ResultSet<FieldBundle> readRaw() {
+    return source.readRaw();
+  }
+
+  protected Iterable<T> buffer(ResultSet<T> scanner) {
+    return FluentIterable.from(Iterables.partition(scanner, PARTITION_SIZE))
+        .transformAndConcat(this::transformBuffer);
+  }
+
+  protected List<T> transformBuffer(List<T> buffer) {
+    return buffer;
+  }
+
+  @Override
+  public int getCardinality() {
+    return cardinality;
+  }
+
+  protected boolean match(T object) {
+    return true;
+  }
+
+  protected boolean isMatchable() {
+    return true;
+  }
+}
diff --git a/java/com/google/gerrit/index/query/PaginatingSource.java b/java/com/google/gerrit/index/query/PaginatingSource.java
index 98a0ed33f8..be789ee8f7 100644
--- a/java/com/google/gerrit/index/query/PaginatingSource.java
+++ b/java/com/google/gerrit/index/query/PaginatingSource.java
@@ -14,11 +14,7 @@
 
 package com.google.gerrit.index.query;
 
-import static com.google.common.base.Preconditions.checkArgument;
-
-import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Iterables;
 import com.google.common.collect.Ordering;
 import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.index.IndexConfig;
@@ -27,18 +23,10 @@ import com.google.gerrit.index.QueryOptions;
 import java.util.ArrayList;
 import java.util.List;
 
-public class PaginatingSource<T> implements DataSource<T> {
-  protected final DataSource<T> source;
-  private final int start;
-  private final int cardinality;
-  private final IndexConfig indexConfig;
+public class PaginatingSource<T> extends FilteredSource<T> {
 
   public PaginatingSource(DataSource<T> source, int start, IndexConfig indexConfig) {
-    checkArgument(start >= 0, "negative start: %s", start);
-    this.source = source;
-    this.start = start;
-    this.cardinality = source.getCardinality();
-    this.indexConfig = indexConfig;
+    super(source, start, indexConfig);
   }
 
   @Override
@@ -63,13 +51,7 @@ public class PaginatingSource<T> implements DataSource<T> {
             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)) {
+          if (last != null && source instanceof Paginated) {
             // Restart source and continue if we have not filled the
             // full limit the caller wants.
             //
@@ -117,34 +99,6 @@ public class PaginatingSource<T> implements DataSource<T> {
     throw new UnsupportedOperationException("not implemented");
   }
 
-  private Iterable<T> buffer(ResultSet<T> scanner) {
-    return FluentIterable.from(Iterables.partition(scanner, 50))
-        .transformAndConcat(this::transformBuffer);
-  }
-
-  /**
-   * Checks whether the given object matches.
-   *
-   * @param object the object to be matched
-   * @return whether the given object matches
-   */
-  protected boolean match(T object) {
-    return true;
-  }
-
-  protected boolean isMatchable() {
-    return true;
-  }
-
-  protected List<T> transformBuffer(List<T> buffer) {
-    return buffer;
-  }
-
-  @Override
-  public int getCardinality() {
-    return cardinality;
-  }
-
   private int getNextPageSize(int pageSize, int pageSizeMultiplier) {
     List<Integer> possiblePageSizes = new ArrayList<>(3);
     try {
diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java
index d847a06281..f49cecb089 100644
--- a/java/com/google/gerrit/index/query/QueryProcessor.java
+++ b/java/com/google/gerrit/index/query/QueryProcessor.java
@@ -33,6 +33,7 @@ import com.google.gerrit.index.Index;
 import com.google.gerrit.index.IndexCollection;
 import com.google.gerrit.index.IndexConfig;
 import com.google.gerrit.index.IndexRewriter;
+import com.google.gerrit.index.PaginationType;
 import com.google.gerrit.index.QueryOptions;
 import com.google.gerrit.index.SchemaDefinitions;
 import com.google.gerrit.metrics.Description;
@@ -296,7 +297,9 @@ public abstract class QueryProcessor<T> {
 
         @SuppressWarnings("unchecked")
         DataSource<T> s = (DataSource<T>) pred;
-        if (initialPageSize < limit && !(pred instanceof AndSource)) {
+        if (!indexConfig.paginationType().equals(PaginationType.NONE)
+            && initialPageSize < limit
+            && !(pred instanceof AndSource)) {
           s = new PaginatingSource<>(s, start, indexConfig);
         }
         sources.add(s);
diff --git a/java/com/google/gerrit/server/query/change/AndChangeSource.java b/java/com/google/gerrit/server/query/change/AndChangeSource.java
index e4f768ebcc..6c03425197 100644
--- a/java/com/google/gerrit/server/query/change/AndChangeSource.java
+++ b/java/com/google/gerrit/server/query/change/AndChangeSource.java
@@ -33,7 +33,8 @@ public class AndChangeSource extends AndSource<ChangeData> implements ChangeData
 
   @Override
   public boolean hasChange() {
-    return source instanceof ChangeDataSource && ((ChangeDataSource) source).hasChange();
+    return filteredSource instanceof ChangeDataSource
+        && ((ChangeDataSource) filteredSource).hasChange();
   }
 
   @Override
diff --git a/javatests/com/google/gerrit/index/query/AndSourceTest.java b/javatests/com/google/gerrit/index/query/AndSourceTest.java
index 068ae8c65d..6f20bd7e7f 100644
--- a/javatests/com/google/gerrit/index/query/AndSourceTest.java
+++ b/javatests/com/google/gerrit/index/query/AndSourceTest.java
@@ -20,15 +20,24 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 import com.google.common.collect.Lists;
+import com.google.gerrit.index.IndexConfig;
 import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.testing.ConfigSuite;
+import org.eclipse.jgit.lib.Config;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 
+@RunWith(ConfigSuite.class)
 public class AndSourceTest extends PredicateTest {
+
+  @ConfigSuite.Parameter public Config config;
+
   @Test
   public void ensureLowerCostPredicateRunsFirst() {
     TestDataSourcePredicate p1 = new TestDataSourcePredicate("predicate1", "foo", 10, 10);
     TestDataSourcePredicate p2 = new TestDataSourcePredicate("predicate2", "foo", 1, 10);
-    AndSource<String> andSource = new AndSource<>(Lists.newArrayList(p1, p2), null);
+    AndSource<String> andSource =
+        new AndSource<>(Lists.newArrayList(p1, p2), IndexConfig.fromConfig(config).build());
     assertFalse(andSource.match("bar"));
     assertFalse(p1.ranMatch);
     assertTrue(p2.ranMatch);
diff --git a/javatests/com/google/gerrit/index/query/PredicateTest.java b/javatests/com/google/gerrit/index/query/PredicateTest.java
index d789201005..1853d9884d 100644
--- a/javatests/com/google/gerrit/index/query/PredicateTest.java
+++ b/javatests/com/google/gerrit/index/query/PredicateTest.java
@@ -14,10 +14,32 @@
 
 package com.google.gerrit.index.query;
 
+import com.google.gerrit.testing.ConfigSuite;
+import org.eclipse.jgit.lib.Config;
 import org.junit.Ignore;
 
 @Ignore
 public abstract class PredicateTest {
+
+  @ConfigSuite.Default
+  public static Config defaultConfig() {
+    return com.google.gerrit.testing.IndexConfig.create();
+  }
+
+  @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;
+  }
+
   @SuppressWarnings("ProtectedMembersInFinalClass")
   protected static class TestDataSourcePredicate extends TestMatchablePredicate<String>
       implements DataSource<String> {