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

Prevent loader re-use after mutations #56

Merged
merged 3 commits into from
Jun 8, 2017

Conversation

dylanahsmith
Copy link
Contributor

cc @staugaard

Problem

The loader re-use added by #53 also re-used the loaders and their resolved promises after a mutation field is resolved.

GraphQL::GraphQLTest#test_mutation_execution was a test that was supposed to catch this bug, but it was using CounterLoader.for(ctx).load where load was defined in CounterLoader with def load(key=Object.new), so it was actually using a unique key for each load which prevented it from ever re-using the result of a previous load.

Solution

I've reverted #53 to fix the bug and updated the test to use override cache_key to use the object_id of the array that is used to hold the counter.

If the test is run without the revert, then it has the expected failures of

  1) Failure:
GraphQL::GraphQLTest#test_mutation_execution [test/graphql_test.rb:315]:
--- expected
+++ actual
@@ -1 +1 @@
-{"data"=>{"count1"=>0, "incr1"=>{"value"=>1, "load_value"=>1}, "count2"=>1, "incr2"=>{"value"=>2, "load_value"=>2}}}
+{"data"=>{"count1"=>0, "incr1"=>{"value"=>0, "load_value"=>0}, "count2"=>0, "incr2"=>{"value"=>0, "load_value"=>0}}}

This reverts commit 083bc19.

I realized that this could result in cached values being returned after a
mutation, but there was just a bug in the test that was supposed to prevent
this from happening.
@dylanahsmith dylanahsmith requested a review from xuorig June 6, 2017 18:46
@dylanahsmith
Copy link
Contributor Author

I will need to reopen #43 after this

@staugaard
Copy link
Contributor

Maybe we could clear the loader cache after executing a mutation

@dylanahsmith dylanahsmith merged commit bbe92c3 into master Jun 8, 2017
@dylanahsmith dylanahsmith deleted the fix-mutation-execution-test branch June 8, 2017 14:00
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.

3 participants