From 4ba46a3515679323a09970f63d29a524d8c0c6ac Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 30 Aug 2023 17:45:23 -0600 Subject: [PATCH 1/8] Add test validating that switching variables in quick succession results in bug --- src/core/__tests__/ObservableQuery.ts | 62 +++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index 969057a6a33..deb523b677f 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -25,6 +25,7 @@ import { MockLink, mockSingleLink, subscribeAndCount, + wait, } from "../../testing"; import mockQueryManager from "../../testing/core/mocking/mockQueryManager"; import mockWatchQuery from "../../testing/core/mocking/mockWatchQuery"; @@ -3199,3 +3200,64 @@ test("regression test for #10587", async () => { ); expect(query1Spy.mock.calls).toEqual(finalExpectedCalls.query1); }); + +// https://github.com/apollographql/apollo-client/issues/11184 +test("handles changing variables in rapid succession before other request is aborted", async () => { + interface UserCountQuery { + userCount: number; + } + interface UserCountVariables { + department: "HR" | null; + } + + const query: TypedDocumentNode = gql` + query UserCountQuery($department: Department) { + userCount(department: $department) + } + `; + const mocks = [ + { + request: { query, variables: { department: null } }, + result: { data: { userCount: 10 } }, + }, + { + request: { query, variables: { department: "HR" } }, + result: { data: { userCount: 5 } }, + delay: 50, + }, + ]; + + const client = new ApolloClient({ + link: new MockLink(mocks), + cache: new InMemoryCache(), + }); + + const observable = client.watchQuery({ + query, + variables: { department: null }, + }); + + observable.subscribe(jest.fn()); + + await waitFor(() => { + expect(observable.getCurrentResult(false)).toEqual({ + data: { userCount: 10 }, + loading: false, + networkStatus: NetworkStatus.ready, + }); + }); + + observable.reobserve({ variables: { department: "HR" } }); + await wait(10); + observable.reobserve({ variables: { department: null } }); + + // Wait for request to finish + await wait(50); + + expect(observable.options.variables).toEqual({ department: null }); + expect(observable.getCurrentResult(false)).toEqual({ + data: { userCount: 10 }, + loading: false, + networkStatus: NetworkStatus.ready, + }); +}); From bb93818edffe258462f28e1b7cfb905a9c621b62 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 30 Aug 2023 21:20:36 -0600 Subject: [PATCH 2/8] Remove the argument to QueryInfo#getDiff which is not needed --- src/core/QueryInfo.ts | 6 +++--- src/core/QueryManager.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 0323c42cefb..c147a7eccd2 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -157,14 +157,14 @@ export class QueryInfo { this.dirty = false; } - getDiff(variables = this.variables): Cache.DiffResult { - const options = this.getDiffOptions(variables); + getDiff(): Cache.DiffResult { + const options = this.getDiffOptions(); if (this.lastDiff && equal(options, this.lastDiff.options)) { return this.lastDiff.diff; } - this.updateWatch((this.variables = variables)); + this.updateWatch(this.variables); const oq = this.observableQuery; if (oq && oq.options.fetchPolicy === "no-cache") { diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index a2493076e6a..efdd3c05a92 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -1516,7 +1516,7 @@ export class QueryManager { networkStatus, }); - const readCache = () => queryInfo.getDiff(variables); + const readCache = () => queryInfo.getDiff(); const resultsFromCache = ( diff: Cache.DiffResult, From 1bcaddb10832ba496ce1a42cc2cc2fa331dfbb47 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 30 Aug 2023 21:44:59 -0600 Subject: [PATCH 3/8] Don't write to the cache in markResult if variables don't match --- src/core/QueryInfo.ts | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index c147a7eccd2..61e4e2d8481 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -342,14 +342,20 @@ export class QueryInfo { variables: WatchQueryOptions["variables"] ) { const { lastWrite } = this; - return !( - lastWrite && - // If cache.evict has been called since the last time we wrote this - // data into the cache, there's a chance writing this result into - // the cache will repair what was evicted. - lastWrite.dmCount === destructiveMethodCounts.get(this.cache) && - equal(variables, lastWrite.variables) && - equal(result.data, lastWrite.result.data) + return ( + // In case variables have changed and there is a race condition where the + // previous variables request finishes after the current variables + // request, skip the cache update. + equal(this.variables, variables) && + !( + lastWrite && + // If cache.evict has been called since the last time we wrote this + // data into the cache, there's a chance writing this result into + // the cache will repair what was evicted. + lastWrite.dmCount === destructiveMethodCounts.get(this.cache) && + equal(variables, lastWrite.variables) && + equal(result.data, lastWrite.result.data) + ) ); } From f50ffd03b5adc8a8d087a3469b551d12991b9d53 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 30 Aug 2023 21:49:32 -0600 Subject: [PATCH 4/8] Add changeset --- .changeset/odd-beers-perform.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/odd-beers-perform.md diff --git a/.changeset/odd-beers-perform.md b/.changeset/odd-beers-perform.md new file mode 100644 index 00000000000..e42635ea4be --- /dev/null +++ b/.changeset/odd-beers-perform.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Fixes an issue where triggering a query with differnt variables then rapidly changing back to the initial variables with a cached result would return the wrong result if the previous request finished after switching back to initial variables. From 2d1645413eac200f97371c5ba1bf6fd147cae169 Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Wed, 30 Aug 2023 21:58:04 -0600 Subject: [PATCH 5/8] More accurate language in test description --- src/core/__tests__/ObservableQuery.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index deb523b677f..3104be8ea96 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -3202,7 +3202,7 @@ test("regression test for #10587", async () => { }); // https://github.com/apollographql/apollo-client/issues/11184 -test("handles changing variables in rapid succession before other request is aborted", async () => { +test("handles changing variables in rapid succession before other request is completed", async () => { interface UserCountQuery { userCount: number; } From 20b29328b59823554febc219de635c6453941cfd Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Thu, 31 Aug 2023 00:33:24 -0600 Subject: [PATCH 6/8] Dont update watch if variables don't match --- src/core/QueryInfo.ts | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/core/QueryInfo.ts b/src/core/QueryInfo.ts index 61e4e2d8481..4ddb60fce34 100644 --- a/src/core/QueryInfo.ts +++ b/src/core/QueryInfo.ts @@ -342,20 +342,14 @@ export class QueryInfo { variables: WatchQueryOptions["variables"] ) { const { lastWrite } = this; - return ( - // In case variables have changed and there is a race condition where the - // previous variables request finishes after the current variables - // request, skip the cache update. - equal(this.variables, variables) && - !( - lastWrite && - // If cache.evict has been called since the last time we wrote this - // data into the cache, there's a chance writing this result into - // the cache will repair what was evicted. - lastWrite.dmCount === destructiveMethodCounts.get(this.cache) && - equal(variables, lastWrite.variables) && - equal(result.data, lastWrite.result.data) - ) + return !( + lastWrite && + // If cache.evict has been called since the last time we wrote this + // data into the cache, there's a chance writing this result into + // the cache will repair what was evicted. + lastWrite.dmCount === destructiveMethodCounts.get(this.cache) && + equal(variables, lastWrite.variables) && + equal(result.data, lastWrite.result.data) ); } @@ -466,8 +460,11 @@ export class QueryInfo { // In case the QueryManager stops this QueryInfo before its // results are delivered, it's important to avoid restarting the - // cache watch when markResult is called. - if (!this.stopped) { + // cache watch when markResult is called. We also avoid updating + // the watch if we are writing a result that doesn't match the current + // variables to avoid race conditions from broadcasting the wrong + // result. + if (!this.stopped && equal(this.variables, options.variables)) { // Any time we're about to update this.diff, we need to make // sure we've started watching the cache. this.updateWatch(options.variables); From 7c173a2c5860ed8ed65ff353217a75d08560525e Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Thu, 31 Aug 2023 14:25:55 -0600 Subject: [PATCH 7/8] Fix typo in changeset --- .changeset/odd-beers-perform.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/odd-beers-perform.md b/.changeset/odd-beers-perform.md index e42635ea4be..edf4b5d803a 100644 --- a/.changeset/odd-beers-perform.md +++ b/.changeset/odd-beers-perform.md @@ -2,4 +2,4 @@ "@apollo/client": patch --- -Fixes an issue where triggering a query with differnt variables then rapidly changing back to the initial variables with a cached result would return the wrong result if the previous request finished after switching back to initial variables. +Fixes an issue where triggering a query with different variables, then rapidly changing back to initial variables with a cached result would return the wrong result if the previous request finished after switching back to initial variables. From 2b1ce1d58020b213a0c7eef2a6d8375b7e668a3a Mon Sep 17 00:00:00 2001 From: Jerel Miller Date: Fri, 1 Sep 2023 10:37:28 -0600 Subject: [PATCH 8/8] Better changelog message --- .changeset/odd-beers-perform.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/odd-beers-perform.md b/.changeset/odd-beers-perform.md index edf4b5d803a..84266675835 100644 --- a/.changeset/odd-beers-perform.md +++ b/.changeset/odd-beers-perform.md @@ -2,4 +2,4 @@ "@apollo/client": patch --- -Fixes an issue where triggering a query with different variables, then rapidly changing back to initial variables with a cached result would return the wrong result if the previous request finished after switching back to initial variables. +Fix an issue where race conditions when rapidly switching between variables would sometimes result in the wrong `data` returned from the query. Specifically this occurs when a query is triggered with an initial set of variables (`VariablesA`), then triggers the same query with another set of variables (`VariablesB`) but switches back to the `VariablesA` before the response for `VariablesB` is returned. Previously this would result in the data for `VariablesB` to be displayed while `VariablesA` was active. The data is for `VariablesA` is now properly returned.