Skip to content
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.

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

Fix lazy auth with dataloaded value #4036

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

rmosolgo
Copy link
Owner

Fixes #4021

@@ -1122,4 +1122,64 @@ def nested2
assert_equal({ "data" => { "nested2" => "nested2" } }, NestedDataloaderCallsSchema.execute("{ nested2 }"))
assert_equal({ "data" => { "nested" => "nested", "nested2" => "nested2" } }, NestedDataloaderCallsSchema.execute("{ nested nested2 }"))
end

describe "with lazy authorization hooks" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Caveat: I have fairly low context for this question so please feel free to ignore it.

Was the bug just with authorization methods returning lazy values? Or could other parts of the resolver chain like scope_items or field extensions (that use dataloader) also create similarly shaped issues.

Would it make sense to test various lazy/dataloader nestings of those features as well?

Copy link
Owner Author

Choose a reason for hiding this comment

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

IMO, this test is sufficient. The bug was at a lower level than any particular GraphQL feature -- instead, it was a bug about how dataloader and lazy values hand off control to one another.

Previously, the lazy resolution code was enqueuing itself as a single job in the dataloader. This didn't work if an already-enqueued job would require multiple passes to populate some key/value pairs in the response object. (The lazy resolution code uses response objects directly to find Lazys to resolve.)

So instead of queuing up a single job and assuming that any preceding jobs will populate the response objects, now, it calls dataloader.run to make sure that all preceding jobs have totally finished before using response objects.

@rmosolgo rmosolgo merged commit 6cd409c into master Apr 25, 2022
@rmosolgo rmosolgo deleted the fix-lazy-auth-with-dataloaded-value branch April 25, 2022 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wierd interaction between Lazy's and Dataloader.
2 participants