From 46c58dc48cc0f73c14d5d6087cb5974e9e6b49b6 Mon Sep 17 00:00:00 2001 From: Vijay N Date: Thu, 23 Oct 2025 21:13:15 +0530 Subject: [PATCH 1/7] * Improved property source resolution and logged conflicts * Consolidated sources into hierarchy of precedence: EnvironmentVariables > RuntimeConfiguration > KillBillDefaults * Added status indicators and conflict warnings --- .../config/DefaultKillbillConfigSource.java | 201 ++++++++++++++---- .../config/PropertiesWithSourceCollector.java | 8 +- .../test/config/TestKillbillConfigSource.java | 4 +- 3 files changed, 165 insertions(+), 48 deletions(-) diff --git a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java index e6ae6c97f..7fb3d4359 100644 --- a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java +++ b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java @@ -21,18 +21,21 @@ import java.io.IOException; import java.net.URISyntaxException; -import java.nio.file.Path; -import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Enumeration; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Objects; import java.util.Optional; import java.util.Properties; +import java.util.Set; import java.util.TimeZone; import javax.annotation.Nullable; @@ -98,21 +101,11 @@ public DefaultKillbillConfigSource(@Nullable final String file, final Map propsMap = propertiesToMap(properties); - propertiesCollector.addProperties(category, propsMap); - + final Map propsMap = propertiesToMap(properties); + propertiesCollector.addProperties("RuntimeConfiguration", propsMap); } - for (final Entry entry : extraDefaultProperties.entrySet()) { - if (entry.getValue() != null) { - properties.put(entry.getKey(), entry.getValue()); - } - } - - propertiesCollector.addProperties("ExtraDefaultProperties", extraDefaultProperties); - - populateDefaultProperties(); + populateDefaultProperties(extraDefaultProperties); if (Boolean.parseBoolean(getString(LOOKUP_ENVIRONMENT_VARIABLES))) { overrideWithEnvironmentVariables(); @@ -160,9 +153,8 @@ private Properties loadPropertiesFromFileOrSystemProperties() { final Properties properties = new Properties(); properties.load(UriAccessor.accessUri(propertiesFileLocation)); - final String category = extractFileNameFromPath(propertiesFileLocation); final Map propsMap = propertiesToMap(properties); - propertiesCollector.addProperties(category, propsMap); + propertiesCollector.addProperties("RuntimeConfiguration", propsMap); return properties; } catch (final IOException e) { @@ -172,14 +164,17 @@ private Properties loadPropertiesFromFileOrSystemProperties() { } } - propertiesCollector.addProperties("SystemProperties", propertiesToMap(System.getProperties())); + propertiesCollector.addProperties("RuntimeConfiguration", propertiesToMap(System.getProperties())); return new Properties(System.getProperties()); } @VisibleForTesting - protected void populateDefaultProperties() { + protected void populateDefaultProperties(final Map extraDefaultProperties) { final Properties defaultProperties = getDefaultProperties(); + + extraDefaultProperties.forEach(defaultProperties::putIfAbsent); + for (final String propertyName : defaultProperties.stringPropertyNames()) { // Let the user override these properties if (properties.get(propertyName) == null) { @@ -236,7 +231,7 @@ protected void populateDefaultProperties() { defaultSystemProperties.putAll(defaultProperties); final Map propsMap = propertiesToMap(defaultSystemProperties); - propertiesCollector.addProperties("DefaultSystemProperties", propsMap); + propertiesCollector.addProperties("KillBillDefaults", propsMap); } @Override @@ -257,21 +252,160 @@ public Map> getPropertiesBySource() { } }); + final Map> propertiesBySource = propertiesCollector.getPropertiesBySource(); + final Map effectiveProperties = getCurrentEffectiveProperties(); + + final Map> propertyToSources = new HashMap<>(); + propertiesBySource.forEach((source, properties) -> { + properties.forEach(property -> propertyToSources.computeIfAbsent(property.getKey(), + propName -> new LinkedHashSet<>()).add(source)); + }); + + final List sourceOrder = Arrays.asList("EnvironmentVariables", + "RuntimeConfiguration", + "KillBillDefaults"); + + final Set warnedConflicts = new HashSet<>(); final Map> result = new LinkedHashMap<>(); - propertiesBySource.forEach((source, properties) -> { + for (final String source : sourceOrder) { + final List properties = propertiesBySource.get(source); + if (properties == null || properties.isEmpty()) { + continue; + } + final Map sourceProperties = new LinkedHashMap<>(); - properties.forEach(prop -> { - sourceProperties.put(prop.getKey(), prop.getValue()); - }); - result.put(source, Collections.unmodifiableMap(sourceProperties)); + + final Set processedKeys = new HashSet<>(); + + for (final PropertyWithSource prop : properties) { + final String propertyKey = prop.getKey(); + + if (processedKeys.contains(propertyKey)) { + continue; + } + processedKeys.add(propertyKey); + + final String effectiveValue = effectiveProperties.get(propertyKey); + if (effectiveValue == null) { + continue; + } + + final Set sources = propertyToSources.get(propertyKey); + final boolean hasConflict = sources != null && sources.size() > 1; + + final boolean isEffectiveSource = isEffectiveSourceForProperty(propertyKey, source, sourceOrder, propertyToSources); + + final String displayValue; + if (isEffectiveSource) { + if (hasConflict) { + if (!warnedConflicts.contains(propertyKey)) { + warnedConflicts.add(propertyKey); + logger.warn("Property conflict detected for '{}': defined in sources {} - using value from '{}': '{}'", + propertyKey, new ArrayList<>(sources), source, effectiveValue); + } + + final List overridden = getOverriddenSources(source, sources, sourceOrder); + if (!overridden.isEmpty()) { + displayValue = effectiveValue + " [ACTIVE -- overrides: " + String.join(", ", overridden) + "]"; + } else { + displayValue = effectiveValue + " [ACTIVE]"; + } + } else { + displayValue = effectiveValue; + } + } else { + final String effectiveSourceName = getEffectiveSourceName(propertyKey, sourceOrder, propertyToSources); + displayValue = effectiveValue + " [OVERRIDDEN by " + effectiveSourceName + "]"; + } + + sourceProperties.put(propertyKey, displayValue); + } + + if (!sourceProperties.isEmpty()) { + result.put(source, Collections.unmodifiableMap(sourceProperties)); + } + } + + propertiesBySource.forEach((source, properties) -> { + if (!sourceOrder.contains(source) && !result.containsKey(source)) { + final Map sourceProperties = new LinkedHashMap<>(); + properties.forEach(prop -> { + sourceProperties.put(prop.getKey(), prop.getValue()); + }); + if (!sourceProperties.isEmpty()) { + result.put(source, Collections.unmodifiableMap(sourceProperties)); + } + } }); return Collections.unmodifiableMap(result); } + private boolean isEffectiveSourceForProperty(final String propertyKey, + final String currentSource, + final List sourceOrder, + final Map> propertyToSources) { + final Set sourcesForProperty = propertyToSources.get(propertyKey); + if (sourcesForProperty == null || !sourcesForProperty.contains(currentSource)) { + return false; + } + + for (final String source : sourceOrder) { + if (sourcesForProperty.contains(source)) { + return source.equals(currentSource); + } + } + + return false; + } + + private String getEffectiveSourceName(final String propertyKey, + final List sourceOrder, + final Map> propertyToSources) { + final Set sources = propertyToSources.get(propertyKey); + if (sources == null) { + return "unknown"; + } + + for (final String source : sourceOrder) { + if (sources.contains(source)) { + return source; + } + } + + return "unknown"; + } + + private Map getCurrentEffectiveProperties() { + final Map effectiveProps = new HashMap<>(); + + properties.stringPropertyNames() + .forEach(key -> effectiveProps.put(key, properties.getProperty(key))); + + return effectiveProps; + } + + private List getOverriddenSources(final String currentSource, + final Set allSources, + final List sourceOrder) { + final List overridden = new ArrayList<>(); + final int currentIndex = sourceOrder.indexOf(currentSource); + + for (final String source : allSources) { + if (!source.equals(currentSource)) { + final int sourceIndex = sourceOrder.indexOf(source); + if (sourceIndex > currentIndex && sourceIndex != -1) { + overridden.add(source); + } + } + } + + return overridden; + } + @VisibleForTesting public void setProperty(final String propertyName, final Object propertyValue) { properties.put(propertyName, propertyValue); @@ -386,23 +520,6 @@ private Optional decryptableValue(final String value) { return Optional.empty(); } - private String extractFileNameFromPath(String path) { - if (path == null || path.isEmpty()) { - return "unknown.properties"; - } - - if (path.startsWith("file://")) { - path = path.substring("file://".length()); - } - - final Path fileName = Paths.get(path).getFileName(); - if (fileName == null) { - return "unknown.properties"; - } - - return fileName.toString(); - } - private Map propertiesToMap(final Properties props) { final Map propertiesMap = new HashMap<>(); for (final Map.Entry entry : props.entrySet()) { diff --git a/base/src/main/java/org/killbill/billing/platform/config/PropertiesWithSourceCollector.java b/base/src/main/java/org/killbill/billing/platform/config/PropertiesWithSourceCollector.java index d7d1b4625..f4783209c 100644 --- a/base/src/main/java/org/killbill/billing/platform/config/PropertiesWithSourceCollector.java +++ b/base/src/main/java/org/killbill/billing/platform/config/PropertiesWithSourceCollector.java @@ -29,12 +29,12 @@ public class PropertiesWithSourceCollector { private volatile List properties = new ArrayList<>(); private final Object lock = new Object(); - public void addProperties(String source, Map props) { + public void addProperties(final String source, final Map props) { synchronized (lock) { - List newList = new ArrayList<>(properties); + final List updatedProperties = new ArrayList<>(properties); props.forEach((key, value) -> - newList.add(new PropertyWithSource(source, key, value))); - this.properties = Collections.unmodifiableList(newList); + updatedProperties.add(new PropertyWithSource(source, key, value))); + this.properties = Collections.unmodifiableList(updatedProperties); } } diff --git a/platform-test/src/main/java/org/killbill/billing/platform/test/config/TestKillbillConfigSource.java b/platform-test/src/main/java/org/killbill/billing/platform/test/config/TestKillbillConfigSource.java index 4709b94c1..1ebce418b 100644 --- a/platform-test/src/main/java/org/killbill/billing/platform/test/config/TestKillbillConfigSource.java +++ b/platform-test/src/main/java/org/killbill/billing/platform/test/config/TestKillbillConfigSource.java @@ -54,7 +54,7 @@ public TestKillbillConfigSource(@Nullable final String file, @Nullable final Cla // Set default System Properties before creating the instance of DBTestingHelper. Whereas MySQL loads its // driver at startup, h2 loads it statically and we need System Properties set at that point - populateDefaultProperties(); + populateDefaultProperties(extraDefaults); if (dbTestingHelperKlass != null) { final PlatformDBTestingHelper dbTestingHelper = (PlatformDBTestingHelper) dbTestingHelperKlass.getDeclaredMethod("get").invoke(null); @@ -71,7 +71,7 @@ public TestKillbillConfigSource(@Nullable final String file, @Nullable final Cla this.extraDefaults = extraDefaults; // extraDefaults changed, need to reload defaults - populateDefaultProperties(); + populateDefaultProperties(extraDefaults); } @Override From ee3ac640d43b07181a54e90e58f6b9da15e43a22 Mon Sep 17 00:00:00 2001 From: Vijay N Date: Fri, 31 Oct 2025 01:26:51 +0530 Subject: [PATCH 2/7] * Added ImmutableSystemProperties to properties collector. * Removed property conflict string from property values. * Added a unit test to verify getPropertiesBySource() behavior and source precedence. --- .../config/DefaultKillbillConfigSource.java | 63 +++++++------------ .../TestDefaultKillbillConfigSource.java | 39 ++++++++++-- 2 files changed, 55 insertions(+), 47 deletions(-) diff --git a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java index 7fb3d4359..3d2e820c8 100644 --- a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java +++ b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java @@ -182,6 +182,8 @@ protected void populateDefaultProperties(final Map extraDefaultP } } + final Map immutableProps = new HashMap<>(); + final Properties defaultSystemProperties = getDefaultSystemProperties(); for (final String propertyName : defaultSystemProperties.stringPropertyNames()) { @@ -207,6 +209,10 @@ protected void populateDefaultProperties(final Map extraDefaultP // System.setProperty(propertyName, GMT_ID); TimeZone.setDefault(TimeZone.getTimeZone(GMT_ID)); + + immutableProps.put(PROP_USER_TIME_ZONE, GMT_ID); + properties.put(propertyName, GMT_ID); + continue; } @@ -214,6 +220,10 @@ protected void populateDefaultProperties(final Map extraDefaultP if (System.getProperty(propertyName) == null) { System.setProperty(propertyName, defaultSystemProperties.get(propertyName).toString()); } + + if (properties.get(propertyName) == null) { + properties.put(propertyName, defaultSystemProperties.get(propertyName)); + } } // WARN for missing PROP_SECURITY_EGD @@ -228,6 +238,10 @@ protected void populateDefaultProperties(final Map extraDefaultP } } + if (!immutableProps.isEmpty()) { + propertiesCollector.addProperties("ImmutableSystemProperties", immutableProps); + } + defaultSystemProperties.putAll(defaultProperties); final Map propsMap = propertiesToMap(defaultSystemProperties); @@ -252,7 +266,6 @@ public Map> getPropertiesBySource() { } }); - final Map> propertiesBySource = propertiesCollector.getPropertiesBySource(); final Map effectiveProperties = getCurrentEffectiveProperties(); @@ -262,7 +275,8 @@ public Map> getPropertiesBySource() { propName -> new LinkedHashSet<>()).add(source)); }); - final List sourceOrder = Arrays.asList("EnvironmentVariables", + final List sourceOrder = Arrays.asList("ImmutableSystemProperties", + "EnvironmentVariables", "RuntimeConfiguration", "KillBillDefaults"); @@ -298,30 +312,15 @@ public Map> getPropertiesBySource() { final boolean isEffectiveSource = isEffectiveSourceForProperty(propertyKey, source, sourceOrder, propertyToSources); - final String displayValue; - if (isEffectiveSource) { - if (hasConflict) { - if (!warnedConflicts.contains(propertyKey)) { - warnedConflicts.add(propertyKey); - logger.warn("Property conflict detected for '{}': defined in sources {} - using value from '{}': '{}'", - propertyKey, new ArrayList<>(sources), source, effectiveValue); - } - - final List overridden = getOverriddenSources(source, sources, sourceOrder); - if (!overridden.isEmpty()) { - displayValue = effectiveValue + " [ACTIVE -- overrides: " + String.join(", ", overridden) + "]"; - } else { - displayValue = effectiveValue + " [ACTIVE]"; - } - } else { - displayValue = effectiveValue; + if (isEffectiveSource && hasConflict) { + if (!warnedConflicts.contains(propertyKey)) { + warnedConflicts.add(propertyKey); + logger.warn("Property conflict detected for '{}': defined in sources {} - using value from '{}': '{}'", + propertyKey, new ArrayList<>(sources), source, effectiveValue); } - } else { - final String effectiveSourceName = getEffectiveSourceName(propertyKey, sourceOrder, propertyToSources); - displayValue = effectiveValue + " [OVERRIDDEN by " + effectiveSourceName + "]"; } - sourceProperties.put(propertyKey, displayValue); + sourceProperties.put(propertyKey, effectiveValue); } if (!sourceProperties.isEmpty()) { @@ -388,24 +387,6 @@ private Map getCurrentEffectiveProperties() { return effectiveProps; } - private List getOverriddenSources(final String currentSource, - final Set allSources, - final List sourceOrder) { - final List overridden = new ArrayList<>(); - final int currentIndex = sourceOrder.indexOf(currentSource); - - for (final String source : allSources) { - if (!source.equals(currentSource)) { - final int sourceIndex = sourceOrder.indexOf(source); - if (sourceIndex > currentIndex && sourceIndex != -1) { - overridden.add(source); - } - } - } - - return overridden; - } - @VisibleForTesting public void setProperty(final String propertyName, final Object propertyValue) { properties.put(propertyName, propertyValue); diff --git a/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java b/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java index 8e6191a08..f8fc68700 100644 --- a/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java +++ b/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java @@ -21,7 +21,10 @@ import java.io.IOException; import java.net.URISyntaxException; +import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.jasypt.encryption.pbe.StandardPBEStringEncryptor; @@ -70,9 +73,12 @@ public void testGetProperties() throws URISyntaxException, IOException { @Test(groups = "fast") public void testGetPropertiesBySource() throws URISyntaxException, IOException { + System.setProperty("org.killbill.dao.user", "root"); + System.setProperty("org.killbill.dao.password", "password"); + final Map configuration = new HashMap<>(); - configuration.put("org.killbill.dao.user", "root"); - configuration.put("org.killbill.dao.password", "password"); + configuration.put("org.killbill.server.shutdownDelay", "1s"); + configuration.put("org.killbill.billing.osgi.dao.logLevel", "ERROR"); final OSGIConfigProperties configSource = new DefaultKillbillConfigSource(null, configuration); @@ -81,10 +87,31 @@ public void testGetPropertiesBySource() throws URISyntaxException, IOException { Assert.assertNotNull(propsBySource); Assert.assertFalse(propsBySource.isEmpty()); - final Map defaultProps = propsBySource.get("ExtraDefaultProperties"); - Assert.assertNotNull(defaultProps); - Assert.assertEquals(defaultProps.get("org.killbill.dao.user"), "root"); - Assert.assertEquals(defaultProps.get("org.killbill.dao.password"), "password"); + Assert.assertTrue(propsBySource.containsKey("ImmutableSystemProperties")); + + final Map immutableProps = propsBySource.get("ImmutableSystemProperties"); + Assert.assertEquals(immutableProps.get("user.timezone"), "GMT"); + + Assert.assertTrue(propsBySource.containsKey("RuntimeConfiguration")); + + final Map runtimeConfig = propsBySource.get("RuntimeConfiguration"); + Assert.assertEquals(runtimeConfig.get("org.killbill.dao.user"), "root"); + Assert.assertEquals(runtimeConfig.get("org.killbill.dao.password"), "password"); + + Assert.assertTrue(propsBySource.containsKey("KillBillDefaults")); + + final Map killBillDefaults = propsBySource.get("KillBillDefaults"); + Assert.assertEquals(killBillDefaults.get("org.killbill.server.enableJasypt"), "false"); + Assert.assertEquals(killBillDefaults.get("org.killbill.persistent.bus.external.tableName"), "bus_ext_events"); + Assert.assertEquals(killBillDefaults.get("org.slf4j.simpleLogger.log.jdbc"), "ERROR"); + + final List actualSourceOrder = new ArrayList<>(propsBySource.keySet()); + + final List expectedPrecedenceOrder = Arrays.asList("ImmutableSystemProperties", + "RuntimeConfiguration", + "KillBillDefaults"); + + Assert.assertEquals(actualSourceOrder, expectedPrecedenceOrder); } @Test(groups = "fast") From bd22b4c752aae39e5820ad293fc87f962cccc95b Mon Sep 17 00:00:00 2001 From: Vijay N Date: Sat, 1 Nov 2025 07:45:31 +0530 Subject: [PATCH 3/7] Fixed property resolution order --- .../config/DefaultKillbillConfigSource.java | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java index 3d2e820c8..2f9b071b8 100644 --- a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java +++ b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java @@ -173,7 +173,7 @@ private Properties loadPropertiesFromFileOrSystemProperties() { protected void populateDefaultProperties(final Map extraDefaultProperties) { final Properties defaultProperties = getDefaultProperties(); - extraDefaultProperties.forEach(defaultProperties::putIfAbsent); + defaultProperties.putAll(extraDefaultProperties); for (final String propertyName : defaultProperties.stringPropertyNames()) { // Let the user override these properties @@ -361,23 +361,6 @@ private boolean isEffectiveSourceForProperty(final String propertyKey, return false; } - private String getEffectiveSourceName(final String propertyKey, - final List sourceOrder, - final Map> propertyToSources) { - final Set sources = propertyToSources.get(propertyKey); - if (sources == null) { - return "unknown"; - } - - for (final String source : sourceOrder) { - if (sources.contains(source)) { - return source; - } - } - - return "unknown"; - } - private Map getCurrentEffectiveProperties() { final Map effectiveProps = new HashMap<>(); From ead5cc0e1fe35fd9f473bef8f02282849429cc9a Mon Sep 17 00:00:00 2001 From: Vijay N Date: Mon, 10 Nov 2025 22:50:51 +0530 Subject: [PATCH 4/7] * Refactored to use a single source of truth for getProperties() and getPropertiesBySource(). * Cached computed results for improved performance. * Added unit tests. --- .../config/DefaultKillbillConfigSource.java | 322 ++++++++++-------- .../TestDefaultKillbillConfigSource.java | 132 ++++++- 2 files changed, 304 insertions(+), 150 deletions(-) diff --git a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java index 2f9b071b8..27f562e59 100644 --- a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java +++ b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java @@ -76,10 +76,17 @@ public class DefaultKillbillConfigSource implements KillbillConfigSource, OSGICo private static volatile int GMT_WARNING = NOT_SHOWN; private static volatile int ENTROPY_WARNING = NOT_SHOWN; - private final PropertiesWithSourceCollector propertiesCollector; + private static final List HIGH_TO_LOW_PRIORITY_ORDER = + Collections.unmodifiableList(Arrays.asList("ImmutableSystemProperties", + "EnvironmentVariables", + "RuntimeConfiguration", + "KillBillDefaults")); + private final PropertiesWithSourceCollector propertiesCollector; private final Properties properties; + private volatile Map> cachedPropertiesBySource; + public DefaultKillbillConfigSource() throws IOException, URISyntaxException { this((String) null); } @@ -124,23 +131,132 @@ public String getString(final String propertyName) { @Override public Properties getProperties() { final Properties result = new Properties(); - // using properties.stringPropertyNames() because `result.putAll(properties)` not working when running inside - // tomcat, if we put configuration in tomcat's catalina.properties - // See: - // - https://github.com/killbill/technical-support/issues/61 - // - https://github.com/killbill/technical-support/issues/67 - // - // We have TestDefaultKillbillConfigSource#testGetProperties() that cover this, but seems like this is similar - // to one of our chicken-egg problem? (see loadPropertiesFromFileOrSystemProperties() below) - properties.stringPropertyNames().forEach(key -> result.setProperty(key, properties.getProperty(key))); + getPropertiesBySource().forEach((source, props) -> props.forEach(result::setProperty)); + + return result; + } + + @Override + public Map> getPropertiesBySource() { + if (cachedPropertiesBySource == null) { + synchronized (lock) { + if (cachedPropertiesBySource == null) { + cachedPropertiesBySource = computePropertiesBySource(); + } + } + } + + return Collections.unmodifiableMap(cachedPropertiesBySource); + } + + private Map> computePropertiesBySource() { + final Map> runtimeBySource = RuntimeConfigRegistry.getAllBySource(); + runtimeBySource.forEach((source, props) -> { + if (!props.isEmpty()) { + propertiesCollector.addProperties(source, props); + } + }); + + final Map effectiveMap = new LinkedHashMap<>(); + properties.stringPropertyNames().forEach(key -> effectiveMap.put(key, properties.getProperty(key))); RuntimeConfigRegistry.getAll().forEach((key, value) -> { - if (!result.containsKey(key)) { - result.setProperty(key, value); + if (!effectiveMap.containsKey(key)) { + effectiveMap.put(key, value); } }); - return result; + final Map> collectorBySource = propertiesCollector.getPropertiesBySource(); + + final Map> propertyToSources = new HashMap<>(); + collectorBySource.forEach((source, properties) -> { + properties.forEach(property -> { + propertyToSources.computeIfAbsent(property.getKey(), k -> new LinkedHashSet<>()).add(source); + }); + }); + + final Set warnedConflicts = new HashSet<>(); + final Map> result = new LinkedHashMap<>(); + + for (final String source : HIGH_TO_LOW_PRIORITY_ORDER) { + final List properties = collectorBySource.get(source); + if (properties == null || properties.isEmpty()) { + continue; + } + + final Map sourceMap = new LinkedHashMap<>(); + final Set processedKeys = new HashSet<>(); + + for (final PropertyWithSource prop : properties) { + final String propertyKey = prop.getKey(); + + if (processedKeys.contains(propertyKey)) { + continue; + } + processedKeys.add(propertyKey); + + final String effectiveValue = prop.getValue(); + + if (effectiveValue == null) { + continue; + } + + if (!isEffectiveSourceForProperty(propertyKey, source, collectorBySource) && HIGH_TO_LOW_PRIORITY_ORDER.contains(source)) { + continue; + } + + final Set sources = propertyToSources.get(propertyKey); + if (sources != null && sources.size() > 1 && !warnedConflicts.contains(propertyKey)) { + warnedConflicts.add(propertyKey); + logger.warn("Property conflict detected for '{}': defined in sources {} - using value from '{}': '{}'", + propertyKey, new ArrayList<>(sources), source, effectiveValue); + } + + sourceMap.put(propertyKey, effectiveValue); + } + + if (!sourceMap.isEmpty()) { + result.put(source, Collections.unmodifiableMap(sourceMap)); + } + } + + collectorBySource.forEach((source, properties) -> { + if (HIGH_TO_LOW_PRIORITY_ORDER.contains(source)) { + return; + } + + final Map sourceMap = new LinkedHashMap<>(); + for (final PropertyWithSource prop : properties) { + final String effectiveValue = effectiveMap.get(prop.getKey()); + if (effectiveValue != null && isEffectiveSourceForProperty(prop.getKey(), source, collectorBySource)) { + sourceMap.put(prop.getKey(), effectiveValue); + } + } + + if (!sourceMap.isEmpty()) { + result.put(source, Collections.unmodifiableMap(sourceMap)); + } + }); + + return Collections.unmodifiableMap(result); + } + + private boolean isEffectiveSourceForProperty(final String key, final String sourceToCheck, + final Map> propertiesBySource) { + final List sourcesForKey = new ArrayList<>(); + propertiesBySource.forEach((source, props) -> { + if (props.stream().anyMatch(p -> p.getKey().equals(key))) { + sourcesForKey.add(source); + } + }); + + for (final String prioritySource : HIGH_TO_LOW_PRIORITY_ORDER) { + if (sourcesForKey.contains(prioritySource)) { + return prioritySource.equals(sourceToCheck); + } + } + + return !sourcesForKey.isEmpty() && sourcesForKey.get(0).equals(sourceToCheck); } private Properties loadPropertiesFromFileOrSystemProperties() { @@ -165,14 +281,12 @@ private Properties loadPropertiesFromFileOrSystemProperties() { } propertiesCollector.addProperties("RuntimeConfiguration", propertiesToMap(System.getProperties())); - return new Properties(System.getProperties()); } @VisibleForTesting protected void populateDefaultProperties(final Map extraDefaultProperties) { final Properties defaultProperties = getDefaultProperties(); - defaultProperties.putAll(extraDefaultProperties); for (final String propertyName : defaultProperties.stringPropertyNames()) { @@ -212,7 +326,6 @@ protected void populateDefaultProperties(final Map extraDefaultP immutableProps.put(PROP_USER_TIME_ZONE, GMT_ID); properties.put(propertyName, GMT_ID); - continue; } @@ -248,131 +361,17 @@ protected void populateDefaultProperties(final Map extraDefaultP propertiesCollector.addProperties("KillBillDefaults", propsMap); } - @Override - public Map> getPropertiesBySource() { - final Map currentProps = new HashMap<>(); - properties.stringPropertyNames().forEach(key -> currentProps.put(key, properties.getProperty(key))); - - final Map> runtimeBySource = RuntimeConfigRegistry.getAllBySource(); - runtimeBySource.forEach((source, props) -> { - final Map filteredProps = new HashMap<>(); - props.forEach((key, value) -> { - if (!currentProps.containsKey(key)) { - filteredProps.put(key, value); - } - }); - if (!filteredProps.isEmpty()) { - propertiesCollector.addProperties(source, filteredProps); - } - }); - - final Map> propertiesBySource = propertiesCollector.getPropertiesBySource(); - final Map effectiveProperties = getCurrentEffectiveProperties(); - - final Map> propertyToSources = new HashMap<>(); - propertiesBySource.forEach((source, properties) -> { - properties.forEach(property -> propertyToSources.computeIfAbsent(property.getKey(), - propName -> new LinkedHashSet<>()).add(source)); - }); - - final List sourceOrder = Arrays.asList("ImmutableSystemProperties", - "EnvironmentVariables", - "RuntimeConfiguration", - "KillBillDefaults"); - - final Set warnedConflicts = new HashSet<>(); - - final Map> result = new LinkedHashMap<>(); - - for (final String source : sourceOrder) { - final List properties = propertiesBySource.get(source); - if (properties == null || properties.isEmpty()) { - continue; - } - - final Map sourceProperties = new LinkedHashMap<>(); - - final Set processedKeys = new HashSet<>(); - - for (final PropertyWithSource prop : properties) { - final String propertyKey = prop.getKey(); - - if (processedKeys.contains(propertyKey)) { - continue; - } - processedKeys.add(propertyKey); - - final String effectiveValue = effectiveProperties.get(propertyKey); - if (effectiveValue == null) { - continue; - } - - final Set sources = propertyToSources.get(propertyKey); - final boolean hasConflict = sources != null && sources.size() > 1; - - final boolean isEffectiveSource = isEffectiveSourceForProperty(propertyKey, source, sourceOrder, propertyToSources); - - if (isEffectiveSource && hasConflict) { - if (!warnedConflicts.contains(propertyKey)) { - warnedConflicts.add(propertyKey); - logger.warn("Property conflict detected for '{}': defined in sources {} - using value from '{}': '{}'", - propertyKey, new ArrayList<>(sources), source, effectiveValue); - } - } - - sourceProperties.put(propertyKey, effectiveValue); - } - - if (!sourceProperties.isEmpty()) { - result.put(source, Collections.unmodifiableMap(sourceProperties)); - } - } - - propertiesBySource.forEach((source, properties) -> { - if (!sourceOrder.contains(source) && !result.containsKey(source)) { - final Map sourceProperties = new LinkedHashMap<>(); - properties.forEach(prop -> { - sourceProperties.put(prop.getKey(), prop.getValue()); - }); - if (!sourceProperties.isEmpty()) { - result.put(source, Collections.unmodifiableMap(sourceProperties)); - } - } - }); - - return Collections.unmodifiableMap(result); - } - - private boolean isEffectiveSourceForProperty(final String propertyKey, - final String currentSource, - final List sourceOrder, - final Map> propertyToSources) { - final Set sourcesForProperty = propertyToSources.get(propertyKey); - if (sourcesForProperty == null || !sourcesForProperty.contains(currentSource)) { - return false; - } - - for (final String source : sourceOrder) { - if (sourcesForProperty.contains(source)) { - return source.equals(currentSource); - } - } - - return false; - } - - private Map getCurrentEffectiveProperties() { - final Map effectiveProps = new HashMap<>(); - - properties.stringPropertyNames() - .forEach(key -> effectiveProps.put(key, properties.getProperty(key))); - - return effectiveProps; - } - @VisibleForTesting public void setProperty(final String propertyName, final Object propertyValue) { properties.put(propertyName, propertyValue); + + final Map override = new HashMap<>(); + override.put(propertyName, String.valueOf(propertyValue)); + propertiesCollector.addProperties("RuntimeConfiguration", override); + + synchronized (lock) { + this.cachedPropertiesBySource = null; + } } @VisibleForTesting @@ -382,6 +381,7 @@ protected Properties getDefaultProperties() { properties.put("org.killbill.persistent.bus.external.historyTableName", "bus_ext_events_history"); properties.put(ENABLE_JASYPT_DECRYPTION, "false"); properties.put(LOOKUP_ENVIRONMENT_VARIABLES, "true"); + return properties; } @@ -402,8 +402,7 @@ protected Properties getDefaultSystemProperties() { private void overrideWithEnvironmentVariables() { // Find all Kill Bill properties in the environment variables - final Map env = System.getenv(); - + final Map env = getEnvironmentVariables(); final Map kbEnvVariables = new HashMap<>(); for (final Entry entry : env.entrySet()) { @@ -421,6 +420,11 @@ private void overrideWithEnvironmentVariables() { propertiesCollector.addProperties("EnvironmentVariables", kbEnvVariables); } + @VisibleForTesting + protected Map getEnvironmentVariables() { + return System.getenv(); + } + public List getAllPropertiesWithSource() { return propertiesCollector.getAllProperties(); } @@ -434,6 +438,8 @@ private void decryptJasyptProperties() { final String password = getEnvironmentVariable(JASYPT_ENCRYPTOR_PASSWORD_KEY, System.getProperty(JASYPT_ENCRYPTOR_PASSWORD_KEY)); final String algorithm = getEnvironmentVariable(JASYPT_ENCRYPTOR_ALGORITHM_KEY, System.getProperty(JASYPT_ENCRYPTOR_ALGORITHM_KEY)); + final Map> decryptedBySource = new HashMap<>(); + final Enumeration keys = properties.keys(); final StandardPBEStringEncryptor encryptor = initializeEncryptor(password, algorithm); // Iterate over all properties and decrypt ones that match @@ -441,8 +447,39 @@ private void decryptJasyptProperties() { final String key = (String) keys.nextElement(); final String value = (String) properties.get(key); final Optional decryptableValue = decryptableValue(value); - decryptableValue.ifPresent(s -> properties.setProperty(key, encryptor.decrypt(s))); + if (decryptableValue.isPresent()) { + final String decryptedValue = encryptor.decrypt(decryptableValue.get()); + properties.setProperty(key, decryptedValue); + + final String source = findSourceForProperty(key); + if (source != null) { + decryptedBySource.computeIfAbsent(source, k -> new HashMap<>()) + .put(key, decryptedValue); + } + } } + + decryptedBySource.forEach(propertiesCollector::addProperties); + } + + private String findSourceForProperty(final String key) { + final Map> propertiesBySource = propertiesCollector.getPropertiesBySource(); + + for (final String source : HIGH_TO_LOW_PRIORITY_ORDER) { + final List props = propertiesBySource.get(source); + if (props != null && props.stream().anyMatch(p -> p.getKey().equals(key))) { + return source; + } + } + + for (final Map.Entry> entry : propertiesBySource.entrySet()) { + if (!HIGH_TO_LOW_PRIORITY_ORDER.contains(entry.getKey()) && + entry.getValue().stream().anyMatch(p -> p.getKey().equals(key))) { + return entry.getKey(); + } + } + + return null; } private StandardPBEStringEncryptor initializeEncryptor(final String password, final String algorithm) { @@ -489,7 +526,6 @@ private Map propertiesToMap(final Properties props) { for (final Map.Entry entry : props.entrySet()) { propertiesMap.put(String.valueOf(entry.getKey()), String.valueOf(entry.getValue())); } - return propertiesMap; } -} +} \ No newline at end of file diff --git a/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java b/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java index f8fc68700..184f23914 100644 --- a/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java +++ b/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java @@ -26,6 +26,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Properties; import org.jasypt.encryption.pbe.StandardPBEStringEncryptor; import org.jasypt.exceptions.EncryptionOperationNotPossibleException; @@ -58,6 +59,101 @@ public void setup() { System.clearProperty(ENCRYPTED_PROPERTY_2); } + @Test + public void testGetPropertiesBySourceContainsExpectedSources() throws URISyntaxException, IOException { + final Map runtimeConfig = new HashMap<>(); + runtimeConfig.put("org.killbill.dao.user", "root"); + + final OSGIConfigProperties configSource = new DefaultKillbillConfigSource(null, runtimeConfig){ + @Override + protected Map getEnvironmentVariables() { + final Map mockEnv = new HashMap<>(); + mockEnv.put(ENVIRONMENT_VARIABLE_PREFIX + "org_killbill_dao_user", "root"); + return mockEnv; + } + }; + + final Map> propsBySource = configSource.getPropertiesBySource(); + + Assert.assertTrue(propsBySource.containsKey("ImmutableSystemProperties")); + Assert.assertTrue(propsBySource.containsKey("EnvironmentVariables")); + Assert.assertTrue(propsBySource.containsKey("RuntimeConfiguration")); + Assert.assertTrue(propsBySource.containsKey("KillBillDefaults")); + } + + @Test(groups = "fast") + public void testGetPropertiesAndGetPropertiesBySourceAreInSync() throws URISyntaxException, IOException { + // RuntimeConfiguration + System.setProperty("org.killbill.dao.user", "root"); + System.setProperty("org.killbill.dao.password", "password"); + + // KillBillDefaults + final Map killbillDefaultConfig = new HashMap<>(); + killbillDefaultConfig.put("org.killbill.server.shutdownDelay", "3s"); + killbillDefaultConfig.put("org.killbill.billing.osgi.dao.logLevel", "INFO"); + + // ImmutableSystemProperties + killbillDefaultConfig.put("user.timezone", "GMT"); + + // EnvironmentVariables + final OSGIConfigProperties configSource = new DefaultKillbillConfigSource(null, killbillDefaultConfig) { + @Override + protected Map getEnvironmentVariables() { + final Map mockEnv = new HashMap<>(); + mockEnv.put(ENVIRONMENT_VARIABLE_PREFIX + "org_killbill_dao_healthCheckConnectionTimeout", "11s"); + return mockEnv; + } + }; + + final Properties mergedProperties = configSource.getProperties(); + final Map> propertiesBySource = configSource.getPropertiesBySource(); + + final Map allProperties = new HashMap<>(); + propertiesBySource.forEach((source, props) -> allProperties.putAll(props)); + + for (final String key : mergedProperties.stringPropertyNames()) { + final String valueFromFlat = mergedProperties.getProperty(key); + final String valueFromSource = allProperties.get(key); + + Assert.assertNotNull(valueFromSource); + Assert.assertEquals(valueFromFlat, valueFromSource); + } + + // Verify that no property appears in multiple sources + final Map propertyCount = new HashMap<>(); + propertiesBySource.forEach((source, props) -> { + props.keySet().forEach(key -> propertyCount.put(key, propertyCount.getOrDefault(key, 0) + 1)); + }); + + propertyCount.forEach((key, count) -> { + Assert.assertEquals(count.intValue(), 1); + }); + + Assert.assertEquals(mergedProperties.size(), allProperties.size()); + } + + @Test + public void testConflictResolutionPriority() throws Exception { + // RuntimeConfiguration + System.setProperty("org.killbill.test", "lowValue"); + + final DefaultKillbillConfigSource testSource = new DefaultKillbillConfigSource((String) null) { + @Override + protected Map getEnvironmentVariables() { + final Map mockEnv = new HashMap<>(); + mockEnv.put(ENVIRONMENT_VARIABLE_PREFIX + "org_killbill_test", "highValue"); + return mockEnv; + } + }; + + testSource.setProperty("org.killbill.test", "lowValue"); + + final Properties properties = testSource.getProperties(); + + final String effectiveValue = properties.getProperty("org.killbill.test"); + Assert.assertEquals(effectiveValue, "highValue"); + } + @Test(groups = "fast") public void testGetProperties() throws URISyntaxException, IOException { final Map configuration = new HashMap<>(); @@ -73,14 +169,29 @@ public void testGetProperties() throws URISyntaxException, IOException { @Test(groups = "fast") public void testGetPropertiesBySource() throws URISyntaxException, IOException { + // RuntimeConfiguration System.setProperty("org.killbill.dao.user", "root"); System.setProperty("org.killbill.dao.password", "password"); - final Map configuration = new HashMap<>(); - configuration.put("org.killbill.server.shutdownDelay", "1s"); - configuration.put("org.killbill.billing.osgi.dao.logLevel", "ERROR"); + // KillBillDefaults + final Map killbillDefaultConfig = new HashMap<>(); + killbillDefaultConfig.put("org.killbill.server.shutdownDelay", "3s"); + killbillDefaultConfig.put("org.killbill.billing.osgi.dao.logLevel", "INFO"); - final OSGIConfigProperties configSource = new DefaultKillbillConfigSource(null, configuration); + // ImmutableSystemProperties + killbillDefaultConfig.put("user.timezone", "GMT"); + + // EnvironmentVariables + final OSGIConfigProperties configSource = new DefaultKillbillConfigSource(null, killbillDefaultConfig) { + @Override + protected Map getEnvironmentVariables() { + final Map mockEnv = new HashMap<>(); + mockEnv.put(ENVIRONMENT_VARIABLE_PREFIX + "org_killbill_dao_healthCheckConnectionTimeout", "11s"); + mockEnv.put(ENVIRONMENT_VARIABLE_PREFIX + "org_killbill_billing_osgi_dao_maxActive", "99"); + + return mockEnv; + } + }; final Map> propsBySource = configSource.getPropertiesBySource(); @@ -92,6 +203,13 @@ public void testGetPropertiesBySource() throws URISyntaxException, IOException { final Map immutableProps = propsBySource.get("ImmutableSystemProperties"); Assert.assertEquals(immutableProps.get("user.timezone"), "GMT"); + + Assert.assertTrue(propsBySource.containsKey("EnvironmentVariables")); + + final Map environmentVariables = propsBySource.get("EnvironmentVariables"); + Assert.assertEquals(environmentVariables.get("org.killbill.dao.healthCheckConnectionTimeout"), "11s"); + Assert.assertEquals(environmentVariables.get("org.killbill.billing.osgi.dao.maxActive"), "99"); + Assert.assertTrue(propsBySource.containsKey("RuntimeConfiguration")); final Map runtimeConfig = propsBySource.get("RuntimeConfiguration"); @@ -101,13 +219,13 @@ public void testGetPropertiesBySource() throws URISyntaxException, IOException { Assert.assertTrue(propsBySource.containsKey("KillBillDefaults")); final Map killBillDefaults = propsBySource.get("KillBillDefaults"); - Assert.assertEquals(killBillDefaults.get("org.killbill.server.enableJasypt"), "false"); - Assert.assertEquals(killBillDefaults.get("org.killbill.persistent.bus.external.tableName"), "bus_ext_events"); - Assert.assertEquals(killBillDefaults.get("org.slf4j.simpleLogger.log.jdbc"), "ERROR"); + Assert.assertEquals(killBillDefaults.get("org.killbill.server.shutdownDelay"), "3s"); + Assert.assertEquals(killBillDefaults.get("org.killbill.billing.osgi.dao.logLevel"), "INFO"); final List actualSourceOrder = new ArrayList<>(propsBySource.keySet()); final List expectedPrecedenceOrder = Arrays.asList("ImmutableSystemProperties", + "EnvironmentVariables", "RuntimeConfiguration", "KillBillDefaults"); From d8c818cf9a0b49164fca51ce3b44e07d1ab6d7a1 Mon Sep 17 00:00:00 2001 From: Vijay N Date: Mon, 10 Nov 2025 23:06:33 +0530 Subject: [PATCH 5/7] * Fixed formatting. --- .../platform/config/TestDefaultKillbillConfigSource.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java b/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java index 184f23914..40a01ed7a 100644 --- a/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java +++ b/base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java @@ -64,7 +64,7 @@ public void testGetPropertiesBySourceContainsExpectedSources() throws URISyntaxE final Map runtimeConfig = new HashMap<>(); runtimeConfig.put("org.killbill.dao.user", "root"); - final OSGIConfigProperties configSource = new DefaultKillbillConfigSource(null, runtimeConfig){ + final OSGIConfigProperties configSource = new DefaultKillbillConfigSource(null, runtimeConfig) { @Override protected Map getEnvironmentVariables() { final Map mockEnv = new HashMap<>(); @@ -97,8 +97,8 @@ public void testGetPropertiesAndGetPropertiesBySourceAreInSync() throws URISynta // EnvironmentVariables final OSGIConfigProperties configSource = new DefaultKillbillConfigSource(null, killbillDefaultConfig) { - @Override - protected Map getEnvironmentVariables() { + @Override + protected Map getEnvironmentVariables() { final Map mockEnv = new HashMap<>(); mockEnv.put(ENVIRONMENT_VARIABLE_PREFIX + "org_killbill_dao_healthCheckConnectionTimeout", "11s"); return mockEnv; @@ -203,7 +203,6 @@ protected Map getEnvironmentVariables() { final Map immutableProps = propsBySource.get("ImmutableSystemProperties"); Assert.assertEquals(immutableProps.get("user.timezone"), "GMT"); - Assert.assertTrue(propsBySource.containsKey("EnvironmentVariables")); final Map environmentVariables = propsBySource.get("EnvironmentVariables"); From e4613cdcafe90f9423c64de84cb940c15d1b267f Mon Sep 17 00:00:00 2001 From: Vijay N Date: Thu, 20 Nov 2025 17:48:07 +0530 Subject: [PATCH 6/7] Added debug msg to TestJNDIManager --- .../config/DefaultKillbillConfigSource.java | 204 +++++++++--------- 1 file changed, 104 insertions(+), 100 deletions(-) diff --git a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java index 27f562e59..4be06ec41 100644 --- a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java +++ b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java @@ -24,11 +24,9 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.Enumeration; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -83,7 +81,6 @@ public class DefaultKillbillConfigSource implements KillbillConfigSource, OSGICo "KillBillDefaults")); private final PropertiesWithSourceCollector propertiesCollector; - private final Properties properties; private volatile Map> cachedPropertiesBySource; @@ -103,10 +100,10 @@ public DefaultKillbillConfigSource(@Nullable final String file, final Map propsMap = propertiesToMap(properties); propertiesCollector.addProperties("RuntimeConfiguration", propsMap); @@ -114,18 +111,31 @@ public DefaultKillbillConfigSource(@Nullable final String file, final Map> bySource = getPropertiesBySource(); + + for (final Map sourceProps : bySource.values()) { + final String value = sourceProps.get(propertyName); + if (value != null) { + return value; + } + } + + return null; } @Override @@ -142,7 +152,7 @@ public Map> getPropertiesBySource() { if (cachedPropertiesBySource == null) { synchronized (lock) { if (cachedPropertiesBySource == null) { - cachedPropertiesBySource = computePropertiesBySource(); + rebuildCache(); } } } @@ -150,34 +160,45 @@ public Map> getPropertiesBySource() { return Collections.unmodifiableMap(cachedPropertiesBySource); } + protected void rebuildCache() { + cachedPropertiesBySource = computePropertiesBySource(); + } + + private void invalidateCache() { + synchronized (lock) { + cachedPropertiesBySource = null; + } + } + private Map> computePropertiesBySource() { final Map> runtimeBySource = RuntimeConfigRegistry.getAllBySource(); + + System.out.println("RuntimeConfigRegistry.getAllBySource..."); + runtimeBySource.forEach((s, stringStringMap) -> { + System.out.println(s); + stringStringMap.forEach((s1, s2) -> System.out.println(" " + s1 + ": " + s2)); + }); + runtimeBySource.forEach((source, props) -> { if (!props.isEmpty()) { propertiesCollector.addProperties(source, props); } }); - final Map effectiveMap = new LinkedHashMap<>(); - properties.stringPropertyNames().forEach(key -> effectiveMap.put(key, properties.getProperty(key))); - RuntimeConfigRegistry.getAll().forEach((key, value) -> { - if (!effectiveMap.containsKey(key)) { - effectiveMap.put(key, value); - } - }); - final Map> collectorBySource = propertiesCollector.getPropertiesBySource(); - final Map> propertyToSources = new HashMap<>(); + final Map> propertyToSources = new HashMap<>(); collectorBySource.forEach((source, properties) -> { properties.forEach(property -> { - propertyToSources.computeIfAbsent(property.getKey(), k -> new LinkedHashSet<>()).add(source); + propertyToSources.computeIfAbsent(property.getKey(), k -> new ArrayList<>()).add(source); }); }); final Set warnedConflicts = new HashSet<>(); final Map> result = new LinkedHashMap<>(); + final Set processedProperties = new HashSet<>(); + for (final String source : HIGH_TO_LOW_PRIORITY_ORDER) { final List properties = collectorBySource.get(source); if (properties == null || properties.isEmpty()) { @@ -185,34 +206,28 @@ private Map> computePropertiesBySource() { } final Map sourceMap = new LinkedHashMap<>(); - final Set processedKeys = new HashSet<>(); for (final PropertyWithSource prop : properties) { final String propertyKey = prop.getKey(); + final String propertyValue = prop.getValue(); - if (processedKeys.contains(propertyKey)) { - continue; - } - processedKeys.add(propertyKey); - - final String effectiveValue = prop.getValue(); - - if (effectiveValue == null) { + if (propertyValue == null) { continue; } - if (!isEffectiveSourceForProperty(propertyKey, source, collectorBySource) && HIGH_TO_LOW_PRIORITY_ORDER.contains(source)) { - continue; - } + if (!processedProperties.contains(propertyKey)) { + sourceMap.put(propertyKey, propertyValue); + processedProperties.add(propertyKey); - final Set sources = propertyToSources.get(propertyKey); - if (sources != null && sources.size() > 1 && !warnedConflicts.contains(propertyKey)) { - warnedConflicts.add(propertyKey); - logger.warn("Property conflict detected for '{}': defined in sources {} - using value from '{}': '{}'", - propertyKey, new ArrayList<>(sources), source, effectiveValue); + final List sources = propertyToSources.get(propertyKey); + if (sources != null && sources.size() > 1 && !warnedConflicts.contains(propertyKey)) { + if (shouldWarnAboutConflict(sources)) { + warnedConflicts.add(propertyKey); + logger.warn("Property conflict detected for '{}': defined in sources {} - using value from '{}': '{}'", + propertyKey, sources, source, propertyValue); + } + } } - - sourceMap.put(propertyKey, effectiveValue); } if (!sourceMap.isEmpty()) { @@ -227,9 +242,16 @@ private Map> computePropertiesBySource() { final Map sourceMap = new LinkedHashMap<>(); for (final PropertyWithSource prop : properties) { - final String effectiveValue = effectiveMap.get(prop.getKey()); - if (effectiveValue != null && isEffectiveSourceForProperty(prop.getKey(), source, collectorBySource)) { - sourceMap.put(prop.getKey(), effectiveValue); + final String propertyKey = prop.getKey(); + final String propertyValue = prop.getValue(); + + if (propertyValue == null) { + continue; + } + + if (!processedProperties.contains(propertyKey)) { + sourceMap.put(propertyKey, propertyValue); + processedProperties.add(propertyKey); } } @@ -238,28 +260,18 @@ private Map> computePropertiesBySource() { } }); - return Collections.unmodifiableMap(result); - } - - private boolean isEffectiveSourceForProperty(final String key, final String sourceToCheck, - final Map> propertiesBySource) { - final List sourcesForKey = new ArrayList<>(); - propertiesBySource.forEach((source, props) -> { - if (props.stream().anyMatch(p -> p.getKey().equals(key))) { - sourcesForKey.add(source); + RuntimeConfigRegistry.getAll().forEach((key, value) -> { + if (!processedProperties.contains(key)) { + result.computeIfAbsent("RuntimeConfigRegistry", k -> new LinkedHashMap<>()) + .put(key, value); } }); - for (final String prioritySource : HIGH_TO_LOW_PRIORITY_ORDER) { - if (sourcesForKey.contains(prioritySource)) { - return prioritySource.equals(sourceToCheck); - } - } - - return !sourcesForKey.isEmpty() && sourcesForKey.get(0).equals(sourceToCheck); + return Collections.unmodifiableMap(result); } - private Properties loadPropertiesFromFileOrSystemProperties() { + + private void loadPropertiesFromFileOrSystemProperties() { // Chicken-egg problem. It would be nice to have the property in e.g. KillbillServerConfig, // but we need to build the ConfigSource first... final String propertiesFileLocation = System.getProperty(PROPERTIES_FILE); @@ -272,7 +284,7 @@ private Properties loadPropertiesFromFileOrSystemProperties() { final Map propsMap = propertiesToMap(properties); propertiesCollector.addProperties("RuntimeConfiguration", propsMap); - return properties; + return; } catch (final IOException e) { logger.warn("Unable to access properties file, defaulting to system properties", e); } catch (final URISyntaxException e) { @@ -281,7 +293,6 @@ private Properties loadPropertiesFromFileOrSystemProperties() { } propertiesCollector.addProperties("RuntimeConfiguration", propertiesToMap(System.getProperties())); - return new Properties(System.getProperties()); } @VisibleForTesting @@ -289,10 +300,12 @@ protected void populateDefaultProperties(final Map extraDefaultP final Properties defaultProperties = getDefaultProperties(); defaultProperties.putAll(extraDefaultProperties); + final Map defaultsToAdd = new HashMap<>(); + for (final String propertyName : defaultProperties.stringPropertyNames()) { // Let the user override these properties - if (properties.get(propertyName) == null) { - properties.put(propertyName, defaultProperties.get(propertyName)); + if (!hasProperty(propertyName)) { + defaultsToAdd.put(propertyName, defaultProperties.getProperty(propertyName)); } } @@ -325,7 +338,8 @@ protected void populateDefaultProperties(final Map extraDefaultP TimeZone.setDefault(TimeZone.getTimeZone(GMT_ID)); immutableProps.put(PROP_USER_TIME_ZONE, GMT_ID); - properties.put(propertyName, GMT_ID); + // defaultsToAdd.put(propertyName, GMT_ID); + continue; } @@ -334,8 +348,8 @@ protected void populateDefaultProperties(final Map extraDefaultP System.setProperty(propertyName, defaultSystemProperties.get(propertyName).toString()); } - if (properties.get(propertyName) == null) { - properties.put(propertyName, defaultSystemProperties.get(propertyName)); + if (!hasProperty(propertyName)) { + defaultsToAdd.put(propertyName, defaultSystemProperties.getProperty(propertyName)); } } @@ -355,23 +369,29 @@ protected void populateDefaultProperties(final Map extraDefaultP propertiesCollector.addProperties("ImmutableSystemProperties", immutableProps); } - defaultSystemProperties.putAll(defaultProperties); + // defaultSystemProperties.putAll(defaultProperties); + + // final Map propsMap = propertiesToMap(defaultSystemProperties); + // propertiesCollector.addProperties("KillBillDefaults", propsMap); + + if (!defaultsToAdd.isEmpty()) { + propertiesCollector.addProperties("KillBillDefaults", defaultsToAdd); + } + } - final Map propsMap = propertiesToMap(defaultSystemProperties); - propertiesCollector.addProperties("KillBillDefaults", propsMap); + private boolean hasProperty(final String propertyName) { + return propertiesCollector.getAllProperties().stream() + .anyMatch(p -> p.getKey().equals(propertyName)); } @VisibleForTesting public void setProperty(final String propertyName, final Object propertyValue) { - properties.put(propertyName, propertyValue); - final Map override = new HashMap<>(); override.put(propertyName, String.valueOf(propertyValue)); propertiesCollector.addProperties("RuntimeConfiguration", override); - synchronized (lock) { - this.cachedPropertiesBySource = null; - } + invalidateCache(); + rebuildCache(); } @VisibleForTesting @@ -414,7 +434,6 @@ private void overrideWithEnvironmentVariables() { final String value = entry.getValue(); kbEnvVariables.put(propertyName, value); - properties.setProperty(propertyName, value); } propertiesCollector.addProperties("EnvironmentVariables", kbEnvVariables); @@ -440,18 +459,17 @@ private void decryptJasyptProperties() { final Map> decryptedBySource = new HashMap<>(); - final Enumeration keys = properties.keys(); final StandardPBEStringEncryptor encryptor = initializeEncryptor(password, algorithm); // Iterate over all properties and decrypt ones that match - while (keys.hasMoreElements()) { - final String key = (String) keys.nextElement(); - final String value = (String) properties.get(key); + final List allProperties = propertiesCollector.getAllProperties(); + for (final PropertyWithSource prop : allProperties) { + final String key = prop.getKey(); + final String value = prop.getValue(); final Optional decryptableValue = decryptableValue(value); if (decryptableValue.isPresent()) { final String decryptedValue = encryptor.decrypt(decryptableValue.get()); - properties.setProperty(key, decryptedValue); - final String source = findSourceForProperty(key); + final String source = prop.getSource(); if (source != null) { decryptedBySource.computeIfAbsent(source, k -> new HashMap<>()) .put(key, decryptedValue); @@ -462,26 +480,6 @@ private void decryptJasyptProperties() { decryptedBySource.forEach(propertiesCollector::addProperties); } - private String findSourceForProperty(final String key) { - final Map> propertiesBySource = propertiesCollector.getPropertiesBySource(); - - for (final String source : HIGH_TO_LOW_PRIORITY_ORDER) { - final List props = propertiesBySource.get(source); - if (props != null && props.stream().anyMatch(p -> p.getKey().equals(key))) { - return source; - } - } - - for (final Map.Entry> entry : propertiesBySource.entrySet()) { - if (!HIGH_TO_LOW_PRIORITY_ORDER.contains(entry.getKey()) && - entry.getValue().stream().anyMatch(p -> p.getKey().equals(key))) { - return entry.getKey(); - } - } - - return null; - } - private StandardPBEStringEncryptor initializeEncryptor(final String password, final String algorithm) { final StandardPBEStringEncryptor encryptor = new StandardPBEStringEncryptor(); @@ -528,4 +526,10 @@ private Map propertiesToMap(final Properties props) { } return propertiesMap; } -} \ No newline at end of file + + private boolean shouldWarnAboutConflict(final List sources) { + return sources != null && + sources.contains("EnvironmentVariables") && + sources.contains("RuntimeConfiguration"); + } +} From 6da3822f0be956939bbad6b145498527bcad38e2 Mon Sep 17 00:00:00 2001 From: Vijay N Date: Thu, 20 Nov 2025 20:19:37 +0530 Subject: [PATCH 7/7] * Removed redundant Properties field from DefaultKillbillConfigSource * Implemented lazy cache initialization in getPropertiesBySource() * Added getPropertyDirect() to avoid circular dependency during initialization * Simplified getString() to use cached properties after initialization * Improved addProperties logic in PropertiesWithSourceCollector * Fixed JNDI lookup to explicitly dereference Reference objects --- .../config/DefaultKillbillConfigSource.java | 49 ++++++++++++------- .../config/PropertiesWithSourceCollector.java | 13 ++++- .../billing/platform/jndi/JNDIManager.java | 13 ++++- .../test/config/TestKillbillConfigSource.java | 1 + 4 files changed, 55 insertions(+), 21 deletions(-) diff --git a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java index 4be06ec41..133f36f5c 100644 --- a/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java +++ b/base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java @@ -111,21 +111,21 @@ public DefaultKillbillConfigSource(@Nullable final String file, final Map> bySource = getPropertiesBySource(); for (final Map sourceProps : bySource.values()) { @@ -172,13 +172,6 @@ private void invalidateCache() { private Map> computePropertiesBySource() { final Map> runtimeBySource = RuntimeConfigRegistry.getAllBySource(); - - System.out.println("RuntimeConfigRegistry.getAllBySource..."); - runtimeBySource.forEach((s, stringStringMap) -> { - System.out.println(s); - stringStringMap.forEach((s1, s2) -> System.out.println(" " + s1 + ": " + s2)); - }); - runtimeBySource.forEach((source, props) -> { if (!props.isEmpty()) { propertiesCollector.addProperties(source, props); @@ -270,7 +263,6 @@ private Map> computePropertiesBySource() { return Collections.unmodifiableMap(result); } - private void loadPropertiesFromFileOrSystemProperties() { // Chicken-egg problem. It would be nice to have the property in e.g. KillbillServerConfig, // but we need to build the ConfigSource first... @@ -338,7 +330,6 @@ protected void populateDefaultProperties(final Map extraDefaultP TimeZone.setDefault(TimeZone.getTimeZone(GMT_ID)); immutableProps.put(PROP_USER_TIME_ZONE, GMT_ID); - // defaultsToAdd.put(propertyName, GMT_ID); continue; } @@ -369,11 +360,6 @@ protected void populateDefaultProperties(final Map extraDefaultP propertiesCollector.addProperties("ImmutableSystemProperties", immutableProps); } - // defaultSystemProperties.putAll(defaultProperties); - - // final Map propsMap = propertiesToMap(defaultSystemProperties); - // propertiesCollector.addProperties("KillBillDefaults", propsMap); - if (!defaultsToAdd.isEmpty()) { propertiesCollector.addProperties("KillBillDefaults", defaultsToAdd); } @@ -453,6 +439,33 @@ String fromEnvVariableName(final String key) { return key.replace(ENVIRONMENT_VARIABLE_PREFIX, "").replaceAll("_", "\\."); } + private String getPropertyDirect(final String propertyName) { + final Map> collectorBySource = propertiesCollector.getPropertiesBySource(); + + for (final String source : HIGH_TO_LOW_PRIORITY_ORDER) { + final List properties = collectorBySource.get(source); + if (properties != null) { + for (final PropertyWithSource prop : properties) { + if (prop.getKey().equals(propertyName)) { + return prop.getValue(); + } + } + } + } + + for (final Map.Entry> entry : collectorBySource.entrySet()) { + if (!HIGH_TO_LOW_PRIORITY_ORDER.contains(entry.getKey())) { + for (final PropertyWithSource prop : entry.getValue()) { + if (prop.getKey().equals(propertyName)) { + return prop.getValue(); + } + } + } + } + + return null; + } + private void decryptJasyptProperties() { final String password = getEnvironmentVariable(JASYPT_ENCRYPTOR_PASSWORD_KEY, System.getProperty(JASYPT_ENCRYPTOR_PASSWORD_KEY)); final String algorithm = getEnvironmentVariable(JASYPT_ENCRYPTOR_ALGORITHM_KEY, System.getProperty(JASYPT_ENCRYPTOR_ALGORITHM_KEY)); diff --git a/base/src/main/java/org/killbill/billing/platform/config/PropertiesWithSourceCollector.java b/base/src/main/java/org/killbill/billing/platform/config/PropertiesWithSourceCollector.java index f4783209c..8bbbbde2f 100644 --- a/base/src/main/java/org/killbill/billing/platform/config/PropertiesWithSourceCollector.java +++ b/base/src/main/java/org/killbill/billing/platform/config/PropertiesWithSourceCollector.java @@ -22,6 +22,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Collectors; public class PropertiesWithSourceCollector { @@ -32,8 +33,16 @@ public class PropertiesWithSourceCollector { public void addProperties(final String source, final Map props) { synchronized (lock) { final List updatedProperties = new ArrayList<>(properties); - props.forEach((key, value) -> - updatedProperties.add(new PropertyWithSource(source, key, value))); + + final Set keysToAdd = props.keySet(); + updatedProperties.removeIf(property -> property.getSource().equals(source) && keysToAdd.contains(property.getKey())); + + props.forEach((key, value) -> { + if (value != null) { + updatedProperties.add(new PropertyWithSource(source, key, value)); + } + }); + this.properties = Collections.unmodifiableList(updatedProperties); } } diff --git a/base/src/main/java/org/killbill/billing/platform/jndi/JNDIManager.java b/base/src/main/java/org/killbill/billing/platform/jndi/JNDIManager.java index 7c044fccd..d4bcb68a7 100644 --- a/base/src/main/java/org/killbill/billing/platform/jndi/JNDIManager.java +++ b/base/src/main/java/org/killbill/billing/platform/jndi/JNDIManager.java @@ -31,6 +31,7 @@ import javax.naming.NamingException; import javax.naming.Reference; import javax.naming.Referenceable; +import javax.naming.spi.NamingManager; import org.killbill.commons.utils.Preconditions; import org.slf4j.Logger; @@ -92,7 +93,17 @@ public Object lookup(final String name) { try { context = getContext(); - return context.lookup(name); + final Object obj = context.lookup(name); + + if (obj instanceof Reference) { + try { + return NamingManager.getObjectInstance(obj, null, null, null); + } catch (final Exception e) { + logger.warn("Failed to dereference JNDI Reference for {}, returning Reference object", name, e); + } + } + + return obj; } catch (final NamingException e) { logger.warn("Error looking up " + name, e); } finally { diff --git a/platform-test/src/main/java/org/killbill/billing/platform/test/config/TestKillbillConfigSource.java b/platform-test/src/main/java/org/killbill/billing/platform/test/config/TestKillbillConfigSource.java index 1ebce418b..68860e5fd 100644 --- a/platform-test/src/main/java/org/killbill/billing/platform/test/config/TestKillbillConfigSource.java +++ b/platform-test/src/main/java/org/killbill/billing/platform/test/config/TestKillbillConfigSource.java @@ -72,6 +72,7 @@ public TestKillbillConfigSource(@Nullable final String file, @Nullable final Cla this.extraDefaults = extraDefaults; // extraDefaults changed, need to reload defaults populateDefaultProperties(extraDefaults); + rebuildCache(); } @Override