Skip to content
Snippets Groups Projects
Commit 251c3dc5 authored by Martin Lowe's avatar Martin Lowe :flag_ca:
Browse files

Fix issue with cache param map leaks, add fuzzy remove tests

Param maps weren't being properly cloned before being used as the keys,
meaning that the key could be modified after being used. This led to
some weird behaviour where keys would duplicate themselves.

Additionally added some fuzzy key removal tests to validate the
functionality as there were some issues in a downstream project that
used the call. There were no issues in code for fuzzyRemove, so it was
left unchanged.
parent 5d23d77e
No related branches found
No related tags found
1 merge request!137Fix issue with cache param map leaks, add fuzzy remove tests
Pipeline #31434 passed
...@@ -53,7 +53,7 @@ import io.quarkus.cache.CaffeineCache; ...@@ -53,7 +53,7 @@ import io.quarkus.cache.CaffeineCache;
@ApplicationScoped @ApplicationScoped
public class QuarkusCachingService implements CachingService { public class QuarkusCachingService implements CachingService {
private static final Logger LOGGER = LoggerFactory.getLogger(QuarkusCachingService.class); private static final Logger LOGGER = LoggerFactory.getLogger(QuarkusCachingService.class);
@ConfigProperty(name = MicroprofilePropertyNames.CACHE_TTL_MAX_SECONDS, defaultValue = "900") @ConfigProperty(name = MicroprofilePropertyNames.CACHE_TTL_MAX_SECONDS, defaultValue = "900")
long ttlWrite; long ttlWrite;
...@@ -67,13 +67,8 @@ public class QuarkusCachingService implements CachingService { ...@@ -67,13 +67,8 @@ public class QuarkusCachingService implements CachingService {
@Override @Override
public <T> Optional<T> get(String id, MultivaluedMap<String, String> params, Class<?> rawType, Callable<? extends T> callable) { public <T> Optional<T> get(String id, MultivaluedMap<String, String> params, Class<?> rawType, Callable<? extends T> callable) {
Objects.requireNonNull(callable); Objects.requireNonNull(callable);
// create the cache key for the entry, cloning the map for key integrity
ParameterizedCacheKey cacheKey = ParameterizedCacheKey ParameterizedCacheKey cacheKey = ParameterizedCacheKey.builder().setClazz(rawType).setId(id).setParams(cloneMap(params)).build();
.builder()
.setClazz(rawType)
.setId(id)
.setParams(params != null ? params : new MultivaluedMapImpl<>())
.build();
LOGGER.debug("Retrieving cache value for '{}'", cacheKey); LOGGER.debug("Retrieving cache value for '{}'", cacheKey);
return Optional.ofNullable(get(cacheKey, callable)); return Optional.ofNullable(get(cacheKey, callable));
} }
...@@ -167,7 +162,7 @@ public class QuarkusCachingService implements CachingService { ...@@ -167,7 +162,7 @@ public class QuarkusCachingService implements CachingService {
} }
@CacheResult(cacheName = "default") @CacheResult(cacheName = "default")
public <T> T get(@CacheKey ParameterizedCacheKey cacheKey, Callable<? extends T> callable) { <T> T get(@CacheKey ParameterizedCacheKey cacheKey, Callable<? extends T> callable) {
// attempt to get the result for the cache entry // attempt to get the result for the cache entry
T out = null; T out = null;
try { try {
...@@ -189,4 +184,18 @@ public class QuarkusCachingService implements CachingService { ...@@ -189,4 +184,18 @@ public class QuarkusCachingService implements CachingService {
return System.currentTimeMillis() + getMaxAge(); return System.currentTimeMillis() + getMaxAge();
} }
/**
* To prevent modification of maps/cache key entries post retrieval, we should be referencing copies of the maps rather
* than the direct passed reference for safety.
*
* @return the copied map, or an empty map if null
*/
private MultivaluedMap<String, String> cloneMap(MultivaluedMap<String, String> paramMap) {
MultivaluedMap<String, String> out = new MultivaluedMapImpl<>();
if (paramMap != null) {
paramMap.entrySet().forEach(e -> out.addAll(e.getKey(), e.getValue()));
}
return out;
}
} }
...@@ -250,6 +250,86 @@ class DefaultQuarkusCachingServiceTest { ...@@ -250,6 +250,86 @@ class DefaultQuarkusCachingServiceTest {
"Unknown key should not have been returned by getCacheKey, but was returned"); "Unknown key should not have been returned by getCacheKey, but was returned");
} }
/*
* fuzzyRemove(id, type)
*/
@Test
void fuzzyRemove_success() {
// generate a random key
String id = UUID.randomUUID().toString();
// create empty map for initial post
MultivaluedMap<String, String> params = new MultivaluedMapImpl<>();
// check that we have no match for ID before proceeding
Assertions.assertFalse(lookupCacheKeyUsingMainCache(id, Optional.empty()), "Expected random UUID key to be empty, but found value");
// request + put the data in cache and check it's there
svc.get(id, params, String.class, () -> "sample");
Assertions
.assertTrue(lookupCacheKeyUsingMainCache(id, Optional.empty()),
"Expected new cache value to have a key present in cache key set with empty map");
// update the map and create another entry
params = new MultivaluedMapImpl<>();
params.add(DefaultUrlParameterNames.IDS_PARAMETER_NAME, id);
Assertions
.assertFalse(lookupCacheKeyUsingMainCache(id, Optional.of(params)),
"Expected random UUID key to be empty, but found value");
// request + put the data in cache and check it's there
svc.get(id, params, String.class, () -> "sample");
Assertions
.assertTrue(lookupCacheKeyUsingMainCache(id, Optional.of(params)),
"Expected new cache value to have a key present in cache key set with empty map");
// do the fuzzy remove operation
svc.fuzzyRemove(id, String.class);
// check that both of the values are gone
Assertions
.assertFalse(lookupCacheKeyUsingMainCache(id, Optional.empty()),
"Expected random UUID key to be empty after fuzzy remove, but found value");
Assertions
.assertFalse(lookupCacheKeyUsingMainCache(id, Optional.of(params)),
"Expected random UUID key + params to be empty after fuzzy remove, but found value");
}
@Test
void fuzzyRemove_success_onlyRemovesForSameType() {
// generate a random key
String id = UUID.randomUUID().toString();
// create empty map for initial post
MultivaluedMap<String, String> params = new MultivaluedMapImpl<>();
// check that we have no match for ID before proceeding
Assertions
.assertFalse(lookupCacheKeyUsingRawCache(id, Optional.of(params), Optional.of(String.class), cache),
"Expected random UUID key to be empty, but found value");
// request + put the data in cache and check it's there
svc.get(id, params, String.class, () -> "sample");
Assertions
.assertTrue(lookupCacheKeyUsingRawCache(id, Optional.of(params), Optional.of(String.class), cache),
"Expected new cache value to have a key present in cache key set with empty map");
// create another entry with the same ID for a different type
Assertions
.assertFalse(lookupCacheKeyUsingRawCache(id, Optional.of(params), Optional.of(Integer.class), cache),
"Expected random UUID key to be empty, but found value");
// request + put the data in cache and check it's there
svc.get(id, params, Integer.class, () -> 1);
Assertions
.assertTrue(lookupCacheKeyUsingRawCache(id, Optional.of(params), Optional.of(Integer.class), cache),
"Expected new cache value to have a key present in cache key set with empty map");
// do the fuzzy remove operation
svc.fuzzyRemove(id, String.class);
// check that only the targeted type is removed
Assertions
.assertFalse(lookupCacheKeyUsingRawCache(id, Optional.of(params), Optional.of(String.class), cache),
"Expected random UUID key to be empty after fuzzy remove, but found value");
Assertions
.assertTrue(lookupCacheKeyUsingRawCache(id, Optional.of(params), Optional.of(Integer.class), cache),
"Expected random UUID key + params to be empty after fuzzy remove, but found value");
}
/* /*
* remove(key) tests * remove(key) tests
*/ */
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment