From ed424d3adb9f074e3b5021b7d3b343b290164389 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Wed, 12 Oct 2022 17:37:08 -0700 Subject: [PATCH] Polish 'Add Log4J2 PropertySource backed by the Spring Environment' See gh-32733 --- .../logging/log4j2/Log4J2LoggingSystem.java | 6 +- ...a => SpringEnvironmentPropertySource.java} | 28 +++---- .../log4j2/Log4J2LoggingSystemTests.java | 24 ++++++ .../SpringEnvironmentPropertySourceTests.java | 83 +++++++++++++++++++ 4 files changed, 122 insertions(+), 19 deletions(-) rename spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/{SpringPropertySource.java => SpringEnvironmentPropertySource.java} (72%) create mode 100644 spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/SpringEnvironmentPropertySourceTests.java diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystem.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystem.java index 00c362e1b3..38c19b4689 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystem.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystem.java @@ -233,8 +233,10 @@ public class Log4J2LoggingSystem extends AbstractLoggingSystem { return; } Environment environment = initializationContext.getEnvironment(); - getLoggerContext().putObjectIfAbsent(ENVIRONMENT_KEY, environment); - PropertiesUtil.getProperties().addPropertySource(new SpringPropertySource(environment)); + if (environment != null) { + getLoggerContext().putObjectIfAbsent(ENVIRONMENT_KEY, environment); + PropertiesUtil.getProperties().addPropertySource(new SpringEnvironmentPropertySource(environment)); + } loggerContext.getConfiguration().removeFilter(FILTER); super.initialize(initializationContext, configLocation, logFile); markAsInitialized(loggerContext); diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/SpringPropertySource.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/SpringEnvironmentPropertySource.java similarity index 72% rename from spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/SpringPropertySource.java rename to spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/SpringEnvironmentPropertySource.java index ea366d993f..8b8671e536 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/SpringPropertySource.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/SpringEnvironmentPropertySource.java @@ -19,46 +19,40 @@ package org.springframework.boot.logging.log4j2; import org.apache.logging.log4j.util.PropertySource; import org.springframework.core.env.Environment; +import org.springframework.util.Assert; /** * Returns properties from Spring. * * @author Ralph Goers - * @since 3.0.0 */ -public class SpringPropertySource implements PropertySource { +class SpringEnvironmentPropertySource implements PropertySource { - private static final int DEFAULT_PRIORITY = -100; + /** + * System properties take precedence followed by properties in Log4j properties files. + */ + private static final int PRIORITY = -100; private final Environment environment; - public SpringPropertySource(Environment environment) { + SpringEnvironmentPropertySource(Environment environment) { + Assert.notNull(environment, "Environment must not be null"); this.environment = environment; } - /** - * System properties take precedence followed by properties in Log4j properties files. - * @return this PropertySource's priority. - */ @Override public int getPriority() { - return DEFAULT_PRIORITY; + return PRIORITY; } @Override public String getProperty(String key) { - if (this.environment != null) { - return this.environment.getProperty(key); - } - return null; + return this.environment.getProperty(key); } @Override public boolean containsProperty(String key) { - if (this.environment != null) { - return this.environment.containsProperty(key); - } - return false; + return this.environment.containsProperty(key); } } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystemTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystemTests.java index 3155bf13f8..ec12461e83 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystemTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystemTests.java @@ -25,6 +25,7 @@ import java.util.EnumSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.logging.Handler; import java.util.logging.Level; @@ -42,6 +43,7 @@ import org.apache.logging.log4j.core.config.composite.CompositeConfiguration; import org.apache.logging.log4j.core.util.ShutdownCallbackRegistry; import org.apache.logging.log4j.jul.Log4jBridgeHandler; import org.apache.logging.log4j.util.PropertiesUtil; +import org.apache.logging.log4j.util.PropertySource; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -59,6 +61,7 @@ import org.springframework.boot.testsupport.system.CapturedOutput; import org.springframework.boot.testsupport.system.OutputCaptureExtension; import org.springframework.core.env.Environment; import org.springframework.mock.env.MockEnvironment; +import org.springframework.test.util.ReflectionTestUtils; import org.springframework.util.ClassUtils; import org.springframework.util.StringUtils; @@ -101,6 +104,7 @@ class Log4J2LoggingSystemTests extends AbstractLoggingSystemTests { this.configuration = loggerContext.getConfiguration(); this.loggingSystem.cleanUp(); this.logger = LogManager.getLogger(getClass()); + cleanUpPropertySources(); } @AfterEach @@ -109,6 +113,16 @@ class Log4J2LoggingSystemTests extends AbstractLoggingSystemTests { LoggerContext loggerContext = (LoggerContext) LogManager.getContext(false); loggerContext.stop(); loggerContext.start(((Reconfigurable) this.configuration).reconfigure()); + cleanUpPropertySources(); + } + + @SuppressWarnings("unchecked") + private void cleanUpPropertySources() { // https://issues.apache.org/jira/browse/LOG4J2-3618 + PropertiesUtil properties = PropertiesUtil.getProperties(); + Object environment = ReflectionTestUtils.getField(properties, "environment"); + Set sources = (Set) ReflectionTestUtils.getField(environment, "sources"); + sources.removeIf((candidate) -> candidate instanceof SpringEnvironmentPropertySource + || candidate instanceof SpringBootPropertySource); } @Test @@ -448,6 +462,16 @@ class Log4J2LoggingSystemTests extends AbstractLoggingSystemTests { assertThat(environment).isSameAs(this.environment); } + @Test + void initializeAddsSpringEnvironmentPropertySource() { + PropertiesUtil properties = PropertiesUtil.getProperties(); + this.environment.setProperty("spring", "boot"); + this.loggingSystem.beforeInitialize(); + this.loggingSystem.initialize(this.initializationContext, null, null); + properties = PropertiesUtil.getProperties(); + assertThat(properties.getStringProperty("spring")).isEqualTo("boot"); + } + private String getRelativeClasspathLocation(String fileName) { String defaultPath = ClassUtils.getPackageName(getClass()); defaultPath = defaultPath.replace('.', '/'); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/SpringEnvironmentPropertySourceTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/SpringEnvironmentPropertySourceTests.java new file mode 100644 index 0000000000..9a7162b250 --- /dev/null +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/SpringEnvironmentPropertySourceTests.java @@ -0,0 +1,83 @@ +/* + * Copyright 2012-2022 the original author or authors. + * + * Licensed 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 + * + * https://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.springframework.boot.logging.log4j2; + +import java.util.Properties; + +import org.apache.logging.log4j.util.PropertiesPropertySource; +import org.apache.logging.log4j.util.SystemPropertiesPropertySource; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import org.springframework.mock.env.MockEnvironment; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; + +/** + * Tests for {@link SpringEnvironmentPropertySource}. + * + * @author Phillip Webb + */ +class SpringEnvironmentPropertySourceTests { + + private MockEnvironment environment; + + private SpringEnvironmentPropertySource propertySource; + + @BeforeEach + void setup() { + this.environment = new MockEnvironment(); + this.environment.setProperty("spring", "boot"); + this.propertySource = new SpringEnvironmentPropertySource(this.environment); + } + + @Test + void createWhenEnvironmentIsNullThrowsException() { + assertThatIllegalArgumentException().isThrownBy(() -> new SpringEnvironmentPropertySource(null)) + .withMessage("Environment must not be null"); + } + + @Test + void getPriorityIsOrderedCorrectly() { + int priority = this.propertySource.getPriority(); + assertThat(priority).isEqualTo(-100); + assertThat(priority).isLessThan(new SystemPropertiesPropertySource().getPriority()); + assertThat(priority).isLessThan(new PropertiesPropertySource(new Properties()).getPriority()); + } + + @Test + void getPropertyWhenInEnvironmentReturnsValue() { + assertThat(this.propertySource.getProperty("spring")).isEqualTo("boot"); + } + + @Test + void getPropertyWhenNotInEnvironmentReturnsNull() { + assertThat(this.propertySource.getProperty("nope")).isNull(); + } + + @Test + void containsPropertyWhenInEnvironmentReturnsTrue() { + assertThat(this.propertySource.containsProperty("spring")).isTrue(); + } + + @Test + void containsPropertyWhenNotInEnvironmentReturnsFalse() { + assertThat(this.propertySource.containsProperty("nope")).isFalse(); + } + +}