From ecfa436776be3004188c9432a81fe67b164e22d8 Mon Sep 17 00:00:00 2001 From: James Roper Date: Mon, 25 Feb 2019 13:36:44 +1100 Subject: [PATCH 1/2] 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. --- README.md | 28 ++++------------- .../com/typesafe/config/ConfigFactory.java | 3 +- .../com/typesafe/config/impl/ConfigImpl.java | 30 ++++++++++++++++--- ...13-application-override-substitutions.conf | 1 + .../test13-reference-bad-substitutions.conf | 1 + .../test13-reference-with-substitutions.conf | 2 ++ .../typesafe/config/impl/PublicApiTest.scala | 22 ++++++++++++++ 7 files changed, 59 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 28d51af89..9a144d726 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 @@ -737,14 +727,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/ConfigFactory.java b/config/src/main/java/com/typesafe/config/ConfigFactory.java index 86d995e36..7cd691256 100644 --- a/config/src/main/java/com/typesafe/config/ConfigFactory.java +++ b/config/src/main/java/com/typesafe/config/ConfigFactory.java @@ -210,7 +210,8 @@ public static Config load(Config config, ConfigResolveOptions resolveOptions) { * @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.verifiedUnresolvedReference(loader)) .resolve(resolveOptions); } 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 9cf49913b..99a53e88f 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java @@ -364,15 +364,37 @@ public static Config defaultReference(final ClassLoader loader) { 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 verifiedUnresolvedReference(final ClassLoader loader) { + // First, verify that `reference.conf` resolves by itself. + defaultReference(loader); + // 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 000000000..506b2cada --- /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 000000000..1b2615077 --- /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 000000000..fcf7f995c --- /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 d25d43659..721d29cd4 100644 --- a/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala +++ b/config/src/test/scala/com/typesafe/config/impl/PublicApiTest.scala @@ -1132,6 +1132,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 { From 4c01e4a09b6dcac2ce17934c769c7f5732a1165a Mon Sep 17 00:00:00 2001 From: James Roper Date: Mon, 29 Jul 2019 15:27:42 +1000 Subject: [PATCH 2/2] Renamed public method to defaultReferenceUnresolved Also added the methods to ConfigFactory, as requested in code review. --- .../com/typesafe/config/ConfigException.java | 12 +++++ .../com/typesafe/config/ConfigFactory.java | 52 ++++++++++++++++++- .../com/typesafe/config/impl/ConfigImpl.java | 8 ++- 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/config/src/main/java/com/typesafe/config/ConfigException.java b/config/src/main/java/com/typesafe/config/ConfigException.java index f35e8dbea..2e6d99ab7 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 Parse(ConfigOrigin origin, String message) { 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 7cd691256..498dda479 100644 --- a/config/src/main/java/com/typesafe/config/ConfigFactory.java +++ b/config/src/main/java/com/typesafe/config/ConfigFactory.java @@ -211,7 +211,7 @@ public static Config load(Config config, ConfigResolveOptions resolveOptions) { */ public static Config load(ClassLoader loader, Config config, ConfigResolveOptions resolveOptions) { return defaultOverrides(loader).withFallback(config) - .withFallback(ConfigImpl.verifiedUnresolvedReference(loader)) + .withFallback(ConfigImpl.defaultReferenceUnresolved(loader)) .resolve(resolveOptions); } @@ -368,6 +368,56 @@ public static Config defaultReference(ClassLoader loader) { 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 99a53e88f..bb3d02a8f 100644 --- a/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java +++ b/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java @@ -387,9 +387,13 @@ public Config call() { * is self contained and doesn't depend on any higher level configuration * files. */ - public static Config verifiedUnresolvedReference(final ClassLoader loader) { + public static Config defaultReferenceUnresolved(final ClassLoader loader) { // First, verify that `reference.conf` resolves by itself. - defaultReference(loader); + 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); }