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

Clear loader cache in Mutation#after_resolve #141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

palasha
Copy link

@palasha palasha commented Jun 15, 2021

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, 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.sync in the resolve method to handle this case, but when I step through the code with puts/byebug, I observe that the authorized? or resolve methods of the mutation are not called at all until after the Promise.sync call is complete. I feel its possible im misunderstanding something.

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.
@swalkinshaw
Copy link
Contributor

Can you try adding a test case to cover this?

If this change is correct, I'd think we could remove the existing clear from resolve? Seems duplicative to call it in both resolve and then after_resolve

@palasha
Copy link
Author

palasha commented Jun 16, 2021

Can you try adding a test case to cover this?

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.

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.

None yet

2 participants