From 6a6c7df824675476a0f7b5bed80c503a9b279c01 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Wed, 26 Jan 2022 14:08:17 +0100 Subject: [PATCH] Polish "Add CacheErrorHandler implementation that logs exceptions" See gh-27826 --- .../interceptor/LoggingCacheErrorHandler.java | 98 +++++++++---------- .../LoggingCacheErrorHandlerTests.java | 59 ++++++----- 2 files changed, 82 insertions(+), 75 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 d1fefcafa4..e2f6c33127 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-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. @@ -24,88 +24,82 @@ import org.springframework.lang.Nullable; import org.springframework.util.Assert; /** - * A {@link CacheErrorHandler} implementation that simply logs error message. + * A {@link CacheErrorHandler} implementation that logs error message. Can be + * used when underlying cache errors should be ignored. * * @author Adam Ostrožlík - * @since 5.3 + * @author Stephane Nicoll + * @since 5.3.16 */ -public final class LoggingCacheErrorHandler implements CacheErrorHandler { +public class LoggingCacheErrorHandler implements CacheErrorHandler { private final Log logger; - private final boolean includeStacktrace; + + private final boolean logStacktrace; + /** - * Construct new {@link LoggingCacheErrorHandler} that may log stacktraces. - * @param includeStacktrace whether to log or not log stacktraces. - * @param logger custom logger. + * Create an instance with the {@link Log logger} to use. + * @param logger the logger to use + * @param logStacktrace whether to log stack trace */ - private LoggingCacheErrorHandler(boolean includeStacktrace, Log logger) { - Assert.notNull(logger, "logger cannot be null"); - this.includeStacktrace = includeStacktrace; + public LoggingCacheErrorHandler(Log logger, boolean logStacktrace) { + Assert.notNull(logger, "Logger must not be null"); this.logger = logger; + this.logStacktrace = logStacktrace; } + /** + * Create an instance that does not log stack traces. + */ + public LoggingCacheErrorHandler() { + this(LogFactory.getLog(LoggingCacheErrorHandler.class), false); + } + + @Override public void handleCacheGetError(RuntimeException exception, Cache cache, Object key) { - logCacheError("Cache '" + cache.getName() + "' failed to get entry with key '" + key + "'", exception); + logCacheError(logger, + createMessage(cache, "failed to get entry with key '" + key + "'"), + exception); } @Override public void handleCachePutError(RuntimeException exception, Cache cache, Object key, @Nullable Object value) { - logCacheError("Cache '" + cache.getName() + "' failed to put entry with key '" + key + "'", exception); + logCacheError(logger, + createMessage(cache, "failed to put entry with key '" + key + "'"), + exception); } @Override public void handleCacheEvictError(RuntimeException exception, Cache cache, Object key) { - logCacheError("Cache '" + cache.getName() + "' failed to evict entry with key '" + key + "'", exception); + logCacheError(logger, + createMessage(cache, "failed to evict entry with key '" + key + "'"), + exception); } @Override public void handleCacheClearError(RuntimeException exception, Cache cache) { - logCacheError("Cache '" + cache.getName() + "' failed to clear itself", exception); - } - - private void logCacheError(String msg, RuntimeException ex) { - if (this.includeStacktrace) { - logger.warn(msg, ex); - } - else { - logger.warn(msg); - } + logCacheError(logger, createMessage(cache, "failed to clear entries"), exception); } /** - * Builder class for {@link LoggingCacheErrorHandler}. + * Log the specified message. + * @param logger the logger + * @param message the message + * @param ex the exception */ - public static class Builder { - private Log logger; - private boolean includeStacktrace; - - /** - * Overrides default logger. - * @param logger new logger. - * @return this builder. - */ - public Builder setLogger(Log logger) { - this.logger = logger; - return this; + protected void logCacheError(Log logger, String message, RuntimeException ex) { + if (this.logStacktrace) { + logger.warn(message, ex); } - - /** - * Enable/disable logging of stacktraces. - * @param includeStacktrace true - include stacktraces; false otherwise. - * @return this builder. - */ - public Builder setIncludeStacktrace(boolean includeStacktrace) { - this.includeStacktrace = includeStacktrace; - return this; + else { + logger.warn(message); } + } - public LoggingCacheErrorHandler build() { - if (logger == null) { - return new LoggingCacheErrorHandler(this.includeStacktrace, LogFactory.getLog(LoggingCacheErrorHandler.class)); - } - return new LoggingCacheErrorHandler(this.includeStacktrace, logger); - } + private String createMessage(Cache cache, String reason) { + return String.format("Cache '%s' %s", cache.getName(), reason); } + } 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 f7618be9f4..39525d715a 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-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. @@ -18,46 +18,59 @@ package org.springframework.cache.interceptor; import org.apache.commons.logging.Log; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mock; -import org.mockito.Mockito; -import org.mockito.junit.jupiter.MockitoExtension; + import org.springframework.cache.support.NoOpCache; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.times; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; /** * Tests for {@link LoggingCacheErrorHandler}. * * @author Adam Ostrožlík + * @author Stephane Nicoll */ -@ExtendWith(MockitoExtension.class) public class LoggingCacheErrorHandlerTests { - @Mock - Log logger; - @Test - void givenHandlerWhenHandleGetErrorThenLogWithoutException() { - LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler.Builder() - .setLogger(logger) - .build(); + void handleGetCacheErrorLogsAppropriateMessage() { + Log logger = mock(Log.class); + LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler(logger, false); handler.handleCacheGetError(new RuntimeException(), new NoOpCache("NOOP"), "key"); - verify(logger, times(1)).warn(anyString()); + verify(logger).warn("Cache 'NOOP' failed to get entry with key 'key'"); + } + + @Test + void handlePutCacheErrorLogsAppropriateMessage() { + Log logger = mock(Log.class); + LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler(logger, false); + handler.handleCachePutError(new RuntimeException(), new NoOpCache("NOOP"), "key", new Object()); + verify(logger).warn("Cache 'NOOP' failed to put entry with key 'key'"); + } + + @Test + void handleEvictCacheErrorLogsAppropriateMessage() { + Log logger = mock(Log.class); + LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler(logger, false); + handler.handleCacheEvictError(new RuntimeException(), new NoOpCache("NOOP"), "key"); + verify(logger).warn("Cache 'NOOP' failed to evict entry with key 'key'"); + } + + @Test + void handleClearErrorLogsAppropriateMessage() { + Log logger = mock(Log.class); + LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler(logger, false); + handler.handleCacheClearError(new RuntimeException(), new NoOpCache("NOOP")); + verify(logger).warn("Cache 'NOOP' failed to clear entries"); } @Test - void givenHandlerWhenHandleGetErrorThenLogWithException() { - LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler.Builder() - .setLogger(logger) - .setIncludeStacktrace(true) - .build(); + void handleCacheErrorWithStacktrace() { + Log logger = mock(Log.class); + LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler(logger, true); RuntimeException exception = new RuntimeException(); handler.handleCacheGetError(exception, new NoOpCache("NOOP"), "key"); - verify(logger, times(1)).warn(anyString(), eq(exception)); + verify(logger).warn("Cache 'NOOP' failed to get entry with key 'key'", exception); } }