From 5c9fbcc23c59ae5b6f8f067198d8050a747a5a2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ostro=C5=BEl=C3=ADk?= Date: Thu, 16 Dec 2021 11:15:23 +0100 Subject: [PATCH 1/2] Add CacheErrorHandler implementation that logs exceptions See gh-27826 --- .../interceptor/LoggingCacheErrorHandler.java | 111 ++++++++++++++++++ .../LoggingCacheErrorHandlerTests.java | 63 ++++++++++ 2 files changed, 174 insertions(+) create mode 100644 spring-context/src/main/java/org/springframework/cache/interceptor/LoggingCacheErrorHandler.java create mode 100644 spring-context/src/test/java/org/springframework/cache/interceptor/LoggingCacheErrorHandlerTests.java 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 new file mode 100644 index 0000000000..d1fefcafa4 --- /dev/null +++ b/spring-context/src/main/java/org/springframework/cache/interceptor/LoggingCacheErrorHandler.java @@ -0,0 +1,111 @@ +/* + * Copyright 2002-2021 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.cache.interceptor; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +import org.springframework.cache.Cache; +import org.springframework.lang.Nullable; +import org.springframework.util.Assert; + +/** + * A {@link CacheErrorHandler} implementation that simply logs error message. + * + * @author Adam Ostrožlík + * @since 5.3 + */ +public final class LoggingCacheErrorHandler implements CacheErrorHandler { + + private final Log logger; + private final boolean includeStacktrace; + + /** + * Construct new {@link LoggingCacheErrorHandler} that may log stacktraces. + * @param includeStacktrace whether to log or not log stacktraces. + * @param logger custom logger. + */ + private LoggingCacheErrorHandler(boolean includeStacktrace, Log logger) { + Assert.notNull(logger, "logger cannot be null"); + this.includeStacktrace = includeStacktrace; + this.logger = logger; + } + + @Override + public void handleCacheGetError(RuntimeException exception, Cache cache, Object key) { + logCacheError("Cache '" + cache.getName() + "' 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); + } + + @Override + public void handleCacheEvictError(RuntimeException exception, Cache cache, Object key) { + logCacheError("Cache '" + cache.getName() + "' 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); + } + } + + /** + * Builder class for {@link LoggingCacheErrorHandler}. + */ + 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; + } + + /** + * 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; + } + + public LoggingCacheErrorHandler build() { + if (logger == null) { + return new LoggingCacheErrorHandler(this.includeStacktrace, LogFactory.getLog(LoggingCacheErrorHandler.class)); + } + return new LoggingCacheErrorHandler(this.includeStacktrace, logger); + } + } +} 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 new file mode 100644 index 0000000000..f7618be9f4 --- /dev/null +++ b/spring-context/src/test/java/org/springframework/cache/interceptor/LoggingCacheErrorHandlerTests.java @@ -0,0 +1,63 @@ +/* + * Copyright 2002-2021 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.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.verify; + +/** + * Tests for {@link LoggingCacheErrorHandler}. + * + * @author Adam Ostrožlík + */ +@ExtendWith(MockitoExtension.class) +public class LoggingCacheErrorHandlerTests { + + @Mock + Log logger; + + @Test + void givenHandlerWhenHandleGetErrorThenLogWithoutException() { + LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler.Builder() + .setLogger(logger) + .build(); + handler.handleCacheGetError(new RuntimeException(), new NoOpCache("NOOP"), "key"); + verify(logger, times(1)).warn(anyString()); + } + + @Test + void givenHandlerWhenHandleGetErrorThenLogWithException() { + LoggingCacheErrorHandler handler = new LoggingCacheErrorHandler.Builder() + .setLogger(logger) + .setIncludeStacktrace(true) + .build(); + RuntimeException exception = new RuntimeException(); + handler.handleCacheGetError(exception, new NoOpCache("NOOP"), "key"); + verify(logger, times(1)).warn(anyString(), eq(exception)); + } + +} From 6a6c7df824675476a0f7b5bed80c503a9b279c01 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Wed, 26 Jan 2022 14:08:17 +0100 Subject: [PATCH 2/2] 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); } }