From 8f5959ba1d036ce54357a871aabdb6af8999db8e Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Sat, 19 Sep 2020 14:19:03 -0700 Subject: [PATCH] Fix LoggingSystem package tangle Introduce a new `LoggingSystemFactory` interface so that the `LoggingSystem` class can find implementations without needing to be directly tied to them. Closes gh-23387 --- .../DelegatingLoggingSystemFactory.java | 53 +++++++++++++++ .../boot/logging/LoggingSystem.java | 37 ++++------- .../boot/logging/LoggingSystemFactory.java | 49 ++++++++++++++ .../boot/logging/java/JavaLoggingSystem.java | 22 ++++++- .../logging/log4j2/Log4J2LoggingSystem.java | 19 ++++++ .../logging/logback/LogbackLoggingSystem.java | 20 ++++++ .../main/resources/META-INF/spring.factories | 6 ++ .../DelegatingLoggingSystemFactoryTests.java | 66 +++++++++++++++++++ 8 files changed, 245 insertions(+), 27 deletions(-) create mode 100644 spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/DelegatingLoggingSystemFactory.java create mode 100644 spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystemFactory.java create mode 100644 spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/DelegatingLoggingSystemFactoryTests.java diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/DelegatingLoggingSystemFactory.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/DelegatingLoggingSystemFactory.java new file mode 100644 index 0000000000..c8d46c5046 --- /dev/null +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/DelegatingLoggingSystemFactory.java @@ -0,0 +1,53 @@ +/* + * Copyright 2012-2020 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; + +import java.util.List; +import java.util.function.Function; + +/** + * {@link LoggingSystemFactory} that delegates to other factories. + * + * @author Phillip Webb + */ +class DelegatingLoggingSystemFactory implements LoggingSystemFactory { + + private final Function> delegates; + + /** + * Create a new {@link DelegatingLoggingSystemFactory} instance. + * @param delegates a function that provides the delegates + */ + DelegatingLoggingSystemFactory(Function> delegates) { + this.delegates = delegates; + } + + @Override + public LoggingSystem getLoggingSystem(ClassLoader classLoader) { + List delegates = (this.delegates != null) ? this.delegates.apply(classLoader) : null; + if (delegates != null) { + for (LoggingSystemFactory delegate : delegates) { + LoggingSystem loggingSystem = delegate.getLoggingSystem(classLoader); + if (loggingSystem != null) { + return loggingSystem; + } + } + } + return null; + } + +} diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystem.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystem.java index fe0616fd5f..0955603039 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystem.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystem.java @@ -21,11 +21,8 @@ import java.util.Collections; import java.util.EnumSet; import java.util.List; import java.util.Set; -import java.util.function.Function; -import org.springframework.boot.logging.java.JavaLoggingSystem; -import org.springframework.boot.logging.log4j2.Log4J2LoggingSystem; -import org.springframework.boot.logging.logback.LogbackLoggingSystem; +import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.StringUtils; @@ -58,21 +55,7 @@ public abstract class LoggingSystem { */ public static final String ROOT_LOGGER_NAME = "ROOT"; - private static final Function SYSTEM_FACTORY = getSystemFactory(); - - private static Function getSystemFactory() { - ClassLoader classLoader = LoggingSystem.class.getClassLoader(); - if (ClassUtils.isPresent("ch.qos.logback.core.Appender", classLoader)) { - return LogbackLoggingSystem::new; - } - if (ClassUtils.isPresent("org.apache.logging.log4j.core.impl.Log4jContextFactory", classLoader)) { - return Log4J2LoggingSystem::new; - } - if (ClassUtils.isPresent("java.util.logging.LogManager", classLoader)) { - return JavaLoggingSystem::new; - } - throw new IllegalStateException("No suitable logging system located"); - } + private static final LoggingSystemFactory SYSTEM_FACTORY = LoggingSystemFactory.fromSpringFactories(); /** * Reset the logging system to be limit output. This method may be called before @@ -155,19 +138,21 @@ public abstract class LoggingSystem { * @return the logging system */ public static LoggingSystem get(ClassLoader classLoader) { - String loggingSystem = System.getProperty(SYSTEM_PROPERTY); - if (StringUtils.hasLength(loggingSystem)) { - if (NONE.equals(loggingSystem)) { + String loggingSystemClassName = System.getProperty(SYSTEM_PROPERTY); + if (StringUtils.hasLength(loggingSystemClassName)) { + if (NONE.equals(loggingSystemClassName)) { return new NoOpLoggingSystem(); } - return get(classLoader, loggingSystem); + return get(classLoader, loggingSystemClassName); } - return SYSTEM_FACTORY.apply(classLoader); + LoggingSystem loggingSystem = SYSTEM_FACTORY.getLoggingSystem(classLoader); + Assert.state(loggingSystem != null, "No suitable logging system located"); + return loggingSystem; } - private static LoggingSystem get(ClassLoader classLoader, String loggingSystemClass) { + private static LoggingSystem get(ClassLoader classLoader, String loggingSystemClassName) { try { - Class systemClass = ClassUtils.forName(loggingSystemClass, classLoader); + Class systemClass = ClassUtils.forName(loggingSystemClassName, classLoader); Constructor constructor = systemClass.getDeclaredConstructor(ClassLoader.class); constructor.setAccessible(true); return (LoggingSystem) constructor.newInstance(classLoader); diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystemFactory.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystemFactory.java new file mode 100644 index 0000000000..befe1505a3 --- /dev/null +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystemFactory.java @@ -0,0 +1,49 @@ +/* + * Copyright 2012-2020 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; + +import org.springframework.boot.env.EnvironmentPostProcessorsFactory; +import org.springframework.core.io.support.SpringFactoriesLoader; + +/** + * Factory class used by {@link LoggingSystem#get(ClassLoader)} to find an actual + * implementation. + * + * @author Phillip Webb + * @since 2.4.0 + */ +public interface LoggingSystemFactory { + + /** + * Return a logging system implementation or {@code null} if not logging system is + * available. + * @param classLoader the class loader to use + * @return a logging system + */ + LoggingSystem getLoggingSystem(ClassLoader classLoader); + + /** + * Return a {@link EnvironmentPostProcessorsFactory} backed by + * {@code spring.factories}. + * @return an {@link LoggingSystemFactory} instance + */ + static LoggingSystemFactory fromSpringFactories() { + return new DelegatingLoggingSystemFactory( + (classLoader) -> SpringFactoriesLoader.loadFactories(LoggingSystemFactory.class, classLoader)); + } + +} diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/java/JavaLoggingSystem.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/java/JavaLoggingSystem.java index e79b751006..3f3a7038fe 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/java/JavaLoggingSystem.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/java/JavaLoggingSystem.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2020 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. @@ -34,7 +34,11 @@ import org.springframework.boot.logging.LogLevel; import org.springframework.boot.logging.LoggerConfiguration; import org.springframework.boot.logging.LoggingInitializationContext; import org.springframework.boot.logging.LoggingSystem; +import org.springframework.boot.logging.LoggingSystemFactory; +import org.springframework.core.Ordered; +import org.springframework.core.annotation.Order; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; import org.springframework.util.FileCopyUtils; import org.springframework.util.ResourceUtils; import org.springframework.util.StringUtils; @@ -177,4 +181,20 @@ public class JavaLoggingSystem extends AbstractLoggingSystem { } + /** + * {@link LoggingSystemFactory} that returns {@link JavaLoggingSystem} if possible. + */ + @Order(Ordered.LOWEST_PRECEDENCE) + public static class Factory implements LoggingSystemFactory { + + @Override + public LoggingSystem getLoggingSystem(ClassLoader classLoader) { + if (ClassUtils.isPresent("java.util.logging.LogManager", classLoader)) { + return new JavaLoggingSystem(classLoader); + } + return null; + } + + } + } 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 12e30eaaf6..3195bb0721 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 @@ -47,7 +47,10 @@ import org.springframework.boot.logging.LogLevel; import org.springframework.boot.logging.LoggerConfiguration; import org.springframework.boot.logging.LoggingInitializationContext; import org.springframework.boot.logging.LoggingSystem; +import org.springframework.boot.logging.LoggingSystemFactory; import org.springframework.boot.logging.Slf4JLoggingSystem; +import org.springframework.core.Ordered; +import org.springframework.core.annotation.Order; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.ResourceUtils; @@ -326,4 +329,20 @@ public class Log4J2LoggingSystem extends Slf4JLoggingSystem { } + /** + * {@link LoggingSystemFactory} that returns {@link Log4J2LoggingSystem} if possible. + */ + @Order(Ordered.LOWEST_PRECEDENCE) + public static class Factory implements LoggingSystemFactory { + + @Override + public LoggingSystem getLoggingSystem(ClassLoader classLoader) { + if (ClassUtils.isPresent("org.apache.logging.log4j.core.impl.Log4jContextFactory", classLoader)) { + return new Log4J2LoggingSystem(classLoader); + } + return null; + } + + } + } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/logback/LogbackLoggingSystem.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/logback/LogbackLoggingSystem.java index 848f3ef59c..082b968210 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/logback/LogbackLoggingSystem.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/logging/logback/LogbackLoggingSystem.java @@ -47,11 +47,15 @@ import org.springframework.boot.logging.LogLevel; import org.springframework.boot.logging.LoggerConfiguration; import org.springframework.boot.logging.LoggingInitializationContext; import org.springframework.boot.logging.LoggingSystem; +import org.springframework.boot.logging.LoggingSystemFactory; import org.springframework.boot.logging.LoggingSystemProperties; import org.springframework.boot.logging.Slf4JLoggingSystem; +import org.springframework.core.Ordered; import org.springframework.core.SpringProperties; +import org.springframework.core.annotation.Order; import org.springframework.core.env.Environment; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; import org.springframework.util.ResourceUtils; import org.springframework.util.StringUtils; @@ -331,4 +335,20 @@ public class LogbackLoggingSystem extends Slf4JLoggingSystem { } + /** + * {@link LoggingSystemFactory} that returns {@link LogbackLoggingSystem} if possible. + */ + @Order(Ordered.LOWEST_PRECEDENCE) + public static class Factory implements LoggingSystemFactory { + + @Override + public LoggingSystem getLoggingSystem(ClassLoader classLoader) { + if (ClassUtils.isPresent("ch.qos.logback.core.Appender", classLoader)) { + return new LogbackLoggingSystem(classLoader); + } + return null; + } + + } + } diff --git a/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories b/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories index ed5dd47955..d460701d02 100644 --- a/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories +++ b/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories @@ -1,3 +1,9 @@ +# Logging Systems +org.springframework.boot.logging.LoggingSystemFactory=\ +org.springframework.boot.logging.logback.LogbackLoggingSystem.Factory,\ +org.springframework.boot.logging.log4j2.Log4J2LoggingSystem.Factory,\ +org.springframework.boot.logging.java.JavaLoggingSystem.Factory + # PropertySource Loaders org.springframework.boot.env.PropertySourceLoader=\ org.springframework.boot.env.PropertiesPropertySourceLoader,\ diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/DelegatingLoggingSystemFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/DelegatingLoggingSystemFactoryTests.java new file mode 100644 index 0000000000..b683ea4796 --- /dev/null +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/logging/DelegatingLoggingSystemFactoryTests.java @@ -0,0 +1,66 @@ +/* + * Copyright 2012-2020 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; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; + +/** + * Tests for {@link DelegatingLoggingSystemFactory}. + * + * @author Phillip Webb + */ +class DelegatingLoggingSystemFactoryTests { + + private ClassLoader classLoader = getClass().getClassLoader(); + + @Test + void getLoggingSystemWhenDelegatesFunctionIsNullReturnsNull() { + DelegatingLoggingSystemFactory factory = new DelegatingLoggingSystemFactory(null); + assertThat(factory.getLoggingSystem(this.classLoader)).isNull(); + } + + @Test + void getLoggingSystemWhenDelegatesFunctionReturnsNullReturnsNull() { + DelegatingLoggingSystemFactory factory = new DelegatingLoggingSystemFactory((cl) -> null); + assertThat(factory.getLoggingSystem(this.classLoader)).isNull(); + } + + @Test + void getLoggingSystemReturnsFirstNonNullLoggingSystem() { + List delegates = new ArrayList<>(); + delegates.add(mock(LoggingSystemFactory.class)); + delegates.add(mock(LoggingSystemFactory.class)); + delegates.add(mock(LoggingSystemFactory.class)); + LoggingSystem result = mock(LoggingSystem.class); + given(delegates.get(1).getLoggingSystem(this.classLoader)).willReturn(result); + DelegatingLoggingSystemFactory factory = new DelegatingLoggingSystemFactory((cl) -> delegates); + assertThat(factory.getLoggingSystem(this.classLoader)).isSameAs(result); + verify(delegates.get(0)).getLoggingSystem(this.classLoader); + verify(delegates.get(1)).getLoggingSystem(this.classLoader); + verifyNoInteractions(delegates.get(2)); + } + +}