Split out a helper method for parsing only application overrides #708

bbaldino · 2020-09-22T22:48:29Z

This came out of a discussion in gitter with @havocp. I had a need to be able to write a custom Config load which would allow an override (via -Dconfig.*) but also fallback to application.conf, even if an override were present. Previously the logic to parse overrides existed only in DefaultConfigLoadingStrategy#parseApplicationConfig, which would replace any application resource with the -Dconfig.* file. This PR moves the logic for parsing the overrides into a separate helper method, which is now used by DefaultConfigLoadingStrategy#parseApplicationConfig but can be used elsewhere as well.

A few questions on things that may need changing:

Is the argument (ConfigParseOptions) to parseApplicationOverride acceptable? There should probably be another version which takes no arguments and passes ConfigParseOptions.defaults().
Is there any issue with returning ConfigImpl.emptyConfig in the case where no override was specified? This requires an origin description and I wasn't sure what would be best to put there, maybe there's a better object to return.
Is throwing from ConfigFactory#parseApplicationOverride acceptable (in the case where no override files were given). Maybe it's better to return empty here as well?

lightbend-cla-validator · 2020-09-22T22:48:31Z

Hi @bbaldino,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

https://www.lightbend.com/contribute/cla

bbaldino · 2020-09-22T22:49:12Z

For a specific use case, this PR allows me to do:

ConfigFactory.parseApplicationOverride(ConfigParseOptions.defaults())
    .withFallback(ConfigFactory.parseResourcesAnySyntax("application"))
    .withFallback(ConfigFactory.defaultReference())

havocp

Thanks! Re: your questions,

Yeah there are various overloads for everything on ConfigFactory and it's a bit annoying (and also hard to get right). The precedent from defaultApplication would be to have one that takes no options and one that takes just a class loader; following that pattern seems reasonable to me.
notes inline, I think use the ensureClassLoader method
I think throwing is right, it's better than silently falling back to application.conf (would be pretty hard to debug why my -Dconfig.whatever was being ignored in that case). Also, it keeps the current behavior, which is nice if in doubt.

havocp · 2020-09-23T13:09:03Z

config/src/main/java/com/typesafe/config/ConfigFactory.java

@@ -8,7 +8,7 @@

 import java.io.File;
 import java.io.Reader;
-import java.net.URL;
+import java.net.*;


My memory isn't 100% but I think the codebase may mostly avoid wildcard imports

havocp · 2020-09-23T13:10:55Z

config/src/main/java/com/typesafe/config/ConfigFactory.java

@@ -1090,6 +1090,63 @@ public static Config parseResourcesAnySyntax(String resourceBasename) {
        return parseResourcesAnySyntax(resourceBasename, ConfigParseOptions.defaults());
    }

+    /**
+     * Parse only any application overrides (those specified by config.{resource,file,url}), returning


The word "override" already has a different meaning for ConfigFactory.defaultOverrides so I wonder what else we could call this ... the README uses the word "replacement" perhaps? parseApplicationReplacement?

havocp · 2020-09-23T13:13:46Z

config/src/main/java/com/typesafe/config/ConfigFactory.java

+        if (loader == null)
+            throw new ConfigException.BugOrBroken(
+                "ClassLoader should have been set here; bug in ConfigFactory. "
+                    + "(You can probably work around this bug by passing in a class loader or calling currentThread().setContextClassLoader() though.)");


this should probably follow the pattern in the rest of ConfigFactory and call ensureClassLoader rather than expecting it to be already set. The assertion here was because ConfigFactory is supposed to be the only caller of the strategy and it always does the ensureClassLoader. So here you can use ensureClassLoader which already throws if it can't find a loader.

havocp · 2020-09-23T13:17:16Z

config/src/main/java/com/typesafe/config/ConfigFactory.java

+            specified += 1;
+
+        if (specified == 0) {
+            return ConfigImpl.emptyConfig("TODO: what to put here? Should something else be returned?");


maybe use Java 8 Optional ... ? I don't know what current best practice is around that, I haven't Java'd in a few years. I think there is a difference here between "something was specified and it was empty" and "nothing was specified."

This sounds good. Other option would be to return null I guess, but the way Optional allows orElseGet in DefaultConfigLoadingStrategy is nice, so will use Optional.

havocp · 2020-09-23T13:18:26Z

config/src/main/java/com/typesafe/config/DefaultConfigLoadingStrategy.java

-            specified += 1;
-
-        if (specified == 0) {
+        Config overrideConfig = ConfigFactory.parseApplicationOverride(parseOptions);


if we rename to replacement or something I'd carry it through variable names and terminology in comments

havocp · 2020-09-23T13:20:45Z

config/src/main/java/com/typesafe/config/DefaultConfigLoadingStrategy.java

-
-        if (specified == 0) {
+        Config overrideConfig = ConfigFactory.parseApplicationOverride(parseOptions);
+        if (overrideConfig.isEmpty()) {


with optional it could be orElseGet(() -> ConfigFactory.parseResourcesAnySyntax("application", parseOptions)) maybe

havocp · 2020-09-23T13:21:38Z

config/src/main/main.iml

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="UTF-8"?>


should .gitignore these probably and remove from the PR

🤦 Will remove this.

bbaldino · 2020-09-23T16:31:45Z

Thanks for the comments, @havocp ! I think I addressed everything. One unfortunate thing I found about the Optional is when the code is used outside, e.g. my use case now becomes:

ConfigFactory.parseApplicationOverride().orElseGet(() -> ConfigFactory.parseString(""))
    .withFallback(ConfigFactory.parseResourcesAnySyntax("application"))
    .withFallback(ConfigFactory.defaultReference())

But it's not terrible. Is there some kind of static EmptyConfig instance that could be used in places like this? Or maybe there's a better way to write this logic.

havocp · 2020-09-23T18:54:51Z

Yep there's a ConfigFactory.empty() I think.

One other thought here is to add @since in javadocs; I believe the next release will be 1.4.1 #702

havocp · 2020-09-23T18:55:10Z

(for since tags, there are some existing examples I believe to copy)

bbaldino · 2020-09-23T19:22:20Z

Yep there's a ConfigFactory.empty() I think.

Ah, great.

One other thought here is to add @since in javadocs; I believe the next release will be 1.4.1 #702

Done. 👍

havocp · 2020-09-23T19:28:46Z

Thanks!

havocp · 2020-09-24T19:07:47Z

config/src/main/java/com/typesafe/config/ConfigFactory.java

+     */
+    public static java.util.Optional<Config> parseApplicationReplacement(ConfigParseOptions parseOptions) {
+        ensureClassLoader(parseOptions, "parseApplicationReplacement");
+        ClassLoader loader = parseOptions.getClassLoader();


Oh, oops - I didn't catch this before merging, but ensureClassLoader returns the new options, doesn't modify the original options.

Oops. I can open a follow up fix PR today.

lukaszlenart · 2021-01-20T13:41:20Z

I think this PR broke backward compatibility. We have been using version 1.4.0 and it was possible to override any config options by using -Dconfig.override_with_env_vars=true and then setting up an env variable, eg.: cms.dgraph.hosts.2=... when having such application.conf

  cms {
    dgraph {
      hosts = ["dgraph-host"]
      port = 9080
    }
...

After switching to version 1.4.1 this stopped working and now we must use CONFIG_FORCE_cms_dgraph_hosts_2=...

I know that this is against the docs but it worked, so in such case you broke backward comaptibility.

lukaszlenart · 2021-01-20T14:03:12Z

My bad, this isn't this PR but this commit - this should be pointed out in Version Notes.

lukaszlenart · 2021-01-20T14:07:27Z

I found a workaround but I need to test it

val config = ConfigFactory.parseMap(System.getenv()).withFallback(ConfigFactory.load())

Split out a helper method for parsing only application overrides

09e2d57

bbaldino force-pushed the parse_app_overrides branch from d315fb2 to 09e2d57 Compare September 22, 2020 22:52

havocp reviewed Sep 23, 2020

View reviewed changes

bbaldino added 7 commits September 23, 2020 08:50

remove unused imports

af6b7b8

rename method

3be726d

don't use wildcard imports

7e1c64c

return an Optional from parseApplicationReplacement

bba56f2

remove accidentally committed files

1428ebb

add parseApplicationReplacement overrides

83349ec

use ensureClassLoader

f3baf1d

add @SInCE tags

5029d93

havocp approved these changes Sep 23, 2020

View reviewed changes

havocp merged commit 2d43727 into lightbend:master Sep 23, 2020

havocp mentioned this pull request Sep 23, 2020

Release 1.4.1? #702

Closed

bbaldino deleted the parse_app_overrides branch September 23, 2020 19:36

havocp reviewed Sep 24, 2020

View reviewed changes

bbaldino mentioned this pull request Sep 24, 2020

use correct ParseOptions object #709

Merged

dependabot bot mentioned this pull request Mar 18, 2021

Bump config from 1.3.3 to 1.4.1 checkmarx-ltd/checkmarx-spring-boot-java-sdk#105

Merged

ekrich mentioned this pull request Jun 16, 2023

Port all non documentation commits from typesafe/config ekrich/sconfig#29

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split out a helper method for parsing only application overrides #708

Split out a helper method for parsing only application overrides #708

bbaldino commented Sep 22, 2020

lightbend-cla-validator commented Sep 22, 2020

bbaldino commented Sep 22, 2020

havocp left a comment

havocp Sep 23, 2020

havocp Sep 23, 2020

havocp Sep 23, 2020

havocp Sep 23, 2020

bbaldino Sep 23, 2020

havocp Sep 23, 2020

havocp Sep 23, 2020

havocp Sep 23, 2020

bbaldino Sep 23, 2020

bbaldino commented Sep 23, 2020 •

edited

Loading

havocp commented Sep 23, 2020

havocp commented Sep 23, 2020

bbaldino commented Sep 23, 2020

havocp commented Sep 23, 2020

havocp Sep 24, 2020

bbaldino Sep 24, 2020

bbaldino Sep 24, 2020

lukaszlenart commented Jan 20, 2021

lukaszlenart commented Jan 20, 2021

lukaszlenart commented Jan 20, 2021

Split out a helper method for parsing only application overrides #708

Split out a helper method for parsing only application overrides #708

Conversation

bbaldino commented Sep 22, 2020

lightbend-cla-validator commented Sep 22, 2020

bbaldino commented Sep 22, 2020

havocp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbaldino commented Sep 23, 2020 • edited Loading

havocp commented Sep 23, 2020

havocp commented Sep 23, 2020

bbaldino commented Sep 23, 2020

havocp commented Sep 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszlenart commented Jan 20, 2021

lukaszlenart commented Jan 20, 2021

lukaszlenart commented Jan 20, 2021

bbaldino commented Sep 23, 2020 •

edited

Loading