From 0dec8a9ab9183eb91b9abee50edebdfcb5969103 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Thu, 23 May 2024 13:32:29 -0400 Subject: [PATCH] Refactored CommonsConfigurationUtils for loading properties configuration. (#13201) * Delete unnecessary setFile call. * Refactored CommonsConfigurationUtils for properties configuration creation and added UTs. --- .../spi/env/CommonsConfigurationUtils.java | 29 +++++++----- .../env/CommonsConfigurationUtilsTest.java | 44 +++++++++++++++++++ 2 files changed, 61 insertions(+), 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 index 1b09d250141e..0613230e09a3 100644 --- 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 index af40022794d2..06174ed21276 100644 --- 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"); + } }