Merge "CheckOperationsImpl#notesAsText: Don't create 2 ObjectReaders"
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..5bff722 100644
--- a/java/com/google/gerrit/plugins/checks/CheckerQuery.java
+++ b/java/com/google/gerrit/plugins/checks/CheckerQuery.java
@@ -22,13 +22,44 @@
import static com.google.gerrit.index.query.QueryParser.OR;
import static java.util.Objects.requireNonNull;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Strings;
+import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableSortedSet;
-import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
+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.reviewdb.client.Project;
+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 java.util.Optional;
+import java.util.function.Consumer;
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.
//
@@ -88,18 +119,8 @@
"unresolved",
"wip");
- /**
- * Cleans a query string for storage in the checker configuration.
- *
- * <p>The query string is interpreted as a change query. Only a subset of query operators are
- * supported, as listed in the REST API documentation and {@link #ALLOWED_OPERATORS}.
- *
- * @param query a change query string.
- * @return the query string, trimmed. May be empty, which indicates all changes match.
- * @throws BadRequestException if the query is not a valid query, or it uses operators outside of
- * the allowed set.
- */
- public static String clean(String query) throws BadRequestException {
+ @VisibleForTesting
+ public static String clean(String query) throws ConfigInvalidException {
String trimmed = requireNonNull(query).trim();
if (trimmed.isEmpty()) {
return trimmed;
@@ -107,13 +128,13 @@
try {
checkOperators(QueryParser.parse(query));
} catch (QueryParseException e) {
- throw new BadRequestException("Invalid query: " + query + "\n" + e.getMessage(), e);
+ throw new ConfigInvalidException("Invalid query: " + query + "\n" + e.getMessage(), e);
}
return trimmed;
}
- private static void checkOperators(Tree node) throws BadRequestException {
+ private static void checkOperators(Tree node) throws ConfigInvalidException {
switch (node.getType()) {
case AND:
case OR:
@@ -125,15 +146,16 @@
case FIELD_NAME:
if (!ALLOWED_OPERATORS.contains(node.getText())) {
- throw new BadRequestException("Unsupported operator: " + node);
+ throw new ConfigInvalidException("Unsupported operator: " + node);
}
break;
case DEFAULT_FIELD:
- throw new BadRequestException("Specific search operator required: " + getOnlyChild(node));
+ throw new ConfigInvalidException(
+ "Specific search operator required: " + getOnlyChild(node));
default:
- throw new BadRequestException("Unsupported query: " + node);
+ throw new ConfigInvalidException("Unsupported query: " + node);
}
}
@@ -142,5 +164,169 @@
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.getUuid(), checker.getRepository(), checker.getQuery());
+ } catch (ConfigInvalidException e) {
+ logger.atWarning().withCause(e).log(
+ "skipping invalid query for checker %s", checker.getUuid());
+ return false;
+ }
+
+ return predicate.asMatchable().match(cd);
+ }
+
+ /**
+ * Cleans and validates a query string for storage in the checker configuration.
+ *
+ * <p>The query string is interpreted as a change query. Only a subset of query operators are
+ * supported, as listed in the REST API documentation and {@link #ALLOWED_OPERATORS}.
+ *
+ * <p>In addition to syntactic validation and checking for allowed operators, this method actually
+ * performs a query against the index, to ensure it passes any restrictions imposed by the index
+ * implementation, such as length limits.
+ *
+ * @param checkerUuid the checker UUID.
+ * @param repository the checker repository.
+ * @param query a change query string, either from the checker in storage or a proposed new value
+ * provided by a user.
+ * @return the query string, trimmed. May be empty, which indicates all changes match.
+ * @throws ConfigInvalidException if the query is not a valid query, or it uses operators outside
+ * of the allowed set.
+ */
+ public String validate(CheckerUuid checkerUuid, Project.NameKey repository, String query)
+ throws ConfigInvalidException, OrmException {
+ // This parses the query string twice, which is unavoidable since there is currently no
+ // QueryProcessor API which takes an Antlr Tree. That's ok; the parse cost is vastly outweighed
+ // by the actual query execution.
+ query = clean(query);
+ queryMatchingChanges(
+ checkerUuid,
+ repository,
+ Optional.ofNullable(Strings.emptyToNull(query)),
+ qp -> qp.setUserProvidedLimit(1));
+ return query;
+ }
+
+ public List<ChangeData> queryMatchingChanges(Checker checker)
+ throws ConfigInvalidException, OrmException {
+ return queryMatchingChanges(
+ checker.getUuid(), checker.getRepository(), checker.getQuery(), qp -> {});
+ }
+
+ private List<ChangeData> queryMatchingChanges(
+ CheckerUuid checkerUuid,
+ Project.NameKey repository,
+ Optional<String> optionalQuery,
+ Consumer<ChangeQueryProcessor> queryProcessorSetup)
+ throws ConfigInvalidException, OrmException {
+ try {
+ return executeIndexQueryWithRetry(
+ queryProcessorSetup, createQueryPredicate(checkerUuid, repository, optionalQuery));
+ } catch (QueryParseException e) {
+ throw invalidQueryException(checkerUuid, optionalQuery, e);
+ }
+ }
+
+ private Predicate<ChangeData> createQueryPredicate(
+ CheckerUuid checkerUuid, Project.NameKey repository, Optional<String> optionalQuery)
+ throws ConfigInvalidException {
+ Predicate<ChangeData> predicate = new ProjectPredicate(repository.get());
+
+ if (optionalQuery.isPresent()) {
+ String query = optionalQuery.get();
+ Predicate<ChangeData> predicateForQuery;
+ try {
+ predicateForQuery = queryBuilder.parse(query);
+ } catch (QueryParseException e) {
+ throw invalidQueryException(checkerUuid, optionalQuery, 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.
+ logger.atWarning().log(
+ "change query of checker %s is not matchable: %s", checkerUuid, optionalQuery.get());
+ throw invalidQueryException(checkerUuid, optionalQuery, null);
+ }
+
+ 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(
+ Consumer<ChangeQueryProcessor> queryProcessorSetup, Predicate<ChangeData> predicate)
+ throws OrmException, QueryParseException {
+ try {
+ return retryHelper.execute(
+ ActionType.INDEX_QUERY,
+ () -> {
+ ChangeQueryProcessor qp = changeQueryProcessorProvider.get();
+ queryProcessorSetup.accept(qp);
+ return qp.query(predicate).entities();
+ },
+ OrmException.class::isInstance);
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ Throwables.throwIfInstanceOf(e, QueryParseException.class);
+ Throwables.throwIfInstanceOf(e, OrmException.class);
+ throw new OrmException(e);
+ }
+ }
+
+ private static ConfigInvalidException invalidQueryException(
+ CheckerUuid checkerUuid,
+ Optional<String> optionalQuery,
+ @Nullable QueryParseException parseException) {
+ String msg =
+ String.format(
+ "change query of checker %s is invalid: %s", checkerUuid, optionalQuery.orElse(""));
+ if (parseException != null) {
+ msg += " (" + parseException.getMessage() + ")";
+ }
+ return new ConfigInvalidException(msg, parseException);
+ }
}
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerTestData.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerTestData.java
index 44883e2..9fd05a0 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerTestData.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerTestData.java
@@ -14,6 +14,10 @@
package com.google.gerrit.plugins.checks.acceptance.testsuite;
+import static java.util.stream.Collectors.joining;
+
+import java.util.stream.Stream;
+
public final class CheckerTestData {
/** An invalid checker UUID. */
public static final String INVALID_UUID = "notauuid";
@@ -38,5 +42,10 @@
/** Query that is not parsable. */
public static final String INVALID_QUERY = ":foo :bar";
+ /** Query with {@code numTerms} total operators, all of which are supported in checker queries. */
+ public static String longQueryWithSupportedOperators(int numTerms) {
+ return Stream.generate(() -> "file:foo").limit(numTerms).collect(joining(" "));
+ }
+
private CheckerTestData() {}
}
diff --git a/java/com/google/gerrit/plugins/checks/api/CreateChecker.java b/java/com/google/gerrit/plugins/checks/api/CreateChecker.java
index 7bf61af..a935f14 100644
--- a/java/com/google/gerrit/plugins/checks/api/CreateChecker.java
+++ b/java/com/google/gerrit/plugins/checks/api/CreateChecker.java
@@ -42,6 +42,7 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gwtorm.server.OrmDuplicateKeyException;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@@ -53,6 +54,7 @@
implements RestCollectionModifyView<TopLevelResource, CheckerResource, CheckerInput> {
private final Provider<CurrentUser> self;
private final PermissionBackend permissionBackend;
+ private final Provider<CheckerQuery> checkerQueryProvider;
private final Provider<CheckersUpdate> checkersUpdate;
private final CheckerJson checkerJson;
private final AdministrateCheckersPermission permission;
@@ -62,12 +64,14 @@
public CreateChecker(
Provider<CurrentUser> self,
PermissionBackend permissionBackend,
+ Provider<CheckerQuery> checkerQueryProvider,
@UserInitiated Provider<CheckersUpdate> checkersUpdate,
CheckerJson checkerJson,
AdministrateCheckersPermission permission,
ProjectCache projectCache) {
this.self = self;
this.permissionBackend = permissionBackend;
+ this.checkerQueryProvider = checkerQueryProvider;
this.checkersUpdate = checkersUpdate;
this.checkerJson = checkerJson;
this.permission = permission;
@@ -76,7 +80,8 @@
@Override
public Response<CheckerInfo> apply(TopLevelResource parentResource, CheckerInput input)
- throws RestApiException, PermissionBackendException, IOException, ConfigInvalidException {
+ throws RestApiException, PermissionBackendException, IOException, ConfigInvalidException,
+ OrmException {
if (!self.get().isIdentifiedUser()) {
throw new AuthException("Authentication required");
}
@@ -116,7 +121,7 @@
checkerUpdateBuilder.setBlockingConditions(ImmutableSortedSet.copyOf(input.blocking));
}
if (input.query != null) {
- checkerUpdateBuilder.setQuery(CheckerQuery.clean(input.query));
+ checkerUpdateBuilder.setQuery(validateQuery(checkerUuid, repository, input.query));
}
try {
Checker checker =
@@ -142,4 +147,13 @@
return projectState.getNameKey();
}
+
+ private String validateQuery(CheckerUuid checkerUuid, Project.NameKey repository, String query)
+ throws BadRequestException, OrmException {
+ try {
+ return checkerQueryProvider.get().validate(checkerUuid, repository, query);
+ } catch (ConfigInvalidException e) {
+ throw new BadRequestException(e.getMessage(), 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..fb7a03a 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,19 +72,16 @@
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()
- throws RestApiException, IOException, ConfigInvalidException, OrmException {
+ throws RestApiException, IOException, ConfigInvalidException, OrmException,
+ QueryParseException {
return apply(TopLevelResource.INSTANCE);
}
@@ -111,11 +104,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) {
@@ -158,7 +147,7 @@
// if the root predicate is an AndPredicate, any of its direct children must be a
// CheckerPredicate, the other child predicates can be anything (including any combination of
// AndPredicate, OrPredicate and NotPredicate).
- if (!predicate.getChildren().stream().anyMatch(CheckerPredicate.class::isInstance)) {
+ if (predicate.getChildren().stream().noneMatch(CheckerPredicate.class::isInstance)) {
throw new BadRequestException(
String.format(
"query must be '%s:<checker-uuid>' or '%s:<checker-uuid> AND <other-operators>'",
diff --git a/java/com/google/gerrit/plugins/checks/api/UpdateChecker.java b/java/com/google/gerrit/plugins/checks/api/UpdateChecker.java
index 362fde8..9c3158b 100644
--- a/java/com/google/gerrit/plugins/checks/api/UpdateChecker.java
+++ b/java/com/google/gerrit/plugins/checks/api/UpdateChecker.java
@@ -26,6 +26,7 @@
import com.google.gerrit.plugins.checks.CheckerName;
import com.google.gerrit.plugins.checks.CheckerQuery;
import com.google.gerrit.plugins.checks.CheckerUpdate;
+import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.CheckersUpdate;
import com.google.gerrit.plugins.checks.NoSuchCheckerException;
import com.google.gerrit.plugins.checks.UrlValidator;
@@ -35,6 +36,7 @@
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
@@ -43,38 +45,41 @@
@Singleton
public class UpdateChecker implements RestModifyView<CheckerResource, CheckerInput> {
- private final PermissionBackend permissionBackend;
- private final Provider<CheckersUpdate> checkersUpdate;
private final CheckerJson checkerJson;
+ private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
-
+ private final Provider<CheckerQuery> checkerQueryProvider;
+ private final Provider<CheckersUpdate> checkersUpdate;
private final AdministrateCheckersPermission permission;
@Inject
public UpdateChecker(
- PermissionBackend permissionBackend,
- @UserInitiated Provider<CheckersUpdate> checkersUpdate,
CheckerJson checkerJson,
- AdministrateCheckersPermission permission,
- ProjectCache projectCache) {
- this.permissionBackend = permissionBackend;
- this.checkersUpdate = checkersUpdate;
+ ProjectCache projectCache,
+ PermissionBackend permissionBackend,
+ Provider<CheckerQuery> checkerQueryProvider,
+ @UserInitiated Provider<CheckersUpdate> checkersUpdate,
+ AdministrateCheckersPermission permission) {
this.checkerJson = checkerJson;
- this.permission = permission;
this.projectCache = projectCache;
+ this.permissionBackend = permissionBackend;
+ this.checkerQueryProvider = checkerQueryProvider;
+ this.checkersUpdate = checkersUpdate;
+ this.permission = permission;
}
@Override
public CheckerInfo apply(CheckerResource resource, CheckerInput input)
throws RestApiException, PermissionBackendException, NoSuchCheckerException, IOException,
- ConfigInvalidException {
+ ConfigInvalidException, OrmException {
permissionBackend.currentUser().check(permission);
+ CheckerUuid checkerUuid = resource.getChecker().getUuid();
CheckerUpdate.Builder checkerUpdateBuilder = CheckerUpdate.builder();
// Callers shouldn't really be providing UUID. If they do, the only legal UUID is exactly the
// current UUID.
- if (input.uuid != null && !input.uuid.equals(resource.getChecker().getUuid().get())) {
+ if (input.uuid != null && !input.uuid.equals(checkerUuid.get())) {
throw new BadRequestException("uuid cannot be updated");
}
@@ -94,9 +99,12 @@
checkerUpdateBuilder.setUrl(UrlValidator.clean(input.url));
}
+ Project.NameKey repository;
if (input.repository != null) {
- Project.NameKey repository = resolveRepository(input.repository);
+ repository = resolveRepository(input.repository);
checkerUpdateBuilder.setRepository(repository);
+ } else {
+ repository = resource.getChecker().getRepository();
}
if (input.status != null) {
@@ -108,13 +116,11 @@
}
if (input.query != null) {
- checkerUpdateBuilder.setQuery(CheckerQuery.clean(input.query));
+ checkerUpdateBuilder.setQuery(validateQuery(checkerUuid, repository, input.query));
}
Checker updatedChecker =
- checkersUpdate
- .get()
- .updateChecker(resource.getChecker().getUuid(), checkerUpdateBuilder.build());
+ checkersUpdate.get().updateChecker(checkerUuid, checkerUpdateBuilder.build());
return checkerJson.format(updatedChecker);
}
@@ -131,4 +137,13 @@
return projectState.getNameKey();
}
+
+ private String validateQuery(CheckerUuid checkerUuid, Project.NameKey repository, String query)
+ throws BadRequestException, OrmException {
+ try {
+ return checkerQueryProvider.get().validate(checkerUuid, repository, query);
+ } catch (ConfigInvalidException e) {
+ throw new BadRequestException(e.getMessage(), e);
+ }
+ }
}
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);
}
diff --git a/javatests/com/google/gerrit/plugins/checks/CheckerQueryTest.java b/javatests/com/google/gerrit/plugins/checks/CheckerQueryTest.java
index 41ce8ee..2ba3061 100644
--- a/javatests/com/google/gerrit/plugins/checks/CheckerQueryTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/CheckerQueryTest.java
@@ -17,12 +17,12 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assert_;
-import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gerrit.testing.GerritBaseTests;
import com.google.gerrit.testing.InMemoryModule;
import com.google.inject.Guice;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.junit.Test;
public class CheckerQueryTest extends GerritBaseTests {
@@ -107,8 +107,8 @@
private static void assertInvalidQuery(String query, String expectedMessage) {
try {
CheckerQuery.clean(query);
- assert_().fail("expected BadRequestException");
- } catch (BadRequestException e) {
+ assert_().fail("expected ConfigInvalidException");
+ } catch (ConfigInvalidException e) {
assertThat(e).hasMessageThat().isEqualTo(expectedMessage);
}
}
@@ -116,7 +116,7 @@
private static void assertValidQuery(String query) {
try {
assertThat(CheckerQuery.clean(query)).isEqualTo(query.trim());
- } catch (BadRequestException e) {
+ } catch (ConfigInvalidException e) {
throw new AssertionError("expected valid query: " + query, e);
}
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
index 1733f57..3a69967 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckerIT.java
@@ -25,6 +25,7 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.plugins.checks.CheckerQuery;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
import com.google.gerrit.plugins.checks.acceptance.testsuite.CheckerOperations.PerCheckerOperations;
@@ -35,19 +36,30 @@
import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.plugins.checks.db.CheckersByRepositoryNotes;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.testing.TestTimeUtil;
import com.google.inject.Inject;
import java.sql.Timestamp;
import java.time.Instant;
import java.util.concurrent.TimeUnit;
+import org.eclipse.jgit.lib.Config;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
public class CreateCheckerIT extends AbstractCheckersTest {
+ private static final int MAX_INDEX_TERMS = 10;
+
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ProjectOperations projectOperations;
+ @ConfigSuite.Default
+ public static Config defaultConfig() {
+ Config cfg = new Config();
+ cfg.setInt("index", null, "maxTerms", MAX_INDEX_TERMS);
+ return cfg;
+ }
+
@Before
public void setTimeForTesting() {
TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
@@ -398,6 +410,28 @@
}
@Test
+ public void createCheckerWithTooLongQueryFails() throws Exception {
+ CheckerInput input = new CheckerInput();
+ input.uuid = "test:my-checker";
+ input.repository = allProjects.get();
+ input.query = CheckerTestData.longQueryWithSupportedOperators(MAX_INDEX_TERMS * 2);
+ assertThat(CheckerQuery.clean(input.query)).isEqualTo(input.query);
+ try {
+ checkersApi.create(input).get();
+ assert_().fail("expected BadRequestException");
+ } catch (BadRequestException e) {
+ assertThat(e)
+ .hasMessageThat()
+ .isEqualTo(
+ "change query of checker "
+ + input.uuid
+ + " is invalid: "
+ + input.query
+ + " (too many terms in query)");
+ }
+ }
+
+ @Test
public void createMultipleCheckers() throws Exception {
Project.NameKey repositoryName1 = projectOperations.newProject().create();
Project.NameKey repositoryName2 = projectOperations.newProject().create();
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java
index a0f2663..2809f02 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckerIT.java
@@ -27,6 +27,7 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
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.acceptance.AbstractCheckersTest;
import com.google.gerrit.plugins.checks.acceptance.testsuite.CheckerOperations.PerCheckerOperations;
@@ -51,16 +52,18 @@
@SkipProjectClone
public class UpdateCheckerIT extends AbstractCheckersTest {
- @Inject private RequestScopeOperations requestScopeOperations;
- @Inject private ProjectOperations projectOperations;
+ private static final int MAX_INDEX_TERMS = 10;
@ConfigSuite.Default
public static Config defaultConfig() {
Config cfg = new Config();
- cfg.setBoolean("checks", "api", "enabled", true);
+ cfg.setInt("index", null, "maxTerms", MAX_INDEX_TERMS);
return cfg;
}
+ @Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private ProjectOperations projectOperations;
+
@Before
public void setTimeForTesting() {
TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
@@ -516,6 +519,31 @@
}
@Test
+ public void updateWithTooLongQuery() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+ Optional<String> oldQuery = checkerOperations.checker(checkerUuid).get().getQuery();
+
+ CheckerInput input = new CheckerInput();
+ input.query = CheckerTestData.longQueryWithSupportedOperators(MAX_INDEX_TERMS * 2);
+ assertThat(CheckerQuery.clean(input.query)).isEqualTo(input.query);
+ try {
+ checkersApi.id(checkerUuid).update(input);
+ assert_().fail("expected BadRequestException");
+ } catch (BadRequestException e) {
+ assertThat(e)
+ .hasMessageThat()
+ .isEqualTo(
+ "change query of checker "
+ + checkerUuid
+ + " is invalid: "
+ + input.query
+ + " (too many terms in query)");
+ }
+
+ assertThat(checkerOperations.checker(checkerUuid).get().getQuery()).isEqualTo(oldQuery);
+ }
+
+ @Test
public void updateResultsInNewUpdatedTimestamp() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().create();
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerTestDataTest.java b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerTestDataTest.java
index f4761fe..37b22e0 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerTestDataTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerTestDataTest.java
@@ -22,6 +22,7 @@
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.UrlValidator;
import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.junit.Test;
public class CheckerTestDataTest extends AbstractCheckersTest {
@@ -49,11 +50,18 @@
assertInvalidQuery(CheckerTestData.INVALID_QUERY, "invalid", CheckerTestData.INVALID_QUERY);
}
+ @Test
+ public void verifyTooLongQuery() throws Exception {
+ String query = CheckerTestData.longQueryWithSupportedOperators(5);
+ assertThat(query).isEqualTo("file:foo file:foo file:foo file:foo file:foo");
+ CheckerQuery.clean(query);
+ }
+
private static void assertInvalidQuery(String query, String... expectedMessageParts) {
try {
CheckerQuery.clean(query);
- assert_().fail("expected BadRequestException");
- } catch (BadRequestException e) {
+ assert_().fail("expected ConfigInvalidException");
+ } catch (ConfigInvalidException e) {
assertMessage(e, expectedMessageParts);
}
}
diff --git a/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java b/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java
index 05ba663..dece6af 100644
--- a/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/db/CheckerConfigTest.java
@@ -21,7 +21,6 @@
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.truth.StringSubject;
-import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.plugins.checks.CheckerCreation;
import com.google.gerrit.plugins.checks.CheckerQuery;
import com.google.gerrit.plugins.checks.CheckerUpdate;
@@ -461,8 +460,8 @@
String query = "foo:bar";
try {
CheckerQuery.clean(query);
- assert_().fail("expected BadRequestException");
- } catch (BadRequestException e) {
+ assert_().fail("expected ConfigInvalidException");
+ } catch (ConfigInvalidException e) {
// Expected.
}
