Skip to content
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.

Sign up for GitHub

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

Fix precedence for explicit sys prop and env var sources. #1002

Merged
merged 3 commits into from
Sep 9, 2019
Merged

Fix precedence for explicit sys prop and env var sources. #1002

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d89e609
Fix precedence for explicit sys prop and env var sources.
batsatt Sep 6, 2019
d737d03
Introduce explicit types for sys props and env vars config sources.
batsatt Sep 8, 2019
f095b90
Copyright fix.
batsatt Sep 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions config/config/src/main/java/io/helidon/config/BuilderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import io.helidon.config.ConfigMapperManager.MapperProviders;
import io.helidon.config.internal.ConfigThreadFactory;
import io.helidon.config.internal.ConfigUtils;
import io.helidon.config.internal.MapConfigSource;
import io.helidon.config.spi.ConfigContext;
import io.helidon.config.spi.ConfigFilter;
import io.helidon.config.spi.ConfigMapper;
Expand Down Expand Up @@ -312,13 +313,19 @@ private ProviderImpl buildProvider() {

private ConfigSource targetConfigSource(ConfigContext context) {
List<ConfigSource> targetSources = new LinkedList<>();
if (environmentVariablesSourceEnabled) {

if (hasMapSource(ConfigSources.ENV_VARS_NAME)) {
envVarAliasGeneratorEnabled = true;
} else if (environmentVariablesSourceEnabled) {
targetSources.add(ConfigSources.environmentVariables());
envVarAliasGeneratorEnabled = true;
}
if (systemPropertiesSourceEnabled) {

if (systemPropertiesSourceEnabled
&& !hasMapSource(ConfigSources.SYS_PROPS_NAME)) {
targetSources.add(ConfigSources.systemProperties());
}

if (sources != null) {
targetSources.addAll(sources);
} else {
Expand All @@ -335,6 +342,19 @@ private ConfigSource targetConfigSource(ConfigContext context) {
return targetConfigSource;
}

private boolean hasMapSource(String name) {
if (sources != null) {
String qualifiedName = "[" + name + "]";
for (ConfigSource source : sources) {
if (source instanceof MapConfigSource
&& source.description().contains(qualifiedName)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we rely on a description, you should also update the place the description is created to make sure it is not changed.
I would prefer a more deterministic way of finding these sources out - maybe wrapping them in a specific class (EnvVarConfigSource and SysPropConfigSource or similar) and looking for these types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed, this is a little fragile; I'm just about to lose internet for a couple of days, so will deal with this next week.

return true;
}
}
}
return false;
}

@SuppressWarnings("ParameterNumber")
ProviderImpl createProvider(ConfigMapperManager configMapperManager,
ConfigSource targetConfigSource,
Expand Down
89 changes: 83 additions & 6 deletions config/config/src/test/java/io/helidon/config/ConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import io.helidon.config.Config.Key;
import io.helidon.config.spi.ConfigNode.ListNode;
import io.helidon.config.spi.ConfigNode.ObjectNode;
import io.helidon.config.spi.ConfigSourceTest;
import io.helidon.config.test.infra.RestoreSystemPropertiesExt;

import org.hamcrest.Matcher;
Expand All @@ -36,8 +37,6 @@
import static io.helidon.config.Config.Type.LIST;
import static io.helidon.config.Config.Type.OBJECT;
import static io.helidon.config.Config.Type.VALUE;
import static io.helidon.config.spi.ConfigSourceTest.TEST_ENV_VAR_NAME;
import static io.helidon.config.spi.ConfigSourceTest.TEST_ENV_VAR_VALUE;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.equalTo;
Expand All @@ -61,12 +60,19 @@ public class ConfigTest {
private static final String TEST_SYS_PROP_NAME = "this_is_my_property-ConfigTest";
private static final String TEST_SYS_PROP_VALUE = "This Is My SYS PROPS Value.";

private static final String TEST_ENV_VAR_NAME = "FOO_BAR";
private static final String TEST_ENV_VAR_VALUE = "mapped-env-value";

private static final String OVERRIDE_NAME = "simple";
private static final String OVERRIDE_ENV_VAR_VALUE = "unmapped-env-value";
private static final String OVERRIDE_SYS_PROP_VALUE = "unmapped-sys-prop-value";

private static final boolean LOG = false;

private static final String OBJECT_VALUE_PREFIX = "object-";
private static final String LIST_VALUE_PREFIX = "list-";

private void testKeyNotSet(Config config) {
private static void testKeyNotSet(Config config) {
assertThat(config, not(nullValue()));
assertThat(config.traverse().collect(Collectors.toList()), not(empty()));
assertThat(config.get(TEST_SYS_PROP_NAME).type(), is(Config.Type.MISSING));
Expand Down Expand Up @@ -105,19 +111,19 @@ public void testBuilderDefaultConfigSourceKeyFromSysProps() {
private void testKeyFromEnvVars(Config config) {
assertThat(config, not(nullValue()));
assertThat(config.traverse().collect(Collectors.toList()), not(empty()));
assertThat(config.get(TEST_ENV_VAR_NAME).asString(), is(ConfigValues.simpleValue(TEST_ENV_VAR_VALUE)));
assertThat(config.get(ConfigSourceTest.TEST_ENV_VAR_NAME).asString(), is(ConfigValues.simpleValue(ConfigSourceTest.TEST_ENV_VAR_VALUE)));
}

@Test
public void testCreateKeyFromEnvVars() {
System.setProperty(TEST_ENV_VAR_NAME, "This value is not used, but from Env Vars, see pom.xml!");
System.setProperty(ConfigSourceTest.TEST_ENV_VAR_NAME, "This value is not used, but from Env Vars, see pom.xml!");

testKeyFromEnvVars(Config.create());
}

@Test
public void testBuilderDefaultConfigSourceKeyFromEnvVars() {
System.setProperty(TEST_ENV_VAR_NAME, "This value is not used, but from Env Vars, see pom.xml!");
System.setProperty(ConfigSourceTest.TEST_ENV_VAR_NAME, "This value is not used, but from Env Vars, see pom.xml!");

testKeyFromEnvVars(Config.builder().build());
}
Expand Down Expand Up @@ -616,6 +622,77 @@ objects to assign values to the complex nodes (object- and list-type)
assertThat(config.get("list-1").asString(), is(ConfigValues.simpleValue(LIST_VALUE_PREFIX + valueQual + "-2")));
}

@Test
void testImplicitSysPropAndEnvVarPrecedence() {
System.setProperty(OVERRIDE_NAME, OVERRIDE_SYS_PROP_VALUE);
System.setProperty(TEST_SYS_PROP_NAME, TEST_SYS_PROP_VALUE);

Config config = Config.create();

assertThat(config.get(TEST_SYS_PROP_NAME).asString().get(), is(TEST_SYS_PROP_VALUE));
assertThat(config.get(TEST_ENV_VAR_NAME).asString().get(), is(TEST_ENV_VAR_VALUE));

assertThat(config.get(OVERRIDE_NAME).asString().get(), is(OVERRIDE_ENV_VAR_VALUE));
}

@Test
void testExplicitSysPropAndEnvVarPrecedence() {
System.setProperty(OVERRIDE_NAME, OVERRIDE_SYS_PROP_VALUE);
System.setProperty(TEST_SYS_PROP_NAME, TEST_SYS_PROP_VALUE);

Config config = Config.builder()
.sources(ConfigSources.systemProperties(),
ConfigSources.environmentVariables())
.build();

assertThat(config.get(TEST_SYS_PROP_NAME).asString().get(), is(TEST_SYS_PROP_VALUE));
assertThat(config.get(TEST_ENV_VAR_NAME).asString().get(), is(TEST_ENV_VAR_VALUE));

assertThat(config.get(OVERRIDE_NAME).asString().get(), is(OVERRIDE_SYS_PROP_VALUE));

config = Config.builder()
.sources(ConfigSources.environmentVariables(),
ConfigSources.systemProperties())
.build();

assertThat(config.get(TEST_SYS_PROP_NAME).asString().get(), is(TEST_SYS_PROP_VALUE));
assertThat(config.get(TEST_ENV_VAR_NAME).asString().get(), is(TEST_ENV_VAR_VALUE));

assertThat(config.get(OVERRIDE_NAME).asString().get(), is(OVERRIDE_ENV_VAR_VALUE));
}

@Test
void testExplicitEnvVarSourceAndImplicitSysPropSourcePrecedence() {
System.setProperty(OVERRIDE_NAME, OVERRIDE_SYS_PROP_VALUE);
System.setProperty(TEST_SYS_PROP_NAME, TEST_SYS_PROP_VALUE);

Config config = Config.builder()
.sources(ConfigSources.environmentVariables())
.build();

assertThat(config.get(TEST_SYS_PROP_NAME).asString().get(), is(TEST_SYS_PROP_VALUE));
assertThat(config.get(TEST_ENV_VAR_NAME).asString().get(), is(TEST_ENV_VAR_VALUE));

// Implicit sources always take precedence! (??)
assertThat(config.get(OVERRIDE_NAME).asString().get(), is(OVERRIDE_SYS_PROP_VALUE));
}

@Test
void testExplicitSysPropSourceAndImplicitEnvVarSourcePrecedence() {
System.setProperty(OVERRIDE_NAME, OVERRIDE_SYS_PROP_VALUE);
System.setProperty(TEST_SYS_PROP_NAME, TEST_SYS_PROP_VALUE);

Config config = Config.builder()
.sources(ConfigSources.systemProperties())
.build();

assertThat(config.get(TEST_SYS_PROP_NAME).asString().get(), is(TEST_SYS_PROP_VALUE));
assertThat(config.get(TEST_ENV_VAR_NAME).asString().get(), is(TEST_ENV_VAR_VALUE));

// Implicit sources always take precedence! (??)
assertThat(config.get(OVERRIDE_NAME).asString().get(), is(OVERRIDE_ENV_VAR_VALUE));
}

private void testConfigKeyEscapeUnescapeName(String name, String escapedName) {
assertThat(Key.escapeName(name), is(escapedName));
assertThat(Key.unescapeName(escapedName), is(name));
Expand Down