Prevent QueryInfo#markResult mutation of result.data and return cache data consistently whether complete or incomplete
#11202
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
size-limit report 馃摝
|
|
@jerelmiller @phryneas @alessbell I guess I'm not sure how to update the |
Should help with #9293.
Most of these test tweaks are reasonable improvements necessitated by fixing a bug that allowed queries to receive raw network results with extraneous fields when the results were incomplete. Now, the extraneous fields are no longer delivered, since they were not requested. The test I removed completely does not make sense, and was only passing previously because of the mock link running out of results.
benjamn
force-pushed
the
issue-9293-fix-QueryInfo-markResult-result.data-mutation
branch
from
fc09281
to
d8c6072
Compare
|
Ran |
| info: { | ||
| a: "ay", | ||
| }, | ||
| info: {}, |
There was a problem hiding this comment.
I'm not sure why this result disappeared here?
There was a problem hiding this comment.
This is a somewhat weird test that exercises the feud-stopping logic described by the long comment in QueryInfo#markResult. I ultimately want to remove that logic and this test, but the test currently captures expected behavior.
Specifically, the data.info.a property already disappeared in the previous case (count === 3) because of writing the { info: { b: "bee" }} result into the cache, clobbering the info.a field. Because the most recent { info: { a: "ay" }} result is the same as before, markResult skips writing it to the cache (not happy with this compromise, but see the long comment for rationale), so it does not reappear in the cache in the count === 4 case.
What changed with this PR is that we used to return the raw network result, which does have { a: "ay" }, because the result from the cache would be incomplete. Now, we always return what we get from the cache, which is incomplete (missing the a: "ay" property).
If we allowed these two queries to keep clobbering each other's properties (because we can't merge the Query.info object, because it has no identity and hasn't been configured with merge: true), then we'd get an infinite request loop, which is arguably worse than the weird behavior of this test. Ultimately, I think we should be able to make it easier (even default) for these two queries to have their own separate views of the Query.info object, so they don't keep stepping on each other's data, but the standard current solution is to mark the Query.info field as mergeable, so info.a and info.b can coexist:
new InMemoryCache({
typePolicies: {
Query: {
fields: {
info: { merge: true }
}
}
}
})There was a problem hiding this comment.
Makes sense, thanks for the explanation!
src/core/QueryInfo.ts
Outdated
| result = { | ||
| ...result, | ||
| data: this.lastDiff.diff.result, | ||
| }; |
There was a problem hiding this comment.
| result = { | |
| ...result, | |
| data: this.lastDiff.diff.result, | |
| }; | |
| result.data = this.lastDiff.diff.result; |
See other comment.
| // Make a shallow defensive copy of the result object, in case we | ||
| // later later modify result.data in place, since we don't want | ||
| // that mutation affecting the saved lastWrite.result.data. | ||
| result: { ...result }, |
There was a problem hiding this comment.
Just wanted to add: I'm definitely not challenging this defensive copy - 馃挴 agree!
There was a problem hiding this comment.
Approved!
@benjamn we already talked about this - I changed the defensive copy logic in this commit: ec5d86c (#11202)
Could you take another look at that before merging?
benjamn
deleted the
issue-9293-fix-QueryInfo-markResult-result.data-mutation
branch
|
Your changes look good to me. Thanks for jumping in and reviewing! |
This PR fixes issue #9293 by ensuring all cache-friendly fetch policies (including
cache-{first,only,and-network}andnetwork-onlybut excludingno-cache) return result data after re-reading the data from the cache, even when the data are incomplete (fixing #9293), so any customreadfunctions (or other cache logic) can be applied consistently whether or not the cache data are complete. I've added a new test that exercises all the fetch policies, since test coverage was previously inadequate in this area.While diagnosing this problem, I noticed
QueryInfo#markResultpreviously updatedresult.datausing a destructive mutation, which could easily go unnoticed by the caller ofmarkResult, so I refactored that code a bit to return a newresultobject instead of modifying the one provided.As @CarsonF's reproduction from #9293 suggests, consistently returning cache data is important when using a
readfunction to implement a scalar wrapper likeDatefor an otherwise string-valued field. I tested these changes against the reproduction repo, and while there's still some weirdness related to the use ofoffsetLimitPaginationfor theQuery.peoplelist field, the problem of the disappearingDatewrappers seems to be solved. More generally, I believe this change allows a more reliable implementation our most popular feature request (by 馃憤 reactions), apollographql/apollo-feature-requests#368.While I stand behind the logic of this change for consistency's sake, it's worth mentioning the old behavior allowed returning raw network results with extraneous fields not present in the query, in cases where the server violates GraphQL validation rules by returning more properties than requested, but also does not return all the requested properties. See my test changes involving
todoWithoutPriceandcarDataWithoutVinefor examples of this edge case. Now that we always re-read results from the cache using the provided query, you should always receive only the queried fields, so any extraneous properties will (correctly) disappear.