From 918190e6a8a38e778716d7533df56c526f87f122 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Fri, 23 Feb 2024 19:03:46 +0100 Subject: [PATCH 1/8] Add time-zone support to `StatusLogger` Without a time-zone, it is not possible to format certain date & time fields that are time-zone-specific, e.g., year-of-era. --- .../log4j/status/StatusLoggerDateTest.java | 118 ++++++++++++++++++ .../log4j/status/StatusLoggerLevelTest.java | 2 + .../logging/log4j/status/StatusData.java | 7 +- .../logging/log4j/status/StatusLogger.java | 55 +++++++- 4 files changed, 175 insertions(+), 7 deletions(-) create mode 100644 log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerDateTest.java diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerDateTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerDateTest.java new file mode 100644 index 00000000000..de09d6571ee --- /dev/null +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerDateTest.java @@ -0,0 +1,118 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.status; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.time.Instant; +import java.util.ArrayList; +import java.util.List; +import java.util.Properties; +import java.util.function.Supplier; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.message.ParameterizedNoReferenceMessageFactory; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.mockito.Mockito; +import org.mockito.stubbing.Answer; + +class StatusLoggerDateTest { + + private static final String INSTANT_YEAR = "1970"; + + private static final String INSTANT_MONTH = "12"; + + private static final String INSTANT_DAY = "27"; + + private static final String INSTANT_HOUR = "12"; + + private static final String INSTANT_MINUTE = "34"; + + private static final String INSTANT_SECOND = "56"; + + private static final String INSTANT_FRACTION = "789"; + + private static final Instant INSTANT = Instant.parse(INSTANT_YEAR + + '-' + + INSTANT_MONTH + + '-' + + INSTANT_DAY + + 'T' + + INSTANT_HOUR + + ':' + + INSTANT_MINUTE + + ':' + + INSTANT_SECOND + + '.' + + INSTANT_FRACTION + + 'Z'); + + private static final Supplier CLOCK = () -> INSTANT; + + @ParameterizedTest + @CsvSource({ + "yyyy-MM-dd," + (INSTANT_YEAR + '-' + INSTANT_MONTH + '-' + INSTANT_DAY), + "HH:mm:ss," + (INSTANT_HOUR + ':' + INSTANT_MINUTE + ':' + INSTANT_SECOND), + "HH:mm:ss.SSS," + (INSTANT_HOUR + ':' + INSTANT_MINUTE + ':' + INSTANT_SECOND + '.' + INSTANT_FRACTION) + }) + void common_date_patterns_should_work(final String instantPattern, final String formattedInstant) { + + // Create a `StatusLogger` configuration + final Properties statusLoggerConfigProperties = new Properties(); + statusLoggerConfigProperties.put(StatusLogger.STATUS_DATE_FORMAT, instantPattern); + statusLoggerConfigProperties.put(StatusLogger.STATUS_DATE_FORMAT_ZONE, "UTC"); + final StatusLogger.Config statusLoggerConfig = new StatusLogger.Config(statusLoggerConfigProperties); + + // Create a `StatusConsoleListener` recording `StatusData` + final StatusConsoleListener statusConsoleListener = mock(StatusConsoleListener.class); + when(statusConsoleListener.getStatusLevel()).thenReturn(Level.ALL); + final List loggedStatusData = new ArrayList<>(); + doAnswer((Answer) invocation -> { + final StatusData statusData = invocation.getArgument(0, StatusData.class); + loggedStatusData.add(statusData); + return null; + }) + .when(statusConsoleListener) + .log(Mockito.any()); + + // Create the `StatusLogger` + final StatusLogger logger = new StatusLogger( + StatusLoggerDateTest.class.getSimpleName(), + ParameterizedNoReferenceMessageFactory.INSTANCE, + statusLoggerConfig, + CLOCK, + statusConsoleListener); + + // Log a message + final String message = "test message"; + final Level level = Level.ERROR; + final Throwable throwable = new RuntimeException("test failure"); + logger.log(level, message, throwable); + + // Verify the logging + assertThat(loggedStatusData).hasSize(1); + final StatusData statusData = loggedStatusData.get(0); + assertThat(statusData.getLevel()).isEqualTo(level); + assertThat(statusData.getThrowable()).isSameAs(throwable); + assertThat(statusData.getFormattedStatus()) + .matches("(?s)^" + formattedInstant + " .+ " + level + ' ' + message + ".*" + throwable.getMessage() + + ".*"); + } +} diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java index 5dedd835edf..c049195d89c 100644 --- a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java @@ -22,10 +22,12 @@ import org.apache.logging.log4j.Level; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.ResourceLock; class StatusLoggerLevelTest { @Test + @ResourceLock("log4j2.StatusLogger") void effective_level_should_be_the_least_specific_one() { // Verify the initial level diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusData.java b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusData.java index e3b13e8dedc..a56cf0e9f99 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusData.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusData.java @@ -68,7 +68,7 @@ public StatusData( final Message message, @Nullable final Throwable throwable, @Nullable final String threadName) { - this(caller, level, message, throwable, threadName, null); + this(caller, level, message, throwable, threadName, null, Instant.now()); } StatusData( @@ -77,9 +77,10 @@ public StatusData( final Message message, @Nullable final Throwable throwable, @Nullable final String threadName, - @Nullable final DateTimeFormatter instantFormatter) { + @Nullable final DateTimeFormatter instantFormatter, + final Instant instant) { this.instantFormatter = instantFormatter; - this.instant = Instant.now(); + this.instant = instant; this.caller = caller; this.level = requireNonNull(level, "level"); this.message = requireNonNull(message, "message"); diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java index 2abc93857d6..450717a929e 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java @@ -22,6 +22,8 @@ import java.io.IOException; import java.io.InputStream; import java.net.URL; +import java.time.Instant; +import java.time.ZoneId; import java.time.format.DateTimeFormatter; import java.util.ArrayList; import java.util.Collections; @@ -33,6 +35,7 @@ import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Supplier; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.Marker; import org.apache.logging.log4j.message.Message; @@ -158,11 +161,26 @@ public class StatusLogger extends AbstractLogger { /** * The name of the system property that can be configured with a {@link java.time.format.DateTimeFormatter} pattern that will be used while formatting the created {@link StatusData}. + *

+ * When not provided, {@link Instant#toString()} will be used. + *

* + * @see #STATUS_DATE_FORMAT_ZONE * @since 2.11.0 */ public static final String STATUS_DATE_FORMAT = "log4j2.StatusLogger.DateFormat"; + /** + * The name of the system property that can be configured with a time-zone ID (e.g., {@code Europe/Amsterdam}, {@code UTC+01:00}) that will be used while formatting the created {@link StatusData}. + *

+ * When not provided, {@link ZoneId#systemDefault()} will be used. + *

+ * + * @see #STATUS_DATE_FORMAT + * @since 2.23.1 + */ + public static final String STATUS_DATE_FORMAT_ZONE = "log4j2.StatusLogger.DateFormatZone"; + /** * The name of the file to be searched in the classpath to read properties from. * @@ -215,10 +233,16 @@ public Config(boolean debugEnabled, int bufferCapacity, @Nullable DateTimeFormat } /** - * Constructs an instance using either system properties or a property file (i.e., {@value Config#PROPERTIES_FILE_NAME}) in the classpath, if available. + * Constructs the default instance using either system properties or a property file (i.e., {@value Config#PROPERTIES_FILE_NAME}) in the classpath, if available. */ private Config() { - final Properties fileProvidedProperties = readPropertiesFile(); + this(readPropertiesFile()); + } + + /** + * The lowest-level constructor intended for tests. + */ + Config(final Properties fileProvidedProperties) { this.debugEnabled = readDebugEnabled(fileProvidedProperties); this.bufferCapacity = readBufferCapacity(fileProvidedProperties); this.fallbackListenerLevel = readFallbackListenerLevel(fileProvidedProperties); @@ -251,7 +275,13 @@ private static Level readFallbackListenerLevel(final Properties fileProvidedProp private static DateTimeFormatter readInstantFormatter(final Properties fileProvidedProperties) { final String format = readProperty(fileProvidedProperties, STATUS_DATE_FORMAT); - return format != null ? DateTimeFormatter.ofPattern(format) : null; + if (format == null) { + return null; + } + final DateTimeFormatter formatter = DateTimeFormatter.ofPattern(format); + final String zoneId = readProperty(fileProvidedProperties, STATUS_DATE_FORMAT_ZONE); + final ZoneId effectiveZoneId = zoneId != null ? ZoneId.of(zoneId) : ZoneId.systemDefault(); + return formatter.withZone(effectiveZoneId); } private static String readProperty(final Properties fileProvidedProperties, final String propertyName) { @@ -299,6 +329,8 @@ private static final class InstanceHolder { private final Config config; + private final Supplier clock; + private final StatusConsoleListener fallbackListener; private final List listeners; @@ -319,6 +351,7 @@ private StatusLogger() { StatusLogger.class.getSimpleName(), ParameterizedNoReferenceMessageFactory.INSTANCE, Config.getInstance(), + Instant::now, new StatusConsoleListener(Config.getInstance().fallbackListenerLevel)); } @@ -338,8 +371,21 @@ public StatusLogger( final MessageFactory messageFactory, final Config config, final StatusConsoleListener fallbackListener) { + this(name, messageFactory, config, Instant::now, fallbackListener); + } + + /** + * The lowest-level constructor intended for tests. + */ + StatusLogger( + final String name, + final MessageFactory messageFactory, + final Config config, + final Supplier clock, + final StatusConsoleListener fallbackListener) { super(requireNonNull(name, "name"), requireNonNull(messageFactory, "messageFactory")); this.config = requireNonNull(config, "config"); + this.clock = requireNonNull(clock, "clock"); this.fallbackListener = requireNonNull(fallbackListener, "fallbackListener"); this.listeners = new ArrayList<>(); } @@ -562,7 +608,8 @@ private StatusData createStatusData( final Message message, @Nullable final Throwable throwable) { final StackTraceElement caller = getStackTraceElement(fqcn); - return new StatusData(caller, level, message, throwable, null, config.instantFormatter); + final Instant instant = clock.get(); + return new StatusData(caller, level, message, throwable, null, config.instantFormatter, instant); } @Nullable From 78e2f1443114c1eeebae9caebadf3e993def51d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Fri, 23 Feb 2024 21:47:04 +0100 Subject: [PATCH 2/8] Make sure `StatusLogger#logMessage()` failures don't cause stack overflow --- .../StatusLoggerFailingListenerTest.java | 66 +++++++++++++++++++ .../log4j/status/StatusLoggerLevelTest.java | 4 +- .../logging/log4j/status/StatusLogger.java | 20 ++++-- 3 files changed, 83 insertions(+), 7 deletions(-) create mode 100644 log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerFailingListenerTest.java diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerFailingListenerTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerFailingListenerTest.java new file mode 100644 index 00000000000..ede9cb3af9d --- /dev/null +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerFailingListenerTest.java @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.status; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import org.apache.logging.log4j.Level; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.parallel.ResourceLock; +import uk.org.webcompere.systemstubs.SystemStubs; +import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension; + +@ExtendWith(SystemStubsExtension.class) +@ResourceLock("log4j2.StatusLogger") +class StatusLoggerFailingListenerTest { + + public static final StatusLogger STATUS_LOGGER = StatusLogger.getLogger(); + + private StatusListener listener; + + @BeforeEach + void createAndRegisterListener() { + listener = mock(StatusListener.class); + STATUS_LOGGER.registerListener(listener); + } + + @AfterEach + void unregisterListener() { + STATUS_LOGGER.removeListener(listener); + } + + @Test + void logging_with_failing_listener_should_not_cause_stack_overflow() throws Exception { + + // Set up a failing listener on `log(StatusData)` + when(listener.getStatusLevel()).thenReturn(Level.ALL); + final Exception listenerFailure = new RuntimeException("test failure " + Math.random()); + doThrow(listenerFailure).when(listener).log(any()); + + // Log something and verify exception dump + final String stderr = SystemStubs.tapSystemErr(() -> STATUS_LOGGER.error("foo")); + final String listenerFailureClassName = listenerFailure.getClass().getCanonicalName(); + assertThat(stderr).contains(listenerFailureClassName + ": " + listenerFailure.getMessage()); + } +} diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java index c049195d89c..bbe0f7786e3 100644 --- a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java @@ -22,16 +22,14 @@ import org.apache.logging.log4j.Level; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.parallel.ResourceLock; class StatusLoggerLevelTest { @Test - @ResourceLock("log4j2.StatusLogger") void effective_level_should_be_the_least_specific_one() { // Verify the initial level - final StatusLogger logger = StatusLogger.getLogger(); + final StatusLogger logger = new StatusLogger(); final Level fallbackListenerLevel = Level.ERROR; assertThat(logger.getLevel()).isEqualTo(fallbackListenerLevel); diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java index 450717a929e..e1fffc08014 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java @@ -36,6 +36,8 @@ import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Supplier; + +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.Marker; import org.apache.logging.log4j.message.Message; @@ -345,8 +347,11 @@ private static final class InstanceHolder { /** * Constructs the default instance. + *

+ * This method is visible for tests. + *

*/ - private StatusLogger() { + StatusLogger() { this( StatusLogger.class.getSimpleName(), ParameterizedNoReferenceMessageFactory.INSTANCE, @@ -561,15 +566,22 @@ public Level getLevel() { } @Override + @SuppressFBWarnings("INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE") public void logMessage( final String fqcn, final Level level, final Marker marker, final Message message, final Throwable throwable) { - final StatusData statusData = createStatusData(fqcn, level, message, throwable); - buffer(statusData); - notifyListeners(statusData); + try { + final StatusData statusData = createStatusData(fqcn, level, message, throwable); + buffer(statusData); + notifyListeners(statusData); + } catch (final Exception error) { + // We are at the lowest level of the system. + // Hence, there is nothing better we can do but dumping the failure. + error.printStackTrace(System.err); + } } private void buffer(final StatusData statusData) { From 00cf39d9728fa555dc62b082089222a5c9707b90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Fri, 23 Feb 2024 22:11:58 +0100 Subject: [PATCH 3/8] Fix build failures --- .../java/org/apache/logging/log4j/status/StatusLogger.java | 5 ++--- .../java/org/apache/logging/log4j/status/package-info.java | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java index e1fffc08014..7db30dfedda 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java @@ -19,6 +19,7 @@ import static java.util.Objects.requireNonNull; import edu.umd.cs.findbugs.annotations.Nullable; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.IOException; import java.io.InputStream; import java.net.URL; @@ -36,8 +37,6 @@ import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Supplier; - -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.Marker; import org.apache.logging.log4j.message.Message; @@ -181,7 +180,7 @@ public class StatusLogger extends AbstractLogger { * @see #STATUS_DATE_FORMAT * @since 2.23.1 */ - public static final String STATUS_DATE_FORMAT_ZONE = "log4j2.StatusLogger.DateFormatZone"; + static final String STATUS_DATE_FORMAT_ZONE = "log4j2.StatusLogger.DateFormatZone"; /** * The name of the file to be searched in the classpath to read properties from. diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/status/package-info.java b/log4j-api/src/main/java/org/apache/logging/log4j/status/package-info.java index 407e82cfc0a..12fddb60395 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/status/package-info.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/status/package-info.java @@ -19,7 +19,7 @@ * used by applications reporting on the status of the logging system */ @Export -@Version("2.23.0") +@Version("2.23.1") package org.apache.logging.log4j.status; import org.osgi.annotation.bundle.Export; From 89417c77401cf5209b6b1ead1216031dfc1b9efe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Fri, 23 Feb 2024 22:17:18 +0100 Subject: [PATCH 4/8] Add changelog entries --- .../.2.x.x/fix_StatusLogger_instant_formatting.xml | 10 ++++++++++ .../.2.x.x/fix_StatusLogger_stack_overflow.xml | 7 +++++++ 2 files changed, 17 insertions(+) create mode 100644 src/changelog/.2.x.x/fix_StatusLogger_instant_formatting.xml create mode 100644 src/changelog/.2.x.x/fix_StatusLogger_stack_overflow.xml diff --git a/src/changelog/.2.x.x/fix_StatusLogger_instant_formatting.xml b/src/changelog/.2.x.x/fix_StatusLogger_instant_formatting.xml new file mode 100644 index 00000000000..daecc007228 --- /dev/null +++ b/src/changelog/.2.x.x/fix_StatusLogger_instant_formatting.xml @@ -0,0 +1,10 @@ + + + + Add `log4j2.StatusLogger.DateFormatZone` system property to set the time-zone `StatusLogger` uses to format `java.time.Instant`. + Without this formatting patterns accessing to time-zone-specific fields (e.g., year-of-era) cause failures. + + diff --git a/src/changelog/.2.x.x/fix_StatusLogger_stack_overflow.xml b/src/changelog/.2.x.x/fix_StatusLogger_stack_overflow.xml new file mode 100644 index 00000000000..084385c14df --- /dev/null +++ b/src/changelog/.2.x.x/fix_StatusLogger_stack_overflow.xml @@ -0,0 +1,7 @@ + + + Fix stack overflow in `StatusLogger` + From db30243f20011586926567352fdd137f1dc3a0cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Tue, 27 Feb 2024 09:30:24 +0100 Subject: [PATCH 5/8] Harden invalid argument fallbacks --- .../StatusLoggerBufferCapacityTest.java | 60 ++++++++ .../log4j/status/StatusLoggerDateTest.java | 145 ++++++++---------- .../log4j/status/StatusLoggerLevelTest.java | 24 ++- .../logging/log4j/status/StatusLogger.java | 137 +++++++++++++---- 4 files changed, 254 insertions(+), 112 deletions(-) create mode 100644 log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerBufferCapacityTest.java diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerBufferCapacityTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerBufferCapacityTest.java new file mode 100644 index 00000000000..0dc0b5c65d3 --- /dev/null +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerBufferCapacityTest.java @@ -0,0 +1,60 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.status; + +import static org.apache.logging.log4j.status.StatusLogger.DEFAULT_FALLBACK_LISTENER_BUFFER_CAPACITY; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Properties; +import org.junit.jupiter.api.Test; +import uk.org.webcompere.systemstubs.SystemStubs; + +class StatusLoggerBufferCapacityTest { + + @Test + void valid_buffer_capacity_should_be_effective() { + + // Create a `StatusLogger` configuration + final Properties statusLoggerConfigProperties = new Properties(); + final int bufferCapacity = 10; + assertThat(bufferCapacity).isNotEqualTo(DEFAULT_FALLBACK_LISTENER_BUFFER_CAPACITY); + statusLoggerConfigProperties.put(StatusLogger.MAX_STATUS_ENTRIES, "" + bufferCapacity); + final StatusLogger.Config statusLoggerConfig = new StatusLogger.Config(statusLoggerConfigProperties); + + // Verify the buffer capacity + assertThat(statusLoggerConfig.bufferCapacity).isEqualTo(bufferCapacity); + } + + @Test + void invalid_buffer_capacity_should_cause_fallback_to_defaults() throws Exception { + + // Create a `StatusLogger` configuration using an invalid buffer capacity + final Properties statusLoggerConfigProperties = new Properties(); + final int invalidBufferCapacity = -10; + statusLoggerConfigProperties.put(StatusLogger.MAX_STATUS_ENTRIES, "" + invalidBufferCapacity); + final StatusLogger.Config[] statusLoggerConfigRef = {null}; + final String stderr = SystemStubs.tapSystemErr( + () -> statusLoggerConfigRef[0] = new StatusLogger.Config(statusLoggerConfigProperties)); + final StatusLogger.Config statusLoggerConfig = statusLoggerConfigRef[0]; + + // Verify the stderr dump + assertThat(stderr).contains("Failed reading the buffer capacity"); + + // Verify the buffer capacity + assertThat(statusLoggerConfig.bufferCapacity).isEqualTo(DEFAULT_FALLBACK_LISTENER_BUFFER_CAPACITY); + } +} diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerDateTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerDateTest.java index de09d6571ee..0978587692a 100644 --- a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerDateTest.java +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerDateTest.java @@ -17,102 +17,89 @@ package org.apache.logging.log4j.status; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import edu.umd.cs.findbugs.annotations.Nullable; import java.time.Instant; -import java.util.ArrayList; -import java.util.List; +import java.time.ZoneId; +import java.time.format.DateTimeFormatter; import java.util.Properties; -import java.util.function.Supplier; -import org.apache.logging.log4j.Level; -import org.apache.logging.log4j.message.ParameterizedNoReferenceMessageFactory; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; -import org.mockito.Mockito; -import org.mockito.stubbing.Answer; +import uk.org.webcompere.systemstubs.SystemStubs; class StatusLoggerDateTest { - private static final String INSTANT_YEAR = "1970"; - - private static final String INSTANT_MONTH = "12"; - - private static final String INSTANT_DAY = "27"; - - private static final String INSTANT_HOUR = "12"; - - private static final String INSTANT_MINUTE = "34"; - - private static final String INSTANT_SECOND = "56"; - - private static final String INSTANT_FRACTION = "789"; - - private static final Instant INSTANT = Instant.parse(INSTANT_YEAR - + '-' - + INSTANT_MONTH - + '-' - + INSTANT_DAY - + 'T' - + INSTANT_HOUR - + ':' - + INSTANT_MINUTE - + ':' - + INSTANT_SECOND - + '.' - + INSTANT_FRACTION - + 'Z'); - - private static final Supplier CLOCK = () -> INSTANT; - @ParameterizedTest - @CsvSource({ - "yyyy-MM-dd," + (INSTANT_YEAR + '-' + INSTANT_MONTH + '-' + INSTANT_DAY), - "HH:mm:ss," + (INSTANT_HOUR + ':' + INSTANT_MINUTE + ':' + INSTANT_SECOND), - "HH:mm:ss.SSS," + (INSTANT_HOUR + ':' + INSTANT_MINUTE + ':' + INSTANT_SECOND + '.' + INSTANT_FRACTION) - }) - void common_date_patterns_should_work(final String instantPattern, final String formattedInstant) { + @CsvSource({"yyyy-MM-dd", "HH:mm:ss", "HH:mm:ss.SSS"}) + void common_date_patterns_should_work(final String instantPattern) { // Create a `StatusLogger` configuration final Properties statusLoggerConfigProperties = new Properties(); statusLoggerConfigProperties.put(StatusLogger.STATUS_DATE_FORMAT, instantPattern); - statusLoggerConfigProperties.put(StatusLogger.STATUS_DATE_FORMAT_ZONE, "UTC"); + final ZoneId zoneId = ZoneId.of("UTC"); + statusLoggerConfigProperties.put(StatusLogger.STATUS_DATE_FORMAT_ZONE, zoneId.toString()); final StatusLogger.Config statusLoggerConfig = new StatusLogger.Config(statusLoggerConfigProperties); - // Create a `StatusConsoleListener` recording `StatusData` - final StatusConsoleListener statusConsoleListener = mock(StatusConsoleListener.class); - when(statusConsoleListener.getStatusLevel()).thenReturn(Level.ALL); - final List loggedStatusData = new ArrayList<>(); - doAnswer((Answer) invocation -> { - final StatusData statusData = invocation.getArgument(0, StatusData.class); - loggedStatusData.add(statusData); - return null; - }) - .when(statusConsoleListener) - .log(Mockito.any()); + // Verify the formatter + final DateTimeFormatter formatter = + DateTimeFormatter.ofPattern(instantPattern).withZone(zoneId); + verifyFormatter(statusLoggerConfig.instantFormatter, formatter); + } + + @Test + void invalid_date_format_should_cause_fallback_to_defaults() throws Exception { + final String invalidFormat = "l"; + verifyInvalidDateFormatAndZone(invalidFormat, "UTC", "failed reading the instant format", null); + } + + @Test + void invalid_date_format_zone_should_cause_fallback_to_defaults() throws Exception { + final String invalidZone = "XXX"; + final String format = "yyyy"; + verifyInvalidDateFormatAndZone( + format, + invalidZone, + "Failed reading the instant formatting zone ID", + DateTimeFormatter.ofPattern(format).withZone(ZoneId.systemDefault())); + } - // Create the `StatusLogger` - final StatusLogger logger = new StatusLogger( - StatusLoggerDateTest.class.getSimpleName(), - ParameterizedNoReferenceMessageFactory.INSTANCE, - statusLoggerConfig, - CLOCK, - statusConsoleListener); + private static void verifyInvalidDateFormatAndZone( + final String format, + final String zone, + final String stderrMessage, + @Nullable final DateTimeFormatter formatter) + throws Exception { - // Log a message - final String message = "test message"; - final Level level = Level.ERROR; - final Throwable throwable = new RuntimeException("test failure"); - logger.log(level, message, throwable); + // Create a `StatusLogger` configuration using invalid input + final Properties statusLoggerConfigProperties = new Properties(); + statusLoggerConfigProperties.put(StatusLogger.STATUS_DATE_FORMAT, format); + statusLoggerConfigProperties.put(StatusLogger.STATUS_DATE_FORMAT_ZONE, zone); + final StatusLogger.Config[] statusLoggerConfigRef = {null}; + final String stderr = SystemStubs.tapSystemErr( + () -> statusLoggerConfigRef[0] = new StatusLogger.Config(statusLoggerConfigProperties)); + final StatusLogger.Config statusLoggerConfig = statusLoggerConfigRef[0]; + + // Verify the stderr dump + assertThat(stderr).contains(stderrMessage); + + // Verify the formatter + verifyFormatter(statusLoggerConfig.instantFormatter, formatter); + } - // Verify the logging - assertThat(loggedStatusData).hasSize(1); - final StatusData statusData = loggedStatusData.get(0); - assertThat(statusData.getLevel()).isEqualTo(level); - assertThat(statusData.getThrowable()).isSameAs(throwable); - assertThat(statusData.getFormattedStatus()) - .matches("(?s)^" + formattedInstant + " .+ " + level + ' ' + message + ".*" + throwable.getMessage() - + ".*"); + /** + * {@link DateTimeFormatter} doesn't have an {@link Object#equals(Object)} implementation, hence this manual behavioral comparison. + * + * @param actual the actual formatter + * @param expected the expected formatter + */ + private static void verifyFormatter(@Nullable DateTimeFormatter actual, @Nullable DateTimeFormatter expected) { + if (expected == null) { + assertThat(actual).isNull(); + } else { + assertThat(actual).isNotNull(); + final Instant instant = Instant.now(); + assertThat(actual.format(instant)).isEqualTo(expected.format(instant)); + } } } diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java index bbe0f7786e3..3d58cf454a7 100644 --- a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerLevelTest.java @@ -16,12 +16,15 @@ */ package org.apache.logging.log4j.status; +import static org.apache.logging.log4j.status.StatusLogger.DEFAULT_FALLBACK_LISTENER_LEVEL; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import java.util.Properties; import org.apache.logging.log4j.Level; import org.junit.jupiter.api.Test; +import uk.org.webcompere.systemstubs.SystemStubs; class StatusLoggerLevelTest { @@ -30,7 +33,7 @@ void effective_level_should_be_the_least_specific_one() { // Verify the initial level final StatusLogger logger = new StatusLogger(); - final Level fallbackListenerLevel = Level.ERROR; + final Level fallbackListenerLevel = DEFAULT_FALLBACK_LISTENER_LEVEL; assertThat(logger.getLevel()).isEqualTo(fallbackListenerLevel); // Register a less specific listener @@ -82,4 +85,23 @@ void effective_level_should_be_the_least_specific_one() { logger.removeListener(listener1); assertThat(logger.getLevel()).isEqualTo(fallbackListenerLevel); // Verify that the level is changed } + + @Test + void invalid_level_should_cause_fallback_to_defaults() throws Exception { + + // Create a `StatusLogger` configuration using an invalid level + final Properties statusLoggerConfigProperties = new Properties(); + final String invalidLevelName = "FOO"; + statusLoggerConfigProperties.put(StatusLogger.DEFAULT_STATUS_LISTENER_LEVEL, invalidLevelName); + final StatusLogger.Config[] statusLoggerConfigRef = {null}; + final String stderr = SystemStubs.tapSystemErr( + () -> statusLoggerConfigRef[0] = new StatusLogger.Config(statusLoggerConfigProperties)); + final StatusLogger.Config statusLoggerConfig = statusLoggerConfigRef[0]; + + // Verify the stderr dump + assertThat(stderr).contains("Failed reading the level"); + + // Verify the level + assertThat(statusLoggerConfig.fallbackListenerLevel).isEqualTo(DEFAULT_FALLBACK_LISTENER_LEVEL); + } } diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java index 7db30dfedda..858859d0f15 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java @@ -36,7 +36,6 @@ import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; -import java.util.function.Supplier; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.Marker; import org.apache.logging.log4j.message.Message; @@ -149,6 +148,16 @@ public class StatusLogger extends AbstractLogger { */ public static final String MAX_STATUS_ENTRIES = "log4j2.status.entries"; + /** + * The default fallback listener buffer capacity. + *

+ * This constant is intended for tests. + *

+ * + * @see #MAX_STATUS_ENTRIES + */ + static final int DEFAULT_FALLBACK_LISTENER_BUFFER_CAPACITY = 0; + /** * The name of the system property that can be configured with the {@link Level} name to use as the fallback listener level. *

@@ -160,6 +169,14 @@ public class StatusLogger extends AbstractLogger { */ public static final String DEFAULT_STATUS_LISTENER_LEVEL = "log4j2.StatusLogger.level"; + /** + * The default fallback listener level. + *

+ * This constant is intended for tests and indeed makes things awfully confusing given the {@link #DEFAULT_STATUS_LISTENER_LEVEL} property, which is actually intended to be a property name, not its default value. + *

+ */ + static final Level DEFAULT_FALLBACK_LISTENER_LEVEL = Level.ERROR; + /** * The name of the system property that can be configured with a {@link java.time.format.DateTimeFormatter} pattern that will be used while formatting the created {@link StatusData}. *

@@ -200,13 +217,16 @@ public static final class Config { private final boolean debugEnabled; - private final int bufferCapacity; + // Visible for tests + final int bufferCapacity; + // Visible for tests @Nullable - private final Level fallbackListenerLevel; + final Level fallbackListenerLevel; + // Visible for tests @Nullable - private final DateTimeFormatter instantFormatter; + final DateTimeFormatter instantFormatter; /** * Constructs an instance using the given properties. @@ -265,26 +285,90 @@ private static boolean readDebugEnabled(final Properties fileProvidedProperties) } private static int readBufferCapacity(final Properties fileProvidedProperties) { - final String capacityString = readProperty(fileProvidedProperties, MAX_STATUS_ENTRIES); - return capacityString != null ? Integer.parseInt(capacityString) : 0; + final String propertyName = MAX_STATUS_ENTRIES; + final String capacityString = readProperty(fileProvidedProperties, propertyName); + final int defaultCapacity = DEFAULT_FALLBACK_LISTENER_BUFFER_CAPACITY; + int effectiveCapacity = defaultCapacity; + if (capacityString != null) { + try { + final int capacity = Integer.parseInt(capacityString); + if (capacity < 0) { + final String message = + String.format("was expecting a positive buffer capacity, found: %d", capacity); + throw new IllegalArgumentException(message); + } + effectiveCapacity = capacity; + } catch (final Exception error) { + final String message = String.format( + "Failed reading the buffer capacity from the `%s` property: `%s`. Falling back to the default: %d.", + propertyName, capacityString, defaultCapacity); + final IllegalArgumentException extendedError = new IllegalArgumentException(message, error); + // There is no logging system at this stage. + // There is nothing we can do but simply dumping the failure. + extendedError.printStackTrace(System.err); + } + } + return effectiveCapacity; } private static Level readFallbackListenerLevel(final Properties fileProvidedProperties) { - final String level = readProperty(fileProvidedProperties, DEFAULT_STATUS_LISTENER_LEVEL); - return level != null ? Level.valueOf(level) : Level.ERROR; + final String propertyName = DEFAULT_STATUS_LISTENER_LEVEL; + final String level = readProperty(fileProvidedProperties, propertyName); + final Level defaultLevel = DEFAULT_FALLBACK_LISTENER_LEVEL; + try { + return level != null ? Level.valueOf(level) : defaultLevel; + } catch (final Exception error) { + final String message = String.format( + "Failed reading the level from the `%s` property: `%s`. Falling back to the default: `%s`.", + propertyName, level, defaultLevel); + final IllegalArgumentException extendedError = new IllegalArgumentException(message, error); + // There is no logging system at this stage. + // There is nothing we can do but simply dumping the failure. + extendedError.printStackTrace(System.err); + return defaultLevel; + } } + @Nullable private static DateTimeFormatter readInstantFormatter(final Properties fileProvidedProperties) { - final String format = readProperty(fileProvidedProperties, STATUS_DATE_FORMAT); + final String formatPropertyName = STATUS_DATE_FORMAT; + final String format = readProperty(fileProvidedProperties, formatPropertyName); if (format == null) { return null; } - final DateTimeFormatter formatter = DateTimeFormatter.ofPattern(format); - final String zoneId = readProperty(fileProvidedProperties, STATUS_DATE_FORMAT_ZONE); - final ZoneId effectiveZoneId = zoneId != null ? ZoneId.of(zoneId) : ZoneId.systemDefault(); - return formatter.withZone(effectiveZoneId); + final DateTimeFormatter formatter; + try { + formatter = DateTimeFormatter.ofPattern(format); + } catch (final Exception error) { + final String message = String.format( + "failed reading the instant format from the `%s` property: `%s`", formatPropertyName, format); + final IllegalArgumentException extendedError = new IllegalArgumentException(message, error); + // There is no logging system at this stage. + // There is nothing we can do but simply dumping the failure. + extendedError.printStackTrace(System.err); + return null; + } + final String zonePropertyName = STATUS_DATE_FORMAT_ZONE; + final String zoneIdString = readProperty(fileProvidedProperties, zonePropertyName); + final ZoneId defaultZoneId = ZoneId.systemDefault(); + ZoneId zoneId = defaultZoneId; + if (zoneIdString != null) { + try { + zoneId = ZoneId.of(zoneIdString); + } catch (final Exception error) { + final String message = String.format( + "Failed reading the instant formatting zone ID from the `%s` property: `%s`. Falling back to the default: `%s`.", + zonePropertyName, zoneIdString, defaultZoneId); + final IllegalArgumentException extendedError = new IllegalArgumentException(message, error); + // There is no logging system at this stage. + // There is nothing we can do but simply dumping the failure. + extendedError.printStackTrace(System.err); + } + } + return formatter.withZone(zoneId); } + @Nullable private static String readProperty(final Properties fileProvidedProperties, final String propertyName) { final String systemProvidedValue = System.getProperty(propertyName); return systemProvidedValue != null @@ -306,9 +390,11 @@ private static Properties readPropertiesFile() { try (final InputStream stream = url.openStream()) { properties.load(stream); } catch (final IOException error) { + final String message = String.format("failed reading properties from `%s`", PROPERTIES_FILE_NAME); + final RuntimeException extendedError = new RuntimeException(message, error); // There is no logging system at this stage. // There is nothing we can do but simply dumping the failure. - error.printStackTrace(System.err); + extendedError.printStackTrace(System.err); } return properties; } @@ -330,8 +416,6 @@ private static final class InstanceHolder { private final Config config; - private final Supplier clock; - private final StatusConsoleListener fallbackListener; private final List listeners; @@ -355,7 +439,6 @@ private static final class InstanceHolder { StatusLogger.class.getSimpleName(), ParameterizedNoReferenceMessageFactory.INSTANCE, Config.getInstance(), - Instant::now, new StatusConsoleListener(Config.getInstance().fallbackListenerLevel)); } @@ -375,21 +458,8 @@ public StatusLogger( final MessageFactory messageFactory, final Config config, final StatusConsoleListener fallbackListener) { - this(name, messageFactory, config, Instant::now, fallbackListener); - } - - /** - * The lowest-level constructor intended for tests. - */ - StatusLogger( - final String name, - final MessageFactory messageFactory, - final Config config, - final Supplier clock, - final StatusConsoleListener fallbackListener) { super(requireNonNull(name, "name"), requireNonNull(messageFactory, "messageFactory")); this.config = requireNonNull(config, "config"); - this.clock = requireNonNull(clock, "clock"); this.fallbackListener = requireNonNull(fallbackListener, "fallbackListener"); this.listeners = new ArrayList<>(); } @@ -517,7 +587,10 @@ private static void closeListenerSafely(final StatusListener listener) { listener.close(); } catch (final IOException error) { final String message = String.format("failed closing listener: %s", listener); - new RuntimeException(message, error).printStackTrace(System.err); + final RuntimeException extendedError = new RuntimeException(message, error); + // There is no logging system at this stage. + // There is nothing we can do but simply dumping the failure. + extendedError.printStackTrace(System.err); } } @@ -619,7 +692,7 @@ private StatusData createStatusData( final Message message, @Nullable final Throwable throwable) { final StackTraceElement caller = getStackTraceElement(fqcn); - final Instant instant = clock.get(); + final Instant instant = Instant.now(); return new StatusData(caller, level, message, throwable, null, config.instantFormatter, instant); } From ee4d52049c4f194744c0a6139833a1ae20978ffb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Tue, 27 Feb 2024 12:49:41 +0100 Subject: [PATCH 6/8] Support environment variables --- .../StatusLoggerPropertiesUtilDoubleTest.java | 126 +++++++++++++++++ .../logging/log4j/status/StatusLogger.java | 131 +++++++++++++++--- 2 files changed, 235 insertions(+), 22 deletions(-) create mode 100644 log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerPropertiesUtilDoubleTest.java diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerPropertiesUtilDoubleTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerPropertiesUtilDoubleTest.java new file mode 100644 index 00000000000..421cacee3c6 --- /dev/null +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerPropertiesUtilDoubleTest.java @@ -0,0 +1,126 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.logging.log4j.status; + +import static org.apache.logging.log4j.status.StatusLogger.PropertiesUtilsDouble.readAllAvailableProperties; +import static org.apache.logging.log4j.status.StatusLogger.PropertiesUtilsDouble.readProperty; +import static org.assertj.core.api.Assertions.assertThat; +import static uk.org.webcompere.systemstubs.SystemStubs.restoreSystemProperties; +import static uk.org.webcompere.systemstubs.SystemStubs.withEnvironmentVariable; + +import java.util.Arrays; +import java.util.Map; +import java.util.stream.Stream; +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.MethodSource; + +class StatusLoggerPropertiesUtilDoubleTest { + + private static final String[] NOT_PREFIXED_MATCHING_PROPERTY_NAMES = new String[] { + // For system properties: + "StatusLogger.DateFormat", + "StatusLogger.dateFormat", + "StatusLogger.dateformat", + "StatusLogger.Dateformat", + "Status.Logger.Dateformat", + "Status.Logger.Date.Format", + "statusLoggerDateFormat", + "Status.Logger.Date.Format", + "status.logger.date.format", + // For environment variables: + "STATUSLOGGER_DATEFORMAT", + "STATUS_LOGGER_DATE_FORMAT" + }; + + private static final String[] MATCHING_PROPERTY_NAMES = Stream.of( + // For system properties: + "log4j.", "log4j2.", + // For environment variables: + "LOG4J_", "LOG4J2_") + .flatMap(prefix -> Arrays.stream(NOT_PREFIXED_MATCHING_PROPERTY_NAMES) + .map(notPrefixedPropertyName -> prefix + notPrefixedPropertyName)) + .toArray(String[]::new); + + private static final String[] NOT_MATCHING_PROPERTY_NAMES = + new String[] {"log4j2.StatusLogger$DateFormat", "log4j2.StàtusLögger.DateFormat"}; + + private static final class TestCase { + + private final boolean matching; + + private final String propertyName; + + private final String userProvidedPropertyName; + + private TestCase(final boolean matching, final String propertyName, final String userProvidedPropertyName) { + this.matching = matching; + this.propertyName = propertyName; + this.userProvidedPropertyName = userProvidedPropertyName; + } + + @Override + public String toString() { + return String.format("`%s` %s `%s`", propertyName, matching ? "==" : "!=", userProvidedPropertyName); + } + } + + static Stream testCases() { + return Stream.concat( + testCases(true, MATCHING_PROPERTY_NAMES, MATCHING_PROPERTY_NAMES), + testCases(false, MATCHING_PROPERTY_NAMES, NOT_MATCHING_PROPERTY_NAMES)); + } + + private static Stream testCases( + final boolean matching, final String[] propertyNames, final String[] userProvidedPropertyNames) { + return Arrays.stream(propertyNames).flatMap(propertyName -> Arrays.stream(userProvidedPropertyNames) + .map(userProvidedPropertyName -> new TestCase(matching, propertyName, userProvidedPropertyName))); + } + + @ParameterizedTest + @MethodSource("testCases") + @ResourceLock(value = Resources.SYSTEM_PROPERTIES, mode = ResourceAccessMode.READ_WRITE) + void system_properties_should_work(final TestCase testCase) throws Exception { + restoreSystemProperties(() -> { + final String expectedValue = "foo"; + System.setProperty(testCase.propertyName, expectedValue); + verifyProperty(testCase, expectedValue); + }); + } + + @ParameterizedTest + @MethodSource("testCases") + @ResourceLock(value = Resources.GLOBAL, mode = ResourceAccessMode.READ_WRITE) + void environment_variables_should_work(final TestCase testCase) throws Exception { + final String expectedValue = "bar"; + withEnvironmentVariable(testCase.propertyName, expectedValue).execute(() -> { + verifyProperty(testCase, expectedValue); + }); + } + + private static void verifyProperty(final TestCase testCase, final String expectedValue) { + final Map normalizedProperties = readAllAvailableProperties(); + final String actualValue = readProperty(normalizedProperties, testCase.userProvidedPropertyName); + if (testCase.matching) { + assertThat(actualValue).describedAs("" + testCase).isEqualTo(expectedValue); + } else { + assertThat(actualValue).describedAs("" + testCase).isNull(); + } + } +} diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java index 858859d0f15..c7a5ce13d4e 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java @@ -28,8 +28,11 @@ import java.time.format.DateTimeFormatter; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.Iterator; import java.util.List; +import java.util.Locale; +import java.util.Map; import java.util.Properties; import java.util.Queue; import java.util.concurrent.ConcurrentLinkedQueue; @@ -254,20 +257,27 @@ public Config(boolean debugEnabled, int bufferCapacity, @Nullable DateTimeFormat } /** - * Constructs the default instance using either system properties or a property file (i.e., {@value Config#PROPERTIES_FILE_NAME}) in the classpath, if available. + * Constructs the default instance using system properties and a property file (i.e., {@value Config#PROPERTIES_FILE_NAME}) in the classpath, if available. */ private Config() { - this(readPropertiesFile()); + this(PropertiesUtilsDouble.readAllAvailableProperties()); } /** - * The lowest-level constructor intended for tests. + * A low-level constructor intended for tests. */ - Config(final Properties fileProvidedProperties) { - this.debugEnabled = readDebugEnabled(fileProvidedProperties); - this.bufferCapacity = readBufferCapacity(fileProvidedProperties); - this.fallbackListenerLevel = readFallbackListenerLevel(fileProvidedProperties); - this.instantFormatter = readInstantFormatter(fileProvidedProperties); + Config(final Properties... propertiesList) { + this(PropertiesUtilsDouble.normalizeProperties(propertiesList)); + } + + /** + * The lowest-level constructor. + */ + private Config(final Map normalizedProperties) { + this.debugEnabled = readDebugEnabled(normalizedProperties); + this.bufferCapacity = readBufferCapacity(normalizedProperties); + this.fallbackListenerLevel = readFallbackListenerLevel(normalizedProperties); + this.instantFormatter = readInstantFormatter(normalizedProperties); } /** @@ -279,14 +289,14 @@ public static Config getInstance() { return INSTANCE; } - private static boolean readDebugEnabled(final Properties fileProvidedProperties) { - final String debug = readProperty(fileProvidedProperties, DEBUG_PROPERTY_NAME); + private static boolean readDebugEnabled(final Map normalizedProperties) { + final String debug = PropertiesUtilsDouble.readProperty(normalizedProperties, DEBUG_PROPERTY_NAME); return debug != null; } - private static int readBufferCapacity(final Properties fileProvidedProperties) { + private static int readBufferCapacity(final Map normalizedProperties) { final String propertyName = MAX_STATUS_ENTRIES; - final String capacityString = readProperty(fileProvidedProperties, propertyName); + final String capacityString = PropertiesUtilsDouble.readProperty(normalizedProperties, propertyName); final int defaultCapacity = DEFAULT_FALLBACK_LISTENER_BUFFER_CAPACITY; int effectiveCapacity = defaultCapacity; if (capacityString != null) { @@ -311,9 +321,9 @@ private static int readBufferCapacity(final Properties fileProvidedProperties) { return effectiveCapacity; } - private static Level readFallbackListenerLevel(final Properties fileProvidedProperties) { + private static Level readFallbackListenerLevel(final Map normalizedProperties) { final String propertyName = DEFAULT_STATUS_LISTENER_LEVEL; - final String level = readProperty(fileProvidedProperties, propertyName); + final String level = PropertiesUtilsDouble.readProperty(normalizedProperties, propertyName); final Level defaultLevel = DEFAULT_FALLBACK_LISTENER_LEVEL; try { return level != null ? Level.valueOf(level) : defaultLevel; @@ -330,9 +340,11 @@ private static Level readFallbackListenerLevel(final Properties fileProvidedProp } @Nullable - private static DateTimeFormatter readInstantFormatter(final Properties fileProvidedProperties) { + private static DateTimeFormatter readInstantFormatter(final Map normalizedProperties) { + + // Read the format final String formatPropertyName = STATUS_DATE_FORMAT; - final String format = readProperty(fileProvidedProperties, formatPropertyName); + final String format = PropertiesUtilsDouble.readProperty(normalizedProperties, formatPropertyName); if (format == null) { return null; } @@ -348,8 +360,10 @@ private static DateTimeFormatter readInstantFormatter(final Properties fileProvi extendedError.printStackTrace(System.err); return null; } + + // Read the zone final String zonePropertyName = STATUS_DATE_FORMAT_ZONE; - final String zoneIdString = readProperty(fileProvidedProperties, zonePropertyName); + final String zoneIdString = PropertiesUtilsDouble.readProperty(normalizedProperties, zonePropertyName); final ZoneId defaultZoneId = ZoneId.systemDefault(); ZoneId zoneId = defaultZoneId; if (zoneIdString != null) { @@ -367,13 +381,37 @@ private static DateTimeFormatter readInstantFormatter(final Properties fileProvi } return formatter.withZone(zoneId); } + } + + /** + * This is a thin double of {@link org.apache.logging.log4j.util.PropertiesUtil}. + *

+ * We could have used {@code PropertiesUtil}, {@link org.apache.logging.log4j.util.PropertyFilePropertySource}, etc. + * Consequently, they would delegate to {@link org.apache.logging.log4j.util.LoaderUtil}, etc. + * All these mechanisms expect a working {@code StatusLogger}. + * In order to be self-sufficient, we cannot rely on them, hence this double. + *

+ */ + static final class PropertiesUtilsDouble { @Nullable - private static String readProperty(final Properties fileProvidedProperties, final String propertyName) { - final String systemProvidedValue = System.getProperty(propertyName); - return systemProvidedValue != null - ? systemProvidedValue - : (String) fileProvidedProperties.get(propertyName); + static String readProperty(final Map normalizedProperties, final String propertyName) { + final String normalizedPropertyName = normalizePropertyName(propertyName); + final Object value = normalizedProperties.get(normalizedPropertyName); + return (value instanceof String) ? (String) value : null; + } + + static Map readAllAvailableProperties() { + final Properties systemProperties = System.getProperties(); + final Properties environmentProperties = readEnvironmentProperties(); + final Properties fileProvidedProperties = readPropertiesFile(); + return normalizeProperties(systemProperties, environmentProperties, fileProvidedProperties); + } + + private static Properties readEnvironmentProperties() { + final Properties properties = new Properties(); + properties.putAll(System.getenv()); + return properties; } // We need to roll out our own `.properties` reader. @@ -398,6 +436,55 @@ private static Properties readPropertiesFile() { } return properties; } + + private static Map normalizeProperties(Properties... propertiesList) { + Map map = new HashMap<>(); + for (Properties properties : propertiesList) { + properties.forEach((name, value) -> { + final boolean relevant = isRelevantPropertyName(name); + if (relevant) { + final String normalizedName = normalizePropertyName((String) name); + map.put(normalizedName, value); + } + }); + } + return map; + } + + /** + * Filter to exclude irrelevant property names (i.e., non-string and not {@code log4j}-prefixed) to speed up matching. + * @param propertyName a property name + * @return {@code true}, if the property name is relevant; {@code false}, otherwise + */ + private static boolean isRelevantPropertyName(@Nullable final Object propertyName) { + return propertyName instanceof String && ((String) propertyName).matches("^(?i)log4j.*"); + } + + /** + * An imperfect property name normalization routine. + *

+ * It is imperfect, because {@code foo.bar} would match with {@code fo.obar}. + * But it is good enough for the {@code StatusLogger} needs. + *

+ * + * @param propertyName the input property name + * @return the normalized property name + */ + private static String normalizePropertyName(final String propertyName) { + return propertyName + // Remove separators: + // - dots (properties) + // - dashes (kebab-case) + // - underscores (environment variables) + .replaceAll("[._-]", "") + // Replace all non-ASCII characters. + // Don't remove, otherwise `fooàö` would incorrectly match with `foo`. + // It is safe to replace them with dots, since we've just removed all dots above. + .replaceAll("\\P{InBasic_Latin}", ".") + // Lowercase ASCII – this is safe, since we've just removed all non-ASCII + .toLowerCase(Locale.US) + .replaceAll("^log4j2", "log4j"); + } } /** From d65611a2824fda0a229316632ab3fea24801c9c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Tue, 27 Feb 2024 12:52:19 +0100 Subject: [PATCH 7/8] Use the new property naming style --- .../java/org/apache/logging/log4j/status/StatusLogger.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java index c7a5ce13d4e..bdebc1150a0 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/status/StatusLogger.java @@ -189,7 +189,7 @@ public class StatusLogger extends AbstractLogger { * @see #STATUS_DATE_FORMAT_ZONE * @since 2.11.0 */ - public static final String STATUS_DATE_FORMAT = "log4j2.StatusLogger.DateFormat"; + public static final String STATUS_DATE_FORMAT = "log4j2.StatusLogger.dateFormat"; /** * The name of the system property that can be configured with a time-zone ID (e.g., {@code Europe/Amsterdam}, {@code UTC+01:00}) that will be used while formatting the created {@link StatusData}. @@ -200,7 +200,7 @@ public class StatusLogger extends AbstractLogger { * @see #STATUS_DATE_FORMAT * @since 2.23.1 */ - static final String STATUS_DATE_FORMAT_ZONE = "log4j2.StatusLogger.DateFormatZone"; + static final String STATUS_DATE_FORMAT_ZONE = "log4j2.StatusLogger.dateFormatZone"; /** * The name of the file to be searched in the classpath to read properties from. From 9c52bfebd8e2292c1eb549f058ae16394de009f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Tue, 27 Feb 2024 20:28:46 +0100 Subject: [PATCH 8/8] Simplify `StatusLoggerPropertiesUtilDoubleTest` test cases --- .../StatusLoggerPropertiesUtilDoubleTest.java | 32 ++++++------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerPropertiesUtilDoubleTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerPropertiesUtilDoubleTest.java index 421cacee3c6..16feb542cca 100644 --- a/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerPropertiesUtilDoubleTest.java +++ b/log4j-api-test/src/test/java/org/apache/logging/log4j/status/StatusLoggerPropertiesUtilDoubleTest.java @@ -33,31 +33,17 @@ class StatusLoggerPropertiesUtilDoubleTest { - private static final String[] NOT_PREFIXED_MATCHING_PROPERTY_NAMES = new String[] { - // For system properties: - "StatusLogger.DateFormat", - "StatusLogger.dateFormat", - "StatusLogger.dateformat", - "StatusLogger.Dateformat", - "Status.Logger.Dateformat", - "Status.Logger.Date.Format", - "statusLoggerDateFormat", - "Status.Logger.Date.Format", - "status.logger.date.format", - // For environment variables: - "STATUSLOGGER_DATEFORMAT", - "STATUS_LOGGER_DATE_FORMAT" + private static final String[] MATCHING_PROPERTY_NAMES = new String[] { + // System properties for version range `[, 2.10)` + "log4j2.StatusLogger.DateFormat", + // System properties for version range `[2.10, 3)` + "log4j2.statusLoggerDateFormat", + // System properties for version range `[3,)` + "log4j2.StatusLogger.dateFormat", + // Environment variables + "LOG4J_STATUS_LOGGER_DATE_FORMAT" }; - private static final String[] MATCHING_PROPERTY_NAMES = Stream.of( - // For system properties: - "log4j.", "log4j2.", - // For environment variables: - "LOG4J_", "LOG4J2_") - .flatMap(prefix -> Arrays.stream(NOT_PREFIXED_MATCHING_PROPERTY_NAMES) - .map(notPrefixedPropertyName -> prefix + notPrefixedPropertyName)) - .toArray(String[]::new); - private static final String[] NOT_MATCHING_PROPERTY_NAMES = new String[] {"log4j2.StatusLogger$DateFormat", "log4j2.StàtusLögger.DateFormat"};