Set makes more sense than List with Mapped batch loaders

Author: bbakerman

README.md

@@ -217,15 +217,17 @@ For example, let's assume you want to load users from a database, you could prob
  create the list of results, with nulls filling in for missing values.
 
 ```java
-        MapBatchLoader<Long, User> mapBatchLoader = new MapBatchLoader<Long, User>() {
+        MappedBatchLoaderWithContext<Long, User> mapBatchLoader = new MappedBatchLoaderWithContext<Long, User>() {
             @Override
-            public CompletionStage<Map<Long, User>> load(List<Long> userIds, BatchLoaderEnvironment environment) {
+            public CompletionStage<Map<Long, User>> load(Set<Long> userIds, BatchLoaderEnvironment environment) {
                 SecurityCtx callCtx = environment.getContext();
                 return CompletableFuture.supplyAsync(() -> userManager.loadMapOfUsersById(callCtx, userIds));
             }
         };
 
-        DataLoader<Long, User> userLoader = DataLoader.newDataLoader(mapBatchLoader);
+        DataLoader<Long, User> userLoader = DataLoader.newMappedDataLoader(mapBatchLoader);
+
+        // ...
 
         // ...
 ```

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

@@ -6,6 +6,7 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.CompletableFuture;
@@ -275,9 +276,9 @@ private CompletionStage<List<V>> invokeListBatchLoader(List<K> keys, BatchLoader
     private CompletionStage<List<V>> invokeMapBatchLoader(List<K> keys, BatchLoaderEnvironment environment) {
         CompletionStage<Map<K, V>> loadResult;
         if (batchLoadFunction instanceof MappedBatchLoaderWithContext) {
-            loadResult = ((MappedBatchLoaderWithContext<K, V>) batchLoadFunction).load(keys, environment);
+            loadResult = ((MappedBatchLoaderWithContext<K, V>) batchLoadFunction).load(new LinkedHashSet<>(keys), environment);
         } else {
-            loadResult = ((MappedBatchLoader<K, V>) batchLoadFunction).load(keys);
+            loadResult = ((MappedBatchLoader<K, V>) batchLoadFunction).load(new LinkedHashSet<>(keys));
         }
         CompletionStage<Map<K, V>> mapBatchLoad = nonNull(loadResult, "Your batch loader function MUST return a non null CompletionStage promise");
         return mapBatchLoad.thenApply(map -> {

src/main/java/org/dataloader/MappedBatchLoader.java

@@ -18,19 +18,19 @@
 
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.CompletionStage;
 
 /**
- * A function that is invoked for batch loading a map of of data values indicated by the provided list of keys. The
+ * A function that is invoked for batch loading a map of of data values indicated by the provided set of keys. The
  * function returns a promise of a map of results of individual load requests.
  * <p>
  * There are a few constraints that must be upheld:
  * <ul>
  * <li>The keys MUST be able to be first class keys in a Java map.  Get your equals() and hashCode() methods in order</li>
- * <li>The caller of the {@link org.dataloader.DataLoader} that uses this batch loader function MUSt be able to cope with
+ * <li>The caller of the {@link org.dataloader.DataLoader} that uses this batch loader function MUST be able to cope with
  * null values coming back as results
  * </li>
- * <li>The function MUST be resilient to the same key being presented twice.</li>
  * </ul>
  * <p>
  * This form is useful when you don't have a 1:1 mapping of keys to values or when null is an acceptable value for a missing value.
@@ -50,9 +50,6 @@
  * <p>
  * This means that if 10 keys are asked for then {@link DataLoader#dispatch()} will return a promise of 10 value results and each
  * of the {@link org.dataloader.DataLoader#load(Object)} will complete with a value, null or an exception.
- * <p>
- * When caching is disabled, its possible for the same key to be presented in the list of keys more than once.  Your map
- * batch loader function needs to be resilient to this situation.
  *
  * @param <K> type parameter indicating the type of keys to use for data load requests.
  * @param <V> type parameter indicating the type of values returned
@@ -63,10 +60,10 @@ public interface MappedBatchLoader<K, V> {
     /**
      * Called to batch load the provided keys and return a promise to a map of values.
      *
-     * @param keys        the collection of keys to load
+     * @param keys        the set of keys to load
      *
      * @return a promise to a map of values for those keys
      */
     @SuppressWarnings("unused")
-    CompletionStage<Map<K, V>> load(List<K> keys);
+    CompletionStage<Map<K, V>> load(Set<K> keys);
 }

src/main/java/org/dataloader/MappedBatchLoaderWithContext.java

@@ -18,6 +18,7 @@
 
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.CompletionStage;
 
 /**
@@ -32,11 +33,11 @@ public interface MappedBatchLoaderWithContext<K, V> {
     /**
      * Called to batch load the provided keys and return a promise to a map of values.
      *
-     * @param keys        the collection of keys to load
+     * @param keys        the set of keys to load
      * @param environment the calling environment
      *
      * @return a promise to a map of values for those keys
      */
     @SuppressWarnings("unused")
-    CompletionStage<Map<K, V>> load(List<K> keys, BatchLoaderEnvironment environment);
+    CompletionStage<Map<K, V>> load(Set<K> keys, BatchLoaderEnvironment environment);
 }

src/test/java/ReadmeExamples.java

@@ -15,6 +15,7 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionStage;
 import java.util.stream.Collectors;
@@ -105,7 +106,7 @@ private CompletionStage<List<String>> callDatabaseForResults(SecurityCtx callCtx
     private void mapBatchLoader() {
         MappedBatchLoaderWithContext<Long, User> mapBatchLoader = new MappedBatchLoaderWithContext<Long, User>() {
             @Override
-            public CompletionStage<Map<Long, User>> load(List<Long> userIds, BatchLoaderEnvironment environment) {
+            public CompletionStage<Map<Long, User>> load(Set<Long> userIds, BatchLoaderEnvironment environment) {
                 SecurityCtx callCtx = environment.getContext();
                 return CompletableFuture.supplyAsync(() -> userManager.loadMapOfUsersById(callCtx, userIds));
             }

src/test/java/org/dataloader/DataLoaderMapBatchLoaderTest.java

@@ -9,6 +9,7 @@
 import java.util.Map;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import static java.util.Arrays.asList;
 import static java.util.Collections.singletonList;
@@ -33,12 +34,14 @@ public class DataLoaderMapBatchLoaderTest {
 
     MappedBatchLoader<String, String> evensOnlyMappedBatchLoader = (keys) -> {
         Map<String, String> mapOfResults = new HashMap<>();
-        for (int i = 0; i < keys.size(); i++) {
-            String k = keys.get(i);
+
+        AtomicInteger index = new AtomicInteger();
+        keys.forEach(k -> {
+            int i = index.getAndIncrement();
             if (i % 2 == 0) {
                 mapOfResults.put(k, k);
             }
-        }
+        });
         return CompletableFuture.completedFuture(mapOfResults);
     };
 
@@ -153,7 +156,9 @@ public void should_work_with_duplicate_keys_when_caching_disabled() throws Execu
         assertThat(future1.get(), equalTo("A"));
         assertThat(future2.get(), equalTo("B"));
         assertThat(future3.get(), equalTo("A"));
-        assertThat(loadCalls, equalTo(singletonList(asList("A", "B", "A"))));
+
+        // the map batch functions use a set of keys as input and hence remove duplicates unlike list variant
+        assertThat(loadCalls, equalTo(singletonList(asList("A", "B"))));
     }
 
     @Test

src/test/java/org/dataloader/DataLoaderWithTryTest.java

@@ -8,6 +8,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
 
 import static java.util.Arrays.asList;
 import static java.util.Collections.singletonList;
@@ -45,7 +46,7 @@ public void should_handle_Trys_coming_back_from_mapped_batchLoader() throws Exce
 
         List<List<String>> batchKeyCalls = new ArrayList<>();
         MappedBatchLoaderWithContext<String, Try<String>> batchLoader = (keys, environment) -> {
-            batchKeyCalls.add(keys);
+            batchKeyCalls.add(new ArrayList<>(keys));
 
             Map<String, Try<String>> result = new HashMap<>();
             for (String key : keys) {

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

@@ -4,6 +4,7 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.stream.Collectors;
 
 @SuppressWarnings({"unused", "SpellCheckingInspection", "NonAsciiCharacters"})
@@ -51,7 +52,7 @@ public List<User> loadUsersById(List<Long> userIds) {
         return userIds.stream().map(this::loadUserById).collect(Collectors.toList());
     }
 
-    public Map<Long, User> loadMapOfUsersById(SecurityCtx callCtx, List<Long> userIds) {
+    public Map<Long, User> loadMapOfUsersById(SecurityCtx callCtx, Set<Long> userIds) {
         Map<Long, User> map = new HashMap<>();
         userIds.forEach(userId -> {
             User user = loadUserById(userId);