From 9a396c8ed472f8ecd17b3f162b79844088e8c82f Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Thu, 10 Sep 2026 16:08:23 +0200 Subject: [PATCH 1/2] Improve Javadoc for LogAccessor --- .../springframework/core/log/LogAccessor.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/log/LogAccessor.java b/spring-core/src/main/java/org/springframework/core/log/LogAccessor.java index 571504560e2..bfda34053dd 100644 --- a/spring-core/src/main/java/org/springframework/core/log/LogAccessor.java +++ b/spring-core/src/main/java/org/springframework/core/log/LogAccessor.java @@ -124,7 +124,7 @@ public class LogAccessor { } /** - * Log an error with fatal log level. + * Log an exception with fatal log level. * @param cause the exception to log * @param message the message to log */ @@ -141,7 +141,7 @@ public class LogAccessor { } /** - * Log an error with error log level. + * Log an exception with error log level. * @param cause the exception to log * @param message the message to log */ @@ -158,7 +158,7 @@ public class LogAccessor { } /** - * Log an error with warn log level. + * Log an exception with warn log level. * @param cause the exception to log * @param message the message to log */ @@ -175,7 +175,7 @@ public class LogAccessor { } /** - * Log an error with info log level. + * Log an exception with info log level. * @param cause the exception to log * @param message the message to log */ @@ -192,7 +192,7 @@ public class LogAccessor { } /** - * Log an error with debug log level. + * Log an exception with debug log level. * @param cause the exception to log * @param message the message to log */ @@ -209,7 +209,7 @@ public class LogAccessor { } /** - * Log an error with trace log level. + * Log an exception with trace log level. * @param cause the exception to log * @param message the message to log */ @@ -231,7 +231,7 @@ public class LogAccessor { } /** - * Log an error with fatal log level. + * Log an exception with fatal log level. * @param cause the exception to log * @param messageSupplier a lazy supplier for the message to log */ @@ -252,7 +252,7 @@ public class LogAccessor { } /** - * Log an error with error log level. + * Log an exception with error log level. * @param cause the exception to log * @param messageSupplier a lazy supplier for the message to log */ @@ -273,7 +273,7 @@ public class LogAccessor { } /** - * Log an error with warn log level. + * Log an exception with warn log level. * @param cause the exception to log * @param messageSupplier a lazy supplier for the message to log */ @@ -294,7 +294,7 @@ public class LogAccessor { } /** - * Log an error with info log level. + * Log an exception with info log level. * @param cause the exception to log * @param messageSupplier a lazy supplier for the message to log */ @@ -315,7 +315,7 @@ public class LogAccessor { } /** - * Log an error with debug log level. + * Log an exception with debug log level. * @param cause the exception to log * @param messageSupplier a lazy supplier for the message to log */ @@ -336,7 +336,7 @@ public class LogAccessor { } /** - * Log an error with trace log level. + * Log an exception with trace log level. * @param cause the exception to log * @param messageSupplier a lazy supplier for the message to log */ From b96592e4afebfaeb64903c7f8397fef48abc6e3b Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Thu, 10 Sep 2026 16:02:59 +0200 Subject: [PATCH 2/2] Guard CharSequence-based logging methods in LogAccessor Prior to this commit, LogAccessor's CharSequence-based logging methods delegated directly to the corresponding method on the underlying commons-logging Log instance without first checking whether the target level was enabled. This differed from the Supplier-based overloads, which have checked isXxxEnabled() before delegating since Spring Framework 5.2.9. That asymmetry was harmless as long as spring-jcl supplied the underlying Log implementation, since its SLF4J adapter itself checked the level before rendering the message. However, since Spring Framework 7 replaced spring-jcl with Apache commons-logging, whose SLF4J adapters call String.valueOf(message) unconditionally, any CharSequence argument -- most notably a LogMessage supplied via LogMessage.format(...) or LogMessage.of(...) -- is now rendered eagerly, even when the corresponding level is disabled. Since LogMessage exists specifically to defer that work, and the idiom is used extensively throughout the framework and its portfolio projects, this leads to unnecessary computation and allocation whenever logging is disabled. To address this, this commit adds the same isXxxEnabled() guard to all twelve CharSequence-based methods in LogAccessor, matching the existing Supplier-based overloads and making LogAccessor's laziness guarantee independent of the underlying Log implementation. This commit also introduces LogAccessorTests, which verifies that a lazily rendering LogMessage passed to one of the CharSequence-based methods is only rendered when the corresponding level is enabled. See gh-25741 Closes gh-37266 --- .../springframework/core/log/LogAccessor.java | 48 +++-- .../core/log/LogAccessorTests.java | 174 ++++++++++++++++++ 2 files changed, 210 insertions(+), 12 deletions(-) create mode 100644 spring-core/src/test/java/org/springframework/core/log/LogAccessorTests.java diff --git a/spring-core/src/main/java/org/springframework/core/log/LogAccessor.java b/spring-core/src/main/java/org/springframework/core/log/LogAccessor.java index bfda34053dd..0509d041590 100644 --- a/spring-core/src/main/java/org/springframework/core/log/LogAccessor.java +++ b/spring-core/src/main/java/org/springframework/core/log/LogAccessor.java @@ -120,7 +120,9 @@ public class LogAccessor { * @param message the message to log */ public void fatal(CharSequence message) { - this.log.fatal(message); + if (this.log.isFatalEnabled()) { + this.log.fatal(message); + } } /** @@ -129,7 +131,9 @@ public class LogAccessor { * @param message the message to log */ public void fatal(Throwable cause, CharSequence message) { - this.log.fatal(message, cause); + if (this.log.isFatalEnabled()) { + this.log.fatal(message, cause); + } } /** @@ -137,7 +141,9 @@ public class LogAccessor { * @param message the message to log */ public void error(CharSequence message) { - this.log.error(message); + if (this.log.isErrorEnabled()) { + this.log.error(message); + } } /** @@ -146,7 +152,9 @@ public class LogAccessor { * @param message the message to log */ public void error(Throwable cause, CharSequence message) { - this.log.error(message, cause); + if (this.log.isErrorEnabled()) { + this.log.error(message, cause); + } } /** @@ -154,7 +162,9 @@ public class LogAccessor { * @param message the message to log */ public void warn(CharSequence message) { - this.log.warn(message); + if (this.log.isWarnEnabled()) { + this.log.warn(message); + } } /** @@ -163,7 +173,9 @@ public class LogAccessor { * @param message the message to log */ public void warn(Throwable cause, CharSequence message) { - this.log.warn(message, cause); + if (this.log.isWarnEnabled()) { + this.log.warn(message, cause); + } } /** @@ -171,7 +183,9 @@ public class LogAccessor { * @param message the message to log */ public void info(CharSequence message) { - this.log.info(message); + if (this.log.isInfoEnabled()) { + this.log.info(message); + } } /** @@ -180,7 +194,9 @@ public class LogAccessor { * @param message the message to log */ public void info(Throwable cause, CharSequence message) { - this.log.info(message, cause); + if (this.log.isInfoEnabled()) { + this.log.info(message, cause); + } } /** @@ -188,7 +204,9 @@ public class LogAccessor { * @param message the message to log */ public void debug(CharSequence message) { - this.log.debug(message); + if (this.log.isDebugEnabled()) { + this.log.debug(message); + } } /** @@ -197,7 +215,9 @@ public class LogAccessor { * @param message the message to log */ public void debug(Throwable cause, CharSequence message) { - this.log.debug(message, cause); + if (this.log.isDebugEnabled()) { + this.log.debug(message, cause); + } } /** @@ -205,7 +225,9 @@ public class LogAccessor { * @param message the message to log */ public void trace(CharSequence message) { - this.log.trace(message); + if (this.log.isTraceEnabled()) { + this.log.trace(message); + } } /** @@ -214,7 +236,9 @@ public class LogAccessor { * @param message the message to log */ public void trace(Throwable cause, CharSequence message) { - this.log.trace(message, cause); + if (this.log.isTraceEnabled()) { + this.log.trace(message, cause); + } } diff --git a/spring-core/src/test/java/org/springframework/core/log/LogAccessorTests.java b/spring-core/src/test/java/org/springframework/core/log/LogAccessorTests.java new file mode 100644 index 00000000000..524dc4bc4c3 --- /dev/null +++ b/spring-core/src/test/java/org/springframework/core/log/LogAccessorTests.java @@ -0,0 +1,174 @@ +/* + * Copyright 2002-present 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.core.log; + +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.apache.commons.logging.Log; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.FieldSource; +import org.mockito.stubbing.Answer; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.params.provider.Arguments.argumentSet; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Tests for {@link LogAccessor}. + * + * @author Sam Brannen + * @since 7.0.10 + */ +class LogAccessorTests { + + private final Log log = mock(); + + private final LogAccessor logAccessor = new LogAccessor(this.log); + + + @BeforeEach + void renderMessagesWhenDelegatingToLog() { + // Simulate a Log implementation (such as commons-logging's SLF4J bridge) that renders the + // message unconditionally, in order to verify that LogAccessor itself guards against that. + Answer render = invocation -> { + invocation.getArgument(0).toString(); + return null; + }; + doAnswer(render).when(this.log).fatal(any()); + doAnswer(render).when(this.log).fatal(any(), any()); + doAnswer(render).when(this.log).error(any()); + doAnswer(render).when(this.log).error(any(), any()); + doAnswer(render).when(this.log).warn(any()); + doAnswer(render).when(this.log).warn(any(), any()); + doAnswer(render).when(this.log).info(any()); + doAnswer(render).when(this.log).info(any(), any()); + doAnswer(render).when(this.log).debug(any()); + doAnswer(render).when(this.log).debug(any(), any()); + doAnswer(render).when(this.log).trace(any()); + doAnswer(render).when(this.log).trace(any(), any()); + } + + @ParameterizedTest + @FieldSource("levels") + void messageIsNotRenderedWhenLevelIsDisabled(LevelEnabler enabler, LogInvoker invoker) { + enabler.enable(this.log, false); + + AtomicBoolean rendered = new AtomicBoolean(); + invoker.invoke(this.logAccessor, lazyMessage(rendered)); + + assertThat(rendered).isFalse(); + } + + @ParameterizedTest + @FieldSource("levels") + void messageIsRenderedWhenLevelIsEnabled(LevelEnabler enabler, LogInvoker invoker) { + enabler.enable(this.log, true); + + AtomicBoolean rendered = new AtomicBoolean(); + invoker.invoke(this.logAccessor, lazyMessage(rendered)); + + assertThat(rendered).isTrue(); + } + + @ParameterizedTest + @FieldSource("levelsWithCause") + void messageWithCauseIsNotRenderedWhenLevelIsDisabled(LevelEnabler enabler, CauseLogInvoker invoker) { + enabler.enable(this.log, false); + + AtomicBoolean rendered = new AtomicBoolean(); + invoker.invoke(this.logAccessor, new RuntimeException(), lazyMessage(rendered)); + + assertThat(rendered).isFalse(); + } + + @ParameterizedTest + @FieldSource("levelsWithCause") + void messageWithCauseIsRenderedWhenLevelIsEnabled(LevelEnabler enabler, CauseLogInvoker invoker) { + enabler.enable(this.log, true); + + AtomicBoolean rendered = new AtomicBoolean(); + invoker.invoke(this.logAccessor, new RuntimeException(), lazyMessage(rendered)); + + assertThat(rendered).isTrue(); + } + + + private static LogMessage lazyMessage(AtomicBoolean rendered) { + return LogMessage.of(() -> { + rendered.set(true); + return "message"; + }); + } + + @SuppressWarnings("unused") + private static List levels = List.of( + argumentSet("fatal", (LevelEnabler) (log, enabled) -> when(log.isFatalEnabled()).thenReturn(enabled), + (LogInvoker) LogAccessor::fatal), + argumentSet("error", (LevelEnabler) (log, enabled) -> when(log.isErrorEnabled()).thenReturn(enabled), + (LogInvoker) LogAccessor::error), + argumentSet("warn", (LevelEnabler) (log, enabled) -> when(log.isWarnEnabled()).thenReturn(enabled), + (LogInvoker) LogAccessor::warn), + argumentSet("info", (LevelEnabler) (log, enabled) -> when(log.isInfoEnabled()).thenReturn(enabled), + (LogInvoker) LogAccessor::info), + argumentSet("debug", (LevelEnabler) (log, enabled) -> when(log.isDebugEnabled()).thenReturn(enabled), + (LogInvoker) LogAccessor::debug), + argumentSet("trace", (LevelEnabler) (log, enabled) -> when(log.isTraceEnabled()).thenReturn(enabled), + (LogInvoker) LogAccessor::trace) + ); + + @SuppressWarnings("unused") + private static List levelsWithCause = List.of( + argumentSet("fatal", (LevelEnabler) (log, enabled) -> when(log.isFatalEnabled()).thenReturn(enabled), + (CauseLogInvoker) LogAccessor::fatal), + argumentSet("error", (LevelEnabler) (log, enabled) -> when(log.isErrorEnabled()).thenReturn(enabled), + (CauseLogInvoker) LogAccessor::error), + argumentSet("warn", (LevelEnabler) (log, enabled) -> when(log.isWarnEnabled()).thenReturn(enabled), + (CauseLogInvoker) LogAccessor::warn), + argumentSet("info", (LevelEnabler) (log, enabled) -> when(log.isInfoEnabled()).thenReturn(enabled), + (CauseLogInvoker) LogAccessor::info), + argumentSet("debug", (LevelEnabler) (log, enabled) -> when(log.isDebugEnabled()).thenReturn(enabled), + (CauseLogInvoker) LogAccessor::debug), + argumentSet("trace", (LevelEnabler) (log, enabled) -> when(log.isTraceEnabled()).thenReturn(enabled), + (CauseLogInvoker) LogAccessor::trace) + ); + + + @FunctionalInterface + private interface LevelEnabler { + + void enable(Log log, boolean enabled); + } + + @FunctionalInterface + private interface LogInvoker { + + void invoke(LogAccessor logAccessor, CharSequence message); + } + + @FunctionalInterface + private interface CauseLogInvoker { + + void invoke(LogAccessor logAccessor, Throwable cause, CharSequence message); + } + +}