Skip to content

Enable DataLoader nesting/chaining #3872

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

Jump to bottom
Open
wants to merge 54 commits into
base: master
Choose a base branch
from
Open

Enable DataLoader nesting/chaining #3872

wants to merge 54 commits into from

Conversation

andimarek
Copy link
Member

@andimarek andimarek commented Mar 30, 2025 •
edited
Loading

enabling scheduling of nested/chained DataLoader calls

@andimarek andimarek changed the title introducing of a special CompletableFuture Enable DataLoader nesting/chaining Mar 30, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 30, 2025 •
edited
Loading

Test Results

  313 files  + 1    313 suites  +1   2m 25s ⏱️ + 1m 32s
3 616 tests +35  3 611 ✅ +35  5 💤 ±0  0 ❌ ±0 
3 705 runs  +35  3 700 ✅ +35  5 💤 ±0  0 ❌ ±0 

Results for commit 81c4b09. ± Comparison against base commit ea9104d.

This pull request removes 206 and adds 219 tests. Note that renamed tests count towards both. 
	?
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
                a2 :  __type(name : "t1") { name }
…

graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure23@7bbbb6a8 delegate=graphql.AssertTest@34dc85a owner=graphql.AssertTest@34dc85a thisObject=graphql.AssertTest@34dc85a resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure24@2ff95fc6 delegate=graphql.AssertTest@34dc85a owner=graphql.AssertTest@34dc85a thisObject=graphql.AssertTest@34dc85a resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure25@9d1a267 delegate=graphql.AssertTest@34dc85a owner=graphql.AssertTest@34dc85a thisObject=graphql.AssertTest@34dc85a resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure20@4e904fd5 delegate=graphql.AssertTest@34dc85a owner=graphql.AssertTest@34dc85a thisObject=graphql.AssertTest@34dc85a resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure21@b18c4 delegate=graphql.AssertTest@34dc85a owner=graphql.AssertTest@34dc85a thisObject=graphql.AssertTest@34dc85a resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure22@4cbf4f53 delegate=graphql.AssertTest@34dc85a owner=graphql.AssertTest@34dc85a thisObject=graphql.AssertTest@34dc85a resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure3@545607f2 delegate=graphql.AssertTest@34dc85a owner=graphql.AssertTest@34dc85a thisObject=graphql.AssertTest@34dc85a resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure4@2bf94401 delegate=graphql.AssertTest@34dc85a owner=graphql.AssertTest@34dc85a thisObject=graphql.AssertTest@34dc85a resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure5@3d36dff4 delegate=graphql.AssertTest@34dc85a owner=graphql.AssertTest@34dc85a thisObject=graphql.AssertTest@34dc85a resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of error args with non null does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_6prov0_closure6@63718b93 delegate=graphql.AssertTest@34dc85a owner=graphql.AssertTest@34dc85a thisObject=graphql.AssertTest@34dc85a resolveStrategy=0 directive=0 parameterTypes=[class java.lang.Object] maximumNumberOfParameters=1 bcw=null>, expectedMessage: error arg1, #0]
…

♻️ This comment has been updated with latest results.

@andimarek andimarek mentioned this pull request Mar 30, 2025
Closed
bbakerman
bbakerman requested changes May 1, 2025
View reviewed changes
Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

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

This is a breaking change for tests

bbakerman
bbakerman reviewed May 3, 2025
View reviewed changes
src/main/java/graphql/schema/DataFetchingEnvironmentImpl.java
@Override
public <K, V> @Nullable DataLoader<K, V> getDataLoader(String dataLoaderName) {
return dataLoaderRegistry.getDataLoader(dataLoaderName);
return new DataLoaderWithContext<>(this, dataLoaderName, dataLoaderRegistry.getDataLoader(dataLoaderName));
Copy link
Member

Choose a reason for hiding this comment

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

This code creates a new object every time its called

If code was to call this N time inside the same DFE then it would create new object each time which seems wasteful

Can we have a map inside the DFE (or a component class that holds that) which remembers which DataLoaderWithContext object it has made.

Perhaps a DataLoaderWithContextFactory which is just the wrapper of the map and with enough knowledge to know what represent a proper key (its more than data loader name I suspect)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbakerman bbakerman bbakerman requested changes

@Hc747 Hc747 Hc747 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
23.1 non-breaking changes and bugfixes
Development

Successfully merging this pull request may close these issues.
4 participants
@andimarek @bbakerman @Hc747 @dondonz