Skip to content

Breaking change - renaming old mutable setXX methods #191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 12, 2025
Merged

Breaking change - renaming old mutable setXX methods #191

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4569c43
Breaking change - renaming old mutable setXX methods
bbakerman May 1, 2025
533065b
Breaking change - renaming old mutable setXX methods and going to bui…
bbakerman May 2, 2025
2443363
Merge remote-tracking branch 'origin/master' into options-breaking-ch…
bbakerman May 12, 2025
51c21d5
Fixed tests after merging master
bbakerman May 12, 2025
bdb1011
Merge remote-tracking branch 'origin/master' into options-breaking-ch…
bbakerman May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ for the context object.

```java
DataLoaderOptions options = DataLoaderOptions.newOptions()
.setBatchLoaderContextProvider(() -> SecurityCtx.getCallingUserCtx());
.withBatchLoaderContextProvider(() -> SecurityCtx.getCallingUserCtx());

BatchLoaderWithContext<String, String> batchLoader = new BatchLoaderWithContext<String, String>() {
@Override
Expand All @@ -227,7 +227,7 @@ You can gain access to them as a map by key or as the original list of context o

```java
DataLoaderOptions options = DataLoaderOptions.newOptions()
.setBatchLoaderContextProvider(() -> SecurityCtx.getCallingUserCtx());
.withBatchLoaderContextProvider(() -> SecurityCtx.getCallingUserCtx());

BatchLoaderWithContext<String, String> batchLoader = new BatchLoaderWithContext<String, String>() {
@Override
Expand Down Expand Up @@ -433,7 +433,7 @@ However, you can create your own custom future cache and supply it to the data l

```java
MyCustomCache customCache = new MyCustomCache();
DataLoaderOptions options = DataLoaderOptions.newOptions().setCacheMap(customCache);
DataLoaderOptions options = DataLoaderOptions.newOptions().withsetCacheMap(customCache);
DataLoaderFactory.newDataLoader(userBatchLoader, options);
```

Expand Down Expand Up @@ -467,7 +467,7 @@ The tests have an example based on [Caffeine](https://github.com/ben-manes/caffe
In certain uncommon cases, a DataLoader which does not cache may be desirable.

```java
DataLoaderFactory.newDataLoader(userBatchLoader, DataLoaderOptions.newOptions().setCachingEnabled(false));
DataLoaderFactory.newDataLoader(userBatchLoader, DataLoaderOptions.newOptions().withCachingEnabled(false));
```

Calling the above will ensure that every call to `.load()` will produce a new promise, and requested keys will not be saved in memory.
Expand Down Expand Up @@ -533,7 +533,7 @@ Knowing what the behaviour of your data is important for you to understand how e
You can configure the statistics collector used when you build the data loader

```java
DataLoaderOptions options = DataLoaderOptions.newOptions().setStatisticsCollector(() -> new ThreadLocalStatisticsCollector());
DataLoaderOptions options = DataLoaderOptions.newOptions().withStatisticsCollector(() -> new ThreadLocalStatisticsCollector());
DataLoader<String,User> userDataLoader = DataLoaderFactory.newDataLoader(userBatchLoader,options);

```
Expand Down Expand Up @@ -780,7 +780,7 @@ You set the `DataLoaderInstrumentation` into the `DataLoaderOptions` at build ti
});
}
};
DataLoaderOptions options = DataLoaderOptions.newOptions().setInstrumentation(timingInstrumentation);
DataLoaderOptions options = DataLoaderOptions.newOptions().withInstrumentation(timingInstrumentation);
DataLoader<String, User> userDataLoader = DataLoaderFactory.newDataLoader(userBatchLoader, options);

```
Expand Down
24 changes: 12 additions & 12 deletions src/main/java/org/dataloader/DataLoaderOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ public boolean batchingEnabled() {
* @param batchingEnabled {@code true} to enable batch loading, {@code false} otherwise
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setBatchingEnabled(boolean batchingEnabled) {
public DataLoaderOptions withBatchingEnabled(boolean batchingEnabled) {
return builder().setBatchingEnabled(batchingEnabled).build();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the object -> mutate -> new object methods are removed and now only in the builder


Expand All @@ -196,7 +196,7 @@ public boolean cachingEnabled() {
* @param cachingEnabled {@code true} to enable caching, {@code false} otherwise
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setCachingEnabled(boolean cachingEnabled) {
public DataLoaderOptions withCachingEnabled(boolean cachingEnabled) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about making these static to discourage chaining and removing the newOptions function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean by static. But while agree we should have made a compiler breaking change the first time (which we are now) I do still want to retain immutable options chaining.

Copy link

@zsmoore zsmoore May 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chaining is good however even with this change you can still write

DataLoaderOptions options = DataLoaderOptions.newOptions()
                .withMaxBatchSize(5);
        
        options.withBatchingEnabled(false);

Which is breaking

I feel like instead if you remove newOptions() and swap to static the IDE will hint the bad pattern of static methods on the instance, no longer recommend chaining on the concrete instance and hint people towards chaining only on the builder

Screenshot 2025-05-01 at 9 39 25 AM IDE hinting to not use the static methods on the instance which would break the behavior Screenshot 2025-05-01 at 9 48 11 AM

IDE not suggesting chaining of the static methods

Screenshot 2025-05-01 at 9 51 52 AM

(It will allow you to explicitly type it out but won't suggest it and will warn if you do) This code is technically valid but i feel like we want to push people to the Builder since it exists now

Screenshot 2025-05-01 at 9 41 49 AM

Push people towards proper builder pattern

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally I dont mind options.setX(x).setY(y) but since we stuff up the API during the immutability change then we have created a muddy water situation

So you are right, we need to be more clear in this case because of the previous history

So I have deleted all the with method and now only have the builder. So every one has to move to a new place which is more sympathetic to immutability

Thanks for the feedback.

return builder().setCachingEnabled(cachingEnabled).build();
}

Expand All @@ -220,7 +220,7 @@ public boolean cachingExceptionsEnabled() {
* @param cachingExceptionsEnabled {@code true} to enable caching exceptional values, {@code false} otherwise
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setCachingExceptionsEnabled(boolean cachingExceptionsEnabled) {
public DataLoaderOptions withCachingExceptionsEnabled(boolean cachingExceptionsEnabled) {
return builder().setCachingExceptionsEnabled(cachingExceptionsEnabled).build();
}

Expand All @@ -241,7 +241,7 @@ public Optional<CacheKey> cacheKeyFunction() {
* @param cacheKeyFunction the cache key function to use
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setCacheKeyFunction(CacheKey<?> cacheKeyFunction) {
public DataLoaderOptions withCacheKeyFunction(CacheKey<?> cacheKeyFunction) {
return builder().setCacheKeyFunction(cacheKeyFunction).build();
}

Expand All @@ -262,7 +262,7 @@ public DataLoaderOptions setCacheKeyFunction(CacheKey<?> cacheKeyFunction) {
* @param cacheMap the cache map instance
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setCacheMap(CacheMap<?, ?> cacheMap) {
public DataLoaderOptions withCacheMap(CacheMap<?, ?> cacheMap) {
return builder().setCacheMap(cacheMap).build();
}

Expand All @@ -283,7 +283,7 @@ public int maxBatchSize() {
* @param maxBatchSize the maximum batch size
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setMaxBatchSize(int maxBatchSize) {
public DataLoaderOptions withMaxBatchSize(int maxBatchSize) {
return builder().setMaxBatchSize(maxBatchSize).build();
}

Expand All @@ -302,7 +302,7 @@ public StatisticsCollector getStatisticsCollector() {
* @param statisticsCollector the statistics collector to use
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setStatisticsCollector(Supplier<StatisticsCollector> statisticsCollector) {
public DataLoaderOptions withStatisticsCollector(Supplier<StatisticsCollector> statisticsCollector) {
return builder().setStatisticsCollector(nonNull(statisticsCollector)).build();
}

Expand All @@ -319,7 +319,7 @@ public BatchLoaderContextProvider getBatchLoaderContextProvider() {
* @param contextProvider the batch loader context provider
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setBatchLoaderContextProvider(BatchLoaderContextProvider contextProvider) {
public DataLoaderOptions withBatchLoaderContextProvider(BatchLoaderContextProvider contextProvider) {
return builder().setBatchLoaderContextProvider(nonNull(contextProvider)).build();
}

Expand All @@ -340,7 +340,7 @@ public DataLoaderOptions setBatchLoaderContextProvider(BatchLoaderContextProvide
* @param valueCache the value cache instance
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setValueCache(ValueCache<?, ?> valueCache) {
public DataLoaderOptions withValueCache(ValueCache<?, ?> valueCache) {
return builder().setValueCache(valueCache).build();
}

Expand All @@ -357,7 +357,7 @@ public ValueCacheOptions getValueCacheOptions() {
* @param valueCacheOptions the value cache options
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setValueCacheOptions(ValueCacheOptions valueCacheOptions) {
public DataLoaderOptions withValueCacheOptions(ValueCacheOptions valueCacheOptions) {
return builder().setValueCacheOptions(nonNull(valueCacheOptions)).build();
}

Expand All @@ -375,7 +375,7 @@ public BatchLoaderScheduler getBatchLoaderScheduler() {
* @param batchLoaderScheduler the scheduler
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setBatchLoaderScheduler(BatchLoaderScheduler batchLoaderScheduler) {
public DataLoaderOptions withBatchLoaderScheduler(BatchLoaderScheduler batchLoaderScheduler) {
return builder().setBatchLoaderScheduler(batchLoaderScheduler).build();
}

Expand All @@ -392,7 +392,7 @@ public DataLoaderInstrumentation getInstrumentation() {
* @param instrumentation the new {@link DataLoaderInstrumentation}
* @return a new data loader options instance for fluent coding
*/
public DataLoaderOptions setInstrumentation(DataLoaderInstrumentation instrumentation) {
public DataLoaderOptions withInstrumentation(DataLoaderInstrumentation instrumentation) {
return builder().setInstrumentation(instrumentation).build();
}

Expand Down
12 changes: 6 additions & 6 deletions src/test/java/ReadmeExamples.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public CompletionStage<List<User>> load(List<Long> userIds) {

private void callContextExample() {
DataLoaderOptions options = DataLoaderOptions.newOptions()
.setBatchLoaderContextProvider(() -> SecurityCtx.getCallingUserCtx());
.withBatchLoaderContextProvider(() -> SecurityCtx.getCallingUserCtx());

BatchLoaderWithContext<String, String> batchLoader = new BatchLoaderWithContext<String, String>() {
@Override
Expand All @@ -120,7 +120,7 @@ public CompletionStage<List<String>> load(List<String> keys, BatchLoaderEnvironm

private void keyContextExample() {
DataLoaderOptions options = DataLoaderOptions.newOptions()
.setBatchLoaderContextProvider(() -> SecurityCtx.getCallingUserCtx());
.withBatchLoaderContextProvider(() -> SecurityCtx.getCallingUserCtx());

BatchLoaderWithContext<String, String> batchLoader = new BatchLoaderWithContext<String, String>() {
@Override
Expand Down Expand Up @@ -236,7 +236,7 @@ private void clearCacheOnError() {
BatchLoader<String, User> teamsBatchLoader;

private void disableCache() {
DataLoaderFactory.newDataLoader(userBatchLoader, DataLoaderOptions.newOptions().setCachingEnabled(false));
DataLoaderFactory.newDataLoader(userBatchLoader, DataLoaderOptions.newOptions().withCachingEnabled(false));


userDataLoader.load("A");
Expand Down Expand Up @@ -283,7 +283,7 @@ public CacheMap clear() {
private void customCache() {

MyCustomCache customCache = new MyCustomCache();
DataLoaderOptions options = DataLoaderOptions.newOptions().setCacheMap(customCache);
DataLoaderOptions options = DataLoaderOptions.newOptions().withCacheMap(customCache);
DataLoaderFactory.newDataLoader(userBatchLoader, options);
}

Expand Down Expand Up @@ -311,7 +311,7 @@ private void statsExample() {

private void statsConfigExample() {

DataLoaderOptions options = DataLoaderOptions.newOptions().setStatisticsCollector(() -> new ThreadLocalStatisticsCollector());
DataLoaderOptions options = DataLoaderOptions.newOptions().withStatisticsCollector(() -> new ThreadLocalStatisticsCollector());
DataLoader<String, User> userDataLoader = DataLoaderFactory.newDataLoader(userBatchLoader, options);
}

Expand Down Expand Up @@ -410,7 +410,7 @@ public DataLoaderInstrumentationContext<List<?>> beginBatchLoader(DataLoader<?,
});
}
};
DataLoaderOptions options = DataLoaderOptions.newOptions().setInstrumentation(timingInstrumentation);
DataLoaderOptions options = DataLoaderOptions.newOptions().withInstrumentation(timingInstrumentation);
DataLoader<String, User> userDataLoader = DataLoaderFactory.newDataLoader(userBatchLoader, options);
}

Expand Down
16 changes: 8 additions & 8 deletions src/test/java/org/dataloader/DataLoaderBatchLoaderEnvironmentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public void context_is_passed_to_batch_loader_function() {
return CompletableFuture.completedFuture(list);
};
DataLoaderOptions options = DataLoaderOptions.newOptions()
.setBatchLoaderContextProvider(() -> "ctx");
.withBatchLoaderContextProvider(() -> "ctx");
DataLoader<String, String> loader = newDataLoader(batchLoader, options);

loader.load("A");
Expand All @@ -61,7 +61,7 @@ public void context_is_passed_to_batch_loader_function() {
public void key_contexts_are_passed_to_batch_loader_function() {
BatchLoaderWithContext<String, String> batchLoader = contextBatchLoader();
DataLoaderOptions options = DataLoaderOptions.newOptions()
.setBatchLoaderContextProvider(() -> "ctx");
.withBatchLoaderContextProvider(() -> "ctx");
DataLoader<String, String> loader = newDataLoader(batchLoader, options);

loader.load("A", "aCtx");
Expand All @@ -81,8 +81,8 @@ public void key_contexts_are_passed_to_batch_loader_function() {
public void key_contexts_are_passed_to_batch_loader_function_when_batching_disabled() {
BatchLoaderWithContext<String, String> batchLoader = contextBatchLoader();
DataLoaderOptions options = DataLoaderOptions.newOptions()
.setBatchingEnabled(false)
.setBatchLoaderContextProvider(() -> "ctx");
.withBatchingEnabled(false)
.withBatchLoaderContextProvider(() -> "ctx");
DataLoader<String, String> loader = newDataLoader(batchLoader, options);

CompletableFuture<String> aLoad = loader.load("A", "aCtx");
Expand All @@ -104,7 +104,7 @@ public void key_contexts_are_passed_to_batch_loader_function_when_batching_disab
public void missing_key_contexts_are_passed_to_batch_loader_function() {
BatchLoaderWithContext<String, String> batchLoader = contextBatchLoader();
DataLoaderOptions options = DataLoaderOptions.newOptions()
.setBatchLoaderContextProvider(() -> "ctx");
.withBatchLoaderContextProvider(() -> "ctx");
DataLoader<String, String> loader = newDataLoader(batchLoader, options);

loader.load("A", "aCtx");
Expand Down Expand Up @@ -133,7 +133,7 @@ public void context_is_passed_to_map_batch_loader_function() {
return CompletableFuture.completedFuture(map);
};
DataLoaderOptions options = DataLoaderOptions.newOptions()
.setBatchLoaderContextProvider(() -> "ctx");
.withBatchLoaderContextProvider(() -> "ctx");
DataLoader<String, String> loader = newMappedDataLoader(mapBatchLoader, options);

loader.load("A", "aCtx");
Expand Down Expand Up @@ -199,8 +199,8 @@ public void null_is_passed_as_context_to_map_loader_if_you_do_nothing() {
public void mmap_semantics_apply_to_batch_loader_context() {
BatchLoaderWithContext<String, String> batchLoader = contextBatchLoader();
DataLoaderOptions options = DataLoaderOptions.newOptions()
.setBatchLoaderContextProvider(() -> "ctx")
.setCachingEnabled(false);
.withBatchLoaderContextProvider(() -> "ctx")
.withCachingEnabled(false);
DataLoader<String, String> loader = newDataLoader(batchLoader, options);

loader.load("A", "aCtx");
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/dataloader/DataLoaderBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ public class DataLoaderBuilderTest {
BatchLoader<String, Object> batchLoader2 = keys -> null;

DataLoaderOptions defaultOptions = DataLoaderOptions.newOptions();
DataLoaderOptions differentOptions = DataLoaderOptions.newOptions().setCachingEnabled(false);
DataLoaderOptions differentOptions = DataLoaderOptions.newOptions().withCachingEnabled(false);

@Test
void canBuildNewDataLoaders() {
Expand Down
20 changes: 10 additions & 10 deletions src/test/java/org/dataloader/DataLoaderOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,25 +89,25 @@ public Object getKey(Object input) {

@Test
void canBuildOk() {
assertThat(optionsDefault.setBatchingEnabled(false).batchingEnabled(),
assertThat(optionsDefault.withBatchingEnabled(false).batchingEnabled(),
equalTo(false));
assertThat(optionsDefault.setBatchLoaderScheduler(testBatchLoaderScheduler).getBatchLoaderScheduler(),
assertThat(optionsDefault.withBatchLoaderScheduler(testBatchLoaderScheduler).getBatchLoaderScheduler(),
equalTo(testBatchLoaderScheduler));
assertThat(optionsDefault.setBatchLoaderContextProvider(testBatchLoaderContextProvider).getBatchLoaderContextProvider(),
assertThat(optionsDefault.withBatchLoaderContextProvider(testBatchLoaderContextProvider).getBatchLoaderContextProvider(),
equalTo(testBatchLoaderContextProvider));
assertThat(optionsDefault.setCacheMap(testCacheMap).cacheMap().get(),
assertThat(optionsDefault.withCacheMap(testCacheMap).cacheMap().get(),
equalTo(testCacheMap));
assertThat(optionsDefault.setCachingEnabled(false).cachingEnabled(),
assertThat(optionsDefault.withCachingEnabled(false).cachingEnabled(),
equalTo(false));
assertThat(optionsDefault.setValueCacheOptions(testValueCacheOptions).getValueCacheOptions(),
assertThat(optionsDefault.withValueCacheOptions(testValueCacheOptions).getValueCacheOptions(),
equalTo(testValueCacheOptions));
assertThat(optionsDefault.setCacheKeyFunction(testCacheKey).cacheKeyFunction().get(),
assertThat(optionsDefault.withCacheKeyFunction(testCacheKey).cacheKeyFunction().get(),
equalTo(testCacheKey));
assertThat(optionsDefault.setValueCache(testValueCache).valueCache().get(),
assertThat(optionsDefault.withValueCache(testValueCache).valueCache().get(),
equalTo(testValueCache));
assertThat(optionsDefault.setMaxBatchSize(10).maxBatchSize(),
assertThat(optionsDefault.withMaxBatchSize(10).maxBatchSize(),
equalTo(10));
assertThat(optionsDefault.setStatisticsCollector(testStatisticsCollectorSupplier).getStatisticsCollector(),
assertThat(optionsDefault.withStatisticsCollector(testStatisticsCollectorSupplier).getStatisticsCollector(),
equalTo(testStatisticsCollectorSupplier.get()));

DataLoaderOptions builtOptions = optionsDefault.transform(builder -> {
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/org/dataloader/DataLoaderRegistryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,13 @@ public void stats_can_be_collected() {
DataLoaderRegistry registry = new DataLoaderRegistry();

DataLoader<Object, Object> dlA = newDataLoader(identityBatchLoader,
DataLoaderOptions.newOptions().setStatisticsCollector(SimpleStatisticsCollector::new)
DataLoaderOptions.newOptions().withStatisticsCollector(SimpleStatisticsCollector::new)
);
DataLoader<Object, Object> dlB = newDataLoader(identityBatchLoader,
DataLoaderOptions.newOptions().setStatisticsCollector(SimpleStatisticsCollector::new)
DataLoaderOptions.newOptions().withStatisticsCollector(SimpleStatisticsCollector::new)
);
DataLoader<Object, Object> dlC = newDataLoader(identityBatchLoader,
DataLoaderOptions.newOptions().setStatisticsCollector(SimpleStatisticsCollector::new)
DataLoaderOptions.newOptions().withStatisticsCollector(SimpleStatisticsCollector::new)
);

registry.register("a", dlA).register("b", dlB).register("c", dlC);
Expand Down
Loading