Clear loader cache in Mutation#after_resolve#141
Open
palasha wants to merge 1 commit intoShopify:mainfrom
Open
Clear loader cache in Mutation#after_resolve#141palasha wants to merge 1 commit intoShopify:mainfrom
palasha wants to merge 1 commit intoShopify:mainfrom
Conversation
The `resolve` method of a Mutation can make changes to the DB that invalidate any cached Loader results. So, the cache should be cleared after the resolve method has completed, and before any nested fields on the Mutation query are resolved. In our codebase, this causes authorization errors if the Mutation removes a member from a list/connection, and the client requests that same list field as part of the mutation response. Since the Loader cached the pre-mutation result, the wrong data is returned to the client.
Contributor
|
Can you try adding a test case to cover this? If this change is correct, I'd think we could remove the existing |
Author
I really dont understand the lazy execution ordering inside graphql-ruby (reading the interpreter code is a mind bender) clearly enough to do a confident job of testing the right thing. This worked for our codebase, so the PR was intended to be a conversation starter about whether this is the "right" change, and if its the right way to go about it. Definitely happy to write a spec after we figure ^^ out, but right now I wouldn't know where to start on which scenarios to test or how to set them up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
resolvemethod of a Mutation can make changes to the DB thatinvalidate any cached Loader results. So, the cache should be cleared
after the resolve method has completed, and before any nested fields on
the Mutation query are resolved.
In our codebase, this causes authorization errors if the Mutation
removes a member from a list/connection, and the client requests that
same list field, on the same object, as part of the mutation response. Since the Loader
cached the pre-mutation result, the wrong data is returned to the
client.
Note: At first, I was expecting the usage of
::Promise.syncin theresolvemethod to handle this case, but when I step through the code with puts/byebug, I observe that theauthorized?orresolvemethods of the mutation are not called at all until after thePromise.synccall is complete. I feel its possible im misunderstanding something.