AbstractRequiredApprovalConfig: Merge the get methods Currently there is one get method to read the config from a plugin config and one get method to read the config from the global config. Merge boths methods into a single method that first reads the config from the plugin config and then falls back to reading the config from the global config if no value was found. This is more consistent with the other methods for reading config values that all contain the fallback logic. Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: I488f0eeafc193fd583b4c9dfca78403bfb7d9f22

diff --git a/java/com/google/gerrit/plugins/codeowners/config/AbstractRequiredApprovalConfig.java b/java/com/google/gerrit/plugins/codeowners/config/AbstractRequiredApprovalConfig.java
index 81f6cb7..f1e7acb 100644
--- a/java/com/google/gerrit/plugins/codeowners/config/AbstractRequiredApprovalConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/config/AbstractRequiredApprovalConfig.java

@@ -50,46 +50,49 @@
 
   protected abstract String getConfigKey();
 
-  Optional<RequiredApproval> getForProject(ProjectState projectState, Config pluginConfig) {
+  /**
+   * Reads the required approval for the specified project from the given plugin config with
+   * fallback to {@code gerrit.config}.
+   *
+   * @param projectState state of the project for which the required approval should be read
+   * @param pluginConfig the plugin config from which the required approval should be read
+   * @return the required approval, {@link Optional#empty} if none was configured
+   */
+  Optional<RequiredApproval> get(ProjectState projectState, Config pluginConfig) {
     requireNonNull(projectState, "projectState");
     requireNonNull(pluginConfig, "pluginConfig");
+
     String requiredApproval =
         pluginConfig.getString(SECTION_CODE_OWNERS, /* subsection= */ null, getConfigKey());
-    if (requiredApproval == null) {
-      return Optional.empty();
+    if (requiredApproval != null) {
+      try {
+        return Optional.of(RequiredApproval.parse(projectState, requiredApproval));
+      } catch (IllegalStateException | IllegalArgumentException e) {
+        throw new InvalidPluginConfigurationException(
+            pluginName,
+            String.format(
+                "Required approval '%s' that is configured in %s.config"
+                    + " (parameter %s.%s) is invalid: %s",
+                requiredApproval, pluginName, SECTION_CODE_OWNERS, getConfigKey(), e.getMessage()));
+      }
     }
 
-    try {
-      return Optional.of(RequiredApproval.parse(projectState, requiredApproval));
-    } catch (IllegalStateException | IllegalArgumentException e) {
-      throw new InvalidPluginConfigurationException(
-          pluginName,
-          String.format(
-              "Required approval '%s' that is configured in %s.config"
-                  + " (parameter %s.%s) is invalid: %s",
-              requiredApproval, pluginName, SECTION_CODE_OWNERS, getConfigKey(), e.getMessage()));
-    }
-  }
-
-  Optional<RequiredApproval> getFromGlobalPluginConfig(ProjectState projectState) {
-    requireNonNull(projectState, "projectState");
-
-    String requiredApproval =
+    requiredApproval =
         pluginConfigFactory.getFromGerritConfig(pluginName).getString(getConfigKey());
-    if (requiredApproval == null) {
-      return Optional.empty();
+    if (requiredApproval != null) {
+      try {
+        return Optional.of(RequiredApproval.parse(projectState, requiredApproval));
+      } catch (IllegalStateException | IllegalArgumentException e) {
+        throw new InvalidPluginConfigurationException(
+            pluginName,
+            String.format(
+                "Required approval '%s' that is configured in gerrit.config"
+                    + " (parameter plugin.%s.%s) is invalid: %s",
+                requiredApproval, pluginName, getConfigKey(), e.getMessage()));
+      }
     }
 
-    try {
-      return Optional.of(RequiredApproval.parse(projectState, requiredApproval));
-    } catch (IllegalStateException | IllegalArgumentException e) {
-      throw new InvalidPluginConfigurationException(
-          pluginName,
-          String.format(
-              "Required approval '%s' that is configured in gerrit.config"
-                  + " (parameter plugin.%s.%s) is invalid: %s",
-              requiredApproval, pluginName, getConfigKey(), e.getMessage()));
-    }
+    return Optional.empty();
   }
 
   /**

diff --git a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java
index ac3a7d0..8288c03 100644
--- a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java
+++ b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java

@@ -388,23 +388,8 @@
   private Optional<RequiredApproval> getConfiguredRequiredApproval(
       AbstractRequiredApprovalConfig requiredApprovalConfig, Project.NameKey project) {
     Config pluginConfig = getPluginConfig(project);
-
     ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
-
-    // check if a project specific required approval is configured
-    Optional<RequiredApproval> requiredApproval =
-        requiredApprovalConfig.getForProject(projectState, pluginConfig);
-    if (requiredApproval.isPresent()) {
-      return requiredApproval;
-    }
-
-    // check if a required approval is globally configured
-    requiredApproval = requiredApprovalConfig.getFromGlobalPluginConfig(projectState);
-    if (requiredApproval.isPresent()) {
-      return requiredApproval;
-    }
-
-    return Optional.empty();
+    return requiredApprovalConfig.get(projectState, pluginConfig);
   }
 
   /**

diff --git a/javatests/com/google/gerrit/plugins/codeowners/config/AbstractRequiredApprovalConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/config/AbstractRequiredApprovalConfigTest.java
index c9e6b36..f067d4a 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/config/AbstractRequiredApprovalConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/config/AbstractRequiredApprovalConfigTest.java

@@ -34,13 +34,12 @@
   /** Must return the {@link AbstractRequiredApprovalConfig} that should be tested. */
   protected abstract AbstractRequiredApprovalConfig getRequiredApprovalConfig();
 
-  protected void testCannotGetFromGlobalPluginConfigIfConfigIsInvalid(String invalidValue)
-      throws Exception {
+  protected void testCannotGetIfGlobalConfigIsInvalid(String invalidValue) throws Exception {
     ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
     InvalidPluginConfigurationException exception =
         assertThrows(
             InvalidPluginConfigurationException.class,
-            () -> getRequiredApprovalConfig().getFromGlobalPluginConfig(projectState));
+            () -> getRequiredApprovalConfig().get(projectState, new Config()));
     assertThat(exception)
         .hasMessageThat()
         .isEqualTo(
@@ -52,34 +51,32 @@
   }
 
   @Test
-  public void cannotGetForProjectForNullProjectState() throws Exception {
+  public void cannotGetForNullProjectState() throws Exception {
     NullPointerException npe =
         assertThrows(
             NullPointerException.class,
-            () ->
-                getRequiredApprovalConfig().getForProject(/* projectState= */ null, new Config()));
+            () -> getRequiredApprovalConfig().get(/* projectState= */ null, new Config()));
     assertThat(npe).hasMessageThat().isEqualTo("projectState");
   }
 
   @Test
-  public void cannotGetForProjectForNullConfig() throws Exception {
+  public void cannotGetForNullConfig() throws Exception {
     ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
     NullPointerException npe =
         assertThrows(
             NullPointerException.class,
-            () ->
-                getRequiredApprovalConfig().getForProject(projectState, /* pluginConfig= */ null));
+            () -> getRequiredApprovalConfig().get(projectState, /* pluginConfig= */ null));
     assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
   }
 
   @Test
-  public void getForProjectWhenRequiredApprovalIsNotSet() throws Exception {
+  public void getWhenRequiredApprovalIsNotSet() throws Exception {
     ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
-    assertThat(getRequiredApprovalConfig().getForProject(projectState, new Config())).isEmpty();
+    assertThat(getRequiredApprovalConfig().get(projectState, new Config())).isEmpty();
   }
 
   @Test
-  public void getForProject() throws Exception {
+  public void getFromPluginConfig() throws Exception {
     ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
     Config cfg = new Config();
     cfg.setString(
@@ -88,14 +85,14 @@
         getRequiredApprovalConfig().getConfigKey(),
         "Code-Review+2");
     Optional<RequiredApproval> requiredApproval =
-        getRequiredApprovalConfig().getForProject(projectState, cfg);
+        getRequiredApprovalConfig().get(projectState, cfg);
     assertThat(requiredApproval).isPresent();
     assertThat(requiredApproval.get().labelType().getName()).isEqualTo("Code-Review");
     assertThat(requiredApproval.get().value()).isEqualTo(2);
   }
 
   @Test
-  public void cannotGetForProjectIfConfigIsInvalid() throws Exception {
+  public void cannotGetFromPluginConfigIfConfigIsInvalid() throws Exception {
     ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
     Config cfg = new Config();
     cfg.setString(
@@ -106,7 +103,7 @@
     InvalidPluginConfigurationException exception =
         assertThrows(
             InvalidPluginConfigurationException.class,
-            () -> getRequiredApprovalConfig().getForProject(projectState, cfg));
+            () -> getRequiredApprovalConfig().get(projectState, cfg));
     assertThat(exception)
         .hasMessageThat()
         .isEqualTo(
@@ -118,21 +115,6 @@
   }
 
   @Test
-  public void cannotGetFromGlobalPluginConfigForNullProjectState() throws Exception {
-    NullPointerException npe =
-        assertThrows(
-            NullPointerException.class,
-            () -> getRequiredApprovalConfig().getFromGlobalPluginConfig(/* projectState */ null));
-    assertThat(npe).hasMessageThat().isEqualTo("projectState");
-  }
-
-  @Test
-  public void getFromGlobalPluginConfigWhenRequiredApprovalIsNotSet() throws Exception {
-    ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
-    assertThat(getRequiredApprovalConfig().getFromGlobalPluginConfig(projectState)).isEmpty();
-  }
-
-  @Test
   public void cannotValidateProjectLevelConfigWithNullProjectState() throws Exception {
     NullPointerException npe =
         assertThrows(

diff --git a/javatests/com/google/gerrit/plugins/codeowners/config/OverrideApprovalConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/config/OverrideApprovalConfigTest.java
index 3591eca..648afcf 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/config/OverrideApprovalConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/config/OverrideApprovalConfigTest.java

@@ -21,6 +21,7 @@
 import com.google.gerrit.acceptance.config.GerritConfig;
 import com.google.gerrit.server.project.ProjectState;
 import java.util.Optional;
+import org.eclipse.jgit.lib.Config;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -45,7 +46,7 @@
 
     ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
     Optional<RequiredApproval> requiredApproval =
-        getRequiredApprovalConfig().getFromGlobalPluginConfig(projectState);
+        getRequiredApprovalConfig().get(projectState, new Config());
     assertThat(requiredApproval).isPresent();
     assertThat(requiredApproval.get().labelType().getName()).isEqualTo("Owners-Override");
     assertThat(requiredApproval.get().value()).isEqualTo(1);
@@ -53,7 +54,7 @@
 
   @Test
   @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "INVALID")
-  public void cannotGetFromGlobalPluginConfigIfConfigIsInvalid() throws Exception {
-    testCannotGetFromGlobalPluginConfigIfConfigIsInvalid("INVALID");
+  public void cannotGetIfGlobalConfigIsInvalid() throws Exception {
+    testCannotGetIfGlobalConfigIsInvalid("INVALID");
   }
 }

diff --git a/javatests/com/google/gerrit/plugins/codeowners/config/RequiredApprovalConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/config/RequiredApprovalConfigTest.java
index 198ac0c..f91faee 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/config/RequiredApprovalConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/config/RequiredApprovalConfigTest.java

@@ -22,6 +22,7 @@
 import com.google.gerrit.acceptance.config.GerritConfig;
 import com.google.gerrit.server.project.ProjectState;
 import java.util.Optional;
+import org.eclipse.jgit.lib.Config;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -44,7 +45,7 @@
   public void getFromGlobalPluginConfig() throws Exception {
     ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
     Optional<RequiredApproval> requiredApproval =
-        getRequiredApprovalConfig().getFromGlobalPluginConfig(projectState);
+        getRequiredApprovalConfig().get(projectState, new Config());
     assertThat(requiredApproval).isPresent();
     assertThat(requiredApproval.get().labelType().getName()).isEqualTo("Code-Review");
     assertThat(requiredApproval.get().value()).isEqualTo(2);
@@ -52,8 +53,8 @@
 
   @Test
   @GerritConfig(name = "plugin.code-owners.requiredApproval", value = "INVALID")
-  public void cannotGetFromGlobalPluginConfigIfConfigIsInvalid() throws Exception {
-    testCannotGetFromGlobalPluginConfigIfConfigIsInvalid("INVALID");
+  public void cannotIfGlobalConfigIsInvalid() throws Exception {
+    testCannotGetIfGlobalConfigIsInvalid("INVALID");
   }
 
   @Test