Allow application.conf to override variables in reference.conf #619
Conversation
Fixes lightbend#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 lightbend#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.
| * is self contained and doesn't depend on any higher level configuration | ||
| * files. | ||
| */ | ||
| public static Config verifiedUnresolvedReference(final ClassLoader loader) { |
There was a problem hiding this comment.
@havocp So we have this as public API, would you like it in ConfigFactory, with an overload similar to defaultReference? Is the name ok?
There was a problem hiding this comment.
I was thinking of the name defaultReference as "literally the reference used by default in ConfigFactory.load", paralleled by defaultApplication and defaultOverrides ; we do need to break that meaning to do this PR, which is OK, but I wonder if I've sort of encoded in the docs in spots.
A possible name here could be defaultReferenceUnresolved which would kind of keep the parallelism showing this is part of the default stack, but with the tweak that there's an unresolved version.
I think verified is a bit of an implementation detail and not the main point of the method.
I do think I'd put it in ConfigFactory; it's fairly common for people to slightly tweak ConfigFactory.load in some way, so that's why its individual "component parts" are made available, trying to make it easy to rebuild load with tweaks.
There was a problem hiding this comment.
Ok, I've renamed as requested, and copied it to ConfigFactory.
There was a problem hiding this comment.
cc @patriknw and @ekrich since this is a change with some implications.
The API docs for defaultApplication() have a long explanation of how default* are stacked that looks like we'd want to update it; the docs for defaultReference say it's used by load(), so that could be made more precise too.
There's also a note about the current behavior here https://github.com/lightbend/config/blob/master/HOCON.md#conventional-configuration-files-for-jvm-apps that should be updated.
Potentially in HOCON.md in particular it's worth explaining that the behavior has changed between versions.
| * is self contained and doesn't depend on any higher level configuration | ||
| * files. | ||
| */ | ||
| public static Config verifiedUnresolvedReference(final ClassLoader loader) { |
There was a problem hiding this comment.
I was thinking of the name defaultReference as "literally the reference used by default in ConfigFactory.load", paralleled by defaultApplication and defaultOverrides ; we do need to break that meaning to do this PR, which is OK, but I wonder if I've sort of encoded in the docs in spots.
A possible name here could be defaultReferenceUnresolved which would kind of keep the parallelism showing this is part of the default stack, but with the tweak that there's an unresolved version.
I think verified is a bit of an implementation detail and not the main point of the method.
I do think I'd put it in ConfigFactory; it's fairly common for people to slightly tweak ConfigFactory.load in some way, so that's why its individual "component parts" are made available, trying to make it easy to rebuild load with tweaks.
| */ | ||
| public static Config verifiedUnresolvedReference(final ClassLoader loader) { | ||
| // First, verify that `reference.conf` resolves by itself. | ||
| defaultReference(loader); |
There was a problem hiding this comment.
The exception this throws may be a bit confusing; if you've defined foo in application.conf but not reference.conf it's going to say "couldn't resolve foo" or whatever with no explanation of the convoluted thing that yeah you can override it in application.conf but the reference config needs to be standalone-resolveable also.
I wonder if it would be helpful to catch the exception and add some detail like "reference.conf must have a default value for all substitutions" - could still use the same UnresolvedSubstitution exception type, but with an extra line of text perhaps, not sure how annoying it is to do.
There was a problem hiding this comment.
I've done this, I had to hack UnresolvedSubstitution a little bit to create a sensible error message, since it was losing the detail in a message string, and always prepending its own description to the passed in detail.
|
This behavior would be very useful. |
Also added the methods to ConfigFactory, as requested in code review.
|
Updated as requested. |
|
Thanks @jroper |
|
1.3.5-RC1 is on its way to maven central right now. |
|
For reference we will make this 1.4.0 when we release it, not 1.3.5 since the change in behavior is a bit too much for a patch version. |
Fixes #167
This only affects the output of
ConfigFactory.load. It does not changeConfigFactory.defaultReference. This uses the unresolvedreference.confin the building of configuration inConfigFactory.load, effectively allowingapplication.confproperties to override variable substitutions inreference.conf.However, it still requires
reference.confto 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 ofreference.confisn'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.conffromapplication.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.