Fail with ConfigInvalidException if checker config contains invalid blocking conditions
If in a checker config file the blocking conditions contain a value
that cannot be resolved to a BlockingCondition enum we failed with an
IllegalArgumentException, but since this is an invalid config it's
better to throw ConfigInvalidException.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: If1b868d8b1f9288c2b94407a2556c8dce03e9b30
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java
index dc1fe68..34c5ad3 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImpl.java
@@ -42,11 +42,15 @@
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicInteger;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.BlobBasedConfig;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Ref;
@@ -246,6 +250,24 @@
}
}
+ if (testCheckerUpdate.forceInvalidBlockingCondition()) {
+ try (Repository repo = repoManager.openRepository(allProjectsName)) {
+ TestRepository<Repository> testRepo = new TestRepository<>(repo);
+ Config checkerConfig =
+ readConfig(testRepo, checkerUuid.toRefName(), CheckerConfig.CHECKER_CONFIG_FILE);
+ List<String> blocking =
+ new ArrayList<>(
+ Arrays.asList(checkerConfig.getStringList("checker", null, "blocking")));
+ blocking.add("invalid");
+ checkerConfig.setStringList("checker", null, "blocking", blocking);
+ testRepo
+ .branch(checkerUuid.toRefName())
+ .commit()
+ .add(CheckerConfig.CHECKER_CONFIG_FILE, checkerConfig.toText())
+ .create();
+ }
+ }
+
if (testCheckerUpdate.deleteRef()) {
try (Repository repo = repoManager.openRepository(allProjectsName)) {
RefUpdate ru =
@@ -267,5 +289,11 @@
checkerUpdate.query().ifPresent(builder::setQuery);
return builder.build();
}
+
+ private Config readConfig(TestRepository<?> testRepo, String ref, String fileName)
+ throws Exception {
+ Repository repo = testRepo.getRepository();
+ return new BlobBasedConfig(null, repo, repo.resolve(ref), fileName);
+ }
}
}
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerUpdate.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerUpdate.java
index 8de1eb6..76d41c0 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/TestCheckerUpdate.java
@@ -45,6 +45,8 @@
public abstract boolean forceInvalidConfig();
+ public abstract boolean forceInvalidBlockingCondition();
+
public abstract boolean deleteRef();
abstract ThrowingConsumer<TestCheckerUpdate> checkerUpdater();
@@ -53,6 +55,7 @@
return new AutoValue_TestCheckerUpdate.Builder()
.checkerUpdater(checkerUpdater)
.forceInvalidConfig(false)
+ .forceInvalidBlockingCondition(false)
.deleteRef(false);
}
@@ -91,6 +94,12 @@
abstract Builder forceInvalidConfig(boolean forceInvalidConfig);
+ public Builder forceInvalidBlockingCondition() {
+ return forceInvalidBlockingCondition(true);
+ }
+
+ abstract Builder forceInvalidBlockingCondition(boolean forceInvalidBlockingCondition);
+
public Builder deleteRef() {
return deleteRef(true);
}
diff --git a/java/com/google/gerrit/plugins/checks/db/CheckerConfigEntry.java b/java/com/google/gerrit/plugins/checks/db/CheckerConfigEntry.java
index a4bb1ca..d1d49ef 100644
--- a/java/com/google/gerrit/plugins/checks/db/CheckerConfigEntry.java
+++ b/java/com/google/gerrit/plugins/checks/db/CheckerConfigEntry.java
@@ -15,8 +15,6 @@
package com.google.gerrit.plugins.checks.db;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
-import static java.util.Comparator.naturalOrder;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
@@ -31,7 +29,6 @@
import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.reviewdb.client.Project;
import java.text.MessageFormat;
-import java.util.Arrays;
import java.util.Locale;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.internal.JGitText;
@@ -251,7 +248,8 @@
BLOCKING_CONDITIONS("blocking") {
@Override
- void readFromConfig(CheckerUuid checkerUuid, Checker.Builder checker, Config config) {
+ void readFromConfig(CheckerUuid checkerUuid, Checker.Builder checker, Config config)
+ throws ConfigInvalidException {
checker.setBlockingConditions(
getEnumSet(config, SECTION_NAME, null, super.keyName, BlockingCondition.values()));
}
@@ -301,14 +299,18 @@
};
private static <T extends Enum<T>> ImmutableSortedSet<T> getEnumSet(
- Config config, String section, @Nullable String subsection, String name, T[] all) {
- return Arrays.stream(config.getStringList(section, subsection, name))
- .map(v -> resolveEnum(section, subsection, name, v, all))
- .collect(toImmutableSortedSet(naturalOrder()));
+ Config config, String section, @Nullable String subsection, String name, T[] all)
+ throws ConfigInvalidException {
+ ImmutableSortedSet.Builder<T> enumBuilder = ImmutableSortedSet.naturalOrder();
+ for (String value : config.getStringList(section, subsection, name)) {
+ enumBuilder.add(resolveEnum(section, subsection, name, value, all));
+ }
+ return enumBuilder.build();
}
private static <T extends Enum<T>> T resolveEnum(
- String section, @Nullable String subsection, String name, String value, T[] all) {
+ String section, @Nullable String subsection, String name, String value, T[] all)
+ throws ConfigInvalidException {
// Match some resolution semantics of DefaultTypedConfigGetter#getEnum.
// TODO(dborowitz): Sure would be nice if Config exposed this logic (or getEnumList) so we
// didn't have to replicate it.
@@ -319,11 +321,11 @@
}
}
if (subsection != null) {
- throw new IllegalArgumentException(
+ throw new ConfigInvalidException(
MessageFormat.format(
JGitText.get().enumValueNotSupported3, section, subsection, name, value));
}
- throw new IllegalArgumentException(
+ throw new ConfigInvalidException(
MessageFormat.format(JGitText.get().enumValueNotSupported2, section, name, value));
}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
index 32f4649..2cb2ec8 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/GetCheckerIT.java
@@ -212,6 +212,16 @@
}
@Test
+ public void getCheckerWithInvalidBlockingConditionFails() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+ checkerOperations.checker(checkerUuid).forUpdate().forceInvalidBlockingCondition().update();
+
+ exception.expect(RestApiException.class);
+ exception.expectMessage("Cannot retrieve checker " + checkerUuid);
+ getCheckerInfo(checkerUuid);
+ }
+
+ @Test
public void getCheckerByNameFails() throws Exception {
String name = "my-checker";
checkerOperations.newChecker().name(name).create();
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
index 33d5733..ffe26fc 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ListCheckersIT.java
@@ -87,10 +87,15 @@
@Test
public void listIgnoresInvalidCheckers() throws Exception {
- CheckerUuid checkerUuid =
- checkerOperations.newChecker().name("checker-with-name-only").create();
- CheckerUuid invalidCheckerUuid = checkerOperations.newChecker().create();
- checkerOperations.checker(invalidCheckerUuid).forUpdate().forceInvalidConfig().update();
+ CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+ CheckerUuid invalidCheckerUuid1 = checkerOperations.newChecker().create();
+ CheckerUuid invalidCheckerUuid2 = checkerOperations.newChecker().create();
+ checkerOperations.checker(invalidCheckerUuid1).forUpdate().forceInvalidConfig().update();
+ checkerOperations
+ .checker(invalidCheckerUuid2)
+ .forUpdate()
+ .forceInvalidBlockingCondition()
+ .update();
List<CheckerInfo> allCheckers = checkersApi.all();
assertThat(allCheckers).containsExactly(checkerOperations.checker(checkerUuid).asInfo());
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImplTest.java b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImplTest.java
index 4060c82..b282e94 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImplTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckerOperationsImplTest.java
@@ -513,6 +513,21 @@
}
@Test
+ public void blockingConditionsCanBeMadeInvalid() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+
+ checkerOperations.checker(checkerUuid).forUpdate().forceInvalidBlockingCondition().update();
+
+ try {
+ checkers.getChecker(checkerUuid);
+ assert_().fail("expected ConfigInvalidException");
+ } catch (ConfigInvalidException e) {
+ // expected
+ assertThat(e.getMessage()).contains("Invalid value: checker.blocking");
+ }
+ }
+
+ @Test
public void refCanBeDeleted() throws Exception {
CheckerUuid checkerUuid = checkerOperations.newChecker().create();
