Various improvements to ROOT_MUTATION caching #8280
Merged
Conversation
This file contains 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
benjamn
commented
May 24, 2021
benjamn
commented
May 24, 2021
brainkim
reviewed
May 26, 2021
brainkim
approved these changes
May 26, 2021
Should alleviate #3592, while also helping to isolate mutations from one another, so multiple mutation results never overlap in the cache.
While I was working with this code, I noticed that the fetchPolicy: "no-cache" option for mutations was not always respected (cache writes would happen anyway), so I fixed that.
I suspect this change will be slightly less disruptive than completely removing the ROOT_MUTATION object from the cache after each mutation. It's safe to keep __typename because it isn't sensitive information, like the arguments of root mutation fields. It's convenient to keep __typename because that data (and the enclosing object) would otherwise have to be recreated the next time a mutation writes its results into the cache. We could use the mutation.document root fields to perform this removal more precisely, but that's more work for no clear gain.
benjamn
force-pushed
the
ROOT_MUTATION-caching-adjustments
branch
from
885105b to
8a8f1d1
Compare
This option should limit the disruptiveness of PR #8280 (even though removing ROOT_MUTATION fields is the default behavior), because at least there's an easy way to achieve the old behavior again (that is, by passing `keepRootFields: true` to `client.mutate`).
|
Great plan @benjamn - thanks! |
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fixes #7665 by allowing field policy
readfunctions to run for fields within mutation results, making it easier for mutation result data to use custom scalar types (to borrow @stephenh's use case from #7665).In order to keep each
ROOT_MUTATIONwrite isolated from others that might be happening at the same time, this PR also removes the temporaryROOT_MUTATIONobject from the cache immediately after theupdatefunction runs, so information like arguments passed to mutation root fields will not be retained in the cache longer than need be, thereby resolving #3592. To be clear, the Apollo Client team does not believe the previous behavior of leavingROOT_MUTATIONin the cache created any meaningful opportunities for attackers to read cached data, but we believe in defense in depth and agree with the principles articulated by @danilobuerger in #3592 (comment).The removal/impermanence of
ROOT_MUTATIONcould be a disruptive change for anyone who depends on readingROOT_MUTATIONdata from the cache after the mutation has finished, but that's not encouraged or well supported right now, so we think/hope the potential disruption is justified in order to achieve better security by default. We can revisit this before Apollo Client v3.4 is released, if anyone reports problems.