You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/02/06 15:53:44 UTC
[GitHub] [logging-log4j2] ppkarwasz opened a new pull request #742: LOG4J2-3366 Fixes order of property sources
ppkarwasz opened a new pull request #742:
URL: https://github.com/apache/logging-log4j2/pull/742
This PR fixes the order of the property sources to:
1. Spring Boot properties with lowest numerical priority (overrides all others). In a Spring Boot application with `log4j-spring` present it will use in order: the servlet context init parameters, JNDI environment entries (from `java:comp/env` exclusively), Java system properties and system environment variables (among others, cf. [the complete list](https://docs.spring.io/spring-boot/docs/current/reference/html/features.html#features.external-config)).
2. Java system properties,
3. OS environment variables,
4. `log4j2.component.properties` to allow devs to provide some defaults.
Due to some issues with the previous implementation, the logic of the `Environment` class has been rewritten:
- the `normalized` map maps **any** key to the value of its normalization in the highest priority property source. E.g.: the key `log4j.configurationFile` will contain the value of the `log4j2.configurationFile` (remark the `2`) in Java's system properties or the value of the `LOG4J_CONFIGURATION_FILE` environment variable. Previously it contained two separate mappings: `log4j2.configurationFile` -> value in Java's system properties, `LOG4J_CONFIGURATION_FILE` -> value of the environment variable. This caused a miss for 95% of the `PropertiesUtil#getStringProperty` calls, which usually use the legacy property name.
- the three caches (`normalized`, `literal` and `tokenized`) are filled in two steps: first we recover all the keys from **enumerable** sources and then we query **all** sources to retrieve the appropriate values. Previously non-enumerable sources didn't get a chance to provide a value in these caches (the Spring Boot property source is non-enumerable).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #742: LOG4J2-3366 Fixes order of property sources
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #742:
URL: https://github.com/apache/logging-log4j2/pull/742#discussion_r801300914
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java
##########
@@ -454,28 +472,36 @@ private synchronized void reload() {
literal.clear();
normalized.clear();
tokenized.clear();
- for (final PropertySource source : sources) {
- source.forEach((key, value) -> {
- if (key != null && value != null) {
- literal.put(key, value);
- final List<CharSequence> tokens = PropertySource.Util.tokenize(key);
- if (tokens.isEmpty()) {
- normalized.put(source.getNormalForm(Collections.singleton(key)), value);
- } else {
- normalized.put(source.getNormalForm(tokens), value);
- tokenized.put(tokens, value);
+ // 1. Collects all property keys from enumerable sources.
+ final Set<String> keys = new HashSet<>();
+ sources.stream()
+ .map(PropertySource::getPropertyNames)
+ .reduce(keys, (left, right) -> {
+ left.addAll(right);
+ return left;
+ });
+ // 2. Fills the property caches. Sources with higher priority values don't override the previous ones.
+ keys.stream()
+ .filter(Objects::nonNull)
+ .forEach(key -> {
+ final List<CharSequence> tokens = PropertySource.Util.tokenize(key);
+ sources.forEach(source -> {
+ final String value = source.getProperty(key);
+ if (value != null) {
+ literal.putIfAbsent(key, value);
+ if (!tokens.isEmpty()) {
+ tokenized.putIfAbsent(tokens, value);
+ }
}
- }
+ final CharSequence normalKey = source.getNormalForm(tokens);
+ if (normalKey != null) {
+ final String normalValue = source.getProperty(normalKey.toString());
+ if (normalValue != null) {
+ normalized.putIfAbsent(key, normalValue);
+ }
+ }
+ });
});
- }
- }
-
- private static boolean hasSystemProperty(final String key) {
- try {
- return System.getProperties().containsKey(key);
- } catch (final SecurityException ignored) {
- return false;
- }
Review comment:
System properties are resolved by the `SystemPropertiesPropertySource`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] Kay-kay2019 commented on a change in pull request #742: LOG4J2-3366 Fixes order of property sources
Posted by GitBox <gi...@apache.org>.
Kay-kay2019 commented on a change in pull request #742:
URL: https://github.com/apache/logging-log4j2/pull/742#discussion_r801560830
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java
##########
@@ -435,17 +452,18 @@ private Environment(final PropertySource propertySource) {
// Access to System Properties is restricted so just skip it.
}
sources.add(propertySource);
- for (final ClassLoader classLoader : LoaderUtil.getClassLoaders()) {
- try {
- for (final PropertySource source : ServiceLoader.load(PropertySource.class, classLoader)) {
- sources.add(source);
- }
- } catch (final Throwable ex) {
- /* Don't log anything to the console. It may not be a problem that a PropertySource
- * isn't accessible.
- */
- }
- }
+ for (final ClassLoader classLoader : LoaderUtil.getClassLoaders()) {
Review comment:
Okay, thank you so much.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #742: LOG4J2-3366 Fixes order of property sources
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #742:
URL: https://github.com/apache/logging-log4j2/pull/742#discussion_r801299292
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java
##########
@@ -435,17 +452,18 @@ private Environment(final PropertySource propertySource) {
// Access to System Properties is restricted so just skip it.
}
sources.add(propertySource);
- for (final ClassLoader classLoader : LoaderUtil.getClassLoaders()) {
- try {
- for (final PropertySource source : ServiceLoader.load(PropertySource.class, classLoader)) {
- sources.add(source);
- }
- } catch (final Throwable ex) {
- /* Don't log anything to the console. It may not be a problem that a PropertySource
- * isn't accessible.
- */
- }
- }
+ for (final ClassLoader classLoader : LoaderUtil.getClassLoaders()) {
Review comment:
This was indented with tabs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on a change in pull request #742: LOG4J2-3366 Fixes order of property sources
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #742:
URL: https://github.com/apache/logging-log4j2/pull/742#discussion_r801556257
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java
##########
@@ -435,17 +452,18 @@ private Environment(final PropertySource propertySource) {
// Access to System Properties is restricted so just skip it.
}
sources.add(propertySource);
- for (final ClassLoader classLoader : LoaderUtil.getClassLoaders()) {
- try {
- for (final PropertySource source : ServiceLoader.load(PropertySource.class, classLoader)) {
- sources.add(source);
- }
- } catch (final Throwable ex) {
- /* Don't log anything to the console. It may not be a problem that a PropertySource
- * isn't accessible.
- */
- }
- }
+ for (final ClassLoader classLoader : LoaderUtil.getClassLoaders()) {
Review comment:
Never tabs, only spaces in this project; -)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #742: LOG4J2-3366 Fixes order of property sources
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #742:
URL: https://github.com/apache/logging-log4j2/pull/742#discussion_r801615031
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java
##########
@@ -435,17 +452,18 @@ private Environment(final PropertySource propertySource) {
// Access to System Properties is restricted so just skip it.
}
sources.add(propertySource);
- for (final ClassLoader classLoader : LoaderUtil.getClassLoaders()) {
- try {
- for (final PropertySource source : ServiceLoader.load(PropertySource.class, classLoader)) {
- sources.add(source);
- }
- } catch (final Throwable ex) {
- /* Don't log anything to the console. It may not be a problem that a PropertySource
- * isn't accessible.
- */
- }
- }
+ for (final ClassLoader classLoader : LoaderUtil.getClassLoaders()) {
Review comment:
@garydgregory, would you mind replacing tabs with spaces in `log4j-core`? IIRC you already did it for `log4j-1.2-api`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #742: LOG4J2-3366 Fixes order of property sources
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on pull request #742:
URL: https://github.com/apache/logging-log4j2/pull/742#issuecomment-1030915802
This PR should also solve the problem of PR #601.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #742: LOG4J2-3366 Fixes order of property sources
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #742:
URL: https://github.com/apache/logging-log4j2/pull/742#discussion_r801301570
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/PropertySource.java
##########
@@ -34,7 +38,7 @@
/**
* Returns the order in which this PropertySource has priority. A higher value means that the source will be
- * applied later so as to take precedence over other property sources.
+ * searched later and can be overridden by other property sources.
Review comment:
The documentation actually says the opposite: sources with lower numerical priority override the others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org