From ab890103dd86542c26440371ea26cae4a00a7a22 Mon Sep 17 00:00:00 2001 From: James Roper Date: Tue, 20 Aug 2019 23:02:50 +1000 Subject: [PATCH] Allow application.conf to override variables in reference.conf (#619) * Allow application.conf to override variables in reference.conf Fixes #167 This only affects the output of `ConfigFactory.load`. It does not change `ConfigFactory.defaultReference`. This uses the unresolved `reference.conf` in the building of configuration in `ConfigFactory.load`, effectively allowing `application.conf` properties to override variable substitutions in `reference.conf`. However, it still requires `reference.conf` to be fully resolvable, if it isn't, an exception will be thrown. So two resolves are still done during load, it's just that the output of the resolve of `reference.conf` isn't used in building the final configuration. The documentation has been updated to reflect this behavior. The reasoning behind this change can be read about in #167, but essentially, it is not uncommon for configuration properties to depend on each other by default, a good example of this is directory hierarchies, where you might have a configuration option for a base directory, and then a configuration for the log directory that by default is under the base directory, and within that a configuration for individual log files which by default are under the log directory. Without allowing variable substitutions in `reference.conf` from `application.conf`, there is no point in defining a configuration option for the base directory since changing it won't have any impact, and each path defined that depends on it will have to be manually overridden. This limitation is contrary to convention over configuration best practices, and hence not desirable in a configuration library. * Renamed public method to defaultReferenceUnresolved Also added the methods to ConfigFactory, as requested in code review. --- README.md | 28 ++-------- .../com/typesafe/config/ConfigException.java | 12 +++++ .../com/typesafe/config/ConfigFactory.java | 53 ++++++++++++++++++- .../com/typesafe/config/impl/ConfigImpl.java | 34 ++++++++++-- ...13-application-override-substitutions.conf | 1 + .../test13-reference-bad-substitutions.conf | 1 + .../test13-reference-with-substitutions.conf | 2 + .../typesafe/config/impl/PublicApiTest.scala | 22 ++++++++ 8 files changed, 125 insertions(+), 28 deletions(-) create mode 100644 config/src/test/resources/test13-application-override-substitutions.conf create mode 100644 config/src/test/resources/test13-reference-bad-substitutions.conf create mode 100644 config/src/test/resources/test13-reference-with-substitutions.conf diff --git a/README.md b/README.md index 79f8de1b..5f24a47c 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,6 @@ to merge it in. - [Inheritance](#inheritance) - [Optional system or env variable overrides](#optional-system-or-env-variable-overrides) - [Concatenation](#concatenation) - - [`reference.conf` can't refer to `application.conf`](#referenceconf-cant-refer-to-applicationconf) - [Miscellaneous Notes](#miscellaneous-notes) - [Debugging Your Configuration](#debugging-your-configuration) - [Supports Java 8 and Later](#supports-java-8-and-later) @@ -277,23 +276,14 @@ system properties. The substitution syntax `${foo.bar}` will be resolved twice. First, all the `reference.conf` files are merged and then the result gets resolved. Second, all the `application.conf` are -layered over the `reference.conf` and the result of that gets -resolved again. +layered over the unresolved `reference.conf` and the result of that +gets resolved again. The implication of this is that the `reference.conf` stack has to be self-contained; you can't leave an undefined value `${foo.bar}` -to be provided by `application.conf`, or refer to `${foo.bar}` in -a way that you want to allow `application.conf` to -override. However, `application.conf` can refer to a `${foo.bar}` -in `reference.conf`. - -This can be frustrating at times, but possible workarounds -include: - - * putting an `application.conf` in a library jar, alongside the -`reference.conf`, with values intended for later resolution. - * putting some logic in code instead of building up values in the - config itself. +to be provided by `application.conf`. It is however possible to +override a variable that `reference.conf` refers to, as long as +`reference.conf` also defines that variable itself. ### Merging config trees @@ -758,14 +748,6 @@ concatenation, but not object/array concatenation. `+=` does not work in Play/Akka 2.0 either. Post-2.0 versions support these features. -### `reference.conf` can't refer to `application.conf` - -Please see this -earlier section; all `reference.conf` have substitutions -resolved first, without `application.conf` in the stack, so the -reference stack has to be self-contained. - ## Miscellaneous Notes ### Debugging Your Configuration diff --git a/config/src/main/java/com/typesafe/config/ConfigException.java b/config/src/main/java/com/typesafe/config/ConfigException.java index f35e8dbe..2e6d99ab 100644 --- a/config/src/main/java/com/typesafe/config/ConfigException.java +++ b/config/src/main/java/com/typesafe/config/ConfigException.java @@ -284,13 +284,25 @@ public abstract class ConfigException extends RuntimeException implements Serial public static class UnresolvedSubstitution extends Parse { private static final long serialVersionUID = 1L; + private final String detail; + public UnresolvedSubstitution(ConfigOrigin origin, String detail, Throwable cause) { super(origin, "Could not resolve substitution to a value: " + detail, cause); + this.detail = detail; } public UnresolvedSubstitution(ConfigOrigin origin, String detail) { this(origin, detail, null); } + + private UnresolvedSubstitution(UnresolvedSubstitution wrapped, ConfigOrigin origin, String message) { + super(origin, message, wrapped); + this.detail = wrapped.detail; + } + + public UnresolvedSubstitution addExtraDetail(String extra) { + return new UnresolvedSubstitution(this, origin(), String.format(extra, detail)); + } } /** diff --git a/config/src/main/java/com/typesafe/config/ConfigFactory.java b/config/src/main/java/com/typesafe/config/ConfigFactory.java index 4b91a1ea..12f9aaec 100644 --- a/config/src/main/java/com/typesafe/config/ConfigFactory.java +++ b/config/src/main/java/com/typesafe/config/ConfigFactory.java @@ -211,7 +211,8 @@ public final class ConfigFactory { * @return resolved configuration with overrides and fallbacks added */ public static Config load(ClassLoader loader, Config config, ConfigResolveOptions resolveOptions) { - return defaultOverrides(loader).withFallback(config).withFallback(defaultReference(loader)) + return defaultOverrides(loader).withFallback(config) + .withFallback(ConfigImpl.defaultReferenceUnresolved(loader)) .resolve(resolveOptions); } @@ -368,6 +369,56 @@ public final class ConfigFactory { return ConfigImpl.defaultReference(loader); } + /** + * Obtains the default reference configuration, which is currently created + * by merging all resources "reference.conf" found on the classpath and + * overriding the result with system properties. + * + *

+ * While the returned reference configuration is guaranteed to be + * resolvable (that is, there will be no substitutions that cannot be + * resolved), it is returned in an unresolved state for the purpose of + * allowing substitutions to be overridden by a config layer that falls + * back to this one. + * + *

+ * Libraries and frameworks should ship with a "reference.conf" in their + * jar. + * + *

+ * The reference config must be looked up in the class loader that contains + * the libraries that you want to use with this config, so the + * "reference.conf" for each library can be found. Use + * {@link #defaultReference(ClassLoader)} if the context class loader is not + * suitable. + * + *

+ * The {@link #load()} methods merge this configuration for you + * automatically. + * + *

+ * Future versions may look for reference configuration in more places. It + * is not guaranteed that this method only looks at + * "reference.conf". + * + * @return the unresolved default reference config for the context class + * loader + */ + public static Config defaultReferenceUnresolved() { + return defaultReferenceUnresolved(checkedContextClassLoader("defaultReferenceUnresolved")); + } + + /** + * Like {@link #defaultReferenceUnresolved()} but allows you to specify a + * class loader to use rather than the current context class loader. + * + * @param loader class loader to look for resources in + * @return the unresolved default reference config for this class loader + */ + public static Config defaultReferenceUnresolved(ClassLoader loader) { + return ConfigImpl.defaultReferenceUnresolved(loader); + } + /** * Obtains the default override configuration, which currently consists of * system properties. The returned override configuration will already have diff --git a/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java b/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java index c807046d..f14c12af 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java @@ -408,15 +408,41 @@ public class ConfigImpl { return computeCachedConfig(loader, "defaultReference", new Callable() { @Override public Config call() { - Config unresolvedResources = Parseable - .newResources("reference.conf", - ConfigParseOptions.defaults().setClassLoader(loader)) - .parse().toConfig(); + Config unresolvedResources = unresolvedReference(loader); return systemPropertiesAsConfig().withFallback(unresolvedResources).resolve(); } }); } + private static Config unresolvedReference(final ClassLoader loader) { + return computeCachedConfig(loader, "unresolvedReference", new Callable() { + @Override + public Config call() { + return Parseable.newResources("reference.conf", + ConfigParseOptions.defaults().setClassLoader(loader)) + .parse().toConfig(); + } + }); + } + + /** + * This returns the unresolved reference configuration, but before doing so, + * it verifies that the reference configuration resolves, to ensure that it + * is self contained and doesn't depend on any higher level configuration + * files. + */ + public static Config defaultReferenceUnresolved(final ClassLoader loader) { + // First, verify that `reference.conf` resolves by itself. + try { + defaultReference(loader); + } catch (ConfigException.UnresolvedSubstitution e) { + throw e.addExtraDetail("Could not resolve substitution in reference.conf to a value: %s. All reference.conf files are required to be fully, independently resolvable, and should not require the presence of values for substitutions from further up the hierarchy."); + } + // Now load the unresolved version + return unresolvedReference(loader); + } + + private static class DebugHolder { private static String LOADS = "loads"; private static String SUBSTITUTIONS = "substitutions"; diff --git a/config/src/test/resources/test13-application-override-substitutions.conf b/config/src/test/resources/test13-application-override-substitutions.conf new file mode 100644 index 00000000..506b2cad --- /dev/null +++ b/config/src/test/resources/test13-application-override-substitutions.conf @@ -0,0 +1 @@ +b = "overridden" \ No newline at end of file diff --git a/config/src/test/resources/test13-reference-bad-substitutions.conf b/config/src/test/resources/test13-reference-bad-substitutions.conf new file mode 100644 index 00000000..1b261507 --- /dev/null +++ b/config/src/test/resources/test13-reference-bad-substitutions.conf @@ -0,0 +1 @@ +a = ${b} \ No newline at end of file diff --git a/config/src/test/resources/test13-reference-with-substitutions.conf b/config/src/test/resources/test13-reference-with-substitutions.conf new file mode 100644 index 00000000..fcf7f995 --- /dev/null +++ b/config/src/test/resources/test13-reference-with-substitutions.conf @@ -0,0 +1,2 @@ +a = ${b} +b = "b" \ No newline at end of file diff --git a/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala b/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala index 4c72643e..1643a4a3 100644 --- a/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala @@ -1152,6 +1152,28 @@ include "onclasspath" // missing underneath missing intercept[ConfigException.Missing] { conf.getIsNull("x.c.y") } } + + @Test + def applicationConfCanOverrideReferenceConf(): Unit = { + val loader = new TestClassLoader(this.getClass.getClassLoader, + Map( + "reference.conf" -> resourceFile("test13-reference-with-substitutions.conf").toURI.toURL, + "application.conf" -> resourceFile("test13-application-override-substitutions.conf").toURI.toURL)) + + assertEquals("b", ConfigFactory.defaultReference(loader).getString("a")) + assertEquals("overridden", ConfigFactory.load(loader).getString("a")) + } + + @Test(expected = classOf[ConfigException.UnresolvedSubstitution]) + def referenceConfMustResolveIndependently(): Unit = { + val loader = new TestClassLoader(this.getClass.getClassLoader, + Map( + "reference.conf" -> resourceFile("test13-reference-bad-substitutions.conf").toURI.toURL, + "application.conf" -> resourceFile("test13-application-override-substitutions.conf").toURI.toURL)) + + ConfigFactory.load(loader) + } + } class TestStrategy extends DefaultConfigLoadingStrategy {