Merge pull request #181 from graphql-java/dataloader-options-spring-bug

Fixed a bug that the Spring team found

Author: bbakerman

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

@@ -139,7 +139,7 @@ public static DataLoaderOptions.Builder newDataLoaderOptions(DataLoaderOptions o
      * @return a new {@link DataLoaderOptions} object
      */
     public DataLoaderOptions transform(Consumer<Builder> builderConsumer) {
-        Builder builder = newOptionsBuilder();
+        Builder builder = newDataLoaderOptions(this);
         builderConsumer.accept(builder);
         return builder.build();
     }

src/test/java/org/dataloader/DataLoaderBuilderTest.java

@@ -46,19 +46,31 @@ void canBuildNewDataLoaders() {
 
     @Test
     void theDataLoaderCanTransform() {
-        DataLoader<String, Object> dataLoader1 = DataLoaderFactory.newDataLoader(batchLoader1, defaultOptions);
-        assertThat(dataLoader1.getOptions(), equalTo(defaultOptions));
-        assertThat(dataLoader1.getBatchLoadFunction(), equalTo(batchLoader1));
+        DataLoader<String, Object> dataLoaderOrig = DataLoaderFactory.newDataLoader(batchLoader1, defaultOptions);
+        assertThat(dataLoaderOrig.getOptions(), equalTo(defaultOptions));
+        assertThat(dataLoaderOrig.getBatchLoadFunction(), equalTo(batchLoader1));
         //
         // we can transform the data loader
         //
-        DataLoader<String, Object> dataLoader2 = dataLoader1.transform(it -> {
+        DataLoader<String, Object> dataLoaderTransformed = dataLoaderOrig.transform(it -> {
             it.options(differentOptions);
             it.batchLoadFunction(batchLoader2);
         });
 
-        assertThat(dataLoader2, not(equalTo(dataLoader1)));
-        assertThat(dataLoader2.getOptions(), equalTo(differentOptions));
-        assertThat(dataLoader2.getBatchLoadFunction(), equalTo(batchLoader2));
+        assertThat(dataLoaderTransformed, not(equalTo(dataLoaderOrig)));
+        assertThat(dataLoaderTransformed.getOptions(), equalTo(differentOptions));
+        assertThat(dataLoaderTransformed.getBatchLoadFunction(), equalTo(batchLoader2));
+
+        // can copy values
+        dataLoaderOrig = DataLoaderFactory.newDataLoader(batchLoader1, defaultOptions);
+
+        dataLoaderTransformed = dataLoaderOrig.transform(it -> {
+            it.batchLoadFunction(batchLoader2);
+        });
+
+        assertThat(dataLoaderTransformed, not(equalTo(dataLoaderOrig)));
+        assertThat(dataLoaderTransformed.getOptions(), equalTo(defaultOptions));
+        assertThat(dataLoaderTransformed.getBatchLoadFunction(), equalTo(batchLoader2));
+
     }
 }

src/test/java/org/dataloader/DataLoaderOptionsTest.java

@@ -2,9 +2,11 @@
 
 import org.dataloader.impl.DefaultCacheMap;
 import org.dataloader.impl.NoOpValueCache;
+import org.dataloader.instrumentation.DataLoaderInstrumentation;
 import org.dataloader.scheduler.BatchLoaderScheduler;
 import org.dataloader.stats.NoOpStatisticsCollector;
 import org.dataloader.stats.StatisticsCollector;
+import org.hamcrest.CoreMatchers;
 import org.junit.jupiter.api.Test;
 
 import java.util.List;
@@ -184,4 +186,45 @@ void canBuildViaBuilderOk() {
         assertThat(builtOptions.getStatisticsCollector(),
                 equalTo(testStatisticsCollectorSupplier.get()));
     }
+
+    @Test
+    void canCopyExistingOptionValuesOnTransform() {
+
+        DataLoaderInstrumentation instrumentation1 = new DataLoaderInstrumentation() {
+        };
+        DataLoaderInstrumentation instrumentation2 = new DataLoaderInstrumentation() {
+        };
+        BatchLoaderContextProvider contextProvider1 = () -> null;
+
+        DataLoaderOptions startingOptions = DataLoaderOptions.newOptionsBuilder().setBatchingEnabled(false)
+                .setCachingEnabled(false)
+                .setInstrumentation(instrumentation1)
+                .setBatchLoaderContextProvider(contextProvider1)
+                .build();
+
+        assertThat(startingOptions.batchingEnabled(), equalTo(false));
+        assertThat(startingOptions.cachingEnabled(), equalTo(false));
+        assertThat(startingOptions.getInstrumentation(), equalTo(instrumentation1));
+        assertThat(startingOptions.getBatchLoaderContextProvider(), equalTo(contextProvider1));
+
+        DataLoaderOptions newOptions = startingOptions.transform(builder ->
+                builder.setBatchingEnabled(true).setInstrumentation(instrumentation2));
+
+
+        // immutable
+        assertThat(newOptions, CoreMatchers.not(startingOptions));
+        assertThat(startingOptions.batchingEnabled(), equalTo(false));
+        assertThat(startingOptions.cachingEnabled(), equalTo(false));
+        assertThat(startingOptions.getInstrumentation(), equalTo(instrumentation1));
+        assertThat(startingOptions.getBatchLoaderContextProvider(), equalTo(contextProvider1));
+
+        // stayed the same
+        assertThat(newOptions.cachingEnabled(), equalTo(false));
+        assertThat(newOptions.getBatchLoaderContextProvider(), equalTo(contextProvider1));
+
+        // was changed
+        assertThat(newOptions.batchingEnabled(), equalTo(true));
+        assertThat(newOptions.getInstrumentation(), equalTo(instrumentation2));
+
+    }
 }