Move checker query methods from Checker to CheckerQuery
Checker remains mostly a dumb value type. CheckerQuery becomes a
non-singleton instance that can have members like RetryHelper injected.
The name stays the same, by a loose analogy to InternalChangeQuery.
In addition to allowing for simpler injection and to separate complex
executions from value types, this will allow us to reuse the query logic
without having an actual Checker instance available.
Change-Id: I7c2e703b676d5bcc08e7f0c8127a1e7293705087
diff --git a/java/com/google/gerrit/plugins/checks/Checker.java b/java/com/google/gerrit/plugins/checks/Checker.java
index 56fa64b..075e2c9 100644
--- a/java/com/google/gerrit/plugins/checks/Checker.java
+++ b/java/com/google/gerrit/plugins/checks/Checker.java
@@ -15,37 +15,18 @@
package com.google.gerrit.plugins.checks;
import com.google.auto.value.AutoValue;
-import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.index.query.IndexPredicate;
-import com.google.gerrit.index.query.Predicate;
-import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.plugins.checks.api.BlockingCondition;
import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.server.index.change.ChangeField;
-import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.query.change.ChangeQueryBuilder;
-import com.google.gerrit.server.query.change.ChangeQueryProcessor;
-import com.google.gerrit.server.query.change.ChangeStatusPredicate;
-import com.google.gerrit.server.query.change.ProjectPredicate;
-import com.google.gerrit.server.update.RetryHelper;
-import com.google.gerrit.server.update.RetryHelper.ActionType;
-import com.google.gwtorm.server.OrmException;
-import com.google.inject.Provider;
import java.sql.Timestamp;
-import java.util.List;
import java.util.Optional;
-import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
/** Definition of a checker. */
@AutoValue
public abstract class Checker {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
/**
* Returns the UUID of the checker.
*
@@ -145,91 +126,6 @@
return CheckerStatus.DISABLED == getStatus();
}
- public boolean isCheckerRelevant(ChangeData cd, ChangeQueryBuilder changeQueryBuilder)
- throws OrmException {
- if (!getQuery().isPresent()) {
- return cd.change().isNew();
- }
-
- Predicate<ChangeData> predicate;
- try {
- predicate = createQueryPredicate(changeQueryBuilder);
- } catch (ConfigInvalidException e) {
- logger.atWarning().withCause(e).log("skipping invalid query for checker %s", getUuid());
- return false;
- }
-
- return predicate.asMatchable().match(cd);
- }
-
- public List<ChangeData> queryMatchingChanges(
- RetryHelper retryHelper,
- ChangeQueryBuilder changeQueryBuilder,
- Provider<ChangeQueryProcessor> changeQueryProcessorProvider)
- throws ConfigInvalidException, OrmException {
- return executeIndexQueryWithRetry(
- retryHelper, changeQueryProcessorProvider, createQueryPredicate(changeQueryBuilder));
- }
-
- private Predicate<ChangeData> createQueryPredicate(ChangeQueryBuilder changeQueryBuilder)
- throws ConfigInvalidException {
- Predicate<ChangeData> predicate = new ProjectPredicate(getRepository().get());
-
- if (getQuery().isPresent()) {
- String query = getQuery().get();
- Predicate<ChangeData> predicateForQuery;
- try {
- predicateForQuery = changeQueryBuilder.parse(query);
- } catch (QueryParseException e) {
- throw new ConfigInvalidException(
- String.format("change query of checker %s is invalid: %s", getUuid(), query), e);
- }
-
- if (!predicateForQuery.isMatchable()) {
- // Assuming nobody modified the query behind Gerrit's back, this is programmer error:
- // CheckerQuery should not be able to produce non-matchable queries.
- throw new ConfigInvalidException(
- String.format("change query of checker %s is non-matchable: %s", getUuid(), query));
- }
-
- predicate = Predicate.and(predicate, predicateForQuery);
- }
-
- if (!hasStatusPredicate(predicate)) {
- predicate = Predicate.and(ChangeStatusPredicate.open(), predicate);
- }
-
- return predicate;
- }
-
- private static boolean hasStatusPredicate(Predicate<ChangeData> predicate) {
- if (predicate instanceof IndexPredicate) {
- return ((IndexPredicate<ChangeData>) predicate)
- .getField()
- .getName()
- .equals(ChangeField.STATUS.getName());
- }
- return predicate.getChildren().stream().anyMatch(Checker::hasStatusPredicate);
- }
-
- // TODO(ekempin): Retrying the query should be done by ChangeQueryProcessor.
- private List<ChangeData> executeIndexQueryWithRetry(
- RetryHelper retryHelper,
- Provider<ChangeQueryProcessor> changeQueryProcessorProvider,
- Predicate<ChangeData> predicate)
- throws OrmException {
- try {
- return retryHelper.execute(
- ActionType.INDEX_QUERY,
- () -> changeQueryProcessorProvider.get().query(predicate).entities(),
- OrmException.class::isInstance);
- } catch (Exception e) {
- Throwables.throwIfUnchecked(e);
- Throwables.throwIfInstanceOf(e, OrmException.class);
- throw new OrmException(e);
- }
- }
-
/**
* Checks whether a {@link Checker} is required for submission or not.
*
diff --git a/java/com/google/gerrit/plugins/checks/CheckerQuery.java b/java/com/google/gerrit/plugins/checks/CheckerQuery.java
index 5712c93..0830d8b 100644
--- a/java/com/google/gerrit/plugins/checks/CheckerQuery.java
+++ b/java/com/google/gerrit/plugins/checks/CheckerQuery.java
@@ -22,13 +22,39 @@
import static com.google.gerrit.index.query.QueryParser.OR;
import static java.util.Objects.requireNonNull;
+import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.index.query.IndexPredicate;
+import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.index.query.QueryParser;
+import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.index.change.ChangeField;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
+import com.google.gerrit.server.query.change.ChangeQueryProcessor;
+import com.google.gerrit.server.query.change.ChangeStatusPredicate;
+import com.google.gerrit.server.query.change.ProjectPredicate;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryHelper.ActionType;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import java.util.List;
import org.antlr.runtime.tree.Tree;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+/**
+ * Utility for validating and executing relevancy queries for checkers.
+ *
+ * <p>Instances are not threadsafe and should not be reused across requests. However, they may be
+ * reused within a single request.
+ */
public class CheckerQuery {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
// Note that this list contains *operators*, not predicates. If there are multiple operators
// aliased together to the same predicate ("f:", "file:"), they all need to be listed explicitly.
//
@@ -142,5 +168,103 @@
return node.getChild(0);
}
- private CheckerQuery() {}
+ private final RetryHelper retryHelper;
+ private final Provider<ChangeQueryProcessor> changeQueryProcessorProvider;
+ private final ChangeQueryBuilder queryBuilder;
+
+ @Inject
+ CheckerQuery(
+ RetryHelper retryHelper,
+ Provider<AnonymousUser> anonymousUserProvider,
+ Provider<ChangeQueryBuilder> queryBuilderProvider,
+ Provider<ChangeQueryProcessor> changeQueryProcessorProvider) {
+ this.retryHelper = retryHelper;
+ this.changeQueryProcessorProvider = changeQueryProcessorProvider;
+ // The user passed to the ChangeQueryBuilder just controls how it parses "self". Anonymous means
+ // "self" is disallowed, which is correct for checker queries, since the results should not
+ // depend on the calling user. However, note that results are still filtered by visibility, but
+ // visibility is controlled by ChangeQueryProcessor, which always uses the current user and
+ // can't be overridden.
+ this.queryBuilder = queryBuilderProvider.get().asUser(anonymousUserProvider.get());
+ }
+
+ public boolean isCheckerRelevant(Checker checker, ChangeData cd) throws OrmException {
+ if (!checker.getQuery().isPresent()) {
+ return cd.change().isNew();
+ }
+
+ Predicate<ChangeData> predicate;
+ try {
+ predicate = createQueryPredicate(checker);
+ } catch (ConfigInvalidException e) {
+ logger.atWarning().withCause(e).log(
+ "skipping invalid query for checker %s", checker.getUuid());
+ return false;
+ }
+
+ return predicate.asMatchable().match(cd);
+ }
+
+ public List<ChangeData> queryMatchingChanges(Checker checker)
+ throws ConfigInvalidException, OrmException {
+ return executeIndexQueryWithRetry(createQueryPredicate(checker));
+ }
+
+ private Predicate<ChangeData> createQueryPredicate(Checker checker)
+ throws ConfigInvalidException {
+ Predicate<ChangeData> predicate = new ProjectPredicate(checker.getRepository().get());
+
+ if (checker.getQuery().isPresent()) {
+ String query = checker.getQuery().get();
+ Predicate<ChangeData> predicateForQuery;
+ try {
+ predicateForQuery = queryBuilder.parse(query);
+ } catch (QueryParseException e) {
+ throw new ConfigInvalidException(
+ String.format("change query of checker %s is invalid: %s", checker.getUuid(), query),
+ e);
+ }
+
+ if (!predicateForQuery.isMatchable()) {
+ // Assuming nobody modified the query behind Gerrit's back, this is programmer error:
+ // CheckerQuery should not be able to produce non-matchable queries.
+ throw new ConfigInvalidException(
+ String.format(
+ "change query of checker %s is non-matchable: %s", checker.getUuid(), query));
+ }
+
+ predicate = Predicate.and(predicate, predicateForQuery);
+ }
+
+ if (!hasStatusPredicate(predicate)) {
+ predicate = Predicate.and(ChangeStatusPredicate.open(), predicate);
+ }
+
+ return predicate;
+ }
+
+ private static boolean hasStatusPredicate(Predicate<ChangeData> predicate) {
+ if (predicate instanceof IndexPredicate) {
+ return ((IndexPredicate<ChangeData>) predicate)
+ .getField()
+ .getName()
+ .equals(ChangeField.STATUS.getName());
+ }
+ return predicate.getChildren().stream().anyMatch(CheckerQuery::hasStatusPredicate);
+ }
+
+ // TODO(ekempin): Retrying the query should be done by ChangeQueryProcessor.
+ private List<ChangeData> executeIndexQueryWithRetry(Predicate<ChangeData> predicate)
+ throws OrmException {
+ try {
+ return retryHelper.execute(
+ ActionType.INDEX_QUERY,
+ () -> changeQueryProcessorProvider.get().query(predicate).entities(),
+ OrmException.class::isInstance);
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ Throwables.throwIfInstanceOf(e, OrmException.class);
+ throw new OrmException(e);
+ }
+ }
}
diff --git a/java/com/google/gerrit/plugins/checks/api/QueryPendingChecks.java b/java/com/google/gerrit/plugins/checks/api/QueryPendingChecks.java
index cededfa..57f7be0 100644
--- a/java/com/google/gerrit/plugins/checks/api/QueryPendingChecks.java
+++ b/java/com/google/gerrit/plugins/checks/api/QueryPendingChecks.java
@@ -28,6 +28,7 @@
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.Checker;
+import com.google.gerrit.plugins.checks.CheckerQuery;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.plugins.checks.Checks;
@@ -38,9 +39,6 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.query.change.ChangeQueryBuilder;
-import com.google.gerrit.server.query.change.ChangeQueryProcessor;
-import com.google.gerrit.server.update.RetryHelper;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -55,9 +53,7 @@
private final CheckQueryBuilder checkQueryBuilder;
private final Checkers checkers;
private final Checks checks;
- private final RetryHelper retryHelper;
- private final Provider<ChangeQueryBuilder> queryBuilderProvider;
- private final Provider<ChangeQueryProcessor> changeQueryProcessorProvider;
+ private final Provider<CheckerQuery> checkerQueryProvider;
private String queryString;
@@ -76,15 +72,11 @@
CheckQueryBuilder checkQueryBuilder,
Checkers checkers,
Checks checks,
- RetryHelper retryHelper,
- Provider<ChangeQueryBuilder> queryBuilderProvider,
- Provider<ChangeQueryProcessor> changeQueryProcessorProvider) {
+ Provider<CheckerQuery> checkerQueryProvider) {
this.checkQueryBuilder = checkQueryBuilder;
this.checkers = checkers;
this.checks = checks;
- this.retryHelper = retryHelper;
- this.queryBuilderProvider = queryBuilderProvider;
- this.changeQueryProcessorProvider = changeQueryProcessorProvider;
+ this.checkerQueryProvider = checkerQueryProvider;
}
public List<PendingChecksInfo> apply()
@@ -111,11 +103,7 @@
// The query system can only match against the current patch set; ignore non-current patch sets
// for now.
- List<ChangeData> changes =
- checker
- .get()
- .queryMatchingChanges(
- retryHelper, queryBuilderProvider.get(), changeQueryProcessorProvider);
+ List<ChangeData> changes = checkerQueryProvider.get().queryMatchingChanges(checker.get());
CheckerUuid checkerUuid = checker.get().getUuid();
List<PendingChecksInfo> pendingChecks = new ArrayList<>(changes.size());
for (ChangeData cd : changes) {
diff --git a/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java b/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
index 2b1a26a..41d3592 100644
--- a/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
+++ b/java/com/google/gerrit/plugins/checks/db/CheckBackfiller.java
@@ -17,13 +17,12 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.Checker;
+import com.google.gerrit.plugins.checks.CheckerQuery;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -36,17 +35,12 @@
@Singleton
class CheckBackfiller {
private final Checkers checkers;
- private final Provider<AnonymousUser> anonymousUserProvider;
- private final Provider<ChangeQueryBuilder> queryBuilderProvider;
+ private final Provider<CheckerQuery> checkerQueryProvider;
@Inject
- CheckBackfiller(
- Checkers checkers,
- Provider<AnonymousUser> anonymousUserProvider,
- Provider<ChangeQueryBuilder> queryBuilderProvider) {
+ CheckBackfiller(Checkers checkers, Provider<CheckerQuery> checkerQueryProvider) {
this.checkers = checkers;
- this.anonymousUserProvider = anonymousUserProvider;
- this.queryBuilderProvider = queryBuilderProvider;
+ this.checkerQueryProvider = checkerQueryProvider;
}
ImmutableList<Check> getBackfilledChecksForRelevantCheckers(
@@ -61,11 +55,11 @@
}
// All candidates need to be checked for relevance. Any relevant checkers are reported as
// NOT_STARTED, with creation time matching the patch set.
+ CheckerQuery checkerQuery = checkerQueryProvider.get();
ImmutableList.Builder<Check> result = ImmutableList.builderWithExpectedSize(candidates.size());
PatchSet ps = cd.patchSet(psId);
- ChangeQueryBuilder queryBuilder = newQueryBuilder();
for (Checker checker : candidates) {
- if (checker.isCheckerRelevant(cd, queryBuilder)) {
+ if (checkerQuery.isCheckerRelevant(checker, cd)) {
// Add synthetic check at the creation time of the patch set.
result.add(Check.newBackfilledCheck(cd.project(), ps, checker));
}
@@ -90,13 +84,9 @@
}
if (!checker.isPresent()
|| checker.get().getStatus() != CheckerStatus.ENABLED
- || !checker.get().isCheckerRelevant(cd, newQueryBuilder())) {
+ || !checkerQueryProvider.get().isCheckerRelevant(checker.get(), cd)) {
return Optional.empty();
}
return Optional.of(Check.newBackfilledCheck(cd.project(), cd.patchSet(psId), checker.get()));
}
-
- private ChangeQueryBuilder newQueryBuilder() {
- return queryBuilderProvider.get().asUser(anonymousUserProvider.get());
- }
}
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
index c73a0f1..8a3f2fd 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
@@ -24,6 +24,7 @@
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.Checker;
+import com.google.gerrit.plugins.checks.CheckerQuery;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.plugins.checks.Checks;
@@ -34,9 +35,7 @@
import com.google.gerrit.reviewdb.client.PatchSet.Id;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.Project.NameKey;
-import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -54,8 +53,7 @@
private final CheckNotes.Factory checkNotesFactory;
private final Checkers checkers;
private final CheckBackfiller checkBackfiller;
- private final Provider<AnonymousUser> anonymousUserProvider;
- private final Provider<ChangeQueryBuilder> queryBuilderProvider;
+ private final Provider<CheckerQuery> checkerQueryProvider;
@Inject
NoteDbChecks(
@@ -63,14 +61,12 @@
CheckNotes.Factory checkNotesFactory,
Checkers checkers,
CheckBackfiller checkBackfiller,
- Provider<AnonymousUser> anonymousUserProvider,
- Provider<ChangeQueryBuilder> queryBuilderProvider) {
+ Provider<CheckerQuery> checkerQueryProvider) {
this.changeDataFactory = changeDataFactory;
this.checkNotesFactory = checkNotesFactory;
this.checkers = checkers;
this.checkBackfiller = checkBackfiller;
- this.anonymousUserProvider = anonymousUserProvider;
- this.queryBuilderProvider = queryBuilderProvider;
+ this.checkerQueryProvider = checkerQueryProvider;
}
@Override
@@ -133,6 +129,7 @@
public CombinedCheckState getCombinedCheckState(NameKey projectName, Id patchSetId)
throws IOException, OrmException {
ChangeData changeData = changeDataFactory.create(projectName, patchSetId.changeId);
+ CheckerQuery checkerQuery = checkerQueryProvider.get();
ImmutableMap<String, Checker> allCheckersOfProject =
checkers.checkersOf(projectName).stream()
.collect(ImmutableMap.toImmutableMap(c -> c.getUuid().get(), c -> c));
@@ -143,8 +140,6 @@
getChecks(projectName, patchSetId, GetCheckOptions.withBackfilling()).stream()
.collect(ImmutableMap.toImmutableMap(c -> c.key().checkerUuid().get(), c -> c));
- ChangeQueryBuilder queryBuilder =
- queryBuilderProvider.get().asUser(anonymousUserProvider.get());
ImmutableListMultimap.Builder<CheckState, Boolean> statesAndRequired =
ImmutableListMultimap.builder();
@@ -162,7 +157,7 @@
boolean isRequired =
checker.getStatus() == CheckerStatus.ENABLED
&& checker.isRequired()
- && checker.isCheckerRelevant(changeData, queryBuilder);
+ && checkerQuery.isCheckerRelevant(checker, changeData);
statesAndRequired.put(check.state(), isRequired);
}
