From e5805ec41cd614ae1682cbeaacb2c6a2f8ea22ce Mon Sep 17 00:00:00 2001 From: Shivaji Byrapaneni Date: Tue, 8 Sep 2026 10:15:15 +0100 Subject: [PATCH 1/4] feat(calm-hub): add generic Caffeine-backed cache service MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracted from #3001. CalmCacheService/CaffeineCacheService is a generic TTL cache with no GitHub types — unused until a later slice wires a consumer. Original-PR: finos/architecture-as-code#3001 --- .../calm/cache/CaffeineCacheService.java | 73 +++++++++ .../finos/calm/cache/CalmCacheService.java | 15 ++ .../cache/TestCaffeineCacheServiceShould.java | 147 ++++++++++++++++++ 3 files changed, 235 insertions(+) create mode 100644 calm-hub/src/main/java/org/finos/calm/cache/CaffeineCacheService.java create mode 100644 calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java create mode 100644 calm-hub/src/test/java/org/finos/calm/cache/TestCaffeineCacheServiceShould.java diff --git a/calm-hub/src/main/java/org/finos/calm/cache/CaffeineCacheService.java b/calm-hub/src/main/java/org/finos/calm/cache/CaffeineCacheService.java new file mode 100644 index 000000000..6f3c523cd --- /dev/null +++ b/calm-hub/src/main/java/org/finos/calm/cache/CaffeineCacheService.java @@ -0,0 +1,73 @@ +package org.finos.calm.cache; + +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.Expiry; +import jakarta.enterprise.context.ApplicationScoped; + +import java.time.Duration; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; + +@ApplicationScoped +public class CaffeineCacheService implements CalmCacheService { + + private final Cache> cache; + + public CaffeineCacheService() { + this.cache = Caffeine.newBuilder() + .maximumSize(10_000) + .expireAfter(new Expiry>() { + @Override + public long expireAfterCreate(String key, CacheEntry value, long currentTime) { + return value.ttl().toNanos(); + } + + @Override + public long expireAfterUpdate(String key, CacheEntry value, long currentTime, long currentDuration) { + return value.ttl().toNanos(); + } + + @Override + public long expireAfterRead(String key, CacheEntry value, long currentTime, long currentDuration) { + return currentDuration; + } + }) + .build(); + } + + @Override + @SuppressWarnings("unchecked") + public Optional get(String key, Class type) { + CacheEntry entry = cache.getIfPresent(key); + if (entry == null) { + return Optional.empty(); + } + if (!type.isInstance(entry.value())) { + return Optional.empty(); + } + return Optional.of((T) entry.value()); + } + + @Override + public void put(String key, T value, Duration ttl) { + if (value == null) { + return; + } + cache.put(key, new CacheEntry<>(value, ttl)); + } + + @Override + public void evict(String key) { + cache.invalidate(key); + } + + @Override + public void evictByPrefix(String prefix) { + ConcurrentMap> map = cache.asMap(); + map.keySet().removeIf(key -> key.startsWith(prefix)); + } + + record CacheEntry(T value, Duration ttl) {} +} diff --git a/calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java b/calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java new file mode 100644 index 000000000..86cfd63f7 --- /dev/null +++ b/calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java @@ -0,0 +1,15 @@ +package org.finos.calm.cache; + +import java.time.Duration; +import java.util.Optional; + +public interface CalmCacheService { + + Optional get(String key, Class type); + + void put(String key, T value, Duration ttl); + + void evict(String key); + + void evictByPrefix(String prefix); +} diff --git a/calm-hub/src/test/java/org/finos/calm/cache/TestCaffeineCacheServiceShould.java b/calm-hub/src/test/java/org/finos/calm/cache/TestCaffeineCacheServiceShould.java new file mode 100644 index 000000000..b604bc8ab --- /dev/null +++ b/calm-hub/src/test/java/org/finos/calm/cache/TestCaffeineCacheServiceShould.java @@ -0,0 +1,147 @@ +package org.finos.calm.cache; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.time.Duration; +import java.util.Optional; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; + +class TestCaffeineCacheServiceShould { + + private CaffeineCacheService cacheService; + + @BeforeEach + void setup() { + cacheService = new CaffeineCacheService(); + } + + @Test + void return_empty_for_missing_key() { + Optional result = cacheService.get("nonexistent", String.class); + assertThat(result.isEmpty(), is(true)); + } + + @Test + void store_and_retrieve_value() { + cacheService.put("key1", "value1", Duration.ofMinutes(5)); + Optional result = cacheService.get("key1", String.class); + assertThat(result.isPresent(), is(true)); + assertThat(result.get(), equalTo("value1")); + } + + @Test + void store_and_retrieve_different_types() { + cacheService.put("string-key", "hello", Duration.ofMinutes(5)); + cacheService.put("int-key", 42, Duration.ofMinutes(5)); + + assertThat(cacheService.get("string-key", String.class).orElse(null), equalTo("hello")); + assertThat(cacheService.get("int-key", Integer.class).orElse(null), equalTo(42)); + } + + @Test + void return_empty_when_type_does_not_match() { + cacheService.put("key", "string-value", Duration.ofMinutes(5)); + Optional result = cacheService.get("key", Integer.class); + assertThat(result.isEmpty(), is(true)); + } + + @Test + void evict_single_key() { + cacheService.put("key1", "value1", Duration.ofMinutes(5)); + cacheService.put("key2", "value2", Duration.ofMinutes(5)); + + cacheService.evict("key1"); + + assertThat(cacheService.get("key1", String.class).isEmpty(), is(true)); + assertThat(cacheService.get("key2", String.class).isPresent(), is(true)); + } + + @Test + void evict_by_prefix() { + cacheService.put("ns:finos:arch:1", "arch1", Duration.ofMinutes(5)); + cacheService.put("ns:finos:arch:2", "arch2", Duration.ofMinutes(5)); + cacheService.put("ns:finos:pattern:1", "pattern1", Duration.ofMinutes(5)); + cacheService.put("ns:other:arch:1", "other-arch1", Duration.ofMinutes(5)); + + cacheService.evictByPrefix("ns:finos:arch:"); + + assertThat(cacheService.get("ns:finos:arch:1", String.class).isEmpty(), is(true)); + assertThat(cacheService.get("ns:finos:arch:2", String.class).isEmpty(), is(true)); + assertThat(cacheService.get("ns:finos:pattern:1", String.class).isPresent(), is(true)); + assertThat(cacheService.get("ns:other:arch:1", String.class).isPresent(), is(true)); + } + + @Test + void expire_entries_after_ttl() throws InterruptedException { + cacheService.put("short-lived", "value", Duration.ofMillis(50)); + + assertThat(cacheService.get("short-lived", String.class).isPresent(), is(true)); + + Thread.sleep(100); + cacheService.put("trigger-cleanup", "x", Duration.ofMinutes(1)); + + Optional result = cacheService.get("short-lived", String.class); + assertThat(result.isEmpty(), is(true)); + } + + @Test + void overwrite_existing_key_with_new_value_and_ttl() { + cacheService.put("key", "original", Duration.ofMinutes(5)); + cacheService.put("key", "updated", Duration.ofMinutes(10)); + + Optional result = cacheService.get("key", String.class); + assertThat(result.isPresent(), is(true)); + assertThat(result.get(), equalTo("updated")); + } + + @Test + void handle_concurrent_access_safely() throws InterruptedException { + int threadCount = 10; + int iterationsPerThread = 100; + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + CountDownLatch latch = new CountDownLatch(threadCount); + + for (int t = 0; t < threadCount; t++) { + final int threadId = t; + executor.submit(() -> { + try { + for (int i = 0; i < iterationsPerThread; i++) { + String key = "thread-" + threadId + "-key-" + i; + cacheService.put(key, "value-" + i, Duration.ofMinutes(5)); + cacheService.get(key, String.class); + } + } finally { + latch.countDown(); + } + }); + } + + boolean completed = latch.await(10, TimeUnit.SECONDS); + executor.shutdown(); + assertThat(completed, is(true)); + } + + @Test + void evict_by_prefix_when_no_keys_match() { + cacheService.put("other:key", "value", Duration.ofMinutes(5)); + + cacheService.evictByPrefix("nonexistent:"); + + assertThat(cacheService.get("other:key", String.class).isPresent(), is(true)); + } + + @Test + void handle_null_value_gracefully() { + cacheService.put("null-key", null, Duration.ofMinutes(5)); + Optional result = cacheService.get("null-key", Object.class); + assertThat(result.isEmpty(), is(true)); + } +} From 444bb5c1b42d7f52a4e61c3f3feaae700e051170 Mon Sep 17 00:00:00 2001 From: James Gough Date: Wed, 9 Sep 2026 09:15:37 +0100 Subject: [PATCH 2/4] fix(calm-hub): address review findings on the cache service slice - Collapse CalmCacheService/CaffeineCacheService into a single concrete CalmCacheService bean. calm-hub puts an interface in front of a service only where multiple backends are selected at runtime (see store/ and its Mongo/Nitrite producers); a cache with one implementation and no near-term second one doesn't fit that pattern. - Add getList(key, elementType), so the one known consumer (GitHubVersionService, landing in slice 5) doesn't need get(key, List.class) plus an unchecked cast to use a typed list. - Document the class contract: null values are ignored on put, a type mismatch on get/getList returns empty rather than throwing, and TTL is not refreshed on read. - Reject a null ttl in put() with a clear NPE at the call site instead of failing later inside Caffeine's Expiry callback. - Make maximumSize configurable via calm.cache.max-size (default 10000, constructor-injected) instead of hardcoded, following the module's @ConfigProperty convention. - Make the Caffeine Ticker injectable via a package-private constructor, matching the LongSupplier pattern already used by SchemaMigrationInProgressFilter, and use it to make TTL expiry tests deterministic instead of Thread.sleep. - Rewrite the concurrent-access test to assert on final cache state via the submitted Futures, instead of only checking a CountDownLatch that a finally-block would trip even if every task had thrown. --- .../calm/cache/CaffeineCacheService.java | 73 --------- .../finos/calm/cache/CalmCacheService.java | 153 +++++++++++++++++- .../src/main/resources/application.properties | 5 + ...d.java => TestCalmCacheServiceShould.java} | 142 ++++++++++++---- 4 files changed, 260 insertions(+), 113 deletions(-) delete mode 100644 calm-hub/src/main/java/org/finos/calm/cache/CaffeineCacheService.java rename calm-hub/src/test/java/org/finos/calm/cache/{TestCaffeineCacheServiceShould.java => TestCalmCacheServiceShould.java} (55%) diff --git a/calm-hub/src/main/java/org/finos/calm/cache/CaffeineCacheService.java b/calm-hub/src/main/java/org/finos/calm/cache/CaffeineCacheService.java deleted file mode 100644 index 6f3c523cd..000000000 --- a/calm-hub/src/main/java/org/finos/calm/cache/CaffeineCacheService.java +++ /dev/null @@ -1,73 +0,0 @@ -package org.finos.calm.cache; - -import com.github.benmanes.caffeine.cache.Cache; -import com.github.benmanes.caffeine.cache.Caffeine; -import com.github.benmanes.caffeine.cache.Expiry; -import jakarta.enterprise.context.ApplicationScoped; - -import java.time.Duration; -import java.util.Optional; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; - -@ApplicationScoped -public class CaffeineCacheService implements CalmCacheService { - - private final Cache> cache; - - public CaffeineCacheService() { - this.cache = Caffeine.newBuilder() - .maximumSize(10_000) - .expireAfter(new Expiry>() { - @Override - public long expireAfterCreate(String key, CacheEntry value, long currentTime) { - return value.ttl().toNanos(); - } - - @Override - public long expireAfterUpdate(String key, CacheEntry value, long currentTime, long currentDuration) { - return value.ttl().toNanos(); - } - - @Override - public long expireAfterRead(String key, CacheEntry value, long currentTime, long currentDuration) { - return currentDuration; - } - }) - .build(); - } - - @Override - @SuppressWarnings("unchecked") - public Optional get(String key, Class type) { - CacheEntry entry = cache.getIfPresent(key); - if (entry == null) { - return Optional.empty(); - } - if (!type.isInstance(entry.value())) { - return Optional.empty(); - } - return Optional.of((T) entry.value()); - } - - @Override - public void put(String key, T value, Duration ttl) { - if (value == null) { - return; - } - cache.put(key, new CacheEntry<>(value, ttl)); - } - - @Override - public void evict(String key) { - cache.invalidate(key); - } - - @Override - public void evictByPrefix(String prefix) { - ConcurrentMap> map = cache.asMap(); - map.keySet().removeIf(key -> key.startsWith(prefix)); - } - - record CacheEntry(T value, Duration ttl) {} -} diff --git a/calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java b/calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java index 86cfd63f7..859da7e23 100644 --- a/calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java +++ b/calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java @@ -1,15 +1,158 @@ package org.finos.calm.cache; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.Expiry; +import com.github.benmanes.caffeine.cache.Ticker; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import org.eclipse.microprofile.config.inject.ConfigProperty; + import java.time.Duration; +import java.util.List; +import java.util.Objects; import java.util.Optional; +import java.util.concurrent.ConcurrentMap; + +/** + * A generic, in-memory TTL cache backed by Caffeine. It has no knowledge of + * what it stores — namespace keys however you like (a `kind:id` prefix, as + * used by {@link #evictByPrefix}) and choose a TTL per entry. + * + *

Each {@link #put} carries its own TTL rather than the cache having one + * fixed lifetime, so a single instance can serve both short-lived and + * effectively-permanent data — e.g. a 5-minute TTL for a version list next to + * a 365-day TTL for immutable content addressed by commit SHA. That per-call + * TTL is why this is a hand-rolled Caffeine cache rather than the + * {@code quarkus-cache} extension: {@code @CacheResult} has one TTL per + * annotated method, not per entry. + * + *

There is deliberately no interface here: this module puts an interface + * in front of a service only where there are genuinely multiple backends + * selected at runtime (see {@code store/} and its Mongo/Nitrite producers). + * A cache abstraction has exactly one implementation today; if a second + * (e.g. a distributed cache) is ever needed, extract an interface then. + * + *

Contract: + *

    + *
  • {@link #get} and {@link #getList} return {@link Optional#empty()} + * both for a missing key and for a value that isn't an instance of the + * requested type — neither ever throws a {@link ClassCastException}.
    • + *
  • {@link #put} silently ignores a {@code null} value: nothing is + * stored, and any existing entry for the key is left untouched. A + * zero or negative {@code ttl} is accepted and expires the entry + * immediately.
    • + *
  • TTL is measured from the most recent {@link #put} for a key; it is + * not refreshed by {@link #get} or {@link #getList} + * — this is a TTL cache, not an LRU with sliding expiry.
    • + *
+ */ +@ApplicationScoped +public class CalmCacheService { + + private final Cache> cache; + + @Inject + public CalmCacheService(@ConfigProperty(name = "calm.cache.max-size", defaultValue = "10000") long maxSize) { + this(maxSize, Ticker.systemTicker()); + } + + // Package-private: lets tests drive expiry deterministically with a fake Ticker + // instead of Thread.sleep, the same pattern used by SchemaMigrationInProgressFilter's + // injectable LongSupplier. + CalmCacheService(long maxSize, Ticker ticker) { + this.cache = Caffeine.newBuilder() + .maximumSize(maxSize) + .ticker(ticker) + .expireAfter(new Expiry>() { + @Override + public long expireAfterCreate(String key, CacheEntry value, long currentTime) { + return value.ttl().toNanos(); + } + + @Override + public long expireAfterUpdate(String key, CacheEntry value, long currentTime, long currentDuration) { + return value.ttl().toNanos(); + } + + @Override + public long expireAfterRead(String key, CacheEntry value, long currentTime, long currentDuration) { + return currentDuration; + } + }) + .build(); + } + + /** + * Reads a single cached value, validating it is an instance of {@code type}. + * + * @return the cached value, or empty if there is no entry for {@code key} + * or its value is not an instance of {@code type} + */ + @SuppressWarnings("unchecked") + public Optional get(String key, Class type) { + CacheEntry entry = cache.getIfPresent(key); + if (entry == null || !type.isInstance(entry.value())) { + return Optional.empty(); + } + return Optional.of((T) entry.value()); + } -public interface CalmCacheService { + /** + * Reads a cached {@link List}, validating every element is an instance of + * {@code elementType}. {@link Class#isInstance} alone cannot express a + * parameterized type such as {@code List}, so this exists + * alongside {@link #get} for list-valued entries — callers that need a + * typed list should use this rather than {@code get(key, List.class)} + * plus an unchecked cast. + * + * @return the cached list, or empty if there is no entry for {@code key}, + * its value isn't a {@link List}, or any element isn't an + * instance of {@code elementType} + */ + @SuppressWarnings("unchecked") + public Optional> getList(String key, Class elementType) { + CacheEntry entry = cache.getIfPresent(key); + if (entry == null || !(entry.value() instanceof List list)) { + return Optional.empty(); + } + for (Object element : list) { + if (!elementType.isInstance(element)) { + return Optional.empty(); + } + } + return Optional.of((List) list); + } - Optional get(String key, Class type); + /** + * Stores {@code value} under {@code key} for {@code ttl}. A {@code null} + * value is silently ignored — nothing is stored and any existing entry + * for {@code key} is left as-is. + */ + public void put(String key, Object value, Duration ttl) { + Objects.requireNonNull(ttl, "ttl must not be null"); + if (value == null) { + return; + } + cache.put(key, new CacheEntry<>(value, ttl)); + } - void put(String key, T value, Duration ttl); + /** + * Removes a single cached entry. A no-op if {@code key} isn't cached. + */ + public void evict(String key) { + cache.invalidate(key); + } - void evict(String key); + /** + * Removes every cached entry whose key starts with {@code prefix} — for + * invalidating a family of related entries (e.g. everything cached for + * one resource) without tracking each key individually. + */ + public void evictByPrefix(String prefix) { + ConcurrentMap> map = cache.asMap(); + map.keySet().removeIf(key -> key.startsWith(prefix)); + } - void evictByPrefix(String prefix); + record CacheEntry(T value, Duration ttl) {} } diff --git a/calm-hub/src/main/resources/application.properties b/calm-hub/src/main/resources/application.properties index 127bd6f21..2c0ecbd5d 100644 --- a/calm-hub/src/main/resources/application.properties +++ b/calm-hub/src/main/resources/application.properties @@ -17,6 +17,11 @@ calm.standalone.seed-demo-data=false # Read-only mode: when true, opens Nitrite with readOnly(true) and rejects mutating HTTP verbs calm.readonly=false +# Maximum number of entries CalmCacheService (org.finos.calm.cache) holds at +# once, evicted LRU-first once exceeded. TTL is set per entry at the call +# site, not here. +calm.cache.max-size=10000 + # Audit logging: independently toggle persistence to the auditLogs store and # emission of a structured log line under the org.finos.calm.audit category. # Both default to true. sourceIp capture defaults to false and has its own diff --git a/calm-hub/src/test/java/org/finos/calm/cache/TestCaffeineCacheServiceShould.java b/calm-hub/src/test/java/org/finos/calm/cache/TestCalmCacheServiceShould.java similarity index 55% rename from calm-hub/src/test/java/org/finos/calm/cache/TestCaffeineCacheServiceShould.java rename to calm-hub/src/test/java/org/finos/calm/cache/TestCalmCacheServiceShould.java index b604bc8ab..87f25b104 100644 --- a/calm-hub/src/test/java/org/finos/calm/cache/TestCaffeineCacheServiceShould.java +++ b/calm-hub/src/test/java/org/finos/calm/cache/TestCalmCacheServiceShould.java @@ -1,26 +1,41 @@ package org.finos.calm.cache; +import com.github.benmanes.caffeine.cache.Ticker; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import java.time.Duration; +import java.util.List; import java.util.Optional; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.stream.IntStream; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertThrows; -class TestCaffeineCacheServiceShould { +class TestCalmCacheServiceShould { - private CaffeineCacheService cacheService; + private static final long MAX_SIZE = 10_000; + + private final FakeTicker ticker = new FakeTicker(); + private CalmCacheService cacheService; @BeforeEach void setup() { - cacheService = new CaffeineCacheService(); + cacheService = new CalmCacheService(MAX_SIZE, ticker); + } + + @Test + void construct_via_the_config_property_constructor() { + CalmCacheService service = new CalmCacheService(MAX_SIZE); + service.put("key", "value", Duration.ofMinutes(1)); + assertThat(service.get("key", String.class).orElse(null), equalTo("value")); } @Test @@ -53,6 +68,31 @@ void return_empty_when_type_does_not_match() { assertThat(result.isEmpty(), is(true)); } + @Test + void store_and_retrieve_a_list() { + cacheService.put("list-key", List.of("a", "b", "c"), Duration.ofMinutes(5)); + Optional> result = cacheService.getList("list-key", String.class); + assertThat(result.isPresent(), is(true)); + assertThat(result.get(), contains("a", "b", "c")); + } + + @Test + void return_empty_from_getList_for_missing_key() { + assertThat(cacheService.getList("nonexistent", String.class).isEmpty(), is(true)); + } + + @Test + void return_empty_from_getList_when_value_is_not_a_list() { + cacheService.put("not-a-list", "just-a-string", Duration.ofMinutes(5)); + assertThat(cacheService.getList("not-a-list", String.class).isEmpty(), is(true)); + } + + @Test + void return_empty_from_getList_when_an_element_type_does_not_match() { + cacheService.put("mixed-list", List.of("a", 2, "c"), Duration.ofMinutes(5)); + assertThat(cacheService.getList("mixed-list", String.class).isEmpty(), is(true)); + } + @Test void evict_single_key() { cacheService.put("key1", "value1", Duration.ofMinutes(5)); @@ -80,16 +120,33 @@ void evict_by_prefix() { } @Test - void expire_entries_after_ttl() throws InterruptedException { - cacheService.put("short-lived", "value", Duration.ofMillis(50)); + void evict_by_prefix_when_no_keys_match() { + cacheService.put("other:key", "value", Duration.ofMinutes(5)); + cacheService.evictByPrefix("nonexistent:"); + + assertThat(cacheService.get("other:key", String.class).isPresent(), is(true)); + } + + @Test + void expire_entries_after_ttl() { + cacheService.put("short-lived", "value", Duration.ofMillis(50)); assertThat(cacheService.get("short-lived", String.class).isPresent(), is(true)); - Thread.sleep(100); - cacheService.put("trigger-cleanup", "x", Duration.ofMinutes(1)); + ticker.advance(Duration.ofMillis(100)); - Optional result = cacheService.get("short-lived", String.class); - assertThat(result.isEmpty(), is(true)); + assertThat(cacheService.get("short-lived", String.class).isEmpty(), is(true)); + } + + @Test + void not_refresh_ttl_on_read() { + cacheService.put("key", "value", Duration.ofMillis(50)); + ticker.advance(Duration.ofMillis(30)); + assertThat(cacheService.get("key", String.class).isPresent(), is(true)); + + ticker.advance(Duration.ofMillis(30)); + + assertThat(cacheService.get("key", String.class).isEmpty(), is(true)); } @Test @@ -103,45 +160,60 @@ void overwrite_existing_key_with_new_value_and_ttl() { } @Test - void handle_concurrent_access_safely() throws InterruptedException { + void handle_null_value_gracefully() { + cacheService.put("null-key", null, Duration.ofMinutes(5)); + Optional result = cacheService.get("null-key", Object.class); + assertThat(result.isEmpty(), is(true)); + } + + @Test + void reject_a_null_ttl() { + assertThrows(NullPointerException.class, () -> cacheService.put("key", "value", null)); + } + + @Test + void handle_concurrent_access_safely() throws Exception { int threadCount = 10; int iterationsPerThread = 100; ExecutorService executor = Executors.newFixedThreadPool(threadCount); - CountDownLatch latch = new CountDownLatch(threadCount); - for (int t = 0; t < threadCount; t++) { - final int threadId = t; - executor.submit(() -> { - try { + List> futures = IntStream.range(0, threadCount) + .>mapToObj(threadId -> executor.submit(() -> { for (int i = 0; i < iterationsPerThread; i++) { String key = "thread-" + threadId + "-key-" + i; cacheService.put(key, "value-" + i, Duration.ofMinutes(5)); cacheService.get(key, String.class); } - } finally { - latch.countDown(); - } - }); - } + })) + .toList(); - boolean completed = latch.await(10, TimeUnit.SECONDS); + for (Future future : futures) { + future.get(10, TimeUnit.SECONDS); + } executor.shutdown(); - assertThat(completed, is(true)); - } - @Test - void evict_by_prefix_when_no_keys_match() { - cacheService.put("other:key", "value", Duration.ofMinutes(5)); + for (int threadId = 0; threadId < threadCount; threadId++) { + for (int i = 0; i < iterationsPerThread; i++) { + String key = "thread-" + threadId + "-key-" + i; + assertThat(cacheService.get(key, String.class).orElse(null), equalTo("value-" + i)); + } + } + } - cacheService.evictByPrefix("nonexistent:"); + /** + * A manually-advanced {@link Ticker} so expiry tests are deterministic + * instead of relying on {@code Thread.sleep}. + */ + private static final class FakeTicker implements Ticker { + private long nanos = 0; - assertThat(cacheService.get("other:key", String.class).isPresent(), is(true)); - } + @Override + public long read() { + return nanos; + } - @Test - void handle_null_value_gracefully() { - cacheService.put("null-key", null, Duration.ofMinutes(5)); - Optional result = cacheService.get("null-key", Object.class); - assertThat(result.isEmpty(), is(true)); + void advance(Duration duration) { + nanos += duration.toNanos(); + } } } From ae9f0afc25f9e6917dc7174db840fdbaf2018d17 Mon Sep 17 00:00:00 2001 From: James Gough Date: Wed, 9 Sep 2026 11:52:07 +0100 Subject: [PATCH 3/4] fix(calm-hub): rescope the cache as GitHub-only, not a calm-wide generic primitive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CalmCacheService was framed as a generic, calm-hub-wide TTL cache (org.finos.calm.cache package, generic get/put/getList/evict API). That's a real trap in a multi-instance deployment: it's a per-JVM, in-memory cache with no cross-instance coordination, and a generic-shaped, generic-packaged, generic-Javadoc'd class invites being reached for to cache a Mongo/Nitrite-backed read, where a write on one instance would never invalidate another instance's cached read. It's safe for its actual sole use (GitHub API responses) only because that backend is read-only through calm-hub and already tolerates per-instance eventual consistency by design. See #3073 for the full writeup. - Move + rewrite as org.finos.calm.store.github.util.GitHubApiResponseCache (the established package for GitHub-only helpers), with @LookupIfProperty(calm.database.mode=github) matching every sibling. - Replace the generic get/put/getList API with purpose-built methods — getVersions/putVersions, getContentAtSha/putContentAtSha — baking both TTLs (5 min, 365 days) and both key formats in as private constants. This is the real structural barrier: reusing this for Mongo-backed data now requires editing the class, not just calling it differently. - Drop evict/evictByPrefix: unused by all production code, and unnecessary once both TTLs are fixed rather than caller-supplied. - Drop the custom Expiry/CacheEntry machinery (it existed specifically to support a variable per-call TTL) for two plain Caffeine caches with expireAfterWrite. Keep the injectable Ticker for deterministic expiry tests. - Rename calm.cache.max-size to calm.github.cache.max-size. - Full Javadoc rewrite stating the cross-instance limitation plainly, why it's safe here, and an explicit prohibition on reuse for Mongo/Nitrite data. Test file rewritten to match: the type-mismatch and evict tests no longer apply under the new API; added independent-expiry coverage for the two caches. --- .../finos/calm/cache/CalmCacheService.java | 158 ------------- .../github/util/GitHubApiResponseCache.java | 122 ++++++++++ .../src/main/resources/application.properties | 7 +- .../cache/TestCalmCacheServiceShould.java | 219 ------------------ .../TestGitHubApiResponseCacheShould.java | 195 ++++++++++++++++ 5 files changed, 320 insertions(+), 381 deletions(-) delete mode 100644 calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java create mode 100644 calm-hub/src/main/java/org/finos/calm/store/github/util/GitHubApiResponseCache.java delete mode 100644 calm-hub/src/test/java/org/finos/calm/cache/TestCalmCacheServiceShould.java create mode 100644 calm-hub/src/test/java/org/finos/calm/store/github/util/TestGitHubApiResponseCacheShould.java diff --git a/calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java b/calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java deleted file mode 100644 index 859da7e23..000000000 --- a/calm-hub/src/main/java/org/finos/calm/cache/CalmCacheService.java +++ /dev/null @@ -1,158 +0,0 @@ -package org.finos.calm.cache; - -import com.github.benmanes.caffeine.cache.Cache; -import com.github.benmanes.caffeine.cache.Caffeine; -import com.github.benmanes.caffeine.cache.Expiry; -import com.github.benmanes.caffeine.cache.Ticker; -import jakarta.enterprise.context.ApplicationScoped; -import jakarta.inject.Inject; -import org.eclipse.microprofile.config.inject.ConfigProperty; - -import java.time.Duration; -import java.util.List; -import java.util.Objects; -import java.util.Optional; -import java.util.concurrent.ConcurrentMap; - -/** - * A generic, in-memory TTL cache backed by Caffeine. It has no knowledge of - * what it stores — namespace keys however you like (a `kind:id` prefix, as - * used by {@link #evictByPrefix}) and choose a TTL per entry. - * - *

Each {@link #put} carries its own TTL rather than the cache having one - * fixed lifetime, so a single instance can serve both short-lived and - * effectively-permanent data — e.g. a 5-minute TTL for a version list next to - * a 365-day TTL for immutable content addressed by commit SHA. That per-call - * TTL is why this is a hand-rolled Caffeine cache rather than the - * {@code quarkus-cache} extension: {@code @CacheResult} has one TTL per - * annotated method, not per entry. - * - *

There is deliberately no interface here: this module puts an interface - * in front of a service only where there are genuinely multiple backends - * selected at runtime (see {@code store/} and its Mongo/Nitrite producers). - * A cache abstraction has exactly one implementation today; if a second - * (e.g. a distributed cache) is ever needed, extract an interface then. - * - *

Contract: - *

    - *
  • {@link #get} and {@link #getList} return {@link Optional#empty()} - * both for a missing key and for a value that isn't an instance of the - * requested type — neither ever throws a {@link ClassCastException}.
    • - *
  • {@link #put} silently ignores a {@code null} value: nothing is - * stored, and any existing entry for the key is left untouched. A - * zero or negative {@code ttl} is accepted and expires the entry - * immediately.
    • - *
  • TTL is measured from the most recent {@link #put} for a key; it is - * not refreshed by {@link #get} or {@link #getList} - * — this is a TTL cache, not an LRU with sliding expiry.
    • - *
- */ -@ApplicationScoped -public class CalmCacheService { - - private final Cache> cache; - - @Inject - public CalmCacheService(@ConfigProperty(name = "calm.cache.max-size", defaultValue = "10000") long maxSize) { - this(maxSize, Ticker.systemTicker()); - } - - // Package-private: lets tests drive expiry deterministically with a fake Ticker - // instead of Thread.sleep, the same pattern used by SchemaMigrationInProgressFilter's - // injectable LongSupplier. - CalmCacheService(long maxSize, Ticker ticker) { - this.cache = Caffeine.newBuilder() - .maximumSize(maxSize) - .ticker(ticker) - .expireAfter(new Expiry>() { - @Override - public long expireAfterCreate(String key, CacheEntry value, long currentTime) { - return value.ttl().toNanos(); - } - - @Override - public long expireAfterUpdate(String key, CacheEntry value, long currentTime, long currentDuration) { - return value.ttl().toNanos(); - } - - @Override - public long expireAfterRead(String key, CacheEntry value, long currentTime, long currentDuration) { - return currentDuration; - } - }) - .build(); - } - - /** - * Reads a single cached value, validating it is an instance of {@code type}. - * - * @return the cached value, or empty if there is no entry for {@code key} - * or its value is not an instance of {@code type} - */ - @SuppressWarnings("unchecked") - public Optional get(String key, Class type) { - CacheEntry entry = cache.getIfPresent(key); - if (entry == null || !type.isInstance(entry.value())) { - return Optional.empty(); - } - return Optional.of((T) entry.value()); - } - - /** - * Reads a cached {@link List}, validating every element is an instance of - * {@code elementType}. {@link Class#isInstance} alone cannot express a - * parameterized type such as {@code List}, so this exists - * alongside {@link #get} for list-valued entries — callers that need a - * typed list should use this rather than {@code get(key, List.class)} - * plus an unchecked cast. - * - * @return the cached list, or empty if there is no entry for {@code key}, - * its value isn't a {@link List}, or any element isn't an - * instance of {@code elementType} - */ - @SuppressWarnings("unchecked") - public Optional> getList(String key, Class elementType) { - CacheEntry entry = cache.getIfPresent(key); - if (entry == null || !(entry.value() instanceof List list)) { - return Optional.empty(); - } - for (Object element : list) { - if (!elementType.isInstance(element)) { - return Optional.empty(); - } - } - return Optional.of((List) list); - } - - /** - * Stores {@code value} under {@code key} for {@code ttl}. A {@code null} - * value is silently ignored — nothing is stored and any existing entry - * for {@code key} is left as-is. - */ - public void put(String key, Object value, Duration ttl) { - Objects.requireNonNull(ttl, "ttl must not be null"); - if (value == null) { - return; - } - cache.put(key, new CacheEntry<>(value, ttl)); - } - - /** - * Removes a single cached entry. A no-op if {@code key} isn't cached. - */ - public void evict(String key) { - cache.invalidate(key); - } - - /** - * Removes every cached entry whose key starts with {@code prefix} — for - * invalidating a family of related entries (e.g. everything cached for - * one resource) without tracking each key individually. - */ - public void evictByPrefix(String prefix) { - ConcurrentMap> map = cache.asMap(); - map.keySet().removeIf(key -> key.startsWith(prefix)); - } - - record CacheEntry(T value, Duration ttl) {} -} diff --git a/calm-hub/src/main/java/org/finos/calm/store/github/util/GitHubApiResponseCache.java b/calm-hub/src/main/java/org/finos/calm/store/github/util/GitHubApiResponseCache.java new file mode 100644 index 000000000..8068fb257 --- /dev/null +++ b/calm-hub/src/main/java/org/finos/calm/store/github/util/GitHubApiResponseCache.java @@ -0,0 +1,122 @@ +package org.finos.calm.store.github.util; + +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.Ticker; +import io.quarkus.arc.lookup.LookupIfProperty; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import org.eclipse.microprofile.config.inject.ConfigProperty; + +import java.time.Duration; +import java.util.List; +import java.util.Optional; + +/** + * Caches responses from the GitHub REST API on behalf of {@code GitHubVersionService}: + * version lists for a file (5 minutes) and file content at an immutable commit SHA + * (365 days). + * + *

This is a per-JVM, in-memory Caffeine cache with no cross-instance coordination + * and no invalidation broadcast — in a multi-instance deployment, each calm-hub + * instance holds its own independent copy and instances can disagree with each other + * for up to a TTL window. That is safe here, structurally, for two reasons: + *

    + *
  • The GitHub backend is read-only through calm-hub — every create/update/delete + * on {@code GitHubArchitectureStore} and its siblings throws + * {@code GitHubWriteNotSupportedException}, so nothing calm-hub does can ever + * invalidate an entry that needs invalidating.
    • + *
  • The GitHub backend already tolerates per-instance eventual consistency by + * design: {@code GitHubSyncScheduler} runs an unguarded, independent sync per + * instance (no leader election) on a roughly 60-second interval, each instance + * maintaining its own local clone and its own in-memory registry snapshot. + * Instances already legitimately disagree for up to that window before this + * cache is even in the picture.
    • + *
+ * + *

Do not reuse this pattern for Mongo/Nitrite-backed data. That + * backend is the primary one and supports full CRUD through the REST API — a Mongo + * write on one instance would never invalidate another instance's cached read there, + * which is a real correctness problem, not a benign staleness window. This class's API + * is deliberately GitHub-shaped (no generic key/type/TTL parameters) specifically so it + * can't be reached for as a general-purpose cache; see + * #3073 for the + * fuller discussion. + */ +@LookupIfProperty(name = "calm.database.mode", stringValue = "github") +@ApplicationScoped +public class GitHubApiResponseCache { + + private static final Duration VERSIONS_TTL = Duration.ofMinutes(5); + private static final Duration CONTENT_TTL = Duration.ofDays(365); + + private final Cache> versionsCache; + private final Cache contentCache; + + @Inject + public GitHubApiResponseCache(@ConfigProperty(name = "calm.github.cache.max-size", defaultValue = "10000") long maxSize) { + this(maxSize, Ticker.systemTicker()); + } + + // Package-private: lets tests drive expiry deterministically with a fake Ticker + // instead of Thread.sleep, the same pattern used by SchemaMigrationInProgressFilter's + // injectable LongSupplier. + GitHubApiResponseCache(long maxSize, Ticker ticker) { + this.versionsCache = Caffeine.newBuilder() + .maximumSize(maxSize) + .ticker(ticker) + .expireAfterWrite(VERSIONS_TTL) + .build(); + this.contentCache = Caffeine.newBuilder() + .maximumSize(maxSize) + .ticker(ticker) + .expireAfterWrite(CONTENT_TTL) + .build(); + } + + /** + * Reads the cached commit-SHA version list for a file, if present and not + * expired. + */ + public Optional> getVersions(String repoFullName, String filePath) { + return Optional.ofNullable(versionsCache.getIfPresent(versionsKey(repoFullName, filePath))); + } + + /** + * Caches the commit-SHA version list for a file for {@link #VERSIONS_TTL}. A + * {@code null} list is silently ignored. + */ + public void putVersions(String repoFullName, String filePath, List versions) { + if (versions == null) { + return; + } + versionsCache.put(versionsKey(repoFullName, filePath), versions); + } + + /** + * Reads the cached file content at a commit SHA, if present and not expired. + */ + public Optional getContentAtSha(String repoFullName, String filePath, String sha) { + return Optional.ofNullable(contentCache.getIfPresent(contentKey(repoFullName, filePath, sha))); + } + + /** + * Caches file content at a commit SHA for {@link #CONTENT_TTL}. A {@code null} + * content value is silently ignored. Safe to cache for a long TTL because a + * commit SHA is immutable — the same SHA always resolves to the same content. + */ + public void putContentAtSha(String repoFullName, String filePath, String sha, String content) { + if (content == null) { + return; + } + contentCache.put(contentKey(repoFullName, filePath, sha), content); + } + + private static String versionsKey(String repoFullName, String filePath) { + return "versions:" + repoFullName + ":" + filePath; + } + + private static String contentKey(String repoFullName, String filePath, String sha) { + return "content:" + repoFullName + ":" + filePath + ":" + sha; + } +} diff --git a/calm-hub/src/main/resources/application.properties b/calm-hub/src/main/resources/application.properties index 2c0ecbd5d..7152fd985 100644 --- a/calm-hub/src/main/resources/application.properties +++ b/calm-hub/src/main/resources/application.properties @@ -17,10 +17,9 @@ calm.standalone.seed-demo-data=false # Read-only mode: when true, opens Nitrite with readOnly(true) and rejects mutating HTTP verbs calm.readonly=false -# Maximum number of entries CalmCacheService (org.finos.calm.cache) holds at -# once, evicted LRU-first once exceeded. TTL is set per entry at the call -# site, not here. -calm.cache.max-size=10000 +# Maximum number of entries GitHubApiResponseCache holds per cache (versions and +# content are sized independently) — applies when calm.database.mode=github. +calm.github.cache.max-size=10000 # Audit logging: independently toggle persistence to the auditLogs store and # emission of a structured log line under the org.finos.calm.audit category. diff --git a/calm-hub/src/test/java/org/finos/calm/cache/TestCalmCacheServiceShould.java b/calm-hub/src/test/java/org/finos/calm/cache/TestCalmCacheServiceShould.java deleted file mode 100644 index 87f25b104..000000000 --- a/calm-hub/src/test/java/org/finos/calm/cache/TestCalmCacheServiceShould.java +++ /dev/null @@ -1,219 +0,0 @@ -package org.finos.calm.cache; - -import com.github.benmanes.caffeine.cache.Ticker; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; - -import java.time.Duration; -import java.util.List; -import java.util.Optional; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.Future; -import java.util.concurrent.TimeUnit; -import java.util.stream.IntStream; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.contains; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; -import static org.junit.jupiter.api.Assertions.assertThrows; - -class TestCalmCacheServiceShould { - - private static final long MAX_SIZE = 10_000; - - private final FakeTicker ticker = new FakeTicker(); - private CalmCacheService cacheService; - - @BeforeEach - void setup() { - cacheService = new CalmCacheService(MAX_SIZE, ticker); - } - - @Test - void construct_via_the_config_property_constructor() { - CalmCacheService service = new CalmCacheService(MAX_SIZE); - service.put("key", "value", Duration.ofMinutes(1)); - assertThat(service.get("key", String.class).orElse(null), equalTo("value")); - } - - @Test - void return_empty_for_missing_key() { - Optional result = cacheService.get("nonexistent", String.class); - assertThat(result.isEmpty(), is(true)); - } - - @Test - void store_and_retrieve_value() { - cacheService.put("key1", "value1", Duration.ofMinutes(5)); - Optional result = cacheService.get("key1", String.class); - assertThat(result.isPresent(), is(true)); - assertThat(result.get(), equalTo("value1")); - } - - @Test - void store_and_retrieve_different_types() { - cacheService.put("string-key", "hello", Duration.ofMinutes(5)); - cacheService.put("int-key", 42, Duration.ofMinutes(5)); - - assertThat(cacheService.get("string-key", String.class).orElse(null), equalTo("hello")); - assertThat(cacheService.get("int-key", Integer.class).orElse(null), equalTo(42)); - } - - @Test - void return_empty_when_type_does_not_match() { - cacheService.put("key", "string-value", Duration.ofMinutes(5)); - Optional result = cacheService.get("key", Integer.class); - assertThat(result.isEmpty(), is(true)); - } - - @Test - void store_and_retrieve_a_list() { - cacheService.put("list-key", List.of("a", "b", "c"), Duration.ofMinutes(5)); - Optional> result = cacheService.getList("list-key", String.class); - assertThat(result.isPresent(), is(true)); - assertThat(result.get(), contains("a", "b", "c")); - } - - @Test - void return_empty_from_getList_for_missing_key() { - assertThat(cacheService.getList("nonexistent", String.class).isEmpty(), is(true)); - } - - @Test - void return_empty_from_getList_when_value_is_not_a_list() { - cacheService.put("not-a-list", "just-a-string", Duration.ofMinutes(5)); - assertThat(cacheService.getList("not-a-list", String.class).isEmpty(), is(true)); - } - - @Test - void return_empty_from_getList_when_an_element_type_does_not_match() { - cacheService.put("mixed-list", List.of("a", 2, "c"), Duration.ofMinutes(5)); - assertThat(cacheService.getList("mixed-list", String.class).isEmpty(), is(true)); - } - - @Test - void evict_single_key() { - cacheService.put("key1", "value1", Duration.ofMinutes(5)); - cacheService.put("key2", "value2", Duration.ofMinutes(5)); - - cacheService.evict("key1"); - - assertThat(cacheService.get("key1", String.class).isEmpty(), is(true)); - assertThat(cacheService.get("key2", String.class).isPresent(), is(true)); - } - - @Test - void evict_by_prefix() { - cacheService.put("ns:finos:arch:1", "arch1", Duration.ofMinutes(5)); - cacheService.put("ns:finos:arch:2", "arch2", Duration.ofMinutes(5)); - cacheService.put("ns:finos:pattern:1", "pattern1", Duration.ofMinutes(5)); - cacheService.put("ns:other:arch:1", "other-arch1", Duration.ofMinutes(5)); - - cacheService.evictByPrefix("ns:finos:arch:"); - - assertThat(cacheService.get("ns:finos:arch:1", String.class).isEmpty(), is(true)); - assertThat(cacheService.get("ns:finos:arch:2", String.class).isEmpty(), is(true)); - assertThat(cacheService.get("ns:finos:pattern:1", String.class).isPresent(), is(true)); - assertThat(cacheService.get("ns:other:arch:1", String.class).isPresent(), is(true)); - } - - @Test - void evict_by_prefix_when_no_keys_match() { - cacheService.put("other:key", "value", Duration.ofMinutes(5)); - - cacheService.evictByPrefix("nonexistent:"); - - assertThat(cacheService.get("other:key", String.class).isPresent(), is(true)); - } - - @Test - void expire_entries_after_ttl() { - cacheService.put("short-lived", "value", Duration.ofMillis(50)); - assertThat(cacheService.get("short-lived", String.class).isPresent(), is(true)); - - ticker.advance(Duration.ofMillis(100)); - - assertThat(cacheService.get("short-lived", String.class).isEmpty(), is(true)); - } - - @Test - void not_refresh_ttl_on_read() { - cacheService.put("key", "value", Duration.ofMillis(50)); - ticker.advance(Duration.ofMillis(30)); - assertThat(cacheService.get("key", String.class).isPresent(), is(true)); - - ticker.advance(Duration.ofMillis(30)); - - assertThat(cacheService.get("key", String.class).isEmpty(), is(true)); - } - - @Test - void overwrite_existing_key_with_new_value_and_ttl() { - cacheService.put("key", "original", Duration.ofMinutes(5)); - cacheService.put("key", "updated", Duration.ofMinutes(10)); - - Optional result = cacheService.get("key", String.class); - assertThat(result.isPresent(), is(true)); - assertThat(result.get(), equalTo("updated")); - } - - @Test - void handle_null_value_gracefully() { - cacheService.put("null-key", null, Duration.ofMinutes(5)); - Optional result = cacheService.get("null-key", Object.class); - assertThat(result.isEmpty(), is(true)); - } - - @Test - void reject_a_null_ttl() { - assertThrows(NullPointerException.class, () -> cacheService.put("key", "value", null)); - } - - @Test - void handle_concurrent_access_safely() throws Exception { - int threadCount = 10; - int iterationsPerThread = 100; - ExecutorService executor = Executors.newFixedThreadPool(threadCount); - - List> futures = IntStream.range(0, threadCount) - .>mapToObj(threadId -> executor.submit(() -> { - for (int i = 0; i < iterationsPerThread; i++) { - String key = "thread-" + threadId + "-key-" + i; - cacheService.put(key, "value-" + i, Duration.ofMinutes(5)); - cacheService.get(key, String.class); - } - })) - .toList(); - - for (Future future : futures) { - future.get(10, TimeUnit.SECONDS); - } - executor.shutdown(); - - for (int threadId = 0; threadId < threadCount; threadId++) { - for (int i = 0; i < iterationsPerThread; i++) { - String key = "thread-" + threadId + "-key-" + i; - assertThat(cacheService.get(key, String.class).orElse(null), equalTo("value-" + i)); - } - } - } - - /** - * A manually-advanced {@link Ticker} so expiry tests are deterministic - * instead of relying on {@code Thread.sleep}. - */ - private static final class FakeTicker implements Ticker { - private long nanos = 0; - - @Override - public long read() { - return nanos; - } - - void advance(Duration duration) { - nanos += duration.toNanos(); - } - } -} diff --git a/calm-hub/src/test/java/org/finos/calm/store/github/util/TestGitHubApiResponseCacheShould.java b/calm-hub/src/test/java/org/finos/calm/store/github/util/TestGitHubApiResponseCacheShould.java new file mode 100644 index 000000000..1f5833cf4 --- /dev/null +++ b/calm-hub/src/test/java/org/finos/calm/store/github/util/TestGitHubApiResponseCacheShould.java @@ -0,0 +1,195 @@ +package org.finos.calm.store.github.util; + +import com.github.benmanes.caffeine.cache.Ticker; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.time.Duration; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.stream.IntStream; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; + +class TestGitHubApiResponseCacheShould { + + private static final long MAX_SIZE = 10_000; + + private final FakeTicker ticker = new FakeTicker(); + private GitHubApiResponseCache cache; + + @BeforeEach + void setup() { + cache = new GitHubApiResponseCache(MAX_SIZE, ticker); + } + + @Test + void construct_via_the_config_property_constructor() { + GitHubApiResponseCache service = new GitHubApiResponseCache(MAX_SIZE); + service.putVersions("org/repo", "path/file.json", List.of("abc1234")); + assertThat(service.getVersions("org/repo", "path/file.json").orElse(null), contains("abc1234")); + } + + @Test + void return_empty_versions_for_a_missing_key() { + assertThat(cache.getVersions("org/repo", "path/file.json").isEmpty(), is(true)); + } + + @Test + void store_and_retrieve_versions() { + cache.putVersions("org/repo", "path/file.json", List.of("abc1234", "def5678")); + + Optional> result = cache.getVersions("org/repo", "path/file.json"); + + assertThat(result.isPresent(), is(true)); + assertThat(result.get(), contains("abc1234", "def5678")); + } + + @Test + void keep_versions_for_different_files_independent() { + cache.putVersions("org/repo", "path/a.json", List.of("aaa")); + cache.putVersions("org/repo", "path/b.json", List.of("bbb")); + + assertThat(cache.getVersions("org/repo", "path/a.json").orElse(null), contains("aaa")); + assertThat(cache.getVersions("org/repo", "path/b.json").orElse(null), contains("bbb")); + } + + @Test + void overwrite_existing_versions_entry() { + cache.putVersions("org/repo", "path/file.json", List.of("old")); + cache.putVersions("org/repo", "path/file.json", List.of("new")); + + assertThat(cache.getVersions("org/repo", "path/file.json").orElse(null), contains("new")); + } + + @Test + void ignore_a_null_versions_value() { + cache.putVersions("org/repo", "path/file.json", null); + assertThat(cache.getVersions("org/repo", "path/file.json").isEmpty(), is(true)); + } + + @Test + void expire_versions_after_five_minutes() { + cache.putVersions("org/repo", "path/file.json", List.of("abc1234")); + assertThat(cache.getVersions("org/repo", "path/file.json").isPresent(), is(true)); + + ticker.advance(Duration.ofMinutes(5).plusSeconds(1)); + + assertThat(cache.getVersions("org/repo", "path/file.json").isEmpty(), is(true)); + } + + @Test + void not_refresh_versions_ttl_on_read() { + cache.putVersions("org/repo", "path/file.json", List.of("abc1234")); + ticker.advance(Duration.ofMinutes(3)); + assertThat(cache.getVersions("org/repo", "path/file.json").isPresent(), is(true)); + + ticker.advance(Duration.ofMinutes(3)); + + assertThat(cache.getVersions("org/repo", "path/file.json").isEmpty(), is(true)); + } + + @Test + void return_empty_content_for_a_missing_key() { + assertThat(cache.getContentAtSha("org/repo", "path/file.json", "abc1234").isEmpty(), is(true)); + } + + @Test + void store_and_retrieve_content_at_sha() { + cache.putContentAtSha("org/repo", "path/file.json", "abc1234", "file contents"); + + Optional result = cache.getContentAtSha("org/repo", "path/file.json", "abc1234"); + + assertThat(result.isPresent(), is(true)); + assertThat(result.get(), equalTo("file contents")); + } + + @Test + void keep_content_for_different_shas_independent() { + cache.putContentAtSha("org/repo", "path/file.json", "sha1", "content-at-sha1"); + cache.putContentAtSha("org/repo", "path/file.json", "sha2", "content-at-sha2"); + + assertThat(cache.getContentAtSha("org/repo", "path/file.json", "sha1").orElse(null), equalTo("content-at-sha1")); + assertThat(cache.getContentAtSha("org/repo", "path/file.json", "sha2").orElse(null), equalTo("content-at-sha2")); + } + + @Test + void ignore_a_null_content_value() { + cache.putContentAtSha("org/repo", "path/file.json", "abc1234", null); + assertThat(cache.getContentAtSha("org/repo", "path/file.json", "abc1234").isEmpty(), is(true)); + } + + @Test + void expire_content_after_365_days() { + cache.putContentAtSha("org/repo", "path/file.json", "abc1234", "file contents"); + assertThat(cache.getContentAtSha("org/repo", "path/file.json", "abc1234").isPresent(), is(true)); + + ticker.advance(Duration.ofDays(365).plusSeconds(1)); + + assertThat(cache.getContentAtSha("org/repo", "path/file.json", "abc1234").isEmpty(), is(true)); + } + + @Test + void expire_versions_and_content_independently_of_each_other() { + cache.putVersions("org/repo", "path/file.json", List.of("abc1234")); + cache.putContentAtSha("org/repo", "path/file.json", "abc1234", "file contents"); + + ticker.advance(Duration.ofMinutes(5).plusSeconds(1)); + + assertThat(cache.getVersions("org/repo", "path/file.json").isEmpty(), is(true)); + assertThat(cache.getContentAtSha("org/repo", "path/file.json", "abc1234").isPresent(), is(true)); + } + + @Test + void handle_concurrent_access_safely() throws Exception { + int threadCount = 10; + int iterationsPerThread = 100; + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + + List> futures = IntStream.range(0, threadCount) + .>mapToObj(threadId -> executor.submit(() -> { + for (int i = 0; i < iterationsPerThread; i++) { + String filePath = "path/" + threadId + "/" + i + ".json"; + cache.putVersions("org/repo", filePath, List.of("sha-" + i)); + cache.getVersions("org/repo", filePath); + } + })) + .toList(); + + for (Future future : futures) { + future.get(10, TimeUnit.SECONDS); + } + executor.shutdown(); + + for (int threadId = 0; threadId < threadCount; threadId++) { + for (int i = 0; i < iterationsPerThread; i++) { + String filePath = "path/" + threadId + "/" + i + ".json"; + assertThat(cache.getVersions("org/repo", filePath).orElse(null), contains("sha-" + i)); + } + } + } + + /** + * A manually-advanced {@link Ticker} so expiry tests are deterministic + * instead of relying on {@code Thread.sleep}. + */ + private static final class FakeTicker implements Ticker { + private long nanos = 0; + + @Override + public long read() { + return nanos; + } + + void advance(Duration duration) { + nanos += duration.toNanos(); + } + } +} From a507a3228f1ab27a2976caf2932bfdce3b0a33f5 Mon Sep 17 00:00:00 2001 From: James Gough Date: Wed, 9 Sep 2026 12:20:34 +0100 Subject: [PATCH 4/4] fix(calm-hub): address code review findings on the GitHub cache rework MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - getVersions/putVersions aliased the cache's internal List to whatever the caller passed in or read back — a caller mutating either reference would silently corrupt the shared cache entry for every other concurrent reader until TTL expiry. putVersions now stores an immutable List.copyOf(...), so both directions are safe. - Consolidated the four near-identical get/put method bodies into private generic read/write helpers, and the two near-identical Caffeine.newBuilder() chains into a private buildCache helper. --- .../github/util/GitHubApiResponseCache.java | 39 +++++++++++-------- .../TestGitHubApiResponseCacheShould.java | 21 ++++++++++ 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/calm-hub/src/main/java/org/finos/calm/store/github/util/GitHubApiResponseCache.java b/calm-hub/src/main/java/org/finos/calm/store/github/util/GitHubApiResponseCache.java index 8068fb257..89484929e 100644 --- a/calm-hub/src/main/java/org/finos/calm/store/github/util/GitHubApiResponseCache.java +++ b/calm-hub/src/main/java/org/finos/calm/store/github/util/GitHubApiResponseCache.java @@ -62,15 +62,15 @@ public GitHubApiResponseCache(@ConfigProperty(name = "calm.github.cache.max-size // instead of Thread.sleep, the same pattern used by SchemaMigrationInProgressFilter's // injectable LongSupplier. GitHubApiResponseCache(long maxSize, Ticker ticker) { - this.versionsCache = Caffeine.newBuilder() - .maximumSize(maxSize) - .ticker(ticker) - .expireAfterWrite(VERSIONS_TTL) - .build(); - this.contentCache = Caffeine.newBuilder() + this.versionsCache = buildCache(maxSize, ticker, VERSIONS_TTL); + this.contentCache = buildCache(maxSize, ticker, CONTENT_TTL); + } + + private static Cache buildCache(long maxSize, Ticker ticker, Duration ttl) { + return Caffeine.newBuilder() .maximumSize(maxSize) .ticker(ticker) - .expireAfterWrite(CONTENT_TTL) + .expireAfterWrite(ttl) .build(); } @@ -79,25 +79,24 @@ public GitHubApiResponseCache(@ConfigProperty(name = "calm.github.cache.max-size * expired. */ public Optional> getVersions(String repoFullName, String filePath) { - return Optional.ofNullable(versionsCache.getIfPresent(versionsKey(repoFullName, filePath))); + return read(versionsCache, versionsKey(repoFullName, filePath)); } /** * Caches the commit-SHA version list for a file for {@link #VERSIONS_TTL}. A - * {@code null} list is silently ignored. + * {@code null} list is silently ignored. Stores an immutable copy, so a caller + * mutating the list it passed in — or held onto after a {@link #getVersions} + * call — can never corrupt the cached entry. */ public void putVersions(String repoFullName, String filePath, List versions) { - if (versions == null) { - return; - } - versionsCache.put(versionsKey(repoFullName, filePath), versions); + write(versionsCache, versionsKey(repoFullName, filePath), versions == null ? null : List.copyOf(versions)); } /** * Reads the cached file content at a commit SHA, if present and not expired. */ public Optional getContentAtSha(String repoFullName, String filePath, String sha) { - return Optional.ofNullable(contentCache.getIfPresent(contentKey(repoFullName, filePath, sha))); + return read(contentCache, contentKey(repoFullName, filePath, sha)); } /** @@ -106,10 +105,18 @@ public Optional getContentAtSha(String repoFullName, String filePath, St * commit SHA is immutable — the same SHA always resolves to the same content. */ public void putContentAtSha(String repoFullName, String filePath, String sha, String content) { - if (content == null) { + write(contentCache, contentKey(repoFullName, filePath, sha), content); + } + + private static Optional read(Cache cache, String key) { + return Optional.ofNullable(cache.getIfPresent(key)); + } + + private static void write(Cache cache, String key, V value) { + if (value == null) { return; } - contentCache.put(contentKey(repoFullName, filePath, sha), content); + cache.put(key, value); } private static String versionsKey(String repoFullName, String filePath) { diff --git a/calm-hub/src/test/java/org/finos/calm/store/github/util/TestGitHubApiResponseCacheShould.java b/calm-hub/src/test/java/org/finos/calm/store/github/util/TestGitHubApiResponseCacheShould.java index 1f5833cf4..f9ed4c505 100644 --- a/calm-hub/src/test/java/org/finos/calm/store/github/util/TestGitHubApiResponseCacheShould.java +++ b/calm-hub/src/test/java/org/finos/calm/store/github/util/TestGitHubApiResponseCacheShould.java @@ -5,6 +5,7 @@ import org.junit.jupiter.api.Test; import java.time.Duration; +import java.util.ArrayList; import java.util.List; import java.util.Optional; import java.util.concurrent.ExecutorService; @@ -17,6 +18,7 @@ import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertThrows; class TestGitHubApiResponseCacheShould { @@ -75,6 +77,25 @@ void ignore_a_null_versions_value() { assertThat(cache.getVersions("org/repo", "path/file.json").isEmpty(), is(true)); } + @Test + void store_a_defensive_copy_so_a_caller_cannot_mutate_the_cached_entry() { + List mutable = new ArrayList<>(List.of("original")); + cache.putVersions("org/repo", "path/file.json", mutable); + + mutable.add("mutated-after-put"); + + assertThat(cache.getVersions("org/repo", "path/file.json").orElse(null), contains("original")); + } + + @Test + void return_an_immutable_versions_list_so_a_caller_cannot_corrupt_the_cache() { + cache.putVersions("org/repo", "path/file.json", List.of("abc1234")); + + List result = cache.getVersions("org/repo", "path/file.json").orElseThrow(); + + assertThrows(UnsupportedOperationException.class, () -> result.add("should-fail")); + } + @Test void expire_versions_after_five_minutes() { cache.putVersions("org/repo", "path/file.json", List.of("abc1234"));