If caching is off AND batching is off - ValueCache should not be used

Author: bbakerman

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

@@ -175,7 +175,7 @@ CompletableFuture<V> load(K key, Object loadContext) {
         } else {
             stats.incrementBatchLoadCountBy(1, new IncrementBatchLoadCountByStatisticsContext<>(key, loadContext));
             // immediate execution of batch function
-            loadCallFuture = invokeLoaderImmediately(key, loadContext, true);
+            loadCallFuture = invokeLoaderImmediately(key, loadContext, loaderOptions.cachingEnabled());
             if (futureCachingEnabled) {
                 CompletableFuture<V> cachedFuture = futureCache.putIfAbsentAtomically(cacheKey, loadCallFuture);
                 if (cachedFuture != null) {
@@ -228,6 +228,7 @@ DispatchResult<V> dispatch() {
         boolean batchingEnabled = loaderOptions.batchingEnabled();
 
         LoaderQueueEntry<K, V> loaderQueueEntryHead;
+        //noinspection ConditionalBreakInInfiniteLoop
         while (true) {
             loaderQueueEntryHead = loaderQueue.get();
             if (loaderQueue.compareAndSet(loaderQueueEntryHead, null)) {
@@ -244,7 +245,7 @@ DispatchResult<V> dispatch() {
         int queueSize = loaderQueueEntryHead.queueSize;
         // we copy the pre-loaded set of futures ready for dispatch
         Object[] keysArray = new Object[queueSize];
-        CompletableFuture[] queuedFuturesArray = new CompletableFuture[queueSize];
+        CompletableFuture<V>[] queuedFuturesArray = new CompletableFuture[queueSize];
         Object[] callContextsArray = new Object[queueSize];
         int index = queueSize - 1;
         while (loaderQueueEntryHead != null) {

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

@@ -18,6 +18,7 @@
 
 import org.awaitility.Duration;
 import org.dataloader.fixtures.CustomCacheMap;
+import org.dataloader.fixtures.CustomValueCache;
 import org.dataloader.fixtures.JsonObject;
 import org.dataloader.fixtures.User;
 import org.dataloader.fixtures.UserManager;
@@ -70,6 +71,8 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.sameInstance;
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
@@ -1009,16 +1012,27 @@ public void should_degrade_gracefully_if_cache_get_throws(TestDataLoaderFactory
     @MethodSource("org.dataloader.fixtures.parameterized.TestDataLoaderFactories#get")
     public void batching_disabled_should_dispatch_immediately(TestDataLoaderFactory factory) {
         List<Collection<String>> loadCalls = new ArrayList<>();
-        DataLoaderOptions options = newOptions().setBatchingEnabled(false).build();
+
+        CacheMap<String, String> cacheMap = CacheMap.simpleMap();
+        CustomValueCache valueCache = new CustomValueCache();
+
+        DataLoaderOptions options = newOptions().setBatchingEnabled(false)
+                .setCacheMap(cacheMap).setValueCache(valueCache).build();
         DataLoader<String, String> identityLoader = factory.idLoader(options, loadCalls);
 
         CompletableFuture<String> fa = identityLoader.load("A");
         CompletableFuture<String> fb = identityLoader.load("B");
 
+        assertThat(cacheMap.size(), equalTo(2));
+        assertThat(valueCache.asMap().size(), equalTo(2));
+
         // caching is on still
         CompletableFuture<String> fa1 = identityLoader.load("A");
         CompletableFuture<String> fb1 = identityLoader.load("B");
 
+        assertThat(fa, sameInstance(fa1));
+        assertThat(fb, sameInstance(fb1));
+
         List<String> values = CompletableFutureKit.allOf(asList(fa, fb, fa1, fb1)).join();
 
         assertThat(fa.join(), equalTo("A"));
@@ -1038,16 +1052,27 @@ public void batching_disabled_should_dispatch_immediately(TestDataLoaderFactory
     @MethodSource("org.dataloader.fixtures.parameterized.TestDataLoaderFactories#get")
     public void batching_disabled_and_caching_disabled_should_dispatch_immediately_and_forget(TestDataLoaderFactory factory) {
         List<Collection<String>> loadCalls = new ArrayList<>();
-        DataLoaderOptions options = newOptions().setBatchingEnabled(false).setCachingEnabled(false).build();
+
+        CacheMap<String, String> cacheMap = CacheMap.simpleMap();
+        CustomValueCache valueCache = new CustomValueCache();
+
+        DataLoaderOptions options = newOptions().setBatchingEnabled(false).setCachingEnabled(false)
+                .setCacheMap(cacheMap).setValueCache(valueCache).build();
         DataLoader<String, String> identityLoader = factory.idLoader(options, loadCalls);
 
         CompletableFuture<String> fa = identityLoader.load("A");
         CompletableFuture<String> fb = identityLoader.load("B");
 
-        // caching is off
+        // caching is off - it should not go to the Value cache nor the future cache
+        assertThat(cacheMap.size(), equalTo(0));
+        assertThat(valueCache.asMap().size(), equalTo(0));
+
         CompletableFuture<String> fa1 = identityLoader.load("A");
         CompletableFuture<String> fb1 = identityLoader.load("B");
 
+        assertThat(fa, not(sameInstance(fa1)));
+        assertThat(fb, not(sameInstance(fb1)));
+
         List<String> values = CompletableFutureKit.allOf(asList(fa, fb, fa1, fb1)).join();
 
         assertThat(fa.join(), equalTo("A"));

src/test/java/org/dataloader/DataLoaderValueCacheTest.java

@@ -379,6 +379,40 @@ public CompletableFuture<Object> get(String key) {
         assertTrue(customValueCache.asMap().isEmpty());
     }
 
+    @ParameterizedTest
+    @MethodSource("org.dataloader.fixtures.parameterized.TestDataLoaderFactories#get")
+    public void if_caching_is_off_and_batching_is_off_its_never_hit(TestDataLoaderFactory factory) {
+        AtomicInteger getCalls = new AtomicInteger();
+        CustomValueCache customValueCache = new CustomValueCache() {
+
+            @Override
+            public CompletableFuture<Object> get(String key) {
+                getCalls.incrementAndGet();
+                return super.get(key);
+            }
+        };
+
+        List<Collection<String>> loadCalls = new ArrayList<>();
+        DataLoaderOptions options = newOptions().setValueCache(customValueCache).setCachingEnabled(false).setBatchingEnabled(false).build();
+        DataLoader<String, String> identityLoader = factory.idLoader(options, loadCalls);
+
+        CompletableFuture<String> fA = identityLoader.load("a");
+        CompletableFuture<String> fB = identityLoader.load("b");
+        CompletableFuture<String> fC = identityLoader.load("missC");
+        CompletableFuture<String> fD = identityLoader.load("missD");
+
+        await().until(identityLoader.dispatch()::isDone);
+
+        assertThat(fA.join(), equalTo("a"));
+        assertThat(fB.join(), equalTo("b"));
+        assertThat(fC.join(), equalTo("missC"));
+        assertThat(fD.join(), equalTo("missD"));
+
+        assertThat(loadCalls, equalTo(asList(singletonList("a"), singletonList("b"), singletonList("missC"), singletonList("missD"))));
+        assertThat(getCalls.get(), equalTo(0));
+        assertTrue(customValueCache.asMap().isEmpty());
+    }
+
     @ParameterizedTest
     @MethodSource("org.dataloader.fixtures.parameterized.TestDataLoaderFactories#get")
     public void if_everything_is_cached_no_batching_happens(TestDataLoaderFactory factory) {