From d93392827c0dff452854c16ff093c3533a23594a Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 28 Aug 2018 16:52:28 +1000 Subject: [PATCH 1/2] Set makes more sense than List with Mapped batch loaders --- README.md | 8 +++++--- src/main/java/org/dataloader/DataLoaderHelper.java | 5 +++-- src/main/java/org/dataloader/MappedBatchLoader.java | 13 +++++-------- .../dataloader/MappedBatchLoaderWithContext.java | 5 +++-- src/test/java/ReadmeExamples.java | 3 ++- .../dataloader/DataLoaderMapBatchLoaderTest.java | 13 +++++++++---- .../java/org/dataloader/DataLoaderWithTryTest.java | 3 ++- .../java/org/dataloader/fixtures/UserManager.java | 3 ++- 8 files changed, 31 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 227c921..be33862 100644 --- a/README.md +++ b/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 mapBatchLoader = new MapBatchLoader() { + MappedBatchLoaderWithContext mapBatchLoader = new MappedBatchLoaderWithContext() { @Override - public CompletionStage> load(List userIds, BatchLoaderEnvironment environment) { + public CompletionStage> load(Set userIds, BatchLoaderEnvironment environment) { SecurityCtx callCtx = environment.getContext(); return CompletableFuture.supplyAsync(() -> userManager.loadMapOfUsersById(callCtx, userIds)); } }; - DataLoader userLoader = DataLoader.newDataLoader(mapBatchLoader); + DataLoader userLoader = DataLoader.newMappedDataLoader(mapBatchLoader); + + // ... // ... ``` diff --git a/src/main/java/org/dataloader/DataLoaderHelper.java b/src/main/java/org/dataloader/DataLoaderHelper.java index 277b810..0e58bcc 100644 --- a/src/main/java/org/dataloader/DataLoaderHelper.java +++ b/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> invokeListBatchLoader(List keys, BatchLoader private CompletionStage> invokeMapBatchLoader(List keys, BatchLoaderEnvironment environment) { CompletionStage> loadResult; if (batchLoadFunction instanceof MappedBatchLoaderWithContext) { - loadResult = ((MappedBatchLoaderWithContext) batchLoadFunction).load(keys, environment); + loadResult = ((MappedBatchLoaderWithContext) batchLoadFunction).load(new LinkedHashSet<>(keys), environment); } else { - loadResult = ((MappedBatchLoader) batchLoadFunction).load(keys); + loadResult = ((MappedBatchLoader) batchLoadFunction).load(new LinkedHashSet<>(keys)); } CompletionStage> mapBatchLoad = nonNull(loadResult, "Your batch loader function MUST return a non null CompletionStage promise"); return mapBatchLoad.thenApply(map -> { diff --git a/src/main/java/org/dataloader/MappedBatchLoader.java b/src/main/java/org/dataloader/MappedBatchLoader.java index b230e7f..4b489fa 100644 --- a/src/main/java/org/dataloader/MappedBatchLoader.java +++ b/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. *

* There are a few constraints that must be upheld: *

    *
  • The keys MUST be able to be first class keys in a Java map. Get your equals() and hashCode() methods in order
    • - *
  • The caller of the {@link org.dataloader.DataLoader} that uses this batch loader function MUSt be able to cope with + *
  • 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 *
    • - *
  • The function MUST be resilient to the same key being presented twice.
    • *
*

* 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 @@ *

* 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. - *

- * 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 type parameter indicating the type of keys to use for data load requests. * @param type parameter indicating the type of values returned @@ -63,10 +60,10 @@ public interface MappedBatchLoader { /** * 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> load(List keys); + CompletionStage> load(Set keys); } diff --git a/src/main/java/org/dataloader/MappedBatchLoaderWithContext.java b/src/main/java/org/dataloader/MappedBatchLoaderWithContext.java index a899fae..6e3a2f0 100644 --- a/src/main/java/org/dataloader/MappedBatchLoaderWithContext.java +++ b/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 { /** * 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> load(List keys, BatchLoaderEnvironment environment); + CompletionStage> load(Set keys, BatchLoaderEnvironment environment); } diff --git a/src/test/java/ReadmeExamples.java b/src/test/java/ReadmeExamples.java index 0f0d18d..c739244 100644 --- a/src/test/java/ReadmeExamples.java +++ b/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> callDatabaseForResults(SecurityCtx callCtx private void mapBatchLoader() { MappedBatchLoaderWithContext mapBatchLoader = new MappedBatchLoaderWithContext() { @Override - public CompletionStage> load(List userIds, BatchLoaderEnvironment environment) { + public CompletionStage> load(Set userIds, BatchLoaderEnvironment environment) { SecurityCtx callCtx = environment.getContext(); return CompletableFuture.supplyAsync(() -> userManager.loadMapOfUsersById(callCtx, userIds)); } diff --git a/src/test/java/org/dataloader/DataLoaderMapBatchLoaderTest.java b/src/test/java/org/dataloader/DataLoaderMapBatchLoaderTest.java index 8e83f82..1a436c1 100644 --- a/src/test/java/org/dataloader/DataLoaderMapBatchLoaderTest.java +++ b/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 evensOnlyMappedBatchLoader = (keys) -> { Map 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 diff --git a/src/test/java/org/dataloader/DataLoaderWithTryTest.java b/src/test/java/org/dataloader/DataLoaderWithTryTest.java index b926841..b2127e6 100644 --- a/src/test/java/org/dataloader/DataLoaderWithTryTest.java +++ b/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> batchKeyCalls = new ArrayList<>(); MappedBatchLoaderWithContext> batchLoader = (keys, environment) -> { - batchKeyCalls.add(keys); + batchKeyCalls.add(new ArrayList<>(keys)); Map> result = new HashMap<>(); for (String key : keys) { diff --git a/src/test/java/org/dataloader/fixtures/UserManager.java b/src/test/java/org/dataloader/fixtures/UserManager.java index 95b712e..960aeb4 100644 --- a/src/test/java/org/dataloader/fixtures/UserManager.java +++ b/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 loadUsersById(List userIds) { return userIds.stream().map(this::loadUserById).collect(Collectors.toList()); } - public Map loadMapOfUsersById(SecurityCtx callCtx, List userIds) { + public Map loadMapOfUsersById(SecurityCtx callCtx, Set userIds) { Map map = new HashMap<>(); userIds.forEach(userId -> { User user = loadUserById(userId); From 5922579fc3c7268f7afe0e07e4ef1c8fe1dca69a Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 28 Aug 2018 16:56:43 +1000 Subject: [PATCH 2/2] PR changes --- README.md | 4 +--- src/main/java/org/dataloader/DataLoaderHelper.java | 6 ++++-- src/test/java/ReadmeExamples.java | 2 +- src/test/java/org/dataloader/fixtures/UserManager.java | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index be33862..4845a28 100644 --- a/README.md +++ b/README.md @@ -221,15 +221,13 @@ For example, let's assume you want to load users from a database, you could prob @Override public CompletionStage> load(Set userIds, BatchLoaderEnvironment environment) { SecurityCtx callCtx = environment.getContext(); - return CompletableFuture.supplyAsync(() -> userManager.loadMapOfUsersById(callCtx, userIds)); + return CompletableFuture.supplyAsync(() -> userManager.loadMapOfUsersByIds(callCtx, userIds)); } }; DataLoader userLoader = DataLoader.newMappedDataLoader(mapBatchLoader); // ... - - // ... ``` ### Error object is not a thing in a type safe Java world diff --git a/src/main/java/org/dataloader/DataLoaderHelper.java b/src/main/java/org/dataloader/DataLoaderHelper.java index 0e58bcc..d1e4be5 100644 --- a/src/main/java/org/dataloader/DataLoaderHelper.java +++ b/src/main/java/org/dataloader/DataLoaderHelper.java @@ -9,6 +9,7 @@ import java.util.LinkedHashSet; 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; @@ -275,10 +276,11 @@ private CompletionStage> invokeListBatchLoader(List keys, BatchLoader @SuppressWarnings("unchecked") private CompletionStage> invokeMapBatchLoader(List keys, BatchLoaderEnvironment environment) { CompletionStage> loadResult; + Set setOfKeys = new LinkedHashSet<>(keys); if (batchLoadFunction instanceof MappedBatchLoaderWithContext) { - loadResult = ((MappedBatchLoaderWithContext) batchLoadFunction).load(new LinkedHashSet<>(keys), environment); + loadResult = ((MappedBatchLoaderWithContext) batchLoadFunction).load(setOfKeys, environment); } else { - loadResult = ((MappedBatchLoader) batchLoadFunction).load(new LinkedHashSet<>(keys)); + loadResult = ((MappedBatchLoader) batchLoadFunction).load(setOfKeys); } CompletionStage> mapBatchLoad = nonNull(loadResult, "Your batch loader function MUST return a non null CompletionStage promise"); return mapBatchLoad.thenApply(map -> { diff --git a/src/test/java/ReadmeExamples.java b/src/test/java/ReadmeExamples.java index c739244..0803728 100644 --- a/src/test/java/ReadmeExamples.java +++ b/src/test/java/ReadmeExamples.java @@ -108,7 +108,7 @@ private void mapBatchLoader() { @Override public CompletionStage> load(Set userIds, BatchLoaderEnvironment environment) { SecurityCtx callCtx = environment.getContext(); - return CompletableFuture.supplyAsync(() -> userManager.loadMapOfUsersById(callCtx, userIds)); + return CompletableFuture.supplyAsync(() -> userManager.loadMapOfUsersByIds(callCtx, userIds)); } }; diff --git a/src/test/java/org/dataloader/fixtures/UserManager.java b/src/test/java/org/dataloader/fixtures/UserManager.java index 960aeb4..24fee0d 100644 --- a/src/test/java/org/dataloader/fixtures/UserManager.java +++ b/src/test/java/org/dataloader/fixtures/UserManager.java @@ -52,7 +52,7 @@ public List loadUsersById(List userIds) { return userIds.stream().map(this::loadUserById).collect(Collectors.toList()); } - public Map loadMapOfUsersById(SecurityCtx callCtx, Set userIds) { + public Map loadMapOfUsersByIds(SecurityCtx callCtx, Set userIds) { Map map = new HashMap<>(); userIds.forEach(userId -> { User user = loadUserById(userId);