-
Notifications
You must be signed in to change notification settings - Fork 967
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split out a helper method for parsing only application overrides #708
Changes from 1 commit
09e2d57
af6b7b8
3be726d
7e1c64c
bba56f2
1428ebb
83349ec
f3baf1d
5029d93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ | |
|
||
import java.io.File; | ||
import java.io.Reader; | ||
import java.net.URL; | ||
import java.net.*; | ||
import java.util.Map; | ||
import java.util.Properties; | ||
import java.util.concurrent.Callable; | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The word "override" already has a different meaning for |
||
* an empty Config if no overrides were set. | ||
* @param parseOptions parse options | ||
* @return the parsed configuration | ||
*/ | ||
public static Config parseApplicationOverride(ConfigParseOptions parseOptions) { | ||
ClassLoader loader = parseOptions.getClassLoader(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, oops - I didn't catch this before merging, but ensureClassLoader returns the new options, doesn't modify the original options. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops. I can open a follow up fix PR today. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
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.)"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should probably follow the pattern in the rest of ConfigFactory and call |
||
|
||
int specified = 0; | ||
|
||
// override application.conf with config.file, config.resource, | ||
// config.url if requested. | ||
String resource = System.getProperty("config.resource"); | ||
if (resource != null) | ||
specified += 1; | ||
String file = System.getProperty("config.file"); | ||
if (file != null) | ||
specified += 1; | ||
String url = System.getProperty("config.url"); | ||
if (url != null) | ||
specified += 1; | ||
|
||
if (specified == 0) { | ||
return ConfigImpl.emptyConfig("TODO: what to put here? Should something else be returned?"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds good. Other option would be to return null I guess, but the way |
||
} else if (specified > 1) { | ||
throw new ConfigException.Generic("You set more than one of config.file='" + file | ||
+ "', config.url='" + url + "', config.resource='" + resource | ||
+ "'; don't know which one to use!"); | ||
} else { | ||
// the override file/url/resource MUST be present or it's an error | ||
ConfigParseOptions overrideOptions = parseOptions.setAllowMissing(false); | ||
if (resource != null) { | ||
if (resource.startsWith("/")) | ||
resource = resource.substring(1); | ||
// this deliberately does not parseResourcesAnySyntax; if | ||
// people want that they can use an include statement. | ||
return ConfigFactory.parseResources(loader, resource, overrideOptions); | ||
} else if (file != null) { | ||
return ConfigFactory.parseFile(new File(file), overrideOptions); | ||
} else { | ||
try { | ||
return ConfigFactory.parseURL(new URL(url), overrideOptions); | ||
} catch (MalformedURLException e) { | ||
throw new ConfigException.Generic("Bad URL in config.url system property: '" | ||
+ url + "': " + e.getMessage(), e); | ||
} | ||
} | ||
} | ||
|
||
} | ||
|
||
/** | ||
* Parses a string (which should be valid HOCON or JSON by default, or | ||
* the syntax specified in the options otherwise). | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
package com.typesafe.config; | ||
|
||
import com.typesafe.config.impl.*; | ||
|
||
import java.io.File; | ||
import java.net.MalformedURLException; | ||
import java.net.URL; | ||
|
@@ -12,51 +14,11 @@ | |
public class DefaultConfigLoadingStrategy implements ConfigLoadingStrategy { | ||
@Override | ||
public Config parseApplicationConfig(ConfigParseOptions parseOptions) { | ||
ClassLoader loader = parseOptions.getClassLoader(); | ||
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.)"); | ||
|
||
int specified = 0; | ||
|
||
// override application.conf with config.file, config.resource, | ||
// config.url if requested. | ||
String resource = System.getProperty("config.resource"); | ||
if (resource != null) | ||
specified += 1; | ||
String file = System.getProperty("config.file"); | ||
if (file != null) | ||
specified += 1; | ||
String url = System.getProperty("config.url"); | ||
if (url != null) | ||
specified += 1; | ||
|
||
if (specified == 0) { | ||
Config overrideConfig = ConfigFactory.parseApplicationOverride(parseOptions); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we rename to |
||
if (overrideConfig.isEmpty()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with optional it could be |
||
return ConfigFactory.parseResourcesAnySyntax("application", parseOptions); | ||
} else if (specified > 1) { | ||
throw new ConfigException.Generic("You set more than one of config.file='" + file | ||
+ "', config.url='" + url + "', config.resource='" + resource | ||
+ "'; don't know which one to use!"); | ||
} else { | ||
// the override file/url/resource MUST be present or it's an error | ||
ConfigParseOptions overrideOptions = parseOptions.setAllowMissing(false); | ||
if (resource != null) { | ||
if (resource.startsWith("/")) | ||
resource = resource.substring(1); | ||
// this deliberately does not parseResourcesAnySyntax; if | ||
// people want that they can use an include statement. | ||
return ConfigFactory.parseResources(loader, resource, overrideOptions); | ||
} else if (file != null) { | ||
return ConfigFactory.parseFile(new File(file), overrideOptions); | ||
} else { | ||
try { | ||
return ConfigFactory.parseURL(new URL(url), overrideOptions); | ||
} catch (MalformedURLException e) { | ||
throw new ConfigException.Generic("Bad URL in config.url system property: '" | ||
+ url + "': " + e.getMessage(), e); | ||
} | ||
} | ||
return overrideConfig; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should .gitignore these probably and remove from the PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤦 Will remove this. |
||
<module type="JAVA_MODULE" version="4"> | ||
<component name="NewModuleRootManager" inherit-compiler-output="true"> | ||
<exclude-output /> | ||
<content url="file://$MODULE_DIR$"> | ||
<sourceFolder url="file://$MODULE_DIR$/java" isTestSource="false" /> | ||
</content> | ||
<orderEntry type="inheritedJdk" /> | ||
<orderEntry type="sourceFolder" forTests="false" /> | ||
</component> | ||
</module> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<module type="JAVA_MODULE" version="4"> | ||
<component name="NewModuleRootManager" inherit-compiler-output="true"> | ||
<exclude-output /> | ||
<content url="file://$MODULE_DIR$"> | ||
<sourceFolder url="file://$MODULE_DIR$/java" isTestSource="true" /> | ||
<sourceFolder url="file://$MODULE_DIR$/scala" isTestSource="true" /> | ||
</content> | ||
<orderEntry type="inheritedJdk" /> | ||
<orderEntry type="sourceFolder" forTests="false" /> | ||
<orderEntry type="module" module-name="main" /> | ||
</component> | ||
</module> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My memory isn't 100% but I think the codebase may mostly avoid wildcard imports