From 26df4580b3bf8ea91dad13f85bfb2652a8bc5118 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Tue, 21 Jun 2022 16:13:55 +0200 Subject: [PATCH] Revise internals of LoggingCacheErrorHandler Since LoggingCacheErrorHandler was only recently introduced in 5.3.16, we have decided to completely revise its internals (protected API) in 5.3.x while retaining the current public API. Specifically, this commit: - introduces protected getLogger() and isLogStackTraces() methods to improve extensibility - revises logCacheError() to accept a Supplier for lazy resolution of error messages Closes gh-28672 See gh-28670, gh-28648 --- .../interceptor/LoggingCacheErrorHandler.java | 66 +++++++++++++------ .../LoggingCacheErrorHandlerTests.java | 50 ++++++++------ 2 files changed, 75 insertions(+), 41 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/cache/interceptor/LoggingCacheErrorHandler.java b/spring-context/src/main/java/org/springframework/cache/interceptor/LoggingCacheErrorHandler.java index d13a1cb8e2..bfb821e350 100644 --- a/spring-context/src/main/java/org/springframework/cache/interceptor/LoggingCacheErrorHandler.java +++ b/spring-context/src/main/java/org/springframework/cache/interceptor/LoggingCacheErrorHandler.java @@ -16,6 +16,8 @@ package org.springframework.cache.interceptor; +import java.util.function.Supplier; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -78,47 +80,71 @@ public class LoggingCacheErrorHandler implements CacheErrorHandler { @Override public void handleCacheGetError(RuntimeException exception, Cache cache, Object key) { - logCacheError(logger, - createMessage(cache, "failed to get entry with key '" + key + "'"), + logCacheError( + () -> String.format("Cache '%s' failed to get entry with key '%s'", cache.getName(), key), exception); } @Override public void handleCachePutError(RuntimeException exception, Cache cache, Object key, @Nullable Object value) { - logCacheError(logger, - createMessage(cache, "failed to put entry with key '" + key + "'"), + logCacheError( + () -> String.format("Cache '%s' failed to put entry with key '%s'", cache.getName(), key), exception); } @Override public void handleCacheEvictError(RuntimeException exception, Cache cache, Object key) { - logCacheError(logger, - createMessage(cache, "failed to evict entry with key '" + key + "'"), + logCacheError( + () -> String.format("Cache '%s' failed to evict entry with key '%s'", cache.getName(), key), exception); } @Override public void handleCacheClearError(RuntimeException exception, Cache cache) { - logCacheError(logger, createMessage(cache, "failed to clear entries"), exception); + logCacheError( + () -> String.format("Cache '%s' failed to clear entries", cache.getName()), + exception); } + /** - * Log the specified message. - * @param logger the logger - * @param message the message - * @param ex the exception + * Get the logger for this {@code LoggingCacheErrorHandler}. + * @return the logger + * @since 5.3.22 */ - protected void logCacheError(Log logger, String message, RuntimeException ex) { - if (this.logStackTraces) { - logger.warn(message, ex); - } - else { - logger.warn(message); - } + protected final Log getLogger() { + return logger; } - private String createMessage(Cache cache, String reason) { - return String.format("Cache '%s' %s", cache.getName(), reason); + /** + * Get the {@code logStackTraces} flag for this {@code LoggingCacheErrorHandler}. + * @return {@code true} if this {@code LoggingCacheErrorHandler} logs stack traces + * @since 5.3.22 + */ + protected final boolean isLogStackTraces() { + return this.logStackTraces; + } + + /** + * Log the cache error message in the given supplier. + *

If {@link #isLogStackTraces()} is {@code true}, the given + * {@code exception} will be logged as well. + *

The default implementation logs the message as a warning. + * @param messageSupplier the message supplier + * @param exception the exception thrown by the cache provider + * @since 5.3.22 + * @see #isLogStackTraces() + * @see #getLogger() + */ + protected void logCacheError(Supplier messageSupplier, RuntimeException exception) { + if (getLogger().isWarnEnabled()) { + if (isLogStackTraces()) { + getLogger().warn(messageSupplier.get(), exception); + } + else { + getLogger().warn(messageSupplier.get()); + } + } } } diff --git a/spring-context/src/test/java/org/springframework/cache/interceptor/LoggingCacheErrorHandlerTests.java b/spring-context/src/test/java/org/springframework/cache/interceptor/LoggingCacheErrorHandlerTests.java index d07ed63fb0..e9dc064169 100644 --- a/spring-context/src/test/java/org/springframework/cache/interceptor/LoggingCacheErrorHandlerTests.java +++ b/spring-context/src/test/java/org/springframework/cache/interceptor/LoggingCacheErrorHandlerTests.java @@ -17,13 +17,14 @@ package org.springframework.cache.interceptor; import org.apache.commons.logging.Log; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.cache.Cache; import org.springframework.cache.support.NoOpCache; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; /** @@ -32,48 +33,55 @@ import static org.mockito.Mockito.verify; * @author Adam Ostrožlík * @author Stephane Nicoll * @author Vedran Pavic + * @author Sam Brannen */ -@ExtendWith(MockitoExtension.class) class LoggingCacheErrorHandlerTests { - @Mock - private Log logger; + private static final Cache CACHE = new NoOpCache("NOOP"); + + private static final String KEY = "enigma"; + + private final Log logger = mock(Log.class); + + private LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler(this.logger, false); + + + @BeforeEach + void setUp() { + given(this.logger.isWarnEnabled()).willReturn(true); + } @Test void handleGetCacheErrorLogsAppropriateMessage() { - LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler(this.logger, false); - handler.handleCacheGetError(new RuntimeException(), new NoOpCache("NOOP"), "key"); - verify(this.logger).warn("Cache 'NOOP' failed to get entry with key 'key'"); + this.handler.handleCacheGetError(new RuntimeException(), CACHE, KEY); + verify(this.logger).warn("Cache 'NOOP' failed to get entry with key 'enigma'"); } @Test void handlePutCacheErrorLogsAppropriateMessage() { - LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler(this.logger, false); - handler.handleCachePutError(new RuntimeException(), new NoOpCache("NOOP"), "key", new Object()); - verify(this.logger).warn("Cache 'NOOP' failed to put entry with key 'key'"); + this.handler.handleCachePutError(new RuntimeException(), CACHE, KEY, null); + verify(this.logger).warn("Cache 'NOOP' failed to put entry with key 'enigma'"); } @Test void handleEvictCacheErrorLogsAppropriateMessage() { - LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler(this.logger, false); - handler.handleCacheEvictError(new RuntimeException(), new NoOpCache("NOOP"), "key"); - verify(this.logger).warn("Cache 'NOOP' failed to evict entry with key 'key'"); + this.handler.handleCacheEvictError(new RuntimeException(), CACHE, KEY); + verify(this.logger).warn("Cache 'NOOP' failed to evict entry with key 'enigma'"); } @Test void handleClearErrorLogsAppropriateMessage() { - LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler(this.logger, false); - handler.handleCacheClearError(new RuntimeException(), new NoOpCache("NOOP")); + this.handler.handleCacheClearError(new RuntimeException(), CACHE); verify(this.logger).warn("Cache 'NOOP' failed to clear entries"); } @Test - void handleCacheErrorWithStacktrace() { - LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler(this.logger, true); + void handleGetCacheErrorWithStackTraceLoggingEnabled() { + this.handler = new LoggingCacheErrorHandler(this.logger, true); RuntimeException exception = new RuntimeException(); - handler.handleCacheGetError(exception, new NoOpCache("NOOP"), "key"); - verify(this.logger).warn("Cache 'NOOP' failed to get entry with key 'key'", exception); + this.handler.handleCacheGetError(exception, CACHE, KEY); + verify(this.logger).warn("Cache 'NOOP' failed to get entry with key 'enigma'", exception); } }