Merged in master and tweaked immutable building

Author: bbakerman

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

@@ -86,6 +86,7 @@ private DataLoaderOptions(Builder builder) {
         this.environmentProvider = builder.environmentProvider;
         this.valueCacheOptions = builder.valueCacheOptions;
         this.batchLoaderScheduler = builder.batchLoaderScheduler;
+        this.instrumentation = builder.instrumentation;
     }
 
     /**
@@ -174,7 +175,7 @@ public boolean batchingEnabled() {
      * Sets the option that determines whether batch loading is enabled.
      *
      * @param batchingEnabled {@code true} to enable batch loading, {@code false} otherwise
-     * @return the data loader options for fluent coding
+     * @return a new data loader options instance for fluent coding
      */
     public DataLoaderOptions setBatchingEnabled(boolean batchingEnabled) {
         return builder().setBatchingEnabled(batchingEnabled).build();
@@ -193,7 +194,7 @@ public boolean cachingEnabled() {
      * Sets the option that determines whether caching is enabled.
      *
      * @param cachingEnabled {@code true} to enable caching, {@code false} otherwise
-     * @return the data loader options for fluent coding
+     * @return a new data loader options instance for fluent coding
      */
     public DataLoaderOptions setCachingEnabled(boolean cachingEnabled) {
         return builder().setCachingEnabled(cachingEnabled).build();
@@ -217,7 +218,7 @@ public boolean cachingExceptionsEnabled() {
      * Sets the option that determines whether exceptional values are cache enabled.
      *
      * @param cachingExceptionsEnabled {@code true} to enable caching exceptional values, {@code false} otherwise
-     * @return the data loader options for fluent coding
+     * @return a new data loader options instance for fluent coding
      */
     public DataLoaderOptions setCachingExceptionsEnabled(boolean cachingExceptionsEnabled) {
         return builder().setCachingExceptionsEnabled(cachingExceptionsEnabled).build();
@@ -238,7 +239,7 @@ public Optional<CacheKey> cacheKeyFunction() {
      * Sets the function to use for creating the cache key, if caching is enabled.
      *
      * @param cacheKeyFunction the cache key function to use
-     * @return the data loader options for fluent coding
+     * @return a new data loader options instance for fluent coding
      */
     public DataLoaderOptions setCacheKeyFunction(CacheKey<?> cacheKeyFunction) {
         return builder().setCacheKeyFunction(cacheKeyFunction).build();
@@ -259,7 +260,7 @@ public DataLoaderOptions setCacheKeyFunction(CacheKey<?> cacheKeyFunction) {
      * Sets the cache map implementation to use for caching, if caching is enabled.
      *
      * @param cacheMap the cache map instance
-     * @return the data loader options for fluent coding
+     * @return a new data loader options instance for fluent coding
      */
     public DataLoaderOptions setCacheMap(CacheMap<?, ?> cacheMap) {
         return builder().setCacheMap(cacheMap).build();
@@ -280,7 +281,7 @@ public int maxBatchSize() {
      * before they are split into multiple class
      *
      * @param maxBatchSize the maximum batch size
-     * @return the data loader options for fluent coding
+     * @return a new data loader options instance for fluent coding
      */
     public DataLoaderOptions setMaxBatchSize(int maxBatchSize) {
         return builder().setMaxBatchSize(maxBatchSize).build();
@@ -299,7 +300,7 @@ public StatisticsCollector getStatisticsCollector() {
      * a common value
      *
      * @param statisticsCollector the statistics collector to use
-     * @return the data loader options for fluent coding
+     * @return a new data loader options instance for fluent coding
      */
     public DataLoaderOptions setStatisticsCollector(Supplier<StatisticsCollector> statisticsCollector) {
         return builder().setStatisticsCollector(nonNull(statisticsCollector)).build();
@@ -316,7 +317,7 @@ public BatchLoaderContextProvider getBatchLoaderContextProvider() {
      * Sets the batch loader environment provider that will be used to give context to batch load functions
      *
      * @param contextProvider the batch loader context provider
-     * @return the data loader options for fluent coding
+     * @return a new data loader options instance for fluent coding
      */
     public DataLoaderOptions setBatchLoaderContextProvider(BatchLoaderContextProvider contextProvider) {
         return builder().setBatchLoaderContextProvider(nonNull(contextProvider)).build();
@@ -337,7 +338,7 @@ public DataLoaderOptions setBatchLoaderContextProvider(BatchLoaderContextProvide
      * Sets the value cache implementation to use for caching values, if caching is enabled.
      *
      * @param valueCache the value cache instance
-     * @return the data loader options for fluent coding
+     * @return a new data loader options instance for fluent coding
      */
     public DataLoaderOptions setValueCache(ValueCache<?, ?> valueCache) {
         return builder().setValueCache(valueCache).build();
@@ -354,7 +355,7 @@ public ValueCacheOptions getValueCacheOptions() {
      * Sets the {@link ValueCacheOptions} that control how the {@link ValueCache} will be used
      *
      * @param valueCacheOptions the value cache options
-     * @return the data loader options for fluent coding
+     * @return a new data loader options instance for fluent coding
      */
     public DataLoaderOptions setValueCacheOptions(ValueCacheOptions valueCacheOptions) {
         return builder().setValueCacheOptions(nonNull(valueCacheOptions)).build();
@@ -372,12 +373,29 @@ public BatchLoaderScheduler getBatchLoaderScheduler() {
      * to some future time.
      *
      * @param batchLoaderScheduler the scheduler
-     * @return the data loader options for fluent coding
+     * @return a new data loader options instance for fluent coding
      */
     public DataLoaderOptions setBatchLoaderScheduler(BatchLoaderScheduler batchLoaderScheduler) {
         return builder().setBatchLoaderScheduler(batchLoaderScheduler).build();
     }
 
+    /**
+     * @return the {@link DataLoaderInstrumentation} to use
+     */
+    public DataLoaderInstrumentation getInstrumentation() {
+        return instrumentation;
+    }
+
+    /**
+     * Sets in a new {@link DataLoaderInstrumentation}
+     *
+     * @param instrumentation the new {@link DataLoaderInstrumentation}
+     * @return a new data loader options instance for fluent coding
+     */
+    public DataLoaderOptions setInstrumentation(DataLoaderInstrumentation instrumentation) {
+        return builder().setInstrumentation(instrumentation).build();
+    }
+
     private Builder builder() {
         return new Builder(this);
     }
@@ -394,6 +412,7 @@ public static class Builder {
         private BatchLoaderContextProvider environmentProvider;
         private ValueCacheOptions valueCacheOptions;
         private BatchLoaderScheduler batchLoaderScheduler;
+        private DataLoaderInstrumentation instrumentation;
 
         public Builder() {
             this(new DataLoaderOptions()); // use the defaults of the DataLoaderOptions for this builder
@@ -411,6 +430,7 @@ public Builder() {
             this.environmentProvider = other.environmentProvider;
             this.valueCacheOptions = other.valueCacheOptions;
             this.batchLoaderScheduler = other.batchLoaderScheduler;
+            this.instrumentation = other.instrumentation;
         }
 
         public Builder setBatchingEnabled(boolean batchingEnabled) {
@@ -468,27 +488,14 @@ public Builder setBatchLoaderScheduler(BatchLoaderScheduler batchLoaderScheduler
             return this;
         }
 
+        public Builder setInstrumentation(DataLoaderInstrumentation instrumentation) {
+            this.instrumentation = nonNull(instrumentation);
+            return this;
+        }
+
         public DataLoaderOptions build() {
             return new DataLoaderOptions(this);
         }
 
     }
-
-    /**
-     * @return the {@link DataLoaderInstrumentation} to use
-     */
-    public DataLoaderInstrumentation getInstrumentation() {
-        return instrumentation;
-    }
-
-    /**
-     * Sets in a new {@link DataLoaderInstrumentation}
-     *
-     * @param instrumentation the new {@link DataLoaderInstrumentation}
-     * @return the data loader options for fluent coding
-     */
-    public DataLoaderOptions setInstrumentation(DataLoaderInstrumentation instrumentation) {
-        this.instrumentation = nonNull(instrumentation);
-        return this;
-    }
 }

src/main/java/org/dataloader/DataLoaderRegistry.java

@@ -31,7 +31,8 @@
  * <p>
  * If the {@link DataLoader} has no {@link DataLoaderInstrumentation} then the registry one is added to it.  If it does have one already
  * then a {@link ChainedDataLoaderInstrumentation} is created with the registry {@link DataLoaderInstrumentation} in it first and then any other
- * {@link DataLoaderInstrumentation}s added after that.
+ * {@link DataLoaderInstrumentation}s added after that.  If the registry {@link DataLoaderInstrumentation} instance and {@link DataLoader} {@link DataLoaderInstrumentation} instance
+ * are the same object, then nothing is changed, since the same instrumentation code is being run.
  */
 @PublicApi
 public class DataLoaderRegistry {
@@ -40,14 +41,14 @@ public class DataLoaderRegistry {
 
 
     public DataLoaderRegistry() {
-        this(new ConcurrentHashMap<>(),null);
+        this(new ConcurrentHashMap<>(), null);
     }
 
     private DataLoaderRegistry(Builder builder) {
-        this(builder.dataLoaders,builder.instrumentation);
+        this(builder.dataLoaders, builder.instrumentation);
     }
 
-    protected DataLoaderRegistry(Map<String, DataLoader<?, ?>> dataLoaders,  DataLoaderInstrumentation instrumentation ) {
+    protected DataLoaderRegistry(Map<String, DataLoader<?, ?>> dataLoaders, DataLoaderInstrumentation instrumentation) {
         this.dataLoaders = instrumentDLs(dataLoaders, instrumentation);
         this.instrumentation = instrumentation;
     }
@@ -97,12 +98,16 @@ protected DataLoaderRegistry(Map<String, DataLoader<?, ?>> dataLoaders,  DataLoa
     }
 
     private static DataLoader<?, ?> mkInstrumentedDataLoader(DataLoader<?, ?> existingDL, DataLoaderOptions options, DataLoaderInstrumentation newInstrumentation) {
-        return existingDL.transform(builder -> {
-            options.setInstrumentation(newInstrumentation);
-            builder.options(options);
-        });
+        return existingDL.transform(builder -> builder.options(setInInstrumentation(options, newInstrumentation)));
+    }
+
+    private static DataLoaderOptions setInInstrumentation(DataLoaderOptions options, DataLoaderInstrumentation newInstrumentation) {
+        return options.transform(optionsBuilder -> optionsBuilder.setInstrumentation(newInstrumentation));
     }
 
+    /**
+     * @return the {@link DataLoaderInstrumentation} associated with this registry which can be null
+     */
     public DataLoaderInstrumentation getInstrumentation() {
         return instrumentation;
     }

src/test/java/org/dataloader/instrumentation/ChainedDataLoaderInstrumentationTest.java

@@ -14,7 +14,7 @@
 import java.util.concurrent.CompletableFuture;
 
 import static org.awaitility.Awaitility.await;
-import static org.dataloader.DataLoaderOptions.newOptions;
+import static org.dataloader.DataLoaderOptions.newOptionsBuilder;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
 
@@ -37,7 +37,7 @@ void canChainTogetherZeroInstrumentation() {
         // just to prove its useless but harmless
         ChainedDataLoaderInstrumentation chainedItn = new ChainedDataLoaderInstrumentation();
 
-        DataLoaderOptions options = newOptions().setInstrumentation(chainedItn);
+        DataLoaderOptions options = newOptionsBuilder().setInstrumentation(chainedItn).build();
 
         DataLoader<String, String> dl = DataLoaderFactory.newDataLoader(TestKit.keysAsValues(), options);
 
@@ -57,7 +57,7 @@ void canChainTogetherOneInstrumentation() {
         ChainedDataLoaderInstrumentation chainedItn = new ChainedDataLoaderInstrumentation()
                 .add(capturingA);
 
-        DataLoaderOptions options = newOptions().setInstrumentation(chainedItn);
+        DataLoaderOptions options = newOptionsBuilder().setInstrumentation(chainedItn).build();
 
         DataLoader<String, String> dl = DataLoaderFactory.newDataLoader(TestKit.keysAsValues(), options);
 
@@ -83,7 +83,7 @@ public void canChainTogetherManyInstrumentationsWithDifferentBatchLoaders(TestDa
                 .add(capturingB)
                 .add(capturingButReturnsNull);
 
-        DataLoaderOptions options = newOptions().setInstrumentation(chainedItn);
+        DataLoaderOptions options = newOptionsBuilder().setInstrumentation(chainedItn).build();
 
         DataLoader<String, String> dl = factory.idLoader(options);
 

src/test/java/org/dataloader/instrumentation/DataLoaderRegistryInstrumentationTest.java

@@ -1,6 +1,7 @@
 package org.dataloader.instrumentation;
 
 import org.dataloader.DataLoader;
+import org.dataloader.DataLoaderOptions;
 import org.dataloader.DataLoaderRegistry;
 import org.dataloader.fixtures.TestKit;
 import org.dataloader.fixtures.parameterized.TestDataLoaderFactory;
@@ -119,9 +120,9 @@ void wontDoAnyThingIfThereIsNoRegistryInstrumentation() {
 
     @Test
     void wontDoAnyThingIfThereTheyAreTheSameInstrumentationAlready() {
-        DataLoader<String, String> newX = dlX.transform(builder -> dlX.getOptions().setInstrumentation(instrA));
-        DataLoader<String, String> newY = dlX.transform(builder -> dlY.getOptions().setInstrumentation(instrA));
-        DataLoader<String, String> newZ = dlX.transform(builder -> dlY.getOptions().setInstrumentation(instrA));
+        DataLoader<String, String> newX = dlX.transform(builder -> builder.options(dlX.getOptions().setInstrumentation(instrA)));
+        DataLoader<String, String> newY = dlX.transform(builder ->  builder.options(dlY.getOptions().setInstrumentation(instrA)));
+        DataLoader<String, String> newZ = dlX.transform(builder ->  builder.options(dlZ.getOptions().setInstrumentation(instrA)));
         DataLoaderRegistry registry = DataLoaderRegistry.newRegistry()
                 .instrumentation(instrA)
                 .register("X", newX)
@@ -144,7 +145,8 @@ void wontDoAnyThingIfThereTheyAreTheSameInstrumentationAlready() {
 
     @Test
     void ifTheDLHasAInstrumentationThenItsTurnedIntoAChainedOne() {
-        DataLoader<String, String> newX = dlX.transform(builder -> dlX.getOptions().setInstrumentation(instrA));
+        DataLoaderOptions options = dlX.getOptions().setInstrumentation(instrA);
+        DataLoader<String, String> newX = dlX.transform(builder -> builder.options(options));
 
         DataLoaderRegistry registry = DataLoaderRegistry.newRegistry()
                 .instrumentation(instrB)
@@ -162,7 +164,8 @@ void ifTheDLHasAInstrumentationThenItsTurnedIntoAChainedOne() {
 
     @Test
     void chainedInstrumentationsWillBeCombined() {
-        DataLoader<String, String> newX = dlX.transform(builder -> builder.options(dlX.getOptions().setInstrumentation(chainedInstrB)));
+        DataLoaderOptions options = dlX.getOptions().setInstrumentation(chainedInstrB);
+        DataLoader<String, String> newX = dlX.transform(builder -> builder.options(options));
 
         DataLoaderRegistry registry = DataLoaderRegistry.newRegistry()
                 .instrumentation(instrA)