From 3b62515ce179a17ad70af9f6aee9ec24f8194498 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 5 Mar 2019 10:45:03 -0800 Subject: 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) --- .../server/query/change/ChangeIndexPredicate.java | 13 ++++++ .../server/query/change/ChangeStatusPredicate.java | 2 +- .../server/query/change/ConflictsPredicate.java | 21 ++++++++- .../query/change/AbstractQueryChangesTest.java | 51 ++++++++++++++++++---- 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 implements Matchable { + /** + * Returns an index predicate that matches no changes in the index. + * + *

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 none() { + return ChangeStatusPredicate.NONE; + } + protected ChangeIndexPredicate(FieldDef 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 NONE = new ChangeStatusPredicate(null); + static final Predicate NONE = new ChangeStatusPredicate(null); private static final TreeMap> PREDICATES; private static final Predicate 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 = createProject("repo"); + Change change = insert(repo, newChange(repo)); + + assertQuery(ChangeIndexPredicate.none()); + + for (Predicate 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) throws Exception { return newChange(repo, null, null, null, null, false, false); } @@ -3359,21 +3381,32 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { List result = query.get(); Iterable 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 actualIds, Change.Id... expectedChanges) + protected void assertQuery(Predicate predicate, Change... changes) throws Exception { + ImmutableList 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 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 changeIds) throws RestApiException { -- cgit v1.2.3