#513 Refactoring on ConfigJson Module (#514)

* testing no remote sovlers * written failing test which found the bug * test was wrong, feature is working * extracting to specific class * refactoring * created tests
mageddo · Jul 16, 2024 · 5321bd1 · 5321bd1
1 parent 8b53625
commit 5321bd1
Show file tree

Hide file tree

Showing 10 changed files with 272 additions and 73 deletions.
diff --git a/src/main/java/com/mageddo/dnsproxyserver/config/dataprovider/ConfigDAOJson.java b/src/main/java/com/mageddo/dnsproxyserver/config/dataprovider/ConfigDAOJson.java
@@ -1,11 +1,7 @@
 package com.mageddo.dnsproxyserver.config.dataprovider;
 
-import com.mageddo.dnsproxyserver.config.CircuitBreaker;
 import com.mageddo.dnsproxyserver.config.Config;
-import com.mageddo.dnsproxyserver.config.SolverRemote;
-import com.mageddo.dnsproxyserver.config.dataprovider.mapper.ConfigFieldsValuesMapper;
-import com.mageddo.dnsproxyserver.config.dataprovider.vo.ConfigJson;
-import com.mageddo.dnsproxyserver.utils.Booleans;
+import com.mageddo.dnsproxyserver.config.dataprovider.mapper.ConfigJsonV2Mapper;
 import com.mageddo.utils.Files;
 import com.mageddo.utils.Runtime;
 import com.mageddo.utils.Tests;
@@ -37,7 +33,7 @@ public Config find() {
   public Config find(Path configPath) {
     final var jsonConfig = JsonConfigs.loadConfig(configPath);
     log.debug("configPath={}", configPath);
-    return toConfig(jsonConfig, configPath);
+    return ConfigJsonV2Mapper.toConfig(jsonConfig, configPath);
   }
 
   public static Path buildConfigPath(Path workDir, String configPath) {
@@ -65,52 +61,6 @@ static boolean runningInTestsAndNoCustomConfigPath() {
     return !Arrays.toString(ConfigDAOCmdArgs.getArgs()).contains("--conf-path") && Tests.inTest();
   }
 
-  Config toConfig(ConfigJson json, Path configFileAbsolutePath) {
-    return Config.builder()
-      .webServerPort(json.getWebServerPort())
-      .dnsServerPort(json.getDnsServerPort())
-      .defaultDns(json.getDefaultDns())
-      .logLevel(ConfigFieldsValuesMapper.mapLogLevelFrom(json.getLogLevel()))
-      .logFile(ConfigFieldsValuesMapper.mapLogFileFrom(json.getLogFile()))
-      .registerContainerNames(json.getRegisterContainerNames())
-      .hostMachineHostname(json.getHostMachineHostname())
-      .domain(json.getDomain())
-      .mustConfigureDpsNetwork(json.getDpsNetwork())
-      .dpsNetworkAutoConnect(json.getDpsNetworkAutoConnect())
-      .remoteDnsServers(json.getRemoteDnsServers())
-      .serverProtocol(json.getServerProtocol())
-      .dockerHost(json.getDockerHost())
-      .resolvConfOverrideNameServers(json.getResolvConfOverrideNameServers())
-      .noEntriesResponseCode(json.getNoEntriesResponseCode())
-      .dockerSolverHostMachineFallbackActive(json.getDockerSolverHostMachineFallbackActive())
-      .configPath(configFileAbsolutePath)
-      .solverRemote(toSolverRemote(json))
-      .build();
-  }
-
-  static SolverRemote toSolverRemote(ConfigJson json) {
-    final var solverRemote = json.getSolverRemote();
-    if (solverRemote == null) {
-      return null;
-    }
-    final var circuitBreaker = solverRemote.getCircuitBreaker();
-    if (circuitBreaker == null) {
-      return null;
-    }
-    return SolverRemote
-      .builder()
-      .active(Booleans.reverseWhenNotNull(json.getNoRemoteServers()))
-      .circuitBreaker(CircuitBreaker
-        .builder()
-        .failureThreshold(circuitBreaker.getFailureThreshold())
-        .failureThresholdCapacity(circuitBreaker.getFailureThresholdCapacity())
-        .successThreshold(circuitBreaker.getSuccessThreshold())
-        .testDelay(circuitBreaker.getTestDelay())
-        .build()
-      )
-      .build();
-  }
-
   @Override
   public int priority() {
     return 2;

diff --git a/src/main/java/com/mageddo/dnsproxyserver/config/dataprovider/JsonConfigs.java b/src/main/java/com/mageddo/dnsproxyserver/config/dataprovider/JsonConfigs.java
@@ -38,10 +38,16 @@ public static ConfigJson loadConfig(Path configPath) {
       createDefault(configPath);
     }
 
+    return loadConfig(Files.readString(configPath));
+
+  }
+
+  @SneakyThrows
+  public static ConfigJson loadConfig(String jsonContent) {
     final var objectMapper = JsonUtils.instance();
-    final var tree = objectMapper.readTree(configPath.toFile());
+    final var tree = objectMapper.readTree(jsonContent);
     if (tree.isEmpty()) {
-      log.info("status=emptyConfigFile, action=usingDefault, file={}", configPath);
+      log.info("status=emptyConfigFile, action=usingDefault");
       return new ConfigJsonV2();
     }
     final var version = findVersion(tree);
@@ -52,7 +58,6 @@ public static ConfigJson loadConfig(Path configPath) {
         "unsupported config file version=%d, supported=%s", version, supportedVersions
       ));
     };
-
   }
 
   @SneakyThrows

diff --git a/src/main/java/com/mageddo/dnsproxyserver/config/dataprovider/mapper/ConfigJsonV2Mapper.java b/src/main/java/com/mageddo/dnsproxyserver/config/dataprovider/mapper/ConfigJsonV2Mapper.java
@@ -0,0 +1,58 @@
+package com.mageddo.dnsproxyserver.config.dataprovider.mapper;
+
+import com.mageddo.dnsproxyserver.config.CircuitBreaker;
+import com.mageddo.dnsproxyserver.config.Config;
+import com.mageddo.dnsproxyserver.config.SolverRemote;
+import com.mageddo.dnsproxyserver.config.dataprovider.vo.ConfigJson;
+import com.mageddo.dnsproxyserver.utils.Booleans;
+
+import java.nio.file.Path;
+
+public class ConfigJsonV2Mapper {
+
+  public static Config toConfig(ConfigJson json, Path configFileAbsolutePath) {
+    return Config.builder()
+      .webServerPort(json.getWebServerPort())
+      .dnsServerPort(json.getDnsServerPort())
+      .defaultDns(json.getDefaultDns())
+      .logLevel(ConfigFieldsValuesMapper.mapLogLevelFrom(json.getLogLevel()))
+      .logFile(ConfigFieldsValuesMapper.mapLogFileFrom(json.getLogFile()))
+      .registerContainerNames(json.getRegisterContainerNames())
+      .hostMachineHostname(json.getHostMachineHostname())
+      .domain(json.getDomain())
+      .mustConfigureDpsNetwork(json.getDpsNetwork())
+      .dpsNetworkAutoConnect(json.getDpsNetworkAutoConnect())
+      .remoteDnsServers(json.getRemoteDnsServers())
+      .serverProtocol(json.getServerProtocol())
+      .dockerHost(json.getDockerHost())
+      .resolvConfOverrideNameServers(json.getResolvConfOverrideNameServers())
+      .noEntriesResponseCode(json.getNoEntriesResponseCode())
+      .dockerSolverHostMachineFallbackActive(json.getDockerSolverHostMachineFallbackActive())
+      .configPath(configFileAbsolutePath)
+      .solverRemote(toSolverRemote(json))
+      .build();
+  }
+
+  static SolverRemote toSolverRemote(ConfigJson json) {
+    final var solverRemote = json.getSolverRemote();
+    if (solverRemote == null) {
+      return null;
+    }
+    final var circuitBreaker = solverRemote.getCircuitBreaker();
+    if (circuitBreaker == null) {
+      return null;
+    }
+    return SolverRemote
+      .builder()
+      .active(Booleans.reverseWhenNotNull(json.getNoRemoteServers()))
+      .circuitBreaker(CircuitBreaker
+        .builder()
+        .failureThreshold(circuitBreaker.getFailureThreshold())
+        .failureThresholdCapacity(circuitBreaker.getFailureThresholdCapacity())
+        .successThreshold(circuitBreaker.getSuccessThreshold())
+        .testDelay(circuitBreaker.getTestDelay())
+        .build()
+      )
+      .build();
+  }
+}
diff --git a/src/test/java/com/mageddo/dnsproxyserver/config/application/ConfigServiceCompTest.java b/src/test/java/com/mageddo/dnsproxyserver/config/application/ConfigServiceCompTest.java
@@ -2,9 +2,11 @@
 
 import com.mageddo.dnsproxyserver.config.Config;
 import com.mageddo.dnsproxyserver.config.LogLevel;
-import com.mageddo.dnsproxyserver.config.di.Context;
 import com.mageddo.dnsproxyserver.config.dataprovider.ConfigDAOCmdArgs;
+import com.mageddo.dnsproxyserver.config.di.Context;
 import dagger.sheath.junit.DaggerTest;
+import lombok.SneakyThrows;
+import org.apache.commons.io.IOUtils;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 
@@ -14,6 +16,7 @@
 
 import static com.mageddo.utils.TestUtils.readAndSortJson;
 import static com.mageddo.utils.TestUtils.readAndSortJsonExcluding;
+import static com.mageddo.utils.TestUtils.readAsStream;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -55,21 +58,10 @@ void mustParseDefaultConfigsAndCreateJsonConfigFile(@TempDir Path tmpDir) {
       ;
 
     // assert
-    assertParsedConfig(config);
-    assertWrittenFile(jsonConfigFile);
+    assertParsedConfig(config, "/configs-test/001.json");
+    assertWrittenFile("/configs-test/002.json", jsonConfigFile);
   }
 
-  static void assertParsedConfig(Config config) {
-    assertEquals(
-      readAndSortJsonExcluding("/configs-test/001.json", excludingFields),
-      readAndSortJsonExcluding(config, excludingFields)
-    );
-  }
-
-  static void assertWrittenFile(Path jsonConfigFile) {
-    assertTrue(Files.exists(jsonConfigFile));
-    assertEquals(readAndSortJson("/configs-test/002.json"), readAndSortJson(jsonConfigFile));
-  }
 
   @Test
   void mustParseLowerCaseLogLevel(){
@@ -85,4 +77,45 @@ void mustParseLowerCaseLogLevel(){
     // assert
     assertEquals(LogLevel.WARNING, config.getLogLevel());
   }
+
+  @Test
+  void mustDisableRemoteServersRespectingConfig(@TempDir Path tmpDir){
+    // arrange
+    writeAndSetCustomConfigFile(tmpDir, "/configs-test/006.json");
+
+    // act
+    final var config = Configs.getContext()
+      .configService()
+      .findCurrentConfig()
+      ;
+
+    // assert
+    assertFalse(config.isSolverRemoteActive());
+
+  }
+
+  static void assertParsedConfig(Config config, String expectedFilePath) {
+    assertEquals(
+      readAndSortJsonExcluding(expectedFilePath, excludingFields),
+      readAndSortJsonExcluding(config, excludingFields)
+    );
+  }
+
+  static void assertWrittenFile(String expectedFilePath, Path jsonConfigFile) {
+    assertTrue(Files.exists(jsonConfigFile));
+    assertEquals(readAndSortJson(expectedFilePath), readAndSortJson(jsonConfigFile));
+  }
+
+  static void writeAndSetCustomConfigFile(Path tmpDir, String sourceConfigFile) {
+    final var configPathToUse = tmpDir.resolve("tmpfile.json");
+    writeCurrentConfigFile(sourceConfigFile, configPathToUse);
+    ConfigDAOCmdArgs.setArgs(new String[]{"--conf-path", configPathToUse.toString()});
+  }
+
+  @SneakyThrows
+  static void writeCurrentConfigFile(String sourceResource, Path target) {
+    try (var out = Files.newOutputStream(target)) {
+      IOUtils.copy(readAsStream(sourceResource), out);
+    }
+  }
 }
diff --git a/src/test/java/com/mageddo/dnsproxyserver/config/dataprovider/ConfigDAOJsonTest.java b/src/test/java/com/mageddo/dnsproxyserver/config/dataprovider/ConfigDAOJsonTest.java
@@ -16,7 +16,7 @@
 import static com.mageddo.utils.TestUtils.readAsStream;
 import static com.mageddo.utils.TestUtils.sortJsonExcluding;
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 
 @ExtendWith(MockitoExtension.class)
 class ConfigDAOJsonTest {
@@ -46,10 +46,9 @@ void mustBuildConfPathRelativeToWorkDir(@TempDir Path tmpDir){
   @Test
   void mustReadAndRespectStoredConfigFile(@TempDir Path tmpDir) {
     // arrange
-    final var sorceConfigFile = "/configs-test/003.json";
+    final var sourceConfigFile = "/configs-test/003.json";
     final var configPathToUse = tmpDir.resolve("tmpfile.json");
-    writeCurrentConfigFile(sorceConfigFile, configPathToUse);
-    assertTrue(Files.exists(configPathToUse));
+    writeCurrentConfigFile(sourceConfigFile, configPathToUse);
 
     // act
     final var config = this.configDAOJson.find(configPathToUse);
@@ -61,6 +60,20 @@ void mustReadAndRespectStoredConfigFile(@TempDir Path tmpDir) {
     );
   }
 
+  @Test
+  void mustDisableRemoteServersRespectingConfig(@TempDir Path tmpDir) {
+    // arrange
+    final var sourceConfigFile = "/configs-test/005.json";
+    final var configPathToUse = tmpDir.resolve("tmpfile.json");
+    writeCurrentConfigFile(sourceConfigFile, configPathToUse);
+
+    // act
+    final var config = this.configDAOJson.find(configPathToUse);
+
+    // assert
+    assertFalse(config.isSolverRemoteActive());
+  }
+
   @SneakyThrows
   static void writeCurrentConfigFile(String sourceResource, Path target) {
     try (var out = Files.newOutputStream(target)) {

diff --git a/...st/java/com/mageddo/dnsproxyserver/config/dataprovider/mapper/ConfigJsonV2MapperTest.java b/...st/java/com/mageddo/dnsproxyserver/config/dataprovider/mapper/ConfigJsonV2MapperTest.java
@@ -0,0 +1,46 @@
+package com.mageddo.dnsproxyserver.config.dataprovider.mapper;
+
+import com.mageddo.dnsproxyserver.config.Config;
+import com.mageddo.dnsproxyserver.config.dataprovider.vo.ConfigJson;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+import testing.templates.ConfigJsonTemplates;
+
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+
+class ConfigJsonV2MapperTest {
+
+  static final Path RANDOM_CONFIG_PATH = Paths.get("/tmp/conf.json");
+
+  @Test
+  void mustMapSolverRemoteAsInactiveWhenNoRemoteServersFlagIsSet(){
+    // arrange
+    final var configJson = ConfigJsonTemplates.withNoRemoteServersAndCircuitBreakerDefined();
+
+    // act
+    final var config = toConfig(configJson);
+
+    // assert
+    assertFalse(config.isSolverRemoteActive());
+  }
+
+  @Test
+  @Disabled("by the bug reported at #513")
+  void mustMapSolverRemoteAsInactiveEvenWhenCircuitBreakerIsNOTSet(){
+    // arrange
+    final var configJson = ConfigJsonTemplates.withoutCircuitBreakerDefinedWithNoRemoteServers();
+
+    // act
+    final var config = toConfig(configJson);
+
+    // assert
+    assertFalse(config.isSolverRemoteActive());
+  }
+
+  static Config toConfig(ConfigJson configJson) {
+    return ConfigJsonV2Mapper.toConfig(configJson, RANDOM_CONFIG_PATH);
+  }
+}
diff --git a/src/test/java/testing/templates/ConfigJsonTemplates.java b/src/test/java/testing/templates/ConfigJsonTemplates.java
@@ -0,0 +1,18 @@
+package testing.templates;
+
+import com.mageddo.dnsproxyserver.config.dataprovider.JsonConfigs;
+import com.mageddo.dnsproxyserver.config.dataprovider.vo.ConfigJson;
+import com.mageddo.utils.TestUtils;
+
+public class ConfigJsonTemplates {
+
+  public static ConfigJson withNoRemoteServersAndCircuitBreakerDefined() {
+    final var path = "/configs-test/005.json";
+    return JsonConfigs.loadConfig(TestUtils.readString(path));
+  }
+
+  public static ConfigJson withoutCircuitBreakerDefinedWithNoRemoteServers() {
+    final var path = "/configs-test/007.json";
+    return JsonConfigs.loadConfig(TestUtils.readString(path));
+  }
+}
diff --git a/src/test/resources/configs-test/005.json b/src/test/resources/configs-test/005.json
@@ -0,0 +1,28 @@
+{
+  "version" : 2,
+  "activeEnv" : "MARROI",
+  "remoteDnsServers" : [ ],
+  "envs" : [ {
+    "name" : "MARROI",
+    "hostnames" : null
+  } ],
+  "webServerPort" : 9393,
+  "dnsServerPort" : 5391,
+  "defaultDns" : false,
+  "logLevel" : "DEBUG",
+  "logFile" : "true",
+  "registerContainerNames" : true,
+  "hostMachineHostname" : "batata.com",
+  "domain" : "acme",
+  "dpsNetwork" : true,
+  "dpsNetworkAutoConnect" : true,
+  "noRemoteServers": true,
+  "solverRemote" : {
+    "circuitBreaker" : {
+      "failureThreshold" : 3,
+      "failureThresholdCapacity" : 10,
+      "successThreshold" : 5,
+      "testDelay" : "PT1M"
+    }
+  }
+}