From 9058d5f95d71e8154990698077ea2ed85037c23b Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 22 Sep 2021 18:25:46 -0400 Subject: [PATCH 1/2] Prevent optimistic cache evictions from evicting non-optimistic data. After thinking further about the discussion in #7321, I believe the inability to roll back optimistic evictions is simply a bug that we can fix, rather than something the developer should have to worry about (e.g. by passing `optimistic: true`, as I previously proposed in https://github.com/apollographql/apollo-client/issues/7321#issuecomment-732419787). Normally, `cache.optimisticData` points to the topmost currently active optimistic `Layer`, and `cache.data` points to the root/non-optimistic layer. While running an optimistic `update` function, we temporarily set `cache.data` to be `=== cache.optimisticData`, in an attempt to confine the optimistic update to one optimistic layer. Eviction was (until now) an exception to this confinement, because optimistic `Layer` objects still have a chain of `layer.parent.parent...` objects that eventually terminates at the `Root` layer. Before now, eviction was blindly following that entire chain, ignoring the current value of `cache.data`. This commit uses `cache.data` as the stopping point for eviction, which has the effect of confining optimistic evictions to the one layer that they should be updating, hopefully solving #7321 without requiring any intervention by the developer. --- src/cache/inmemory/__tests__/optimistic.ts | 157 +++++++++++++++++++++ src/cache/inmemory/entityStore.ts | 9 +- src/cache/inmemory/inMemoryCache.ts | 5 +- 3 files changed, 167 insertions(+), 4 deletions(-) diff --git a/src/cache/inmemory/__tests__/optimistic.ts b/src/cache/inmemory/__tests__/optimistic.ts index 8030d43ed49..bae86938f32 100644 --- a/src/cache/inmemory/__tests__/optimistic.ts +++ b/src/cache/inmemory/__tests__/optimistic.ts @@ -443,4 +443,161 @@ describe('optimistic cache layers', () => { const resultWithoutOptimisticLayers = readWithAuthors(); expect(resultWithoutOptimisticLayers).toBe(nonOptimisticResult); }); + + describe("eviction during optimistic updates", function () { + it("should not evict from lower layers", function () { + const cache = new InMemoryCache(); + + const query = gql` + query { + counter { + value + } + } + `; + + function write(value: number) { + cache.writeQuery({ + query, + data: { + counter: { + value, + }, + }, + }); + } + + function expectOptimisticCount(value: number) { + expect(cache.readQuery({ + query, + optimistic: true, + })).toEqual({ + counter: { + value, + }, + }); + + expect(cache.extract(true)).toEqual({ + ROOT_QUERY: { + __typename: "Query", + counter: { + value, + }, + }, + }); + } + + function expectNonOptimisticCount(value: number) { + // Reading non-optimistically returns the original non-optimistic data. + expect(cache.readQuery({ + query, + optimistic: false, + })).toEqual({ + counter: { + value, + }, + }); + + // Extracting non-optimistically shows Query.counter === 0 again. + expect(cache.extract(false)).toEqual({ + ROOT_QUERY: { + __typename: "Query", + counter: { + value, + }, + }, + }); + } + + write(0); + expectOptimisticCount(0); + expectNonOptimisticCount(0); + + cache.batch({ + optimistic: "layer 1", + update() { + write(1); + expectOptimisticCount(1); + // Within the update function, non-optimistic cache reads come from + // the current optimistic layer, so we read 1 here instead of 0. + expectNonOptimisticCount(1); + }, + }); + expectOptimisticCount(1); + // Now that we're out of the update function, the non-optimistic data is + // back to looking as it always did. + expectNonOptimisticCount(0); + + cache.batch({ + optimistic: "layer 2", + update() { + write(2); + expectOptimisticCount(2); + }, + }); + expectOptimisticCount(2); + + cache.batch({ + optimistic: "layer 3", + update() { + write(3); + expectOptimisticCount(3); + + expect(cache.evict({ + fieldName: "counter", + })).toBe(true); + + expectOptimisticEviction(); + }, + }); + + function expectOptimisticEviction() { + // Reading optimistically fails because the data have been evicted, + // though only optimistically. + expect(cache.readQuery({ + query, + optimistic: true, + })).toBe(null); + + // Extracting optimistically shows Query.counter undefined. + expect(cache.extract(true)).toEqual({ + ROOT_QUERY: { + __typename: "Query", + counter: void 0, + }, + }); + } + + expectOptimisticEviction(); + + cache.removeOptimistic("layer 2"); + + // Nothing changes because "layer 2" was not the top layer. + expectOptimisticEviction(); + + // Original data still intact, of course. + expectNonOptimisticCount(0); + + cache.removeOptimistic("layer 3"); + + // Since we removed layers 2 and then 3, only layer 1 is left. + expectOptimisticCount(1); + expectNonOptimisticCount(0); + + // Since this eviction is not happening inside an optimistic update + // function, it evicts the Query.counter field from both the optimistic + // layer (1) and the root layer. + expect(cache.evict({ + fieldName: "counter", + })).toBe(true); + + expectOptimisticEviction(); + + cache.removeOptimistic("layer 1"); + + // There are no optimistic layers now, but the root/non-optimistic layer + // also exhibits the eviction. + expectOptimisticEviction(); + }); + }); }); diff --git a/src/cache/inmemory/entityStore.ts b/src/cache/inmemory/entityStore.ts index b1a9e5c4abf..f5fbf610b43 100644 --- a/src/cache/inmemory/entityStore.ts +++ b/src/cache/inmemory/entityStore.ts @@ -291,14 +291,17 @@ export abstract class EntityStore implements NormalizedCache { return false; } - public evict(options: Cache.EvictOptions): boolean { + public evict( + options: Cache.EvictOptions, + limit: EntityStore, + ): boolean { let evicted = false; if (options.id) { if (hasOwn.call(this.data, options.id)) { evicted = this.delete(options.id, options.fieldName, options.args); } - if (this instanceof Layer) { - evicted = this.parent.evict(options) || evicted; + if (this instanceof Layer && this !== limit) { + evicted = this.parent.evict(options, limit) || evicted; } // Always invalidate the field to trigger rereading of watched // queries, even if no cache data was modified by the eviction, diff --git a/src/cache/inmemory/inMemoryCache.ts b/src/cache/inmemory/inMemoryCache.ts index cf058d7889e..ddb2418af88 100644 --- a/src/cache/inmemory/inMemoryCache.ts +++ b/src/cache/inmemory/inMemoryCache.ts @@ -353,7 +353,10 @@ export class InMemoryCache extends ApolloCache { // this.txCount still seems like a good idea, for uniformity with // the other update methods. ++this.txCount; - return this.optimisticData.evict(options); + // Pass this.data as a limit on the depth of the eviction, so evictions + // during optimistic updates (when this.data is temporarily set equal to + // this.optimisticData) do not escape their optimistic Layer. + return this.optimisticData.evict(options, this.data); } finally { if (!--this.txCount && options.broadcast !== false) { this.broadcastWatches(); From 538fc9cd35457be68445dcb01a1016a0f76f6acf Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 23 Sep 2021 09:52:36 -0400 Subject: [PATCH 2/2] Mention PR #8829 in CHANGELOG.md. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f88439d8e61..ca7d42e9bf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ - Stop excluding observerless queries from `refetchQueries: [...]` selection.
[@benjamn](https://github.com/benjamn) in [#8825](https://github.com/apollographql/apollo-client/pull/8825) +- Prevent optimistic cache evictions from evicting non-optimistic data.
+ [@benjamn](https://github.com/benjamn) in [#8829](https://github.com/apollographql/apollo-client/pull/8829) + ## Apollo Client 3.4.13 ### Bug Fixes