PR feedback - removed containsKey, updated a few method names and updated doc

Author: bbakerman

src/main/java/org/dataloader/CachedValueStore.java

@@ -3,7 +3,6 @@
 import org.dataloader.annotations.PublicSpi;
 import org.dataloader.impl.NoOpCachedValueStore;
 
-import java.util.List;
 import java.util.concurrent.CompletableFuture;
 
 /**
@@ -33,36 +32,21 @@ public interface CachedValueStore<K, V> {
      *
      * @return the cache store
      */
-    static <K, V> CachedValueStore<K, V> defaultStore() {
+    static <K, V> CachedValueStore<K, V> defaultCachedValueStore() {
         //noinspection unchecked
         return (CachedValueStore<K, V>) NoOpCachedValueStore.NOOP;
     }
 
     /**
-     * Checks whether the specified key is contained in the store.
-     * <p>
-     * {@link DataLoader} first calls {@link #containsKey(Object)} and then calls {@link #get(Object)}.  If the
-     * backing cache implementation cannot answer the `containsKey` call then simply return true and the
-     * following `get` call can complete exceptionally to cause the {@link DataLoader}
-     * to enqueue the key to the {@link BatchLoader#load(List)} call since it is not present in cache.
-     *
-     * @param key the key to check if its present in the cache
-     *
-     * @return {@code true} if the cache contains the key, {@code false} otherwise
-     */
-    CompletableFuture<Boolean> containsKey(K key);
-
-    /**
-     * Gets the specified key from the store.
+     * Gets the specified key from the store.  if the key si not present, then the implementation MUST return an exceptionally completed future
+     * and not null because null is a valid cacheable value.  Any exception is will cause {@link DataLoader} to load the key via batch loading
+     * instead.
      *
      * @param key the key to retrieve
      *
-     * @return a future containing the cached value (which maybe null) or an exception if the key does
+     * @return a future containing the cached value (which maybe null) or exceptionally completed future if the key does
      * not exist in the cache.
      *
-     * IMPORTANT: The future may fail if the key does not exist depending on implementation. Implementations should
-     * return an exceptional future if the key is not present in the cache, not null which is a valid value
-     * for a key.
      */
     CompletableFuture<V> get(K key);
 

src/main/java/org/dataloader/DataLoader.java

@@ -415,8 +415,8 @@ public DataLoader(BatchLoader<K, V> batchLoadFunction, DataLoaderOptions options
     @VisibleForTesting
     DataLoader(Object batchLoadFunction, DataLoaderOptions options, Clock clock) {
         DataLoaderOptions loaderOptions = options == null ? new DataLoaderOptions() : options;
-        this.futureCache = determineCacheMap(loaderOptions);
-        this.cachedValueStore = determineCacheStore(loaderOptions);
+        this.futureCache = determineFutureCache(loaderOptions);
+        this.cachedValueStore = determineCachedValueStore(loaderOptions);
         // order of keys matter in data loader
         this.stats = nonNull(loaderOptions.getStatisticsCollector());
 
@@ -425,13 +425,13 @@ public DataLoader(BatchLoader<K, V> batchLoadFunction, DataLoaderOptions options
 
 
     @SuppressWarnings("unchecked")
-    private CacheMap<Object, V> determineCacheMap(DataLoaderOptions loaderOptions) {
+    private CacheMap<Object, V> determineFutureCache(DataLoaderOptions loaderOptions) {
         return (CacheMap<Object, V>) loaderOptions.cacheMap().orElseGet(CacheMap::simpleMap);
     }
 
     @SuppressWarnings("unchecked")
-    private CachedValueStore<Object, V> determineCacheStore(DataLoaderOptions loaderOptions) {
-        return (CachedValueStore<Object, V>) loaderOptions.cachedValueStore().orElseGet(CachedValueStore::defaultStore);
+    private CachedValueStore<Object, V> determineCachedValueStore(DataLoaderOptions loaderOptions) {
+        return (CachedValueStore<Object, V>) loaderOptions.cachedValueStore().orElseGet(CachedValueStore::defaultCachedValueStore);
     }
 
     /**

src/main/java/org/dataloader/DataLoaderHelper.java

@@ -307,17 +307,9 @@ private CompletableFuture<V> loadFromCache(K key, Object loadContext, boolean ba
          */
         final CompletableFuture<V> future = new CompletableFuture<>();
 
-        cachedValueStore.containsKey(cacheKey).whenComplete((hasKey, containsCallEx) -> {
-            boolean containsKey = containsCallEx == null && Boolean.TRUE.equals(hasKey);
-            if (containsKey) {
-                cachedValueStore.get(cacheKey).whenComplete((cachedValue, getCallEx) -> {
-                    if (getCallEx == null) {
-                        future.complete(cachedValue);
-                    } else {
-                        queueOrInvokeLoader(key, loadContext, batchingEnabled)
-                                .whenComplete(setValueIntoCacheAndCompleteFuture(cacheKey, future));
-                    }
-                });
+        cachedValueStore.get(cacheKey).whenComplete((cachedValue, getCallEx) -> {
+            if (getCallEx == null) {
+                future.complete(cachedValue);
             } else {
                 queueOrInvokeLoader(key, loadContext, batchingEnabled)
                         .whenComplete(setValueIntoCacheAndCompleteFuture(cacheKey, future));

src/main/java/org/dataloader/impl/NoOpCachedValueStore.java

@@ -22,14 +22,6 @@ public class NoOpCachedValueStore<K, V> implements CachedValueStore<K, V> {
 
     public static NoOpCachedValueStore<?, ?> NOOP = new NoOpCachedValueStore<>();
 
-    /**
-     * {@inheritDoc}
-     */
-    @Override
-    public CompletableFuture<Boolean> containsKey(K key) {
-        return CompletableFuture.completedFuture(false);
-    }
-
     /**
      * {@inheritDoc}
      */

src/test/java/org/dataloader/DataLoaderCachedValueStoreTest.java

@@ -145,47 +145,6 @@ public void can_use_caffeine_for_caching() {
         assertArrayEquals(caffeineCache.asMap().keySet().toArray(), asList("a", "b", "c").toArray());
     }
 
-    @Test
-    public void will_invoke_loader_if_CACHE_CONTAINS_call_throws_exception() {
-        CustomCachedValueStore customStore = new CustomCachedValueStore() {
-            @Override
-            public CompletableFuture<Boolean> containsKey(String key) {
-                if (key.equals("a")) {
-                    return failedFuture(new IllegalStateException("no A"));
-                }
-                if (key.equals("c")) {
-                    return completedFuture(false);
-                }
-                return super.containsKey(key);
-            }
-        };
-        customStore.set("a", "Not From Cache A");
-        customStore.set("b", "From Cache");
-        customStore.set("c", "Not From Cache C");
-
-        List<List<String>> loadCalls = new ArrayList<>();
-        DataLoaderOptions options = newOptions().setCachedValueStore(customStore);
-        DataLoader<String, String> identityLoader = idLoader(options, loadCalls);
-
-        // Fetches as expected
-
-        CompletableFuture<String> fA = identityLoader.load("a");
-        CompletableFuture<String> fB = identityLoader.load("b");
-        CompletableFuture<String> fC = identityLoader.load("c");
-
-        await().until(identityLoader.dispatch()::isDone);
-        assertThat(fA.join(), equalTo("a"));
-        assertThat(fB.join(), equalTo("From Cache"));
-        assertThat(fC.join(), equalTo("c"));
-
-        // a was not in cache (according to containsKey) and hence needed to be loaded
-        assertThat(loadCalls, equalTo(singletonList(asList("a", "c"))));
-
-        // the failed containsKey calls will be SET back into the value cache after batch loading
-        assertArrayEquals(customStore.store.keySet().toArray(), asList("a", "b", "c").toArray());
-        assertArrayEquals(customStore.store.values().toArray(), asList("a", "From Cache", "c").toArray());
-    }
-
     @Test
     public void will_invoke_loader_if_CACHE_GET_call_throws_exception() {
         CustomCachedValueStore customStore = new CustomCachedValueStore() {

src/test/java/org/dataloader/fixtures/CaffeineCachedValueStore.java

@@ -15,12 +15,6 @@ public CaffeineCachedValueStore(Cache<String, Object> cache) {
         this.cache = cache;
     }
 
-    @Override
-    public CompletableFuture<Boolean> containsKey(String key) {
-        // caffeine cant answer this question efficiently so we rely on
-        return CompletableFuture.completedFuture(true);
-    }
-
     @Override
     public CompletableFuture<Object> get(String key) {
         Object value = cache.getIfPresent(key);

src/test/java/org/dataloader/fixtures/CustomCachedValueStore.java

@@ -2,6 +2,7 @@
 
 
 import org.dataloader.CachedValueStore;
+import org.dataloader.impl.CompletableFutureKit;
 
 import java.util.Map;
 import java.util.concurrent.CompletableFuture;
@@ -11,13 +12,11 @@ public class CustomCachedValueStore implements CachedValueStore<String, Object>
 
     public final Map<String, Object> store = new ConcurrentHashMap<>();
 
-    @Override
-    public CompletableFuture<Boolean> containsKey(String key) {
-        return CompletableFuture.completedFuture(store.containsKey(key));
-    }
-
     @Override
     public CompletableFuture<Object> get(String key) {
+        if (!store.containsKey(key)) {
+            return CompletableFutureKit.failedFuture(new RuntimeException("The key is missing"));
+        }
         return CompletableFuture.completedFuture(store.get(key));
     }