Ensure checker query can be executed in the query system
CheckerQuery#clean only checks for allowed operators. It's possible for
a query to consist only of allowed operators but still fail when
executed, for example if it's too long or the backend doesn't support
regex searches. Add a (non-static) validate method which actually runs
the query; this is the only way the index system allows us to really
validate the query. As with clean, this new validate method is only
called when updating the query via the API.
Previously, clean threw BadRequestException; now both clean and validate
throw ConfigInvalidException. From the perspective of CheckerQuery, we
don't know whether the query argument came from an existing checker in
underlying storage, or user input. Of the two exception types,
ConfigInvalidException is more applicable to both situations.
Change-Id: I40c7bfc857c37ffe7fa9fd6411a23b322e4f7de9
diff --git a/java/com/google/gerrit/plugins/checks/CheckerQuery.java b/java/com/google/gerrit/plugins/checks/CheckerQuery.java
index 8a9dc84..5bff722 100644
--- a/java/com/google/gerrit/plugins/checks/CheckerQuery.java
+++ b/java/com/google/gerrit/plugins/checks/CheckerQuery.java
@@ -22,15 +22,17 @@
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.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
-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.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;
@@ -44,6 +46,8 @@
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;
@@ -115,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;
@@ -134,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:
@@ -152,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);
}
}
@@ -196,7 +191,8 @@
Predicate<ChangeData> predicate;
try {
- predicate = createQueryPredicate(checker);
+ predicate =
+ createQueryPredicate(checker.getUuid(), checker.getRepository(), checker.getQuery());
} catch (ConfigInvalidException e) {
logger.atWarning().withCause(e).log(
"skipping invalid query for checker %s", checker.getUuid());
@@ -206,35 +202,78 @@
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(createQueryPredicate(checker));
+ return executeIndexQueryWithRetry(
+ queryProcessorSetup, createQueryPredicate(checkerUuid, repository, optionalQuery));
} catch (QueryParseException e) {
- throw invalidQueryException(checker, e);
+ throw invalidQueryException(checkerUuid, optionalQuery, e);
}
}
- private Predicate<ChangeData> createQueryPredicate(Checker checker)
+ private Predicate<ChangeData> createQueryPredicate(
+ CheckerUuid checkerUuid, Project.NameKey repository, Optional<String> optionalQuery)
throws ConfigInvalidException {
- Predicate<ChangeData> predicate = new ProjectPredicate(checker.getRepository().get());
+ Predicate<ChangeData> predicate = new ProjectPredicate(repository.get());
- if (checker.getQuery().isPresent()) {
- String query = checker.getQuery().get();
+ if (optionalQuery.isPresent()) {
+ String query = optionalQuery.get();
Predicate<ChangeData> predicateForQuery;
try {
predicateForQuery = queryBuilder.parse(query);
} catch (QueryParseException e) {
- throw invalidQueryException(checker, 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",
- checker.getUuid(), checker.getQuery().get());
- throw invalidQueryException(checker, null);
+ "change query of checker %s is not matchable: %s", checkerUuid, optionalQuery.get());
+ throw invalidQueryException(checkerUuid, optionalQuery, null);
}
predicate = Predicate.and(predicate, predicateForQuery);
@@ -258,12 +297,17 @@
}
// TODO(ekempin): Retrying the query should be done by ChangeQueryProcessor.
- private List<ChangeData> executeIndexQueryWithRetry(Predicate<ChangeData> predicate)
+ private List<ChangeData> executeIndexQueryWithRetry(
+ Consumer<ChangeQueryProcessor> queryProcessorSetup, Predicate<ChangeData> predicate)
throws OrmException, QueryParseException {
try {
return retryHelper.execute(
ActionType.INDEX_QUERY,
- () -> changeQueryProcessorProvider.get().query(predicate).entities(),
+ () -> {
+ ChangeQueryProcessor qp = changeQueryProcessorProvider.get();
+ queryProcessorSetup.accept(qp);
+ return qp.query(predicate).entities();
+ },
OrmException.class::isInstance);
} catch (Exception e) {
Throwables.throwIfUnchecked(e);
@@ -274,11 +318,12 @@
}
private static ConfigInvalidException invalidQueryException(
- Checker checker, @Nullable QueryParseException parseException) {
+ CheckerUuid checkerUuid,
+ Optional<String> optionalQuery,
+ @Nullable QueryParseException parseException) {
String msg =
String.format(
- "change query of checker %s is invalid: %s",
- checker.getUuid(), checker.getQuery().orElse(""));
+ "change query of checker %s is invalid: %s", checkerUuid, optionalQuery.orElse(""));
if (parseException != null) {
msg += " (" + parseException.getMessage() + ")";
}
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/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/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..8aa98b8 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,19 @@
@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 +520,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.
}
