Add an index-safe predicate that matches all changes

Iad564dd47 was problematic in the error case because it returned
Predicate.not(Predicate.any()). The "any" predicate is not an index
predicate, which produces at least two problems:

* When used in a with another non-index predicate, it
  might throw "no ChangeDataSource" from AndSource/OrSource.
* More dangerously, when combined with an index predicate, it is used as
  a post-filtering predicate. A post-filtering predicate that matches no
  results means it gets applied to every change in the index before
  returning an empty set, which is bad.

Instead, reuse a handy existing index predicate that is designed to
return no results efficiently from the index:
ChangeStatusPredicate.NONE. Expose this from a more general location,
ChangeIndexPredicate#none().

Release-Notes: skip
Change-Id: Ic9a7e277070cff7768ec91c1972cf8e9f6deacb1
(cherry picked from commit d9679cdb605b8a10977e1fc821f57dd5749b1dd5)

4 files changed, 76 insertions, 11 deletions

diff --git a/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
index 1eb27706e3..7428e3a1c3 100644
--- a/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
@@ -17,9 +17,22 @@ package com.google.gerrit.server.query.change;
 import com.google.gerrit.index.FieldDef;
 import com.google.gerrit.index.query.IndexPredicate;
 import com.google.gerrit.index.query.Matchable;
+import com.google.gerrit.index.query.Predicate;
 
 public abstract class ChangeIndexPredicate extends IndexPredicate<ChangeData>
     implements Matchable<ChangeData> {
+  /**
+   * Returns an index predicate that matches no changes in the index.
+   *
+   * <p>This predicate should be used in preference to a non-index predicate (such as {@code
+   * Predicate.not(Predicate.any())}), since it can be matched efficiently against the index.
+   *
+   * @return an index predicate matching no changes.
+   */
+  public static Predicate<ChangeData> none() {
+    return ChangeStatusPredicate.NONE;
+  }
+
   protected ChangeIndexPredicate(FieldDef<ChangeData, ?> def, String value) {
     super(def, value);
   }
diff --git a/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java b/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
index 8dc17d3040..5a7efb8863 100644
--- a/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
@@ -41,7 +41,7 @@ import java.util.TreeMap;
  */
 public final class ChangeStatusPredicate extends ChangeIndexPredicate {
   private static final String INVALID_STATUS = "__invalid__";
-  private static final Predicate<ChangeData> NONE = new ChangeStatusPredicate(null);
+  static final Predicate<ChangeData> NONE = new ChangeStatusPredicate(null);
 
   private static final TreeMap<String, Predicate<ChangeData>> PREDICATES;
   private static final Predicate<ChangeData> CLOSED;
diff --git a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
index 7dc7a0b080..20c43af9ac 100644
--- a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
@@ -14,6 +14,9 @@
 
 package com.google.gerrit.server.query.change;
 
+import static java.util.concurrent.TimeUnit.MINUTES;
+
+import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.data.SubmitTypeRecord;
 import com.google.gerrit.index.query.PostFilterPredicate;
 import com.google.gerrit.index.query.Predicate;
@@ -41,6 +44,8 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 
 public class ConflictsPredicate {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   // UI code may depend on this string, so use caution when changing.
   protected static final String TOO_MANY_FILES = "too many files to find conflicts";
 
@@ -54,7 +59,12 @@ public class ConflictsPredicate {
       cd = args.changeDataFactory.create(args.db.get(), c);
       files = cd.currentFilePaths();
     } catch (IOException e) {
-      throw new OrmException(e);
+      warnWithOccasionalStackTrace(
+          e,
+          "Error constructing conflicts predicates for change %s in %s",
+          c.getId(),
+          c.getProject());
+      return ChangeIndexPredicate.none();
     }
 
     if (3 + files.size() > args.indexConfig.maxTerms()) {
@@ -201,4 +211,13 @@ public class ConflictsPredicate {
       return alreadyAccepted;
     }
   }
+
+  private static void warnWithOccasionalStackTrace(Throwable cause, String format, Object... args) {
+    logger.atWarning().logVarargs(format, args);
+    logger
+        .atWarning()
+        .withCause(cause)
+        .atMostEvery(1, MINUTES)
+        .logVarargs("(Re-logging with stack trace) " + format, args);
+  }
 }
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 8b606a032d..60289fb289 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.query.change;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.TruthJUnit.assume;
 import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS;
@@ -73,6 +74,7 @@ import com.google.gerrit.index.FieldDef;
 import com.google.gerrit.index.IndexConfig;
 import com.google.gerrit.index.QueryOptions;
 import com.google.gerrit.index.Schema;
+import com.google.gerrit.index.query.Predicate;
 import com.google.gerrit.lifecycle.LifecycleManager;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Account.Id;
@@ -3198,6 +3200,26 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     assertQuery("project:repo+foo", change);
   }
 
+  @Test
+  public void none() throws Exception {
+    TestRepository<Repo> repo = createProject("repo");
+    Change change = insert(repo, newChange(repo));
+
+    assertQuery(ChangeIndexPredicate.none());
+
+    for (Predicate<ChangeData> matchingOneChange :
+        ImmutableList.of(
+            // One index query, one post-filtering query.
+            queryBuilder.parse(change.getId().toString()),
+            queryBuilder.parse("ownerin:Administrators"))) {
+      assertQuery(matchingOneChange, change);
+      assertQuery(Predicate.or(ChangeIndexPredicate.none(), matchingOneChange), change);
+      assertQuery(Predicate.and(ChangeIndexPredicate.none(), matchingOneChange));
+      assertQuery(
+          Predicate.and(Predicate.not(ChangeIndexPredicate.none()), matchingOneChange), change);
+    }
+  }
+
   protected ChangeInserter newChange(TestRepository<Repo> repo) throws Exception {
     return newChange(repo, null, null, null, null, false, false);
   }
@@ -3359,21 +3381,32 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     List<ChangeInfo> result = query.get();
     Iterable<Change.Id> ids = ids(result);
     assertThat(ids)
-        .named(format(query, ids, changes))
+        .named(format(query.getQuery(), ids, changes))
         .containsExactlyElementsIn(Arrays.asList(changes))
         .inOrder();
     return result;
   }
 
-  private String format(
-      QueryRequest query, Iterable<Change.Id> actualIds, Change.Id... expectedChanges)
+  protected void assertQuery(Predicate<ChangeData> predicate, Change... changes) throws Exception {
+    ImmutableList<Change.Id> actualIds =
+        queryProvider.get().query(predicate).stream()
+            .map(ChangeData::getId)
+            .collect(toImmutableList());
+    Change.Id[] expectedIds = Arrays.stream(changes).map(Change::getId).toArray(Change.Id[]::new);
+    assertThat(actualIds)
+        .named(format(predicate.toString(), actualIds, expectedIds))
+        .containsExactlyElementsIn(expectedIds)
+        .inOrder();
+  }
+
+  private String format(String query, Iterable<Change.Id> actualIds, Change.Id... expectedChanges)
       throws RestApiException {
-    StringBuilder b = new StringBuilder();
-    b.append("query '").append(query.getQuery()).append("' with expected changes ");
-    b.append(format(Arrays.asList(expectedChanges)));
-    b.append(" and result ");
-    b.append(format(actualIds));
-    return b.toString();
+    return "query '"
+        + query
+        + "' with expected changes "
+        + format(Arrays.asList(expectedChanges))
+        + " and result "
+        + format(actualIds);
   }
 
   private String format(Iterable<Change.Id> changeIds) throws RestApiException {