Polish 'Add Log4J2 PropertySource backed by the Spring Environment'

See gh-32733

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);

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);
 	}
 
 }

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<PropertySource> sources = (Set<PropertySource>) 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('.', '/');

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();
+	}
+
+}