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)
diff --git a/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
index 1eb2770..7428e3a 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 @@
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 8dc17d3..5a7efb8 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 @@
*/
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 7dc7a0b..20c43af 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.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 @@
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 @@
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 8b606a0..60289fb 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.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 @@
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 @@
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 {
