Added an option to stop the caching of exceptional values

Author: bbakerman

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

@@ -176,14 +176,14 @@ private CompletableFuture<List<V>> dispatchQueueBatch(List<K> keys, List<Object>
                 .thenApply(values -> {
                     assertResultSize(keys, values);
 
+                    List<K> clearCacheKeys = new ArrayList<>();
                     for (int idx = 0; idx < queuedFutures.size(); idx++) {
                         Object value = values.get(idx);
                         CompletableFuture<V> future = queuedFutures.get(idx);
                         if (value instanceof Throwable) {
                             stats.incrementLoadErrorCount();
                             future.completeExceptionally((Throwable) value);
-                            // we don't clear the cached view of this entry to avoid
-                            // frequently loading the same error
+                            clearCacheKeys.add(keys.get(idx));
                         } else if (value instanceof Try) {
                             // we allow the batch loader to return a Try so we can better represent a computation
                             // that might have worked or not.
@@ -193,12 +193,14 @@ private CompletableFuture<List<V>> dispatchQueueBatch(List<K> keys, List<Object>
                             } else {
                                 stats.incrementLoadErrorCount();
                                 future.completeExceptionally(tryValue.getThrowable());
+                                clearCacheKeys.add(keys.get(idx));
                             }
                         } else {
                             V val = (V) value;
                             future.complete(val);
                         }
                     }
+                    possiblyClearCacheEntriesOnExceptions(clearCacheKeys);
                     return values;
                 }).exceptionally(ex -> {
                     stats.incrementBatchLoadExceptionCount();
@@ -218,6 +220,19 @@ private void assertResultSize(List<K> keys, List<V> values) {
         assertState(keys.size() == values.size(), "The size of the promised values MUST be the same size as the key list");
     }
 
+    private void possiblyClearCacheEntriesOnExceptions(List<K> keys) {
+        if (keys.isEmpty()) {
+            return;
+        }
+        // by default we don't clear the cached view of this entry to avoid
+        // frequently loading the same error.  This works for short lived request caches
+        // but might work against long lived caches.  Hence we have an option that allows
+        // it to be cleared
+        if (!loaderOptions.cachingExceptionsEnabled()) {
+            keys.forEach(dataLoader::clear);
+        }
+    }
+
 
     CompletableFuture<V> invokeLoaderImmediately(K key, Object keyContext) {
         List<K> keys = singletonList(key);

src/main/java/org/dataloader/DataLoaderOptions.java

@@ -35,6 +35,7 @@ public class DataLoaderOptions {
 
     private boolean batchingEnabled;
     private boolean cachingEnabled;
+    private boolean cachingExceptionsEnabled;
     private CacheKey cacheKeyFunction;
     private CacheMap cacheMap;
     private int maxBatchSize;
@@ -47,6 +48,7 @@ public class DataLoaderOptions {
     public DataLoaderOptions() {
         batchingEnabled = true;
         cachingEnabled = true;
+        cachingExceptionsEnabled = true;
         maxBatchSize = -1;
         statisticsCollector = SimpleStatisticsCollector::new;
         environmentProvider = NULL_PROVIDER;
@@ -61,6 +63,7 @@ public DataLoaderOptions(DataLoaderOptions other) {
         nonNull(other);
         this.batchingEnabled = other.batchingEnabled;
         this.cachingEnabled = other.cachingEnabled;
+        this.cachingExceptionsEnabled = other.cachingExceptionsEnabled;
         this.cacheKeyFunction = other.cacheKeyFunction;
         this.cacheMap = other.cacheMap;
         this.maxBatchSize = other.maxBatchSize;
@@ -117,6 +120,32 @@ public DataLoaderOptions setCachingEnabled(boolean cachingEnabled) {
         return this;
     }
 
+    /**
+     * Option that determines whether to cache exceptional values (the default), or not.
+     *
+     * For short lived caches (that is request caches) it makes sense to cache exceptions since
+     * its likely the key is still poisoned.  However if you have long lived caches, then it may make
+     * sense to set this to false since the downstream system may have recovered from its failure
+     * mode.
+     *
+     * @return {@code true} when exceptional values are cached is enabled, {@code false} otherwise
+     */
+    public boolean cachingExceptionsEnabled() {
+        return cachingExceptionsEnabled;
+    }
+
+    /**
+     * Sets the option that determines whether exceptional values are cachedis enabled.
+     *
+     * @param cachingExceptionsEnabled {@code true} to enable caching exceptional values, {@code false} otherwise
+     *
+     * @return the data loader options for fluent coding
+     */
+    public DataLoaderOptions setCachingExceptionsEnabled(boolean cachingExceptionsEnabled) {
+        this.cachingExceptionsEnabled = cachingExceptionsEnabled;
+        return this;
+    }
+
     /**
      * Gets an (optional) function to invoke for creation of the cache key, if caching is enabled.
      * <p>

src/test/java/org/dataloader/DataLoaderTest.java

@@ -42,7 +42,6 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertThat;
 
@@ -418,6 +417,29 @@ public void should_Cache_failed_fetches() {
         assertThat(loadCalls, equalTo(singletonList(singletonList(1))));
     }
 
+    @Test
+    public void should_NOT_Cache_failed_fetches_if_told_not_too() {
+        DataLoaderOptions options = DataLoaderOptions.newOptions().setCachingExceptionsEnabled(false);
+        List<Collection<Integer>> loadCalls = new ArrayList<>();
+        DataLoader<Integer, Object> errorLoader = idLoaderAllExceptions(options, loadCalls);
+
+        CompletableFuture<Object> future1 = errorLoader.load(1);
+        errorLoader.dispatch();
+
+        await().until(future1::isDone);
+        assertThat(future1.isCompletedExceptionally(), is(true));
+        assertThat(cause(future1), instanceOf(IllegalStateException.class));
+
+        CompletableFuture<Object> future2 = errorLoader.load(1);
+        errorLoader.dispatch();
+
+        await().until(future2::isDone);
+        assertThat(future2.isCompletedExceptionally(), is(true));
+        assertThat(cause(future2), instanceOf(IllegalStateException.class));
+
+        assertThat(loadCalls, equalTo(asList(singletonList(1), singletonList(1))));
+    }
+
 
     // Accepts object key in custom cacheKey function
 
@@ -968,7 +990,6 @@ public void should_allow_composition_of_data_loader_calls() throws Exception {
     }
 
 
-
     private static CacheKey<JsonObject> getJsonObjectCacheMapFn() {
         return key -> key.stream()
                 .map(entry -> entry.getKey() + ":" + entry.getValue())