Handle invalid values for boolean config parameters gracefully
If an invalid value for a boolean config parameter is set we should fall
back to the parent config or default value.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I47e93d69b16c9235471cec6a2e4d1462737bcbb2
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java
index 8b9407f..a9b5ea6 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfiguration.java
@@ -114,7 +114,7 @@
*/
public boolean areCodeOwnerConfigsReadOnly(Project.NameKey project) {
requireNonNull(project, "project");
- return generalConfig.getReadOnly(getPluginConfig(project));
+ return generalConfig.getReadOnly(project, getPluginConfig(project));
}
/**
@@ -126,7 +126,7 @@
*/
public boolean arePureRevertsExempted(Project.NameKey project) {
requireNonNull(project, "project");
- return generalConfig.getExemptPureReverts(getPluginConfig(project));
+ return generalConfig.getExemptPureReverts(project, getPluginConfig(project));
}
/**
@@ -140,7 +140,7 @@
*/
public boolean rejectNonResolvableCodeOwners(Project.NameKey project) {
requireNonNull(project, "project");
- return generalConfig.getRejectNonResolvableCodeOwners(getPluginConfig(project));
+ return generalConfig.getRejectNonResolvableCodeOwners(project, getPluginConfig(project));
}
/**
@@ -154,7 +154,7 @@
*/
public boolean rejectNonResolvableImports(Project.NameKey project) {
requireNonNull(project, "project");
- return generalConfig.getRejectNonResolvableImports(getPluginConfig(project));
+ return generalConfig.getRejectNonResolvableImports(project, getPluginConfig(project));
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
index 75b3637..28c8456 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
@@ -202,11 +202,12 @@
* <p>The read-only configuration controls whether code owner config files are read-only and all
* modifications of code owner config files should be rejected.
*
+ * @param project the project for which the read-only configuration should be read
* @param pluginConfig the plugin config from which the read-only configuration should be read.
* @return whether code owner config files are read-only
*/
- boolean getReadOnly(Config pluginConfig) {
- return getBooleanConfig(pluginConfig, KEY_READ_ONLY, /* defaultValue= */ false);
+ boolean getReadOnly(Project.NameKey project, Config pluginConfig) {
+ return getBooleanConfig(project, pluginConfig, KEY_READ_ONLY, /* defaultValue= */ false);
}
/**
@@ -216,11 +217,13 @@
* <p>The exempt-pure-reverts configuration controls whether pure revert changes are exempted from
* needing code owner approvals for submit.
*
+ * @param project the project for which the exempt-pure-revert configuration should be read
* @param pluginConfig the plugin config from which the read-only configuration should be read.
* @return whether pure reverts are exempted from needing code owner approvals for submit
*/
- boolean getExemptPureReverts(Config pluginConfig) {
- return getBooleanConfig(pluginConfig, KEY_EXEMPT_PURE_REVERTS, /* defaultValue= */ false);
+ boolean getExemptPureReverts(Project.NameKey project, Config pluginConfig) {
+ return getBooleanConfig(
+ project, pluginConfig, KEY_EXEMPT_PURE_REVERTS, /* defaultValue= */ false);
}
/**
@@ -231,14 +234,16 @@
* with newly added non-resolvable code owners should be rejected on commit received and on
* submit.
*
+ * @param project the project for which the freject-non-resolvable-code-owners configuration
+ * should be read
* @param pluginConfig the plugin config from which the reject-non-resolvable-code-owners
* configuration should be read.
* @return whether code owner config files with newly added non-resolvable code owners should be
* rejected on commit received and on submit
*/
- boolean getRejectNonResolvableCodeOwners(Config pluginConfig) {
+ boolean getRejectNonResolvableCodeOwners(Project.NameKey project, Config pluginConfig) {
return getBooleanConfig(
- pluginConfig, KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS, /* defaultValue= */ true);
+ project, pluginConfig, KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS, /* defaultValue= */ true);
}
/**
@@ -248,26 +253,46 @@
* <p>The reject-non-resolvable-imports configuration controls whether code owner config files
* with newly added non-resolvable imports should be rejected on commit received and on submit.
*
+ * @param project the project for which the freject-non-resolvable-imports configuration should be
+ * read
* @param pluginConfig the plugin config from which the reject-non-resolvable-imports
* configuration should be read.
* @return whether code owner config files with newly added non-resolvable imports should be
* rejected on commit received and on submit
*/
- boolean getRejectNonResolvableImports(Config pluginConfig) {
+ boolean getRejectNonResolvableImports(Project.NameKey project, Config pluginConfig) {
return getBooleanConfig(
- pluginConfig, KEY_REJECT_NON_RESOLVABLE_IMPORTS, /* defaultValue= */ true);
+ project, pluginConfig, KEY_REJECT_NON_RESOLVABLE_IMPORTS, /* defaultValue= */ true);
}
- private boolean getBooleanConfig(Config pluginConfig, String key, boolean defaultValue) {
+ private boolean getBooleanConfig(
+ Project.NameKey project, Config pluginConfig, String key, boolean defaultValue) {
+ requireNonNull(project, "project");
requireNonNull(pluginConfig, "pluginConfig");
requireNonNull(key, "key");
- if (pluginConfig.getString(SECTION_CODE_OWNERS, /* subsection= */ null, key) != null) {
- return pluginConfig.getBoolean(
- SECTION_CODE_OWNERS, /* subsection= */ null, key, defaultValue);
+ String value = pluginConfig.getString(SECTION_CODE_OWNERS, /* subsection= */ null, key);
+ if (value != null) {
+ try {
+ return pluginConfig.getBoolean(
+ SECTION_CODE_OWNERS, /* subsection= */ null, key, defaultValue);
+ } catch (IllegalArgumentException e) {
+ logger.atWarning().withCause(e).log(
+ "Ignoring invalid value %s for %s in '%s.config' of project %s."
+ + " Falling back to global config.",
+ value, key, pluginName, project.get());
+ }
}
- return pluginConfigFromGerritConfig.getBoolean(key, defaultValue);
+ try {
+ return pluginConfigFromGerritConfig.getBoolean(key, defaultValue);
+ } catch (IllegalArgumentException e) {
+ logger.atWarning().withCause(e).log(
+ "Ignoring invalid value %s for %s in gerrit.config (parameter"
+ + " plugin.%s.%s). Falling back to default value %s.",
+ pluginConfigFromGerritConfig.getString(key), key, pluginName, key, defaultValue);
+ return defaultValue;
+ }
}
/**
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
index 4e43414..b5716ee 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
@@ -107,23 +107,33 @@
}
@Test
+ public void cannotGetReadOnlyForNullProject() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> generalConfig.getReadOnly(/* project= */ null, new Config()));
+ assertThat(npe).hasMessageThat().isEqualTo("project");
+ }
+
+ @Test
public void cannotGetReadOnlyForNullPluginConfig() throws Exception {
NullPointerException npe =
assertThrows(
- NullPointerException.class, () -> generalConfig.getReadOnly(/* pluginConfig= */ null));
+ NullPointerException.class,
+ () -> generalConfig.getReadOnly(project, /* pluginConfig= */ null));
assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
}
@Test
public void noReadOnlyConfiguration() throws Exception {
- assertThat(generalConfig.getReadOnly(new Config())).isFalse();
+ assertThat(generalConfig.getReadOnly(project, new Config())).isFalse();
}
@Test
@GerritConfig(name = "plugin.code-owners.readOnly", value = "true")
public void readOnlyConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
throws Exception {
- assertThat(generalConfig.getReadOnly(new Config())).isTrue();
+ assertThat(generalConfig.getReadOnly(project, new Config())).isTrue();
}
@Test
@@ -132,7 +142,30 @@
throws Exception {
Config cfg = new Config();
cfg.setString(SECTION_CODE_OWNERS, /* subsection= */ null, KEY_READ_ONLY, "false");
- assertThat(generalConfig.getReadOnly(cfg)).isFalse();
+ assertThat(generalConfig.getReadOnly(project, cfg)).isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.readOnly", value = "true")
+ public void invalidReadOnlyConfigurationInPluginConfigIsIgnored() throws Exception {
+ Config cfg = new Config();
+ cfg.setString(SECTION_CODE_OWNERS, /* subsection= */ null, KEY_READ_ONLY, "INVALID");
+ assertThat(generalConfig.getReadOnly(project, cfg)).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.readOnly", value = "INVALID")
+ public void invalidReadOnlyConfigurationInGerritConfigIsIgnored() throws Exception {
+ assertThat(generalConfig.getReadOnly(project, new Config())).isFalse();
+ }
+
+ @Test
+ public void cannotGetExemptPureRevertsForNullProject() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> generalConfig.getExemptPureReverts(/* project= */ null, new Config()));
+ assertThat(npe).hasMessageThat().isEqualTo("project");
}
@Test
@@ -140,13 +173,13 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> generalConfig.getExemptPureReverts(/* pluginConfig= */ null));
+ () -> generalConfig.getExemptPureReverts(project, /* pluginConfig= */ null));
assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
}
@Test
public void noExemptPureRevertsConfiguration() throws Exception {
- assertThat(generalConfig.getExemptPureReverts(new Config())).isFalse();
+ assertThat(generalConfig.getExemptPureReverts(project, new Config())).isFalse();
}
@Test
@@ -154,7 +187,7 @@
public void
exemptPureRevertsConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
throws Exception {
- assertThat(generalConfig.getExemptPureReverts(new Config())).isTrue();
+ assertThat(generalConfig.getExemptPureReverts(project, new Config())).isTrue();
}
@Test
@@ -164,7 +197,31 @@
throws Exception {
Config cfg = new Config();
cfg.setString(SECTION_CODE_OWNERS, null, KEY_EXEMPT_PURE_REVERTS, "false");
- assertThat(generalConfig.getExemptPureReverts(cfg)).isFalse();
+ assertThat(generalConfig.getExemptPureReverts(project, cfg)).isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.exemptPureReverts", value = "true")
+ public void invalidExemptPureRevertsInPluginConfigIsIgnored() throws Exception {
+ Config cfg = new Config();
+ cfg.setString(SECTION_CODE_OWNERS, /* subsection= */ null, KEY_EXEMPT_PURE_REVERTS, "INVALID");
+ assertThat(generalConfig.getExemptPureReverts(project, cfg)).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.exemptPureReverts", value = "INVALID")
+ public void invalidExemptPureRevertsConfigurationInGerritConfigIsIgnored() throws Exception {
+ assertThat(generalConfig.getExemptPureReverts(project, new Config())).isFalse();
+ }
+
+ @Test
+ public void cannotGetRejectNonResolvableCodeOwnersForNullProject() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () ->
+ generalConfig.getRejectNonResolvableCodeOwners(/* project= */ null, new Config()));
+ assertThat(npe).hasMessageThat().isEqualTo("project");
}
@Test
@@ -172,13 +229,14 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> generalConfig.getRejectNonResolvableCodeOwners(/* pluginConfig= */ null));
+ () ->
+ generalConfig.getRejectNonResolvableCodeOwners(project, /* pluginConfig= */ null));
assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
}
@Test
public void noRejectNonResolvableCodeOwnersConfiguration() throws Exception {
- assertThat(generalConfig.getRejectNonResolvableCodeOwners(new Config())).isTrue();
+ assertThat(generalConfig.getRejectNonResolvableCodeOwners(project, new Config())).isTrue();
}
@Test
@@ -186,7 +244,7 @@
public void
rejectNonResolvableCodeOwnersConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
throws Exception {
- assertThat(generalConfig.getRejectNonResolvableCodeOwners(new Config())).isFalse();
+ assertThat(generalConfig.getRejectNonResolvableCodeOwners(project, new Config())).isFalse();
}
@Test
@@ -197,7 +255,35 @@
Config cfg = new Config();
cfg.setString(
SECTION_CODE_OWNERS, /* subsection= */ null, KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS, "true");
- assertThat(generalConfig.getRejectNonResolvableCodeOwners(cfg)).isTrue();
+ assertThat(generalConfig.getRejectNonResolvableCodeOwners(project, cfg)).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "false")
+ public void invalidRejectNonResolvableCodeOwnersInPluginConfigIsIgnored() throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS,
+ /* subsection= */ null,
+ KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS,
+ "INVALID");
+ assertThat(generalConfig.getRejectNonResolvableCodeOwners(project, cfg)).isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableCodeOwners", value = "INVALID")
+ public void invalidRejectNonResolvableCodeOwnersConfigurationInGerritConfigIsIgnored()
+ throws Exception {
+ assertThat(generalConfig.getRejectNonResolvableCodeOwners(project, new Config())).isTrue();
+ }
+
+ @Test
+ public void cannotGetRejectNonResolvableImportsForNullProject() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> generalConfig.getRejectNonResolvableImports(/* project= */ null, new Config()));
+ assertThat(npe).hasMessageThat().isEqualTo("project");
}
@Test
@@ -205,13 +291,13 @@
NullPointerException npe =
assertThrows(
NullPointerException.class,
- () -> generalConfig.getRejectNonResolvableImports(/* pluginConfig= */ null));
+ () -> generalConfig.getRejectNonResolvableImports(project, /* pluginConfig= */ null));
assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
}
@Test
public void noRejectNonResolvableImportsConfiguration() throws Exception {
- assertThat(generalConfig.getRejectNonResolvableImports(new Config())).isTrue();
+ assertThat(generalConfig.getRejectNonResolvableImports(project, new Config())).isTrue();
}
@Test
@@ -219,7 +305,7 @@
public void
rejectNonResolvableImportsConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
throws Exception {
- assertThat(generalConfig.getRejectNonResolvableImports(new Config())).isFalse();
+ assertThat(generalConfig.getRejectNonResolvableImports(project, new Config())).isFalse();
}
@Test
@@ -230,7 +316,23 @@
Config cfg = new Config();
cfg.setString(
SECTION_CODE_OWNERS, /* scubsection= */ null, KEY_REJECT_NON_RESOLVABLE_IMPORTS, "true");
- assertThat(generalConfig.getRejectNonResolvableImports(cfg)).isTrue();
+ assertThat(generalConfig.getRejectNonResolvableImports(project, cfg)).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableImports", value = "false")
+ public void invalidRejectNonResolvableImportsInPluginConfigIsIgnored() throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS, /* subsection= */ null, KEY_REJECT_NON_RESOLVABLE_IMPORTS, "INVALID");
+ assertThat(generalConfig.getRejectNonResolvableImports(project, cfg)).isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.rejectNonResolvableImports", value = "INVALID")
+ public void invalidRejectNonResolvableImportsConfigurationInGerritConfigIsIgnored()
+ throws Exception {
+ assertThat(generalConfig.getRejectNonResolvableImports(project, new Config())).isTrue();
}
@Test
