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

Avoid modifying source objects when merging cache results. #4089

Merged
merged 3 commits into from
Nov 2, 2018

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Nov 2, 2018

As #4081 demonstrates, if there are key collisions during result merging, it's possible for source objects that were previously merged by reference into the target object (finalResult.result) to be modified destructively by later merges, which in some cases can lead to cycles in the results returned by the cache.

This version of the merge function uses a copy-on-first-write strategy to ensure we never modify target objects that might once have been source objects. The code could have been considerably simpler if I didn't try to track pastCopies, but performance and memory usage are very important for this code, which is why I went to the trouble of limiting the number of shallow copies made during a single merge.

  • Tests are absolutely necessary before merging this PR, since this behavior would be very easy to regress accidentally.

As #4081 demonstrates, if there are key collisions during result merging,
it's possible for source objects that were previously merged by reference
into the target object (finalResult.result) to be modified destructively
by later merges, which in some cases can lead to cycles in the results
returned by the cache.

This version of the merge function uses a copy-on-first-write strategy to
ensure we never modify target objects that might once have been source
objects. The code could have been considerably simpler if I didn't try to
track pastCopies, but performance and memory usage are very important for
this code, which is why I went to the trouble of limiting the number of
shallow copies made during a single merge.
@benjamn benjamn self-assigned this Nov 2, 2018
@benjamn
Copy link
Member Author

benjamn commented Nov 2, 2018

I had to check my GraphQL facts, but overlapping/duplicated fields within the same selection set are allowed, as long as they follow certain rules: https://facebook.github.io/graphql/draft/#sec-Field-Selection-Merging

I don't think this code is the place to enforce those rules, but it definitely is important to tolerate field collisions when merging results, rather than doing something easy/drastic like rejecting the query.

@benjamn benjamn merged commit 775c240 into master Nov 2, 2018
@benjamn benjamn deleted the issue-4081-prevent-cache-result-cycles branch November 2, 2018 16:04
benjamn added a commit that referenced this pull request Nov 3, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant