From 251c3dc5e88337e811c57505d55120bc280b230d Mon Sep 17 00:00:00 2001
From: Martin Lowe <martin.lowe@eclipse-foundation.org>
Date: Thu, 7 Sep 2023 15:22:18 -0400
Subject: [PATCH] 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.
---
.../service/impl/QuarkusCachingService.java | 27 ++++---
.../DefaultQuarkusCachingServiceTest.java | 80 +++++++++++++++++++
2 files changed, 98 insertions(+), 9 deletions(-)
diff --git a/core/src/main/java/org/eclipsefoundation/core/service/impl/QuarkusCachingService.java b/core/src/main/java/org/eclipsefoundation/core/service/impl/QuarkusCachingService.java
index 4229cbd2..c1f40407 100644
--- a/core/src/main/java/org/eclipsefoundation/core/service/impl/QuarkusCachingService.java
+++ b/core/src/main/java/org/eclipsefoundation/core/service/impl/QuarkusCachingService.java
@@ -53,7 +53,7 @@ import io.quarkus.cache.CaffeineCache;
@ApplicationScoped
public class QuarkusCachingService implements CachingService {
private static final Logger LOGGER = LoggerFactory.getLogger(QuarkusCachingService.class);
-
+
@ConfigProperty(name = MicroprofilePropertyNames.CACHE_TTL_MAX_SECONDS, defaultValue = "900")
long ttlWrite;
@@ -67,13 +67,8 @@ public class QuarkusCachingService implements CachingService {
@Override
public <T> Optional<T> get(String id, MultivaluedMap<String, String> params, Class<?> rawType, Callable<? extends T> callable) {
Objects.requireNonNull(callable);
-
- ParameterizedCacheKey cacheKey = ParameterizedCacheKey
- .builder()
- .setClazz(rawType)
- .setId(id)
- .setParams(params != null ? params : new MultivaluedMapImpl<>())
- .build();
+ // create the cache key for the entry, cloning the map for key integrity
+ ParameterizedCacheKey cacheKey = ParameterizedCacheKey.builder().setClazz(rawType).setId(id).setParams(cloneMap(params)).build();
LOGGER.debug("Retrieving cache value for '{}'", cacheKey);
return Optional.ofNullable(get(cacheKey, callable));
}
@@ -167,7 +162,7 @@ public class QuarkusCachingService implements CachingService {
}
@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
T out = null;
try {
@@ -189,4 +184,18 @@ public class QuarkusCachingService implements CachingService {
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;
+ }
+
}
diff --git a/core/src/test/java/org/eclipsefoundation/core/service/impl/DefaultQuarkusCachingServiceTest.java b/core/src/test/java/org/eclipsefoundation/core/service/impl/DefaultQuarkusCachingServiceTest.java
index 2caf9b8f..222704d6 100644
--- a/core/src/test/java/org/eclipsefoundation/core/service/impl/DefaultQuarkusCachingServiceTest.java
+++ b/core/src/test/java/org/eclipsefoundation/core/service/impl/DefaultQuarkusCachingServiceTest.java
@@ -250,6 +250,86 @@ class DefaultQuarkusCachingServiceTest {
"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
*/
--
GitLab