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
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java
index 92439db..65f9721 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java
+++ b/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.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 @@
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 @@
/**
* 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 @@
}
}
- 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 @@
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 @@
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 @@
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;
}
}
