The automatic configuration of a cache is amazing for the simple cases, however quickly becomes weird if you decide to use multiple CacheManager instances.
For example you might want to have a Distributed Cache using Redis, and a Local Caffeine or ConcurrentMap Cache for small items, you'd need the following configuration.
@Configuration
@EnableCaching
public CachingConfig implements CachingConfigurer, ApplicationContextAware {
private ApplicationContext ac;
@Bean
public CacheManager redisCacheManager(RedisConnectionFactory factory, CacheProperties props) {
// Implementing Redis Cache
}
@Bean
public CacheManager caffeineCache(CacheProperties cacheProperties) {
// Implementing Caffeine Cache
}
@Override
@Bean
public CacheResolver cacheResolver() {
var redisCache = ac.getBean("redisCacheManager", CacheManager.class);
var caffeineCache = ac.getBean("caffeineCache", CacheManager.class);
// Implement Resolver
}
@Override
public KeyGenerator keyGenerator() {
return new SimpleKeyGenerator();
}
@Override
public void setApplicationContext(ApplicationContext applicationContext) throws BeansException {
this.ac= applicationContext;
}
}
From a user perspective there are some parts that are a little verbose compared to other configurations. Highlights would be bringing in the ApplicationContext, but the class really doesn't care for it, implementing the KeyGenerator using the same default Spring does (per CachingConfigurer Docs), and then not overriding CachingConfigurer::cacheManager() for a mixture of reasons.
I personally I think Caching could use a more "Default unless Declared" approach, where we could reduce this down to the following.
@Configuration
@EnableCaching
public CachingConfig {
@Bean
public CacheManager redisCacheManager(RedisConnectionFactory factory, CacheProperties props) {
// Implementing Redis Cache
}
@Bean
public CacheManager caffeineCache(CacheProperties cacheProperties) {
// Implementing Caffeine Cache
}
@Bean
public CacheResolver cacheResolver(
@Qualifier("redisCacheManager") CacheManager redis,
@Qualifier("caffeineCache") CacheManager caffeine
) {
// Implement Resolver
}
}
This removes the KeyGenerator, swaps ApplicationContext::getBean with Parameter Injection, and removes the CachingConfigurer in favor of global bean detection. What other adjustments would the Spring Boot Team be open to?
Comment From: snicoll
The automatic configuration of a cache is amazing for the simple cases, however quickly becomes weird if you decide to use multiple CacheManager instances.
It's a common topic in Spring Boot where users enjoy the automated behavior for configuring the infrastructure but it doesn't expand to multiple instances of the same type, see https://github.com/spring-projects/spring-boot/issues/15732.
From a user perspective there are some parts that are a little verbose compared to other configurations. Highlights would be bringing in the ApplicationContext, but the class really doesn't care for it, implementing the KeyGenerator using the same default Spring does (per CachingConfigurer Docs), and then not overriding CachingConfigurer::cacheManager() for a mixture of reasons.
There's a mix of things here. First of all, you don't have to override keyGenerator. Then a CacheResolver is all that's required to give a cache based on a name. From the pure perspective of the cache infrastructure, a cache resolver is all that's required. You can override cacheManager if you want but it will be superseded (per javadoc).
The problem with that stuff is that the infrastructure has to configure early, to be able to wrap things correctly. In your example above, the two cache managers don't need to be technically bean (albeit declaring them as such makes sure that callbacks are honored, typically on shutdown). Having any infrastructure declaring such types is a call for early initialization I am afraid.
I can see how the contract of the contributor makes it harder than it should.
I wonder if the following would work:
@Configuration
class CacheManagersConfig {
@Bean
public CacheManager redisCacheManager(RedisConnectionFactory factory, CacheProperties props) {
// Implementing Redis Cache
}
@Bean
public CacheManager caffeineCache(CacheProperties cacheProperties) {
// Implementing Caffeine Cache
}
}
@Configuration
@Import(CacheManagersConfig.class)
@EnableCaching
class CachingConfig implements CachingConfigurer {
private final ObjectProvider<CacheManager> cacheManagers;
CachingConfig(ObjectProvider<CacheManager> cacheManagers) { ... }
@Override
@Bean
public CacheResolver cacheResolver() {
// Implement Resolver
}
}
(note I've used ObjectProvider to be concise but you can do the qualifier injection as well).
Comment From: Crain-32
First of all, you don't have to override keyGenerator
The @EnableCaching Javadoc could use a minor update then.
@EnableCachingwill configure Spring's SimpleKeyGenerator for this purpose, but when implementing CachingConfigurer, a key generator must be provided explicitly.
Threw me off, even though the next line does state
Return
nullor new SimpleKeyGenerator() from this method if no customization is necessary
Returning null feels incorrect when stating a key generator must be provided explicitly. Maybe the function can be changed to the following?
public interface CachingConfigurer {
@Nullable
default KeyGenerator keyGenerator() { return new SimpleKeyGenerator(); }
}
Although now that conflicts with the @Bean Requirement.
Going back to the CacheResolver and some thoughts on it,
From the pure perspective of the cache infrastructure, a cache resolver is all that's required
This is reminding me a bit of the following Spring Security Issue. Although a CacheResolver is all that is required, the current documentation points towards using CacheManager, and a CacheManager is really a CacheResolver that only uses the Operation's Cache Name to resolve, along with a helper function.
That approach makes sense with this section's note in Spring Boot Docs,
Similarly to key and keyGenerator, the cacheManager and cacheResolver parameters are mutually exclusive.
Maybe this adjust should be made instead?
public interface CacheManager {
Collection<? extends Cache> resolveCaches(CacheOperationInvovationContext<?> context);
Collection<String> getCacheNames();
}
Then using the ConcurrentMapCacheManager as an example.
public class ConcurrentMapCacheManager implements CacheManager, BeanClassLoaderAware {
@Override
public Collection<? extends Cache> resolveCaches(CacheOperationInvovationContext<?> context) {
Collection<String> cacheNames = context.getOperation().getCacheNames();
// Unsure if the empty collection check should be in here or the caller
return cacheNames.stream().map(this::getCache).filter(Objects::nonNull).toList();
}
@Nullable
private Cache getCache(String name) {
Cache cache = this.cacheMap.get(name);
if (cache == null && this.dynamic) {
cache = this.cacheMap.computeIfAbsent(name, this::createConcurrentMapCache);
}
return cache;
}
}
This feels easier to work with, since there is a single contract to work with. Power users could still used named CacheManager instances as well for special handling.
Comment From: snicoll
Can we focus on the issue as reported please? I'll have a look to the Javadoc and see what needs to be updated but I think I've provided a fairly good route for what you claim is a problem. Please give that a try and report if that works for you.
Comment From: Crain-32
Sorry about that. To make things worse I also realized while writing the reply this was Spring Framework's Repository, not Spring Boot's 🤦🏻♂️. Since the behavior falls into the Cache Auto Configuration, not the Cache Configuration, I'm going to review the CacheAutoConfiguration some more and move the CacheResolver bean only working in a CachingConfigurer implementation to Boot, as that likely stems from there. I'll pop back up in with another issue once I feel like I really understand the CacheResolver vs CacheManager
Your solution does work, thank you.
Comment From: snicoll
Javadoc has been polished in #33288. The requirement mismatch for KeyGenerator is because that part of the doc was written before we could use default methods in interfaces.