From 12a73d31495b71e4f003bc260796718399034909 Mon Sep 17 00:00:00 2001
From: Alexandre Carlton <alexandre.nikolei.carlton@gmail.com>
Date: Mon, 29 Apr 2024 20:10:57 +1000
Subject: [PATCH] Remove CacheMap#containsKey before #get

Currently, when accessing values from the `DataLoader`'s `CacheMap`, we
first check `#containsKey` before invoking `#get`. This is problematic
for two reasons:

 - the underlying cache's metrics are skewed (it will have a 100% hit
   rate).
 - if the cache has an automatic expiration policy, it is possible a
   value will expire between the `containsKey` and `get` invocations
   leading to an incorrect result.

To ameliorate this, we always invoke `#get` and check if it is `null` to
deterine whether the key is present. We wrap the invocation in a
`try`/`catch` as the `#get` method is allowed to through per its
documentation.
---
 src/main/java/org/dataloader/CacheMap.java    |  3 +--
 .../java/org/dataloader/DataLoaderHelper.java | 22 +++++++++++++------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/src/main/java/org/dataloader/CacheMap.java b/src/main/java/org/dataloader/CacheMap.java
index 48d0f41..1a4a455 100644
--- a/src/main/java/org/dataloader/CacheMap.java
+++ b/src/main/java/org/dataloader/CacheMap.java
@@ -65,8 +65,7 @@ static <K, V> CacheMap<K, V> simpleMap() {
     /**
      * Gets the specified key from the cache map.
      * <p>
-     * May throw an exception if the key does not exist, depending on the cache map implementation that is used,
-     * so be sure to check {@link CacheMap#containsKey(Object)} first.
+     * May throw an exception if the key does not exist, depending on the cache map implementation that is used.
      *
      * @param key the key to retrieve
      *
diff --git a/src/main/java/org/dataloader/DataLoaderHelper.java b/src/main/java/org/dataloader/DataLoaderHelper.java
index 1e48a94..d934de2 100644
--- a/src/main/java/org/dataloader/DataLoaderHelper.java
+++ b/src/main/java/org/dataloader/DataLoaderHelper.java
@@ -110,9 +110,13 @@ Optional<CompletableFuture<V>> getIfPresent(K key) {
             boolean cachingEnabled = loaderOptions.cachingEnabled();
             if (cachingEnabled) {
                 Object cacheKey = getCacheKey(nonNull(key));
-                if (futureCache.containsKey(cacheKey)) {
-                    stats.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(key));
-                    return Optional.of(futureCache.get(cacheKey));
+                try {
+                    CompletableFuture<V> cacheValue = futureCache.get(cacheKey);
+                    if (cacheValue != null) {
+                        stats.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(key));
+                        return Optional.of(cacheValue);
+                    }
+                } catch (Exception ignored) {
                 }
             }
         }
@@ -307,10 +311,14 @@ private void possiblyClearCacheEntriesOnExceptions(List<K> keys) {
     private CompletableFuture<V> loadFromCache(K key, Object loadContext, boolean batchingEnabled) {
         final Object cacheKey = loadContext == null ? getCacheKey(key) : getCacheKeyWithContext(key, loadContext);
 
-        if (futureCache.containsKey(cacheKey)) {
-            // We already have a promise for this key, no need to check value cache or queue up load
-            stats.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(key, loadContext));
-            return futureCache.get(cacheKey);
+        try {
+            CompletableFuture<V> cacheValue = futureCache.get(cacheKey);
+            if (cacheValue != null) {
+                // We already have a promise for this key, no need to check value cache or queue up load
+                stats.incrementCacheHitCount(new IncrementCacheHitCountStatisticsContext<>(key, loadContext));
+                return cacheValue;
+            }
+        } catch (Exception ignored) {
         }
 
         CompletableFuture<V> loadCallFuture = queueOrInvokeLoader(key, loadContext, batchingEnabled, true);