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

Do not record local sources in runtime config defaults. #39604

Merged
merged 1 commit into from
Mar 25, 2024
Merged

Do not record local sources in runtime config defaults. #39604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
67 changes: 54 additions & 13 deletions ...yment/src/main/java/io/quarkus/deployment/configuration/BuildTimeConfigurationReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import static io.quarkus.runtime.configuration.PropertiesUtil.filterPropertiesInRoots;
import static io.smallrye.config.ConfigMappings.ConfigClassWithPrefix.configClassWithPrefix;
import static io.smallrye.config.Expressions.withoutExpansion;
import static io.smallrye.config.PropertiesConfigSourceProvider.classPathSources;
import static io.smallrye.config.SmallRyeConfigBuilder.META_INF_MICROPROFILE_CONFIG_PROPERTIES;
import static java.util.stream.Collectors.toSet;

import java.io.IOException;
Expand Down Expand Up @@ -512,10 +514,12 @@ ReadResult run() {
nameBuilder.setLength(0);
}

SmallRyeConfig runtimeConfig = getConfigForRuntimeRecording();

// Register defaults for Roots
allBuildTimeValues.putAll(getDefaults(buildTimePatternMap));
buildTimeRunTimeValues.putAll(getDefaults(buildTimeRunTimePatternMap));
runTimeDefaultValues.putAll(getDefaults(runTimePatternMap));
allBuildTimeValues.putAll(getDefaults(config, buildTimePatternMap));
buildTimeRunTimeValues.putAll(getDefaults(config, buildTimeRunTimePatternMap));
runTimeDefaultValues.putAll(getDefaults(runtimeConfig, runTimePatternMap));
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify something: when you start your runtime app, how are the config sources evaluated?

What I'm wondering is if we need to register default values for runtime? Is it because we don't parse the existing config such as application.properties at runtime?

Asking because my inclination would have been to record nothing for runtime values and let things be determined at runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, we flatten the sources to a single source to optimize lookup and save on loading. We could try to remove the runtime recording and generate the runtime source of each application.properties and let it be loaded by the Config.


// Register defaults for Mappings
// Runtime defaults are added in ConfigGenerationBuildStep.generateBuilders to include user mappings
Expand Down Expand Up @@ -602,7 +606,7 @@ ReadResult run() {
knownProperty = knownProperty || matched != null;
if (matched != null) {
// it's a run-time default (record for later)
ConfigValue configValue = withoutExpansion(() -> config.getConfigValue(propertyName));
ConfigValue configValue = withoutExpansion(() -> runtimeConfig.getConfigValue(propertyName));
if (configValue.getValue() != null) {
runTimeValues.put(configValue.getNameProfiled(), configValue.getValue());
}
Expand All @@ -613,7 +617,7 @@ ReadResult run() {
}
} else {
// it's not managed by us; record it
ConfigValue configValue = withoutExpansion(() -> config.getConfigValue(propertyName));
ConfigValue configValue = withoutExpansion(() -> runtimeConfig.getConfigValue(propertyName));
if (configValue.getValue() != null) {
runTimeValues.put(configValue.getNameProfiled(), configValue.getValue());
}
Expand All @@ -640,7 +644,7 @@ ReadResult run() {
for (String property : mappedProperties) {
unknownBuildProperties.remove(property);
ConfigValue value = config.getConfigValue(property);
if (value != null && value.getRawValue() != null) {
if (value.getRawValue() != null) {
allBuildTimeValues.put(value.getNameProfiled(), value.getRawValue());
}
}
Expand All @@ -652,7 +656,7 @@ ReadResult run() {
for (String property : mappedProperties) {
unknownBuildProperties.remove(property);
ConfigValue value = config.getConfigValue(property);
if (value != null && value.getRawValue() != null) {
if (value.getRawValue() != null) {
allBuildTimeValues.put(value.getNameProfiled(), value.getRawValue());
buildTimeRunTimeValues.put(value.getNameProfiled(), value.getRawValue());
}
Expand All @@ -664,8 +668,8 @@ ReadResult run() {
Set<String> mappedProperties = ConfigMappings.mappedProperties(mapping, allProperties);
for (String property : mappedProperties) {
unknownBuildProperties.remove(property);
ConfigValue value = config.getConfigValue(property);
if (value != null && value.getRawValue() != null) {
ConfigValue value = runtimeConfig.getConfigValue(property);
if (value.getRawValue() != null) {
runTimeValues.put(value.getNameProfiled(), value.getRawValue());
}
}
Expand Down Expand Up @@ -1117,6 +1121,41 @@ public Set<String> getPropertyNames() {
return properties;
}

/**
* Use this Config instance to record the runtime default values. We cannot use the main Config
* instance because it may record values coming from local development sources (Environment Variables,
* System Properties, etc.) in at build time. Local config source values may be completely different between the
Copy link
Member

@maxandersen maxandersen Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

can we be a bit more specific here than just "etc" - what are we actually using from recording?

looking at code below we are not excluding sources but just gathering a list of sources...what is actually defining what we include/exclude here?~

i see explicit exlucde of EnvConfig and SysVar ...does that include .env?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we build a new Config instance, excluding specific sources. The .env sources are also of type EnvConfigSource, so they get excluded (or, to be more correct, not added to the config instance used for recording).

* build environment and the runtime environment, so it doesn't make sense to record these.
*
* @return a new {@link SmallRyeConfig} instance without the local sources, including SysPropConfigSource,
* EnvConfigSource, .env, and Build system sources.
*/
private SmallRyeConfig getConfigForRuntimeRecording() {
SmallRyeConfigBuilder builder = ConfigUtils.emptyConfigBuilder();
builder.getSources().clear();
builder.getSourceProviders().clear();
builder.setAddDefaultSources(false)
// Customizers may duplicate sources, but not much we can do about it, we need to run them
.addDiscoveredCustomizers()
// Read microprofile-config.properties, because we disabled the default sources
.withSources(classPathSources(META_INF_MICROPROFILE_CONFIG_PROPERTIES, classLoader));

// TODO - Should we reset quarkus.config.location to not record from these sources?
Copy link
Member

Choose a reason for hiding this comment

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

As for quarkus.config.locations, it's not that easy to decide what to do.

But it resonates with the comment I made above: I don't fully understand why we need to register defaults for runtime values?
My understanding would be that they should be fully resolved at runtime with whatever config sources are around at runtime.

Copy link
Member

Choose a reason for hiding this comment

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

Originally (IIRC) the idea was that if the user put a value in application.properties for a run time property at build time, then they would expect that value to still be set at run time.

Copy link
Member Author

Choose a reason for hiding this comment

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

It depends on the phase. We need to record the values for build time - runtime fixed to ensure they do not change between build and runtime.

For runtime, it has been a convenience; to flatten the sources to reduce lookups, avoid loading the config files, and ensure that Quarkus would still run even if a property was missing by falling back to the recorded value from the build.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so I'm not aware of all the consequences and maybe it could have some consequences but my intuition tells me to drop the recording for runtime and resolve things at runtime.

Of course, I would try to avoid reading the application.properties we have in the app and have it preprocessed but I wouldn't go further than that.

Now, I might be missing something entirely and it might cause issues I don't foresee, be it for performance or other things.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can certainly do that. I don't foresee any big consequences, and regarding performance, it is hard to tell the impact without measuring it, but we should be able to figure out something.

We must still record the runtime defaults from roots, mappings, and build steps, but we don't need the config instance in that case.

for (ConfigSource configSource : config.getConfigSources()) {
if (configSource instanceof SysPropConfigSource) {
continue;
}
if (configSource instanceof EnvConfigSource) {
continue;
}
if ("PropertiesConfigSource[source=Build system]".equals(configSource.getName())) {
continue;
}
builder.withSources(configSource);
}
return builder.build();
}

private Map<String, String> filterActiveProfileProperties(final Map<String, String> properties) {
Set<String> propertiesToRemove = new HashSet<>();
for (String property : properties.keySet()) {
Expand All @@ -1131,13 +1170,15 @@ private Map<String, String> filterActiveProfileProperties(final Map<String, Stri
return properties;
}

private Map<String, String> getDefaults(final ConfigPatternMap<Container> patternMap) {
private static Map<String, String> getDefaults(final SmallRyeConfig config,
final ConfigPatternMap<Container> patternMap) {
Map<String, String> defaultValues = new TreeMap<>();
getDefaults(defaultValues, new StringBuilder(), patternMap);
getDefaults(config, defaultValues, new StringBuilder(), patternMap);
return defaultValues;
}

private void getDefaults(
private static void getDefaults(
final SmallRyeConfig config,
final Map<String, String> defaultValues,
final StringBuilder propertyName,
final ConfigPatternMap<Container> patternMap) {
Expand All @@ -1164,7 +1205,7 @@ private void getDefaults(
}

for (String childName : patternMap.childNames()) {
getDefaults(defaultValues,
getDefaults(config, defaultValues,
new StringBuilder(propertyName).append(childName.equals(ConfigPatternMap.WILD_CARD) ? "*" : childName),
patternMap.getChild(childName));
}
Expand Down
6 changes: 1 addition & 5 deletions ...red/projects/build-mode-quarkus-profile-override/src/main/java/org/acme/HelloService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,7 @@
@IfBuildProfile("foo")
@ApplicationScoped
public class HelloService {

@ConfigProperty(name = "name")
String name;

public String name() {
return name;
return "from foo";
}
}
2 changes: 0 additions & 2 deletions ...ed/projects/build-mode-quarkus-profile-override/src/main/resources/application.properties
View file
Open in desktop

This file was deleted.

7 changes: 1 addition & 6 deletions ...red/projects/build-mode-quarkus-profile-property/src/main/java/org/acme/HelloService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
package org.acme;

import io.quarkus.arc.profile.IfBuildProfile;
import org.eclipse.microprofile.config.inject.ConfigProperty;

import jakarta.enterprise.context.ApplicationScoped;

@IfBuildProfile("foo")
@ApplicationScoped
public class HelloService {

@ConfigProperty(name = "name")
String name;

public String name() {
return name;
return "from foo";
}
}
2 changes: 0 additions & 2 deletions ...ed/projects/build-mode-quarkus-profile-property/src/main/resources/application.properties
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions ...ation-tests/test-extension/extension/deployment/src/main/resources/application.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ quarkus.arc.unremovable-types=foo
# The YAML source may add an indexed property (depending on how the YAML is laid out). This is not supported by @ConfigRoot
quarkus.arc.unremovable-types[0]=foo

### Do not record env values in build time
bt.ok.to.record=properties
### recording
bt.ok.to.record=from-app
%test.bt.profile.record=properties
Copy link
Member

Choose a reason for hiding this comment

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

Hi @radcortez, quick question, does %test.bt.profile.record=properties mean that it is OK to record the bt profile config property values in the test mode but the recording for this profile is off in the prod mode ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We record all profiled names. We need to ensure that the recorded properties retain their original property name, to not override each other. Local sources are excluded from any recording for any case with this PR.


### mappings
Expand Down
19 changes: 12 additions & 7 deletions ...st-extension/extension/deployment/src/test/java/io/quarkus/extest/ConfiguredBeanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.extest.runtime.config.AnotherPrefixConfig;
import io.quarkus.extest.runtime.config.DoNotRecordEnvConfigSource;
import io.quarkus.extest.runtime.config.MyEnum;
import io.quarkus.extest.runtime.config.NestedConfig;
import io.quarkus.extest.runtime.config.ObjectOfValue;
Expand All @@ -52,10 +53,9 @@ public class ConfiguredBeanTest {
static final QuarkusUnitTest TEST = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
.addClasses(ConfiguredBean.class)
// Don't change this to types, because of classloader class cast exception.
.addAsServiceProvider("org.eclipse.microprofile.config.spi.ConfigSource",
"io.quarkus.extest.runtime.config.OverrideBuildTimeConfigSource\n" +
"io.quarkus.extest.runtime.config.RecordQuarkusSystemPropertiesConfigSource")
.addAsServiceProvider(ConfigSource.class,
OverrideBuildTimeConfigSource.class,
DoNotRecordEnvConfigSource.class)
.addAsResource("application.properties"));

@Inject
Expand Down Expand Up @@ -364,11 +364,16 @@ public void testProfileDefaultValuesSource() {
assertEquals("5678", defaultValues.getValue("%dev.my.prop"));
assertEquals("1234", defaultValues.getValue("%test.my.prop"));
assertEquals("1234", config.getValue("my.prop", String.class));

// runtime properties coming from env must not be recorded
assertNull(defaultValues.getValue("should.not.be.recorded"));
assertNull(defaultValues.getValue("SHOULD_NOT_BE_RECORDED"));
assertEquals("value", defaultValues.getValue("quarkus.mapping.rt.record"));
assertEquals("prod", defaultValues.getValue("%prod.quarkus.mapping.rt.record"));
assertEquals("dev", defaultValues.getValue("%dev.quarkus.mapping.rt.record"));
assertNull(defaultValues.getValue("quarkus.rt.do-not-record"));
assertEquals("value", config.getRawValue("quarkus.rt.do-not-record"));
assertNull(defaultValues.getValue("quarkus.mapping.rt.do-not-record"));
assertNull(defaultValues.getValue("%prod.quarkus.mapping.rt.do-not-record"));
assertNull(defaultValues.getValue("%dev.quarkus.mapping.rt.do-not-record"));
assertEquals("value", config.getRawValue("quarkus.mapping.rt.do-not-record"));
}

@Test
Expand Down
18 changes: 11 additions & 7 deletions ...t-extension/extension/deployment/src/test/java/io/quarkus/extest/RuntimeDefaultsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,15 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.extest.runtime.config.EnvBuildTimeConfigSource;
import io.quarkus.test.QuarkusUnitTest;
import io.smallrye.config.SmallRyeConfig;

public class RuntimeDefaultsTest {
@RegisterExtension
static final QuarkusUnitTest TEST = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar
// Don't change this to types, because of classloader class cast exception.
.addAsServiceProvider("org.eclipse.microprofile.config.spi.ConfigSource",
"io.quarkus.extest.runtime.config.EnvBuildTimeConfigSource")
.addAsServiceProvider(ConfigSource.class, EnvBuildTimeConfigSource.class)
.addAsResource("application.properties"));

@Inject
Expand All @@ -31,20 +30,25 @@ public class RuntimeDefaultsTest {
void doNotRecordEnvRuntimeDefaults() {
Optional<ConfigSource> defaultValues = config.getConfigSource("DefaultValuesConfigSource");
assertTrue(defaultValues.isPresent());
// It's ok to record env properties for a Quarkus root
assertEquals("changed", defaultValues.get().getValue("quarkus.rt.rt-string-opt"));
// It's ok to record env properties for a property available in another source
assertEquals("env-source", defaultValues.get().getValue("bt.ok.to.record"));
// Do not record Env values for runtime
assertNull(defaultValues.get().getValue("quarkus.mapping.rt.do-not-record"));
assertEquals("value", config.getRawValue("quarkus.mapping.rt.do-not-record"));
// Property available in both Env and application.properties, ok to record application.properties value
assertEquals("from-app", defaultValues.get().getValue("bt.ok.to.record"));
// You still get the value from Env
assertEquals("from-env", config.getRawValue("bt.ok.to.record"));
// Do not record any of the other properties
assertNull(defaultValues.get().getValue(("do.not.record")));
assertNull(defaultValues.get().getValue(("DO_NOT_RECORD")));
assertEquals("value", config.getRawValue("do.not.record"));
}

@Test
void doNotRecordActiveUnprofiledPropertiesDefaults() {
Optional<ConfigSource> defaultValues = config.getConfigSource("DefaultValuesConfigSource");
assertTrue(defaultValues.isPresent());
assertEquals("properties", config.getRawValue("bt.profile.record"));
// Property needs to be recorded as is, including the profile name
assertEquals("properties", defaultValues.get().getValue("%test.bt.profile.record"));
assertNull(defaultValues.get().getValue("bt.profile.record"));
}
Expand Down
19 changes: 19 additions & 0 deletions ...on/runtime/src/main/java/io/quarkus/extest/runtime/config/DoNotRecordEnvConfigSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package io.quarkus.extest.runtime.config;

import java.util.Map;

import io.quarkus.runtime.annotations.StaticInitSafe;
import io.smallrye.config.EnvConfigSource;

@StaticInitSafe
public class DoNotRecordEnvConfigSource extends EnvConfigSource {
public DoNotRecordEnvConfigSource() {
super(Map.of(
"SHOULD_NOT_BE_RECORDED", "value",
"should.not.be.recorded", "value",
"quarkus.rt.do-not-record", "value",
"quarkus.mapping.rt.do-not-record", "value",
"%dev.quarkus.mapping.rt.do-not-record", "dev",
"_PROD_QUARKUS_MAPPING_RT_DO_NOT_RECORD", "prod"), 300);
}
}
8 changes: 5 additions & 3 deletions ...sion/runtime/src/main/java/io/quarkus/extest/runtime/config/EnvBuildTimeConfigSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@

public class EnvBuildTimeConfigSource extends EnvConfigSource {
public EnvBuildTimeConfigSource() {
super(Map.of("QUARKUS_RT_RT_STRING_OPT", "changed",
"BT_OK_TO_RECORD", "env-source",
"DO_NOT_RECORD", "record"), Integer.MAX_VALUE);
super(Map.of(
"QUARKUS_MAPPING_RT_DO_NOT_RECORD", "value",
"BT_OK_TO_RECORD", "from-env",
"BT_DO_NOT_RECORD", "value",
"DO_NOT_RECORD", "value"), Integer.MAX_VALUE);
}
}
18 changes: 0 additions & 18 deletions ...main/java/io/quarkus/extest/runtime/config/RecordQuarkusSystemPropertiesConfigSource.java
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion .../extension/runtime/src/main/java/io/quarkus/extest/runtime/config/TestMappingRunTime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public interface TestMappingRunTime {
Group group();

/** Record values from env test **/
Optional<String> record();
Optional<String> doNotRecord();

/** Record values with named profile **/
Optional<String> recordProfiled();
Expand Down
4 changes: 4 additions & 0 deletions ...n/extension/runtime/src/main/java/io/quarkus/extest/runtime/config/TestRunTimeConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ public class TestRunTimeConfig {

public Map<String, Map<String, String>> mapMap;

/** Do not record **/
@ConfigItem
public Optional<String> doNotRecord;

@Override
public String toString() {
return "TestRunTimeConfig{" +
Expand Down
Loading