diff options
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 { |