Add DataLoader constructor for MappedBatchLoader

[aviadavretz]

Right now the only way to create a new DataLoader backed by MappedBatchLoader is through the static methods.

This prevents extending DataLoader when I want to use a Map and not a List.

Is it possible to add another (2, one with options and one without) for MappedBatchLoader?

[bbakerman]

Let me check this out and get back to you... On my phone right now

…

On Sat., 17 Nov. 2018, 06:38 aviadavretz, ***@***.***> wrote: Right now the only way to create a new DataLoader backed by MappedBatchLoader is through the static methods. This prevents extending DataLoader when I want to use a Map and not a List. Is it possible to add another (2, one with options and one without) for MappedBatchLoader? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#37>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABlbSe46oWt2zZGEUX6XSWW5JYl580Lxks5uvxRFgaJpZM4Ym2VM> .

[bbakerman]

I wont be changing this. We don;t want to offer the constructor of the DataLoader as public API any more.

It prevents future extensions and we probably should not have offered it in the first place. The versions there are for backwards compatibility.

The contract on a DataLoader is complicated and allowing extension by subclassing would put us into a compatibility situation we dont want to be in.

This prevents extending DataLoader when I want to use a Map and not a List.

can you expand more here on what you are trying to do? Today MappedBatchLoader returns a Map. Thats its purpose.

[dinhani]

I was going to open a similar issue to request the ability to extend the DataLoader class providing a BatchLoaderWithContext instead.

[bbakerman]

There are static "constructor methods" there that allow exactly that eg

    public static <K, V> DataLoader<K, V> newDataLoader(BatchLoaderWithContext<K, V> batchLoadFunction) ...


DataLoader<String,String> dl = DataLoader.newDataLoader( (keys,context) -> ....)

This create a DataLoader that is backed by a BatchLoaderWithContext loading function.

By hiding the constructors we allow better evolution of the API.

[bbakerman]

Are you unable to do some function here?

[dinhani]

I am already using the static constructors to generate the data loaders I need.

I just think that having my classes extending the DataLoder class would bring two new benefits:

1 - It would facilitate integration with Spring framework. If for some reason two different data loaders have the same signature (same key type and same return type, but the logic for loading the values is different), it is necessary to use Spring beans qualifiers or names to inject them instead of just relying on the class type.

Other approach would be having only one data loader with the signature and chaning the logic dinamically according with some attribute passed by context.

2 - I could use IDE features to find all the usages of a specific data loader in a project. With the current approach this is not possible because I can only look for the generic usage of the DataLoader class.

So for me it is not something that prevents anything, but it would certainly bring improvements to the code organization.

[dinhani]

A specific case of a previous project that would have some conflict:

A video game is published and developed by one or more companies...

// entities
public class Company {
    private String name;
}

public class Game {
    private List<Company> publishers;
    private List<Company> developers;
}

// two different data loaders, one for each relationship create as Spring beans
@Bean
public DataLoader<Game, Company> publisherDataLoader(){ 
    return DataLoader.newDataLoader((games, context) -> loadPublishers(games)); 
}

@Bean
public DataLoader<Game, Company> developerDataLoader(){ 
    return DataLoader.newDataLoader((games, context) -> loadDevelopers(games)); 
}

// then somewhere else (Spring code)
 // impossible to know which data loader should be 
@Autowired
private DataLoader<Game, Company> dataLoader;injected here without using names of qualifiers

// but could work if something like this
@Autowired
private DeveloperDataLoaderByGame dataLoader;

[dinhani]

A third argument would be having a custom load method that enforces that some types are passed.

For example, in my use case, the context consists of mandatory Pagination and Sorting information.

I could have a custom load method that does this:

CustomDataLoader extends DataLoader<String, Something> {
    CompletableFuture<Something> load(String key, Pagination pagination, Sorting sorting){
        return super.load(key, new CustomContextWrapper(pagination, sorting));
    }
}

instead of having to remeber to create the CustomContextWrapper in every data loader call.

[bbakerman]

Ok thanks for the detailed outline of your concerns. I really appreciate the time you have taken on this.

I am still reluctant to allow extension by "subclassing" since it makes for fragile libraries. As I said above the fact that the current constructor was open originally was a mistake. A builder pattern is a much more extensible library pattern

As you alluded to, I suggest you create "Spring wrapper components" that represent different aspects of your data loading.

But once again thanks for taking the time to make a detailed case.