Refactored CommonsConfigurationUtils for loading properties configura…

…tion. (#13201) * Delete unnecessary setFile call. * Refactored CommonsConfigurationUtils for properties configuration creation and added UTs.
apache · May 23, 2024 · 0dec8a9 · 0dec8a9
1 parent 29c560f
commit 0dec8a9
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 12 deletions.
diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java b/pinot-spi/src/main/java/org/apache/pinot/spi/env/CommonsConfigurationUtils.java
@@ -84,11 +84,15 @@ public static PropertiesConfiguration fromPath(String path)
    * @param setDefaultDelimiter representing to set the default list delimiter.
    * @return a {@link PropertiesConfiguration} instance.
    */
-  public static PropertiesConfiguration fromPath(String path, boolean setIOFactory, boolean setDefaultDelimiter)
+  public static PropertiesConfiguration fromPath(@Nullable String path, boolean setIOFactory,
+      boolean setDefaultDelimiter)
       throws ConfigurationException {
     PropertiesConfiguration config = createPropertiesConfiguration(setIOFactory, setDefaultDelimiter);
-    FileHandler fileHandler = new FileHandler(config);
-    fileHandler.load(path);
+    // if provided path is non-empty, load the existing properties from provided file path
+    if (StringUtils.isNotEmpty(path)) {
+      FileHandler fileHandler = new FileHandler(config);
+      fileHandler.load(path);
+    }
     return config;
   }
 
@@ -99,12 +103,15 @@ public static PropertiesConfiguration fromPath(String path, boolean setIOFactory
    * @param setDefaultDelimiter representing to set the default list delimiter.
    * @return a {@link PropertiesConfiguration} instance.
    */
-  public static PropertiesConfiguration fromInputStream(InputStream stream, boolean setIOFactory,
+  public static PropertiesConfiguration fromInputStream(@Nullable InputStream stream, boolean setIOFactory,
       boolean setDefaultDelimiter)
       throws ConfigurationException {
     PropertiesConfiguration config = createPropertiesConfiguration(setIOFactory, setDefaultDelimiter);
-    FileHandler fileHandler = new FileHandler(config);
-    fileHandler.load(stream);
+    // if provided stream is not null, load the existing properties from provided input stream.
+    if (stream != null) {
+      FileHandler fileHandler = new FileHandler(config);
+      fileHandler.load(stream);
+    }
     return config;
   }
 
@@ -115,15 +122,13 @@ public static PropertiesConfiguration fromInputStream(InputStream stream, boolea
    * @param setDefaultDelimiter representing to set the default list delimiter.
    * @return a {@link PropertiesConfiguration} instance.
    */
-  public static PropertiesConfiguration fromFile(File file, boolean setIOFactory, boolean setDefaultDelimiter)
+  public static PropertiesConfiguration fromFile(@Nullable File file, boolean setIOFactory, boolean setDefaultDelimiter)
       throws ConfigurationException {
     PropertiesConfiguration config = createPropertiesConfiguration(setIOFactory, setDefaultDelimiter);
-    FileHandler fileHandler = new FileHandler(config);
-    // check if file exists, load the properties otherwise set the file.
-    if (file.exists()) {
+    // check if file exists, load the existing properties.
+    if (file != null && file.exists()) {
+      FileHandler fileHandler = new FileHandler(config);
       fileHandler.load(file);
-    } else {
-      fileHandler.setFile(file);
     }
     return config;
   }

diff --git a/pinot-spi/src/test/java/org/apache/pinot/spi/env/CommonsConfigurationUtilsTest.java b/pinot-spi/src/test/java/org/apache/pinot/spi/env/CommonsConfigurationUtilsTest.java
@@ -19,8 +19,11 @@
 package org.apache.pinot.spi.env;
 
 import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import org.apache.commons.configuration2.PropertiesConfiguration;
+import org.apache.commons.configuration2.ex.ConfigurationException;
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.lang3.RandomStringUtils;
 import org.apache.commons.lang3.StringUtils;
@@ -29,6 +32,7 @@
 import org.testng.annotations.Test;
 
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotNull;
 import static org.testng.Assert.assertTrue;
 
 
@@ -119,4 +123,44 @@ private void testPropertyValueWithSpecialCharacters(String value)
         (String) configuration.getProperty(PROPERTY_KEY));
     assertEquals(recoveredValue, value);
   }
+
+  @Test
+  public void testPropertiesConfigurationFromFile()
+      throws ConfigurationException {
+    PropertiesConfiguration configuration = CommonsConfigurationUtils.fromFile(null, false, true);
+    assertNotNull(configuration);
+    configuration.setProperty("Test Key", "Test Value");
+    CommonsConfigurationUtils.saveToFile(configuration, CONFIG_FILE);
+
+    configuration = CommonsConfigurationUtils.fromFile(CONFIG_FILE, false, true);
+    assertNotNull(configuration);
+    assertEquals(configuration.getProperty("Test Key"), "Test Value");
+  }
+
+  @Test
+  public void testPropertiesConfigurationFromPath()
+      throws ConfigurationException {
+    PropertiesConfiguration configuration = CommonsConfigurationUtils.fromPath(null, false, true);
+    assertNotNull(configuration);
+    configuration.setProperty("Test Key", "Test Value");
+    CommonsConfigurationUtils.saveToFile(configuration, CONFIG_FILE);
+
+    configuration = CommonsConfigurationUtils.fromPath(CONFIG_FILE.getPath(), false, true);
+    assertNotNull(configuration);
+    assertEquals(configuration.getProperty("Test Key"), "Test Value");
+  }
+
+  @Test
+  public void testPropertiesConfigurationFromInputStream()
+      throws ConfigurationException, FileNotFoundException {
+    PropertiesConfiguration configuration = CommonsConfigurationUtils.fromInputStream(null, false, true);
+    assertNotNull(configuration);
+    configuration.setProperty("Test Key", "Test Value");
+    CommonsConfigurationUtils.saveToFile(configuration, CONFIG_FILE);
+
+    FileInputStream inputStream = new FileInputStream(CONFIG_FILE);
+    configuration = CommonsConfigurationUtils.fromInputStream(inputStream, false, true);
+    assertNotNull(configuration);
+    assertEquals(configuration.getProperty("Test Key"), "Test Value");
+  }
 }