QueryProcessor: Short-circuit return empty results when disabled

When visibility is enforced, it is possible for the query processor to
be disabled due to the user having a non-positive query limit
capability. Previously, calling query in this case may have unexpected
behavior, such as throwing an unchecked exception due to dividing by
zero or constructing a QueryOptions with a negative limit. Avoid
exceptions by short-circuiting and returning no results.

This new implementation means that callers need to call isDisabled to
distinguish between an empty result set and a disabled processor. Most
callers do not care about detecting this specific case and returning an
error message to the user; as a result, it would be onerous to require
them to call isDisabled in all cases. Only the callers that care
specifically need to do so, such as QueryChanges and OutputStreamQuery.

Change-Id: Ie6db9d8d67e39eb5d4676417a21fa8cd9cf830da

gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java

@@ -14,10 +14,15 @@
 
 package com.google.gerrit.server.query;
 
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Ordering;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.GlobalCapability;
 import com.google.gerrit.index.Index;
 import com.google.gerrit.index.IndexCollection;
@@ -46,6 +51,7 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.IntStream;
 
 public abstract class QueryProcessor<T> {
   @Singleton
@@ -105,6 +111,20 @@ public abstract class QueryProcessor<T> {
     return this;
   }
 
+  /**
+   * Specify whether to enforce visibility by filtering out results that are not visible to the
+   * user.
+   *
+   * <p>Enforcing visibility may have performance consequences, as the index system may need to
+   * post-filter a large number of results to fill even a modest limit.
+   *
+   * <p>If visibility is enforced, the user's {@code queryLimit} global capability is also used to
+   * bound the total number of results. If this capability is non-positive, this results in the
+   * entire query processor being {@link #isDisabled() disabled}.
+   *
+   * @param enforce whether to enforce visibility.
+   * @return this.
+   */
   public QueryProcessor<T> enforceVisibility(boolean enforce) {
     enforceVisibility = enforce;
     return this;
@@ -134,6 +154,10 @@ public abstract class QueryProcessor<T> {
   /**
    * Perform multiple queries in parallel.
    *
+   * <p>If querying is disabled, short-circuits the index and returns empty results. Callers that
+   * wish to distinguish this case from a query returning no results from the index may call {@link
+   * #isDisabled()} themselves.
+   *
    * @param queries list of queries.
    * @return results of the queries, one QueryResult per input query, in the same order as the
    *     input.
@@ -152,11 +176,22 @@ public abstract class QueryProcessor<T> {
     }
   }
 
-  private List<QueryResult<T>> query(List<String> queryStrings, List<Predicate<T>> queries)
+  private List<QueryResult<T>> query(
+      @Nullable List<String> queryStrings, List<Predicate<T>> queries)
       throws OrmException, QueryParseException {
     long startNanos = System.nanoTime();
-
     int cnt = queries.size();
+    if (queryStrings != null) {
+      int qs = queryStrings.size();
+      checkArgument(qs == cnt, "got %s query strings but %s predicates", qs, cnt);
+    }
+    if (cnt == 0) {
+      return ImmutableList.of();
+    }
+    if (isDisabled()) {
+      return disabledResults(queryStrings, queries);
+    }
+
     // Parse and rewrite all queries.
     List<Integer> limits = new ArrayList<>(cnt);
     List<Predicate<T>> predicates = new ArrayList<>(cnt);
@@ -206,12 +241,25 @@ public abstract class QueryProcessor<T> {
               matches.get(i).toList()));
     }
 
-    // only measure successful queries
+    // Only measure successful queries that actually touched the index.
     metrics.executionTime.record(
         schemaDef.getName(), System.nanoTime() - startNanos, TimeUnit.NANOSECONDS);
     return out;
   }
 
+  private static <T> ImmutableList<QueryResult<T>> disabledResults(
+      List<String> queryStrings, List<Predicate<T>> queries) {
+    return IntStream.range(0, queries.size())
+        .mapToObj(
+            i ->
+                QueryResult.create(
+                    queryStrings != null ? queryStrings.get(i) : null,
+                    queries.get(i),
+                    0,
+                    ImmutableList.of()))
+        .collect(toImmutableList());
+  }
+
   protected QueryOptions createOptions(
       IndexConfig indexConfig, int start, int limit, Set<String> requestedFields) {
     return QueryOptions.create(indexConfig, start, limit, requestedFields);
@@ -234,6 +282,19 @@ public abstract class QueryProcessor<T> {
     return index != null ? index.getSchema().getStoredFields().keySet() : ImmutableSet.<String>of();
   }
 
+  /**
+   * Check whether querying should be disabled.
+   *
+   * <p>Currently, the only condition that can disable the whole query processor is if both {@link
+   * #enforceVisibility(boolean) visibility is enforced} and the user has a non-positive maximum
+   * value for the {@code queryLimit} capability.
+   *
+   * <p>If querying is disabled, all calls to {@link #query(Predicate)} and {@link #query(List)}
+   * will return empty results. This method can be used if callers wish to distinguish this case
+   * from a query returning no results from the index.
+   *
+   * @return true if querying should be disabled.
+   */
   public boolean isDisabled() {
     return getPermittedLimit() <= 0;
   }
@@ -265,6 +326,9 @@ public abstract class QueryProcessor<T> {
         possibleLimits.add(limitFromPredicate);
       }
     }
-    return Ordering.natural().min(possibleLimits);
+    int result = Ordering.natural().min(possibleLimits);
+    // Should have short-circuited from #query or thrown some other exception before getting here.
+    checkState(result > 0, "effective limit should be positive");
+    return result;
   }
 }