feat: make cache map values accesible for read only purposes

[samuelAndalon]

in an attempt so write a better Instrumentation that could dispatch a DataLoaderRegistry in a more optimized way we created a custom Instrumentation that tracks the state of each ExecutionStrategy of an ExecutionInput and dispatch the DataLoaderRegistry when no more synchronous code could be executed.

more information

graphql-java/graphql-java#2715

this PR adds a method to the DataLoader class that provides a read-only list of CacheMaps associated with the DataLoader, this will help to keep track of the numberOfDependents of each future.

by doing this we would also be solving a problem highlighted here

graphql-java/graphql-java#1198

considering this queries:

astronaut can have many mission
mission can have many planets

in order to access to astronaut planets we need 2 DataLoaders to be chained in the planets resolver

1. GetMissionsByAstronaut
2. GetPlanetsByMission

query GetAstronautPlanets {
    astronaut(id: 1) {
        name
        planets {
            name
        }
    }
}

query GetAstronautMissions {
    astronaut(id: 3) {
        name
        missions {
            designation
            planets {
                name
            }
        }
    }
}

With the custom DataLoaderSyncExecutionExhaustedInstrumentation and this change we were able to get data for those 2 queries by dispatching those 2 dataLoader only once.

we wrote some logic to decorate a DataLoaderRegistry to make CacheMap values public for read-only purposes and that helped to achieve above logic

more detailed test here

https://github.com/ExpediaGroup/graphql-kotlin/blob/1758f9c4fed47f086891c9340ebc995eca1e9083/executions/graphql-kotlin-dataloader-instrumentation/src/test/kotlin/com/expediagroup/graphql/dataloader/instrumentation/syncexhaustion/DataLoaderSyncExecutionExhaustedInstrumentationTest.kt#L345

[bbakerman]

I am happy to take this PR in general - but please see my other comment about what we expose

[bbakerman]

Rather than expose the getAll down in the helper and then in the data loader,

I think we should expose the CacheMap itself on DataLoader

    private final CacheMap<Object, V> futureCache;

is inside DataLoader.

Change this to

    public  CacheMap<Object, V> getCacheMap() 

And for symmetry (since we are going to do this) we should expose the other atts

    private final StatisticsCollector stats;
    private final CacheMap<Object, V> futureCache;
    private final ValueCache<K, V> valueCache;

[samuelAndalon]

Should we worry about making futureCache and valueCache public ? i was trying to just expose the CacheMap values as read-only with unmodifiableCollection to avoid allowing the mutation of their state -- Let me know what you think.

For now i removed the method in the DataLoaderHelper and directly exposed the read-only Collection from the DataLoader

[samuelAndalon]

also was wondering, given that we are adding a new method in the CacheMap interface, should we define a default behavior to return maybe empty collection or null ? otherwise this could be a breaking change if ppl is using a custom implementation of CacheMap

[bbakerman]

I get your point on trying to reduce danger but the fact is the DataLoaderOptions passed in the CachMap / ValueMap or it get defaulted.

I guess we could make a UnmodifiableCacheMap / ValueCache wrapper say so gain write safety

Should we worry about making futureCache and valueCache public ?

My view is sure it could be a foot gun... so dont shoot yourself with it. You needed access to the future cache so lets give it out. After all it was nominally passed in.

But UnmodifiableCacheMap is not a crazy idea

[samuelAndalon]

added the getters for cacheMap and valueCache,
will think about the wrapper interface to remove the mutable methods clear, delete and set and probably open a separate PR, if we decide that we need it

[bbakerman]

Thanks for this PR

[samuelAndalon]

@bbakerman Thank you for the merge, was wondering, when this can be released ?