Fix issue with data leaking inside partial @defer or @stream boundaries during intermediate results - #13347
Conversation
| null | ||
| : result | ||
| : null, | ||
| if ( |
There was a problem hiding this comment.
Note: Many of these changes will be simplified when we get to v5 and can enforce caches to be incremental-aware. To maintain backwards compatibility, we can shift data states around. v5 will also require cache reads to return a dataState which will collapse this further.
| ...incrementalResult, | ||
| dataState: | ||
| incrementalResult.data == null ? "empty" | ||
| // TODO: This is too naive. For stream arrays, this might be complete |
There was a problem hiding this comment.
I need to re-evaluate if this is still true. I'll make sure to remove once I verify if this is ok.
There was a problem hiding this comment.
Re-evaluated and found only one case where it actually was too naive. Fixed in 492337d
| return writeWithErrors; | ||
| } | ||
|
|
||
| function isStreamingPartial( |
There was a problem hiding this comment.
Much of this is now folded into readFromStore which does a prune pass similar to this, but handles many more edge cases that this did/could not.
| const fragment = fragmentMap && fragmentMap[fragmentName]; | ||
| invariant(fragment, `No fragment named %s`, fragmentName); | ||
| return fragment || null; | ||
| return fragment; |
There was a problem hiding this comment.
Spotted this while working on this PR. invariant checks that this is a non-nullish value so no need to fallback to null here.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to release-4.3, this PR will be updated.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ `release-4.3` is currently in **pre mode** so this branch has prereleases rather than normal releases. If you want to exit prereleases, run `changeset pre exit` on `release-4.3`.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ # Releases ## @apollo/client@4.3.0-alpha.4 ### Patch Changes - [#13347](#13347) [`7d543d6`](7d543d6) Thanks [@jerelmiller](https://github.com/jerelmiller)! - Fix an issue where `network-only` incremental queries could cause cache data to leak into the emitted result when a `@defer` or `@stream` boundary already had complete data in the cache. Cache data inside pending `@defer` objects and `@stream` arrays are now pruned so that only completed `@defer` or `@stream` boundaries are returned. NOTE: This change only applies to `InMemoryCache` when using `GraphQL17Alpha9Handler`. - [#13329](#13329) [`1d581d2`](1d581d2) Thanks [@AmariahAK](https://github.com/AmariahAK)! - Cache diffs for incomplete queries no longer pay the cost of building a full `MissingFieldError` when the `missing` property is not accessed. The error object is now only constructed when the `missing` property is accessed the first time. This improves performance by avoiding a V8 stack capture when `missing` is ignored entirely. As an additional small performance improvement, `JSON.stringify` is no longer used in the error message on objects whose cache ID is known. `JSON.stringify` is only used for non-normalized objects. - [#13347](#13347) [`7d543d6`](7d543d6) Thanks [@jerelmiller](https://github.com/jerelmiller)! - Fix an issue where partial cache data could leak into intermediate incremental results. This could cause runtime crashes if you relied on the presence of values to determine whether the `@defer` data had streamed in or not. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…3360) Followup to #13347 Discussed with @phryneas in person. Instead of conditionally passing in the symbol by having caches opt-in, we pass it unconditionally. Caches that want to take advantage of behavior using that symbol can come talk to us on how to implement against the incremental behavior. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Improved incremental, deferred, and streamed cache diff handling. * Updated cache diff results to provide more consistent data-state information. * Simplified incremental data processing across cache implementations. * **Bug Fixes** * Corrected partial-data and missing-field behavior during streaming and incremental reads. * Preserved compatibility for standard cache diff queries that do not use incremental options. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Fixes #13330
The big change in this PR is making
InMemoryCacheincremental-aware for cache reads (though hidden behind an internal symbol that enables it).cache.diffis too naive and classifies in-progress queries with@deferboundaries as partial when@deferdata hasn't streamed in. This is a problem for howQueryInforereads data after a cache write because it won't apply that data to the returned result unless its fully complete. Field values transformed by cache read functions were therefore not applied to intermediate results in a@deferquery.This issue became glaringly apparent with the introduction of custom scalars in 4.3 (currently unreleased) where we need to apply the parsed field values stored in the cache to intermediate chunks. Without that, the user might encounter runtime crashes when expecting the value to be the parsed type.
#13324 was a first attempt at solving this issue across many cases where read functions/custom scalars weren't applied, but it did not fix the issue where partial data inside a
@deferor@streamboundary leaked in intermediate results. It relied on themissingtree to reapply field values from the rereadcache.diff, but did not prune partial boundaries.readFromStorecan now properly handle partial boundaries when provided withreturnPartialData: true(though only when the hidden flag is provided to ensure backwards compatibility until v5). The complex scenarios where partial data can show up inside defer boundaries (especially with overlapping selections in@deferboundaries) is now properly handled during a cache read.Summary by CodeRabbit
Bug Fixes
network-onlyincremental query results.@defercontent and incomplete@streamarray items.New Features