From 5ec75aa00c16b5e7d73e48b939643dbd897ff413 Mon Sep 17 00:00:00 2001 From: elandau Date: Tue, 26 May 2015 13:37:20 -0700 Subject: [PATCH] Modify config loader to load resources as a LinkedHashMap instead of being inconsistent with merging Configs when multiple resources are loaded Move @next processing to PropertiesConfigReader instead of forcing it on all ConfigReader's since this actually breaks Typesafe's loading Add addConfigs and replaceConfigs to CompositeConfig Fix bug where application cascade loading wasn't done correctly --- .../com/netflix/archaius/ConfigLoader.java | 3 +- .../netflix/archaius/DefaultConfigLoader.java | 89 +++++-------------- .../archaius/config/CompositeConfig.java | 46 +++++++--- .../PropertiesConfigReader.java | 29 ++++-- .../archaius/config/CompositeConfigTest.java | 12 +-- .../loaders/PropertyConfigReaderTest.java | 1 + .../archaius/guice/ArchaiusModule.java | 39 ++++---- .../guice/ConfigurationInjectingListener.java | 2 +- .../typesafe/TypesafeConfigLoaderTest.java | 2 +- 9 files changed, 109 insertions(+), 114 deletions(-) rename archaius2-core/src/main/java/com/netflix/archaius/{loaders => readers}/PropertiesConfigReader.java (73%) diff --git a/archaius2-core/src/main/java/com/netflix/archaius/ConfigLoader.java b/archaius2-core/src/main/java/com/netflix/archaius/ConfigLoader.java index 149677f22..8b4266683 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/ConfigLoader.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/ConfigLoader.java @@ -17,6 +17,7 @@ import java.io.File; import java.net.URL; +import java.util.LinkedHashMap; import java.util.Properties; import com.netflix.archaius.exceptions.ConfigException; @@ -69,7 +70,7 @@ public static interface Loader { * @param resourceName * @return */ - Config load(String resourceName) throws ConfigException; + LinkedHashMap load(String resourceName) throws ConfigException; /** * Load configuration from a specific URL diff --git a/archaius2-core/src/main/java/com/netflix/archaius/DefaultConfigLoader.java b/archaius2-core/src/main/java/com/netflix/archaius/DefaultConfigLoader.java index f91e62418..7412b6727 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/DefaultConfigLoader.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/DefaultConfigLoader.java @@ -19,25 +19,21 @@ import java.net.MalformedURLException; import java.net.URL; import java.util.ArrayList; -import java.util.Collections; -import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; import java.util.Properties; import java.util.Set; -import java.util.concurrent.CopyOnWriteArraySet; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import com.netflix.archaius.StrInterpolator.Lookup; import com.netflix.archaius.cascade.NoCascadeStrategy; -import com.netflix.archaius.config.CompositeConfig; import com.netflix.archaius.config.MapConfig; import com.netflix.archaius.exceptions.ConfigException; import com.netflix.archaius.interpolate.CommonsStrInterpolator; import com.netflix.archaius.interpolate.ConfigStrLookup; -import com.netflix.archaius.loaders.PropertiesConfigReader; +import com.netflix.archaius.readers.PropertiesConfigReader; /** * DefaultConfigLoader provides a DSL to load configurations. @@ -47,7 +43,7 @@ */ public class DefaultConfigLoader implements ConfigLoader { - private static final Logger LOGGER = LoggerFactory.getLogger(DefaultConfigLoader.class); + private static final Logger LOG = LoggerFactory.getLogger(DefaultConfigLoader.class); private static final NoCascadeStrategy DEFAULT_CASCADE_STRATEGY = new NoCascadeStrategy(); private static final Lookup DEFAULT_LOOKUP = new Lookup() { @@ -62,7 +58,6 @@ public static class Builder { private List loaders = new ArrayList(); private CascadeStrategy defaultStrategy = DEFAULT_CASCADE_STRATEGY; private boolean failOnFirst = true; - private String includeKey = "@next"; private StrInterpolator interpolator = DEFAULT_INTERPOLATOR; private Lookup lookup = DEFAULT_LOOKUP; @@ -111,12 +106,6 @@ public Builder withConfigReader(List loaders) { return this; } - public Builder withIncludeKey(String key) { - if (key != null) - this.includeKey = key; - return this; - } - public DefaultConfigLoader build() { if (loaders.isEmpty()) { loaders.add(new PropertiesConfigReader()); @@ -131,18 +120,14 @@ public static Builder builder() { private final List loaders; private final CascadeStrategy defaultStrategy; - private final String includeKey; private final StrInterpolator interpolator; - private final Lookup lookup; - private final CopyOnWriteArraySet alreadyLoaded; + private final Lookup lookup; private final boolean defaultFailOnFirst; public DefaultConfigLoader(Builder builder) { - this.includeKey = builder.includeKey; this.loaders = builder.loaders; this.defaultStrategy = builder.defaultStrategy; this.interpolator = builder.interpolator; - this.alreadyLoaded = new CopyOnWriteArraySet(); this.defaultFailOnFirst = builder.failOnFirst; this.lookup = builder.lookup; } @@ -180,65 +165,32 @@ public Loader withOverrides(Properties props) { } @Override - public Config load(String resourceName) throws ConfigException { + public LinkedHashMap load(String resourceName) throws ConfigException { LinkedHashMap configs = new LinkedHashMap(); boolean failIfNotLoaded = failOnFirst; + if (overrides != null && !overrides.isEmpty()) { + configs.put(resourceName, new MapConfig(overrides)); + } + for (String permutationName : strategy.generate(resourceName, interpolator, lookup)) { - LOGGER.info("Attempting to load {}", permutationName); + LOG.info("Attempting to load {}", permutationName); for (ConfigReader loader : loaders) { if (loader.canLoad(classLoader, permutationName)) { - Config config; - String fileToLoad = permutationName; - do { - try { - config = loader.load(classLoader, permutationName); - try { - fileToLoad = config.getString(includeKey); - } - catch (Exception e) { - // TODO: - fileToLoad = null; - } - configs.put(permutationName, config); - } catch (ConfigException e) { - LOGGER.debug("could not load config '{}'. '{}'", new Object[]{fileToLoad, e.getMessage()}); - break; + try { + configs.put(permutationName, loader.load(classLoader, permutationName)); + failIfNotLoaded = false; + } + catch (ConfigException e) { + if (failIfNotLoaded == true) { + throw new ConfigException("Failed to load configuration resource '" + resourceName + "'"); } - } while (fileToLoad != null); + } + break; } } - - if (failIfNotLoaded == true) { - if (configs.isEmpty()) - throw new ConfigException("Failed to load configuration resource '" + resourceName + "'"); - failIfNotLoaded = false; - } } - if (overrides != null && !overrides.isEmpty()) { - configs.put(resourceName, new MapConfig(overrides)); - } - - // none - if (configs.isEmpty()) { - return null; - } - // single - else if (configs.size() == 1) { - return configs.values().iterator().next(); - } - // multiple - else { - CompositeConfig cConfig = new CompositeConfig(); - ArrayList names = new ArrayList(); - names.addAll(configs.keySet()); - Collections.reverse(names); - - for (String name : names) { - cConfig.addConfig(name, configs.get(name)); - } - return cConfig; - } + return configs; } @Override @@ -248,8 +200,7 @@ public Config load(URL url) { try { return loader.load(classLoader, url); } catch (ConfigException e) { - // TODO Auto-generated catch block - e.printStackTrace(); + LOG.debug("Unable to load file '{}'", new Object[]{url, e.getMessage()}); } } } diff --git a/archaius2-core/src/main/java/com/netflix/archaius/config/CompositeConfig.java b/archaius2-core/src/main/java/com/netflix/archaius/config/CompositeConfig.java index 3aa5cff2b..da8aea70d 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/config/CompositeConfig.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/config/CompositeConfig.java @@ -122,17 +122,21 @@ public void onError(Throwable error, Config config) { * @throws ConfigException */ public synchronized void addConfig(String name, Config child) throws ConfigException { + internalAddConfig(name, child); + } + + private synchronized void internalAddConfig(String name, Config child) throws ConfigException { LOG.trace("Adding config {} to {}", name, hashCode()); - if (name == null) { - throw new ConfigException("Child configuration must be named"); - } - if (child == null) { // TODO: Log a warning? return; } + if (name == null) { + throw new ConfigException("Child configuration must be named"); + } + if (lookup.containsKey(name)) { throw new ConfigException(String.format("Configuration with name '%s' already exists", name)); } @@ -148,6 +152,18 @@ public synchronized void addConfig(String name, Config child) throws ConfigExcep postConfigAdded(child); } + public void addConfigs(LinkedHashMap configs) throws ConfigException { + for (Entry entry : configs.entrySet()) { + addConfig(entry.getKey(), entry.getValue()); + } + } + + public void replaceConfigs(LinkedHashMap configs) throws ConfigException { + for (Entry entry : configs.entrySet()) { + replaceConfig(entry.getKey(), entry.getValue()); + } + } + public synchronized Collection getConfigNames() { List result = new ArrayList(); result.addAll(this.lookup.keySet()); @@ -171,14 +187,8 @@ protected void postConfigAdded(Config child) { * @throws ConfigException */ public synchronized void replaceConfig(String name, Config child) throws ConfigException { - if (lookup.containsKey(name)) { - lookup.put(name, child); - child.removeListener(listener); - postConfigAdded(child); - } - else { - addConfig(name, child); - } + internalRemoveConfig(name); + internalAddConfig(name, child); } /** @@ -190,6 +200,10 @@ public synchronized void replaceConfig(String name, Config child) throws ConfigE * @return */ public synchronized Config removeConfig(String name) { + return internalRemoveConfig(name); + } + + public synchronized Config internalRemoveConfig(String name) { Config child = this.lookup.remove(name); if (child != null) { this.children.remove(child); @@ -289,4 +303,12 @@ public synchronized void accept(Visitor visitor) { } } } + + public static CompositeConfig from(LinkedHashMap load) throws ConfigException { + Builder builder = builder(); + for (Entry config : load.entrySet()) { + builder().withConfig(config.getKey(), config.getValue()); + } + return builder.build(); + } } diff --git a/archaius2-core/src/main/java/com/netflix/archaius/loaders/PropertiesConfigReader.java b/archaius2-core/src/main/java/com/netflix/archaius/readers/PropertiesConfigReader.java similarity index 73% rename from archaius2-core/src/main/java/com/netflix/archaius/loaders/PropertiesConfigReader.java rename to archaius2-core/src/main/java/com/netflix/archaius/readers/PropertiesConfigReader.java index 5fd95b6ba..aeffd4693 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/loaders/PropertiesConfigReader.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/readers/PropertiesConfigReader.java @@ -13,20 +13,23 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package com.netflix.archaius.loaders; +package com.netflix.archaius.readers; import java.io.File; import java.io.IOException; import java.net.URL; import java.net.URLDecoder; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Properties; import com.netflix.archaius.Config; import com.netflix.archaius.ConfigReader; import com.netflix.archaius.config.MapConfig; import com.netflix.archaius.exceptions.ConfigException; -import com.netflix.archaius.readers.URLConfigReader; public class PropertiesConfigReader implements ConfigReader { + private String includeKey = "@next"; @Override public Config load(ClassLoader loader, String resourceName) throws ConfigException { @@ -39,10 +42,26 @@ public Config load(ClassLoader loader, String resourceName) throws ConfigExcepti @Override public Config load(ClassLoader loader, URL url) throws ConfigException { + Properties props = new Properties(); + + URL urlToLoad = url; try { - return new MapConfig(new URLConfigReader(url).call().getToAdd()); - } catch (IOException e) { - throw new ConfigException("Unable to load URL " + url.toString(), e); + do { + Map p = new URLConfigReader(urlToLoad).call().getToAdd(); + for (Entry entry : p.entrySet()) { + props.put(entry.getKey(), entry.getValue()); + } + + String next = p.get(includeKey); + urlToLoad = next != null + ? new URL(next) + : null; + } while (urlToLoad != null); + + return new MapConfig(props); + } + catch (IOException e) { + throw new ConfigException("Unable to load configuration " + url, e); } } diff --git a/archaius2-core/src/test/java/com/netflix/archaius/config/CompositeConfigTest.java b/archaius2-core/src/test/java/com/netflix/archaius/config/CompositeConfigTest.java index 05d21b692..a96a8acd1 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/config/CompositeConfigTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/config/CompositeConfigTest.java @@ -45,14 +45,14 @@ public void basicTest() throws ConfigException { .withDefaultCascadingStrategy(ConcatCascadeStrategy.from("${env}")) .build(); - application.addConfig("app", loader.newLoader().load("application")); + application.replaceConfigs(loader.newLoader().load("application")); Assert.assertTrue(config.getBoolean("application.loaded")); Assert.assertTrue(config.getBoolean("application-prod.loaded", false)); Assert.assertFalse(config.getBoolean("libA.loaded", false)); - libraries.addConfig("libA", loader.newLoader().load("libA")); + libraries.replaceConfigs(loader.newLoader().load("libA")); libraries.accept(new PrintStreamVisitor()); config.accept(new PrintStreamVisitor()); @@ -61,7 +61,7 @@ public void basicTest() throws ConfigException { Assert.assertFalse(config.getBoolean("libB.loaded", false)); Assert.assertEquals("libA", config.getString("libA.overrideA")); - libraries.addConfig("libB", loader.newLoader().load("libB")); + libraries.replaceConfigs(loader.newLoader().load("libB")); System.out.println(config.toString()); Assert.assertTrue(config.getBoolean("libA.loaded")); @@ -90,14 +90,14 @@ public void basicReversedTest() throws ConfigException { .withDefaultCascadingStrategy(ConcatCascadeStrategy.from("${env}")) .build(); - application.addConfig("app", loader.newLoader().load("application")); + application.replaceConfigs(loader.newLoader().load("application")); Assert.assertTrue(config.getBoolean("application.loaded")); Assert.assertTrue(config.getBoolean("application-prod.loaded", false)); Assert.assertFalse(config.getBoolean("libA.loaded", false)); - libraries.addConfig("libA", loader.newLoader().load("libA")); + libraries.replaceConfigs(loader.newLoader().load("libA")); libraries.accept(new PrintStreamVisitor()); config.accept(new PrintStreamVisitor()); @@ -106,7 +106,7 @@ public void basicReversedTest() throws ConfigException { Assert.assertFalse(config.getBoolean("libB.loaded", false)); Assert.assertEquals("libA", config.getString("libA.overrideA")); - libraries.addConfig("libB", loader.newLoader().load("libB")); + libraries.replaceConfigs(loader.newLoader().load("libB")); System.out.println(config.toString()); Assert.assertTrue(config.getBoolean("libA.loaded")); diff --git a/archaius2-core/src/test/java/com/netflix/archaius/loaders/PropertyConfigReaderTest.java b/archaius2-core/src/test/java/com/netflix/archaius/loaders/PropertyConfigReaderTest.java index 9c582495b..bc1753b72 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/loaders/PropertyConfigReaderTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/loaders/PropertyConfigReaderTest.java @@ -21,6 +21,7 @@ import com.netflix.archaius.Config; import com.netflix.archaius.exceptions.ConfigException; +import com.netflix.archaius.readers.PropertiesConfigReader; public class PropertyConfigReaderTest { @Test diff --git a/archaius2-guice/src/main/java/com/netflix/archaius/guice/ArchaiusModule.java b/archaius2-guice/src/main/java/com/netflix/archaius/guice/ArchaiusModule.java index c5ce72afe..77701025b 100644 --- a/archaius2-guice/src/main/java/com/netflix/archaius/guice/ArchaiusModule.java +++ b/archaius2-guice/src/main/java/com/netflix/archaius/guice/ArchaiusModule.java @@ -15,6 +15,7 @@ */ package com.netflix.archaius.guice; +import java.util.LinkedHashMap; import java.util.Set; import javax.inject.Inject; @@ -48,7 +49,7 @@ import com.netflix.archaius.inject.RemoteLayer; import com.netflix.archaius.inject.RuntimeLayer; import com.netflix.archaius.interpolate.ConfigStrLookup; -import com.netflix.archaius.loaders.PropertiesConfigReader; +import com.netflix.archaius.readers.PropertiesConfigReader; /** * Guice module with default bindings to enable Config injection and @@ -165,13 +166,6 @@ final SettableConfig getSettableConfig() { return new DefaultSettableConfig(); } - @Provides - @Singleton - @ApplicationLayer - final CascadeStrategy getApplicationCascadeStrategy() { - return new NoCascadeStrategy(); - } - /** * Default loader for the application configuration using replacements from * system and environment configuration @@ -244,8 +238,8 @@ final Config getOverrideLayer(@RemoteLayer CompositeConfig config) { @Provides @Singleton - final CascadeStrategy getCascadeStrategy(@ApplicationLayer CascadeStrategy strategy) { - return strategy; + final CascadeStrategy getCascadeStrategy() { + return new NoCascadeStrategy(); } @Provides @@ -259,8 +253,8 @@ final Decoder getDecoder() { @RootLayer final Config getInternalConfig( @RuntimeLayer SettableConfig settableLayer, - @RemoteLayer Config overrideLayer, - @ApplicationLayer Config applicationLayer, + @RemoteLayer Config overrideLayer, + @ApplicationLayer CompositeConfig applicationLayer, @LibrariesLayer CompositeConfig librariesLayer) throws ConfigException { return CompositeConfig.builder() .withConfig(RUNTIME_LAYER_NAME, settableLayer) @@ -275,7 +269,7 @@ final Config getInternalConfig( /** * All code will ultimately inject Config to gain access to the entire * configuration hierarchy. The empty hierarchy is created by @RootLayer - * but here we do the actual Configuration initialization which include, + * but here we do the actual Configuration initialization which includes, * 1. Loading application properties * 2. Loading runtime overrides * 3. Loading override layer overrides @@ -295,15 +289,16 @@ final Config getConfig( @ApplicationLayer String appName, @RemoteLayer CompositeConfig overrideLayer, @RuntimeLayer SettableConfig runtimeLayer, - ConfigLoader loader, - @RuntimeLayer Set runtimeConfigResolvers, - @RemoteLayer Set remoteConfigResolvers + ConfigLoader loader, + CascadeStrategy cascadeStrategy, + @RuntimeLayer Set runtimeConfigResolvers, + @RemoteLayer Set remoteConfigResolvers ) throws Exception { // First load the application configuration - Config appConfig = loader.newLoader().load(appName); - if (appConfig != null) { - applicationLayer.addConfig(appName, appConfig); + LinkedHashMap loadedConfigs = loader.newLoader().withCascadeStrategy(new NoCascadeStrategy()).load(appName); + if (loadedConfigs != null) { + applicationLayer.addConfigs(loadedConfigs); } // Next load any runtime overrides @@ -316,6 +311,12 @@ final Config getConfig( overrideLayer.addConfig("remote", provider.get(config)); } + // Finally, load any cascaded configuration files for the + // application + loadedConfigs = loader.newLoader().withCascadeStrategy(cascadeStrategy).load(appName); + if (loadedConfigs != null) { + applicationLayer.replaceConfigs(loadedConfigs); + } return config; } diff --git a/archaius2-guice/src/main/java/com/netflix/archaius/guice/ConfigurationInjectingListener.java b/archaius2-guice/src/main/java/com/netflix/archaius/guice/ConfigurationInjectingListener.java index 7242c15ed..5a578f151 100644 --- a/archaius2-guice/src/main/java/com/netflix/archaius/guice/ConfigurationInjectingListener.java +++ b/archaius2-guice/src/main/java/com/netflix/archaius/guice/ConfigurationInjectingListener.java @@ -66,7 +66,7 @@ public void afterInjection(I injectee) { if (source != null) { for (String resourceName : source.value()) { try { - librariesConfig.addConfig(resourceName, loader.newLoader().withCascadeStrategy(strategy).load(resourceName)); + librariesConfig.replaceConfigs(loader.newLoader().withCascadeStrategy(strategy).load(resourceName)); } catch (ConfigException e) { throw new ProvisionException("Unable to load configuration for " + resourceName + " at source " + injectee.getClass(), e); diff --git a/archaius2-typesafe/src/test/java/com/netflix/archaius/typesafe/TypesafeConfigLoaderTest.java b/archaius2-typesafe/src/test/java/com/netflix/archaius/typesafe/TypesafeConfigLoaderTest.java index b3c57a84d..a9546d127 100644 --- a/archaius2-typesafe/src/test/java/com/netflix/archaius/typesafe/TypesafeConfigLoaderTest.java +++ b/archaius2-typesafe/src/test/java/com/netflix/archaius/typesafe/TypesafeConfigLoaderTest.java @@ -38,7 +38,7 @@ public void test() throws ConfigException { .withStrLookup(config) .build(); - config.addConfig("foo", loader.newLoader() + config.addConfigs(loader.newLoader() .withCascadeStrategy(ConcatCascadeStrategy.from("${env}", "${region}")) .load("foo"));