This is an automated email from the ASF dual-hosted git repository. pkarwasz pushed a commit to branch fix/1963_add_logging_to_properties_util in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit ba539356d1b555ee52ddcc7c29be11d5a862ad79 Author: Piotr P. Karwasz <[email protected]> AuthorDate: Thu Mar 28 13:39:53 2024 +0100 Add logging to `PropertiesUtil` and fix `Duration` parser --- .../logging/log4j/util/PropertiesUtilTest.java | 67 ++++++++++ .../log4j/util/EnvironmentPropertySource.java | 30 +++-- .../apache/logging/log4j/util/PropertiesUtil.java | 139 +++++++++++++-------- .../org/apache/logging/log4j/util/Strings.java | 3 +- .../log4j/util/SystemPropertiesPropertySource.java | 43 ++++--- .../.2.x.x/1963_add_logging_to_properties_util.xml | 8 ++ 6 files changed, 213 insertions(+), 77 deletions(-) diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/util/PropertiesUtilTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/util/PropertiesUtilTest.java index 6e3600eafe..07fd8c85e8 100644 --- a/log4j-api-test/src/test/java/org/apache/logging/log4j/util/PropertiesUtilTest.java +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/util/PropertiesUtilTest.java @@ -16,19 +16,36 @@ */ package org.apache.logging.log4j.util; +import static java.time.temporal.ChronoUnit.DAYS; +import static java.time.temporal.ChronoUnit.HOURS; +import static java.time.temporal.ChronoUnit.MICROS; +import static java.time.temporal.ChronoUnit.MILLIS; +import static java.time.temporal.ChronoUnit.MINUTES; +import static java.time.temporal.ChronoUnit.NANOS; +import static java.time.temporal.ChronoUnit.SECONDS; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.time.Duration; +import java.time.format.DateTimeParseException; +import java.util.Arrays; +import java.util.List; import java.util.Map; import java.util.Properties; +import java.util.stream.Stream; +import org.assertj.core.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.parallel.ResourceAccessMode; import org.junit.jupiter.api.parallel.ResourceLock; import org.junit.jupiter.api.parallel.Resources; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.junitpioneer.jupiter.ReadsSystemProperty; public class PropertiesUtilTest { @@ -80,6 +97,56 @@ public class PropertiesUtilTest { assertEquals(Charset.defaultCharset(), pu.getCharsetProperty("e.2")); } + static Stream<Arguments> should_properly_parse_duration() { + return Stream.of( + Arguments.of(Duration.of(1, NANOS), "1 ns"), + Arguments.of(Duration.of(1, NANOS), "1 nano"), + Arguments.of(Duration.of(3, NANOS), "3 nanos"), + Arguments.of(Duration.of(1, NANOS), "1 nanosecond"), + Arguments.of(Duration.of(5, NANOS), "5 nanoseconds"), + Arguments.of(Duration.of(6, MICROS), "6 us"), + Arguments.of(Duration.of(1, MICROS), "1 micro"), + Arguments.of(Duration.of(8, MICROS), "8 micros"), + Arguments.of(Duration.of(1, MICROS), "1 microsecond"), + Arguments.of(Duration.of(10, MICROS), "10 microseconds"), + Arguments.of(Duration.of(11, MILLIS), "11 ms"), + Arguments.of(Duration.of(1, MILLIS), "1 milli"), + Arguments.of(Duration.of(13, MILLIS), "13 millis"), + Arguments.of(Duration.of(1, MILLIS), "1 millisecond"), + Arguments.of(Duration.of(15, MILLIS), "15 milliseconds"), + Arguments.of(Duration.of(16, SECONDS), "16 s"), + Arguments.of(Duration.of(1, SECONDS), "1 second"), + Arguments.of(Duration.of(18, SECONDS), "18 seconds"), + Arguments.of(Duration.of(19, MINUTES), "19 m"), + Arguments.of(Duration.of(1, MINUTES), "1 minute"), + Arguments.of(Duration.of(21, MINUTES), "21 minutes"), + Arguments.of(Duration.of(22, HOURS), "22 h"), + Arguments.of(Duration.of(1, HOURS), "1 hour"), + Arguments.of(Duration.of(24, HOURS), "24 hours"), + Arguments.of(Duration.of(25, DAYS), "25 d"), + Arguments.of(Duration.of(1, DAYS), "1 day"), + Arguments.of(Duration.of(27, DAYS), "27 days"), + Arguments.of(Duration.of(28, MILLIS), "28")); + } + + @ParameterizedTest + @MethodSource + void should_properly_parse_duration(final Duration expected, final String value) { + Assertions.assertThat(PropertiesUtil.parseDuration(value)).isEqualTo(expected); + } + + static List<String> should_throw_on_invalid_duration() { + return Arrays.asList( + // more than long + "18446744073709551616 nanos", "1 month", "invalid pattern"); + } + + @ParameterizedTest + @MethodSource + void should_throw_on_invalid_duration(final String value) { + assertThrows(DateTimeParseException.class, () -> PropertiesUtil.parseDuration(value)); + } + @Test @ResourceLock(value = Resources.SYSTEM_PROPERTIES, mode = ResourceAccessMode.READ) public void testGetMappedProperty_sun_stdout_encoding() { diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/EnvironmentPropertySource.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/EnvironmentPropertySource.java index dc73c5056f..c9edf087d8 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/util/EnvironmentPropertySource.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/util/EnvironmentPropertySource.java @@ -36,19 +36,31 @@ import org.apache.logging.log4j.status.StatusLogger; public class EnvironmentPropertySource implements PropertySource { private static final Logger LOGGER = StatusLogger.getLogger(); - private static final String PREFIX = "LOG4J_"; - private static final int DEFAULT_PRIORITY = 100; + private static final PropertySource INSTANCE = new EnvironmentPropertySource(); + + /** + * Method used by Java 9+ to instantiate providers + * @since 2.24.0 + * @see java.util.ServiceLoader + */ + public static PropertySource provider() { + return INSTANCE; + } + @Override public int getPriority() { return DEFAULT_PRIORITY; } - private void logException(final SecurityException error) { - // There is no status logger yet. - LOGGER.error("The system environment variables are not available to Log4j due to security restrictions", error); + private static void logException(final SecurityException error) { + LOGGER.error("The environment variables are not available to Log4j due to security restrictions.", error); + } + + private static void logException(final SecurityException error, final String key) { + LOGGER.error("The environment variable {} is not available to Log4j due to security restrictions.", key, error); } @Override @@ -88,8 +100,8 @@ public class EnvironmentPropertySource implements PropertySource { return System.getenv().keySet(); } catch (final SecurityException e) { logException(e); - return PropertySource.super.getPropertyNames(); } + return PropertySource.super.getPropertyNames(); } @Override @@ -97,9 +109,9 @@ public class EnvironmentPropertySource implements PropertySource { try { return System.getenv(key); } catch (final SecurityException e) { - logException(e); - return PropertySource.super.getProperty(key); + logException(e, key); } + return PropertySource.super.getProperty(key); } @Override @@ -107,7 +119,7 @@ public class EnvironmentPropertySource implements PropertySource { try { return System.getenv().containsKey(key); } catch (final SecurityException e) { - logException(e); + logException(e, key); return PropertySource.super.containsProperty(key); } } diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java index 310f432edd..9865dd364d 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/util/PropertiesUtil.java @@ -23,6 +23,7 @@ import java.io.IOException; import java.io.InputStream; import java.nio.charset.Charset; import java.time.Duration; +import java.time.format.DateTimeParseException; import java.time.temporal.ChronoUnit; import java.time.temporal.TemporalUnit; import java.util.ArrayList; @@ -36,6 +37,9 @@ import java.util.ServiceLoader; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentSkipListSet; +import java.util.regex.MatchResult; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.status.StatusLogger; @@ -64,6 +68,8 @@ public final class PropertiesUtil { private static final Lazy<PropertiesUtil> COMPONENT_PROPERTIES = Lazy.lazy(() -> new PropertiesUtil(LOG4J_PROPERTIES_FILE_NAME, false)); + private static final Pattern DURATION_PATTERN = Pattern.compile("([+-]?\\d+)\\s*(\\w+)?", Pattern.CASE_INSENSITIVE); + private final Environment environment; /** @@ -200,9 +206,8 @@ public final class PropertiesUtil { * @return The value or null if it is not found. * @since 2.13.0 */ - @SuppressWarnings("deprecation") public Boolean getBooleanProperty(final String[] prefixes, final String key, final Supplier<Boolean> supplier) { - for (String prefix : prefixes) { + for (final String prefix : prefixes) { if (hasProperty(prefix + key)) { return getBooleanProperty(prefix + key); } @@ -243,7 +248,7 @@ public final class PropertiesUtil { return Charset.forName(mapped); } } - LOGGER.error( + LOGGER.warn( "Unable to read charset `{}` from property `{}`. Falling back to the default: `{}`", charsetName, name, @@ -263,8 +268,13 @@ public final class PropertiesUtil { if (prop != null) { try { return Double.parseDouble(prop); - } catch (final Exception ignored) { - // returns default value + } catch (final NumberFormatException e) { + LOGGER.warn( + "Unable to read double `{}` from property `{}`. Falling back to the default: `{}`", + prop, + name, + defaultValue, + e); } } return defaultValue; @@ -283,8 +293,13 @@ public final class PropertiesUtil { if (prop != null) { try { return Integer.parseInt(prop.trim()); - } catch (final Exception ignored) { - // ignore + } catch (final NumberFormatException e) { + LOGGER.warn( + "Unable to read int `{}` from property `{}`. Falling back to the default: `{}`", + prop, + name, + defaultValue, + e); } } return defaultValue; @@ -299,9 +314,8 @@ public final class PropertiesUtil { * @return The value or null if it is not found. * @since 2.13.0 */ - @SuppressWarnings("deprecation") public Integer getIntegerProperty(final String[] prefixes, final String key, final Supplier<Integer> supplier) { - for (String prefix : prefixes) { + for (final String prefix : prefixes) { if (hasProperty(prefix + key)) { return getIntegerProperty(prefix + key, 0); } @@ -321,8 +335,13 @@ public final class PropertiesUtil { if (prop != null) { try { return Long.parseLong(prop); - } catch (final Exception ignored) { - // returns the default value + } catch (final NumberFormatException e) { + LOGGER.warn( + "Unable to read long `{}` from property `{}`. Falling back to the default: `{}`", + prop, + name, + defaultValue, + e); } } return defaultValue; @@ -337,9 +356,8 @@ public final class PropertiesUtil { * @return The value or null if it is not found. * @since 2.13.0 */ - @SuppressWarnings("deprecation") public Long getLongProperty(final String[] prefixes, final String key, final Supplier<Long> supplier) { - for (String prefix : prefixes) { + for (final String prefix : prefixes) { if (hasProperty(prefix + key)) { return getLongProperty(prefix + key, 0); } @@ -357,8 +375,15 @@ public final class PropertiesUtil { */ public Duration getDurationProperty(final String name, final Duration defaultValue) { final String prop = getStringProperty(name); - if (prop != null) { - return TimeUnit.getDuration(prop); + try { + return parseDuration(prop); + } catch (final DateTimeParseException e) { + LOGGER.warn( + "Unable to read duration `{}` from property `{}`. Falling back to the default: `{}`", + prop, + name, + defaultValue, + e); } return defaultValue; } @@ -372,9 +397,8 @@ public final class PropertiesUtil { * @return The value or null if it is not found. * @since 2.13.0 */ - @SuppressWarnings("deprecation") public Duration getDurationProperty(final String[] prefixes, final String key, final Supplier<Duration> supplier) { - for (String prefix : prefixes) { + for (final String prefix : prefixes) { if (hasProperty(prefix + key)) { return getDurationProperty(prefix + key, null); } @@ -391,9 +415,8 @@ public final class PropertiesUtil { * @return The value or null if it is not found. * @since 2.13.0 */ - @SuppressWarnings("deprecation") public String getStringProperty(final String[] prefixes, final String key, final Supplier<String> supplier) { - for (String prefix : prefixes) { + for (final String prefix : prefixes) { final String result = getStringProperty(prefix + key); if (result != null) { return result; @@ -472,16 +495,18 @@ public final class PropertiesUtil { private final Map<List<CharSequence>, String> tokenized = new ConcurrentHashMap<>(); private Environment(final PropertySource propertySource) { - final PropertyFilePropertySource sysProps = - new PropertyFilePropertySource(LOG4J_SYSTEM_PROPERTIES_FILE_NAME, false); + final PropertySource sysProps = new PropertyFilePropertySource(LOG4J_SYSTEM_PROPERTIES_FILE_NAME, false); try { sysProps.forEach((key, value) -> { if (System.getProperty(key) == null) { System.setProperty(key, value); } }); - } catch (SecurityException ex) { - // Access to System Properties is restricted so just skip it. + } catch (final SecurityException e) { + LOGGER.warn( + "Unable to set Java system properties from {} file, due to security restrictions.", + LOG4J_SYSTEM_PROPERTIES_FILE_NAME, + e); } sources.add(propertySource); // We don't log anything on the status logger. @@ -649,45 +674,57 @@ public final class PropertiesUtil { * @return true if system properties tell us we are running on Windows. */ public boolean isOsWindows() { - return getStringProperty("os.name", "").startsWith("Windows"); + return SystemPropertiesPropertySource.getSystemProperty("os.name", "").startsWith("Windows"); + } + + static Duration parseDuration(final String value) { + final Matcher matcher = DURATION_PATTERN.matcher(value); + if (matcher.matches()) { + return Duration.of(TimeUnit.parseAmount(matcher), TimeUnit.parseUnit(matcher)); + } + throw new DateTimeParseException("Invalid duration value: should be in the format nnn[unit].", value, 0); } private enum TimeUnit { - NANOS("ns,nano,nanos,nanosecond,nanoseconds", ChronoUnit.NANOS), - MICROS("us,micro,micros,microsecond,microseconds", ChronoUnit.MICROS), - MILLIS("ms,milli,millis,millsecond,milliseconds", ChronoUnit.MILLIS), - SECONDS("s,second,seconds", ChronoUnit.SECONDS), - MINUTES("m,minute,minutes", ChronoUnit.MINUTES), - HOURS("h,hour,hours", ChronoUnit.HOURS), - DAYS("d,day,days", ChronoUnit.DAYS); - - /* - * https://errorprone.info/bugpattern/ImmutableEnumChecker - * This field is effectively immutable. - */ - @SuppressWarnings("ImmutableEnumChecker") - private final String[] descriptions; + NANOS(new String[] {"ns", "nano", "nanos", "nanosecond", "nanoseconds"}, ChronoUnit.NANOS), + MICROS(new String[] {"us", "micro", "micros", "microsecond", "microseconds"}, ChronoUnit.MICROS), + MILLIS(new String[] {"ms", "milli", "millis", "millisecond", "milliseconds"}, ChronoUnit.MILLIS), + SECONDS(new String[] {"s", "second", "seconds"}, ChronoUnit.SECONDS), + MINUTES(new String[] {"m", "minute", "minutes"}, ChronoUnit.MINUTES), + HOURS(new String[] {"h", "hour", "hours"}, ChronoUnit.HOURS), + DAYS(new String[] {"d", "day", "days"}, ChronoUnit.DAYS); - private final ChronoUnit timeUnit; + private final String[] descriptions; + private final TemporalUnit timeUnit; - TimeUnit(final String descriptions, final ChronoUnit timeUnit) { - this.descriptions = descriptions.split(","); + TimeUnit(final String[] descriptions, final TemporalUnit timeUnit) { + this.descriptions = descriptions; this.timeUnit = timeUnit; } - static Duration getDuration(final String time) { - final String value = time.trim(); - TemporalUnit temporalUnit = ChronoUnit.MILLIS; - long timeVal = 0; - for (TimeUnit timeUnit : values()) { - for (String suffix : timeUnit.descriptions) { - if (value.endsWith(suffix)) { - temporalUnit = timeUnit.timeUnit; - timeVal = Long.parseLong(value.substring(0, value.length() - suffix.length())); + private static long parseAmount(final MatchResult matcher) { + final String amount = matcher.group(1); + try { + return Long.parseLong(amount); + } catch (final NumberFormatException e) { + throw new DateTimeParseException( + "Invalid time amount '" + amount + "'", matcher.group(), matcher.start(1), e); + } + } + + private static TemporalUnit parseUnit(final MatchResult matcher) { + final String unit = matcher.group(2); + if (unit != null) { + for (final TimeUnit value : values()) { + for (final String description : value.descriptions) { + if (unit.equals(description)) { + return value.timeUnit; + } } } + throw new DateTimeParseException("Invalid time unit '" + unit + "'", matcher.group(), matcher.start(2)); } - return Duration.of(timeVal, temporalUnit); + return ChronoUnit.MILLIS; } } } diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/Strings.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/Strings.java index bee54324f4..91057b53d6 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/util/Strings.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/util/Strings.java @@ -52,8 +52,7 @@ public final class Strings { * OS-dependent line separator, defaults to {@code "\n"} if the system property {@code ""line.separator"} cannot be * read. */ - public static final String LINE_SEPARATOR = - SystemPropertiesPropertySource.getSystemProperty("line.separator", "\n"); + public static final String LINE_SEPARATOR = System.lineSeparator(); /** * Returns a double quoted string. diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/SystemPropertiesPropertySource.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/SystemPropertiesPropertySource.java index c7b04ae933..e49dad0dd5 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/util/SystemPropertiesPropertySource.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/util/SystemPropertiesPropertySource.java @@ -21,6 +21,8 @@ import aQute.bnd.annotation.spi.ServiceProvider; import java.util.Collection; import java.util.Objects; import java.util.Properties; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.status.StatusLogger; /** * PropertySource backed by the current system properties. Other than having a @@ -32,19 +34,32 @@ import java.util.Properties; @ServiceProvider(value = PropertySource.class, resolution = Resolution.OPTIONAL) public class SystemPropertiesPropertySource implements PropertySource { + private static final Logger LOGGER = StatusLogger.getLogger(); private static final int DEFAULT_PRIORITY = 0; private static final String PREFIX = "log4j2."; + private static final PropertySource INSTANCE = new SystemPropertiesPropertySource(); + /** - * Used by bootstrap code to get system properties without loading PropertiesUtil. + * Method used by Java 9+ to instantiate providers + * @since 2.24.0 + * @see java.util.ServiceLoader */ + public static PropertySource provider() { + return INSTANCE; + } + public static String getSystemProperty(final String key, final String defaultValue) { - try { - return System.getProperty(key, defaultValue); - } catch (SecurityException e) { - // Silently ignore the exception - return defaultValue; - } + final String value = INSTANCE.getProperty(key); + return value != null ? value : defaultValue; + } + + private static void logException(final SecurityException error) { + LOGGER.error("The Java system properties are not available to Log4j due to security restrictions.", error); + } + + private static void logException(final SecurityException error, final String key) { + LOGGER.error("The Java system property {} is not available to Log4j due to security restrictions.", key, error); } @Override @@ -54,15 +69,11 @@ public class SystemPropertiesPropertySource implements PropertySource { @Override public void forEach(final BiConsumer<String, String> action) { - Properties properties; + final Properties properties; try { properties = System.getProperties(); } catch (final SecurityException e) { - // (1) There is no status logger. - // (2) LowLevelLogUtil also consults system properties ("line.separator") to - // open a BufferedWriter, so this may fail as well. Just having a hard reference - // in this code to LowLevelLogUtil would cause a problem. - // (3) We could log to System.err (nah) or just be quiet as we do now. + logException(e); return; } // Lock properties only long enough to get a thread-safe SAFE snapshot of its @@ -89,8 +100,9 @@ public class SystemPropertiesPropertySource implements PropertySource { try { return System.getProperties().stringPropertyNames(); } catch (final SecurityException e) { - return PropertySource.super.getPropertyNames(); + logException(e); } + return PropertySource.super.getPropertyNames(); } @Override @@ -98,8 +110,9 @@ public class SystemPropertiesPropertySource implements PropertySource { try { return System.getProperty(key); } catch (final SecurityException e) { - return PropertySource.super.getProperty(key); + logException(e, key); } + return PropertySource.super.getProperty(key); } @Override diff --git a/src/changelog/.2.x.x/1963_add_logging_to_properties_util.xml b/src/changelog/.2.x.x/1963_add_logging_to_properties_util.xml new file mode 100644 index 0000000000..5745caf8e1 --- /dev/null +++ b/src/changelog/.2.x.x/1963_add_logging_to_properties_util.xml @@ -0,0 +1,8 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xmlns="http://logging.apache.org/log4j/changelog"; + xsi:schemaLocation="http://logging.apache.org/log4j/changelog https://logging.apache.org/log4j/changelog-0.1.3.xsd"; + type="changed"> + <issue id="1936" link="https://github.com/apache/logging-log4j2/pull/1936"/> + <description format="asciidoc">Add logging to `PropertiesUtil` and fix `Duration` parser.</description> +</entry>
