From bbcc788f609098f3dd22750718b446b122188ae1 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Tue, 18 Jul 2023 22:02:09 +0200 Subject: [PATCH] Decouple exception messages for sync=true from @Cacheable --- .../cache/annotation/Cacheable.java | 8 ++-- .../cache/interceptor/CacheAspectSupport.java | 35 +++++++++--------- .../interceptor/CacheSyncFailureTests.java | 37 ++++++++++--------- 3 files changed, 41 insertions(+), 39 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/cache/annotation/Cacheable.java b/spring-context/src/main/java/org/springframework/cache/annotation/Cacheable.java index 51ea3ec2583..a207d1f0609 100644 --- a/spring-context/src/main/java/org/springframework/cache/annotation/Cacheable.java +++ b/spring-context/src/main/java/org/springframework/cache/annotation/Cacheable.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 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. @@ -177,9 +177,9 @@ public @interface Cacheable { *
  • Only one cache may be specified
    • *
  • No other cache-related operation can be combined
    • * - * This is effectively a hint and the actual cache provider that you are - * using may not support it in a synchronized fashion. Check your provider - * documentation for more details on the actual semantics. + * This is effectively a hint and the chosen cache provider might not actually + * support it in a synchronized fashion. Check your provider documentation for + * more details on the actual semantics. * @since 4.3 * @see org.springframework.cache.Cache#get(Object, Callable) */ diff --git a/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java b/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java index 572d3295db2..20bcb55dc6a 100644 --- a/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java +++ b/spring-context/src/main/java/org/springframework/cache/interceptor/CacheAspectSupport.java @@ -214,7 +214,7 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker @Override public void afterSingletonsInstantiated() { if (getCacheResolver() == null) { - // Lazily initialize cache resolver via default cache manager... + // Lazily initialize cache resolver via default cache manager Assert.state(this.beanFactory != null, "CacheResolver or BeanFactory must be set on cache aspect"); try { setCacheManager(this.beanFactory.getBean(CacheManager.class)); @@ -307,22 +307,22 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker } /** - * Return a bean with the specified name and type. Used to resolve services that - * are referenced by name in a {@link CacheOperation}. - * @param beanName the name of the bean, as defined by the operation - * @param expectedType type for the bean - * @return the bean matching that name + * Retrieve a bean with the specified name and type. + * Used to resolve services that are referenced by name in a {@link CacheOperation}. + * @param name the name of the bean, as defined by the cache operation + * @param serviceType the type expected by the operation's service reference + * @return the bean matching the expected type, qualified by the given name * @throws org.springframework.beans.factory.NoSuchBeanDefinitionException if such bean does not exist * @see CacheOperation#getKeyGenerator() * @see CacheOperation#getCacheManager() * @see CacheOperation#getCacheResolver() */ - protected T getBean(String beanName, Class expectedType) { + protected T getBean(String name, Class serviceType) { if (this.beanFactory == null) { throw new IllegalStateException( - "BeanFactory must be set on cache aspect for " + expectedType.getSimpleName() + " retrieval"); + "BeanFactory must be set on cache aspect for " + serviceType.getSimpleName() + " retrieval"); } - return BeanFactoryAnnotationUtils.qualifiedBeanOfType(this.beanFactory, expectedType, beanName); + return BeanFactoryAnnotationUtils.qualifiedBeanOfType(this.beanFactory, serviceType, name); } /** @@ -388,12 +388,11 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker } } else { - // No caching required, only call the underlying method + // No caching required, just call the underlying method return invokeOperation(invoker); } } - // Process any early evictions processCacheEvicts(contexts.get(CacheEvictOperation.class), true, CacheOperationExpressionEvaluator.NO_RESULT); @@ -641,21 +640,21 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker if (syncEnabled) { if (this.contexts.size() > 1) { throw new IllegalStateException( - "@Cacheable(sync=true) cannot be combined with other cache operations on '" + method + "'"); + "A sync=true operation cannot be combined with other cache operations on '" + method + "'"); } if (cacheOperationContexts.size() > 1) { throw new IllegalStateException( - "Only one @Cacheable(sync=true) entry is allowed on '" + method + "'"); + "Only one sync=true operation is allowed on '" + method + "'"); } CacheOperationContext cacheOperationContext = cacheOperationContexts.iterator().next(); - CacheableOperation operation = (CacheableOperation) cacheOperationContext.getOperation(); + CacheOperation operation = cacheOperationContext.getOperation(); if (cacheOperationContext.getCaches().size() > 1) { throw new IllegalStateException( - "@Cacheable(sync=true) only allows a single cache on '" + operation + "'"); + "A sync=true operation is restricted to a single cache on '" + operation + "'"); } - if (StringUtils.hasText(operation.getUnless())) { + if (operation instanceof CacheableOperation cacheable && StringUtils.hasText(cacheable.getUnless())) { throw new IllegalStateException( - "@Cacheable(sync=true) does not support unless attribute on '" + operation + "'"); + "A sync=true operation does not support the unless attribute on '" + operation + "'"); } return true; } @@ -884,13 +883,13 @@ public abstract class CacheAspectSupport extends AbstractCacheInvoker } } + /** * Internal holder class for recording that a cache method was invoked. */ private static class InvocationAwareResult { boolean invoked; - } } diff --git a/spring-context/src/test/java/org/springframework/cache/interceptor/CacheSyncFailureTests.java b/spring-context/src/test/java/org/springframework/cache/interceptor/CacheSyncFailureTests.java index de4776adae2..56b22970d1a 100644 --- a/spring-context/src/test/java/org/springframework/cache/interceptor/CacheSyncFailureTests.java +++ b/spring-context/src/test/java/org/springframework/cache/interceptor/CacheSyncFailureTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2023 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. @@ -48,8 +48,9 @@ public class CacheSyncFailureTests { private SimpleService simpleService; + @BeforeEach - public void setUp() { + public void setup() { this.context = new AnnotationConfigApplicationContext(Config.class); this.simpleService = this.context.getBean(SimpleService.class); } @@ -61,39 +62,40 @@ public class CacheSyncFailureTests { } } + @Test public void unlessSync() { - assertThatIllegalStateException().isThrownBy(() -> - this.simpleService.unlessSync("key")) - .withMessageContaining("@Cacheable(sync=true) does not support unless attribute"); + assertThatIllegalStateException() + .isThrownBy(() -> this.simpleService.unlessSync("key")) + .withMessageContaining("A sync=true operation does not support the unless attribute"); } @Test public void severalCachesSync() { - assertThatIllegalStateException().isThrownBy(() -> - this.simpleService.severalCachesSync("key")) - .withMessageContaining("@Cacheable(sync=true) only allows a single cache"); + assertThatIllegalStateException() + .isThrownBy(() -> this.simpleService.severalCachesSync("key")) + .withMessageContaining("A sync=true operation is restricted to a single cache"); } @Test public void severalCachesWithResolvedSync() { - assertThatIllegalStateException().isThrownBy(() -> - this.simpleService.severalCachesWithResolvedSync("key")) - .withMessageContaining("@Cacheable(sync=true) only allows a single cache"); + assertThatIllegalStateException() + .isThrownBy(() -> this.simpleService.severalCachesWithResolvedSync("key")) + .withMessageContaining("A sync=true operation is restricted to a single cache"); } @Test public void syncWithAnotherOperation() { - assertThatIllegalStateException().isThrownBy(() -> - this.simpleService.syncWithAnotherOperation("key")) - .withMessageContaining("@Cacheable(sync=true) cannot be combined with other cache operations"); + assertThatIllegalStateException() + .isThrownBy(() -> this.simpleService.syncWithAnotherOperation("key")) + .withMessageContaining("A sync=true operation cannot be combined with other cache operations"); } @Test public void syncWithTwoGetOperations() { - assertThatIllegalStateException().isThrownBy(() -> - this.simpleService.syncWithTwoGetOperations("key")) - .withMessageContaining("Only one @Cacheable(sync=true) entry is allowed"); + assertThatIllegalStateException() + .isThrownBy(() -> this.simpleService.syncWithTwoGetOperations("key")) + .withMessageContaining("Only one sync=true operation is allowed"); } @@ -131,6 +133,7 @@ public class CacheSyncFailureTests { } } + @Configuration @EnableCaching static class Config implements CachingConfigurer {