Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent QueryInfo#markResult mutation of result.data and return cache data consistently whether complete or incomplete #11202

Merged
5 changes: 5 additions & 0 deletions .changeset/shaggy-ears-scream.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": minor
---

Prevent `QueryInfo#markResult` mutation of `result.data` and return cache data consistently whether complete or incomplete.
2 changes: 1 addition & 1 deletion .size-limit.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const checks = [
{
path: "dist/index.js",
import: "{ ApolloClient, InMemoryCache, HttpLink }",
limit: "32044",
limit: "32052",
},
...[
"ApolloProvider",
Expand Down
5 changes: 4 additions & 1 deletion src/__tests__/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2921,7 +2921,10 @@ describe("client", () => {
return client
.query({ query })
.then(({ data }) => {
expect(data).toEqual(result.data);
const { price, ...todoWithoutPrice } = data.todos[0];
expect(data).toEqual({
todos: [todoWithoutPrice],
});
})
.then(resolve, reject);
}
Expand Down
171 changes: 171 additions & 0 deletions src/cache/inmemory/__tests__/client.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
// This file contains InMemoryCache-specific tests that exercise the
// ApolloClient class. Other test modules in this directory only test
// InMemoryCache and related utilities, without involving ApolloClient.

import { ApolloClient, WatchQueryFetchPolicy, gql } from "../../../core";
import { ApolloLink } from "../../../link/core";
import { Observable } from "../../../utilities";
import { InMemoryCache } from "../..";
import { subscribeAndCount } from "../../../testing";

describe("InMemoryCache tests exercising ApolloClient", () => {
it.each<WatchQueryFetchPolicy>([
"cache-first",
"network-only",
"cache-and-network",
"cache-only",
"no-cache",
])(
"results should be read from cache even when incomplete (fetchPolicy %s)",
(fetchPolicy) => {
const dateFromCache = "2023-09-14T13:03:22.616Z";
const dateFromNetwork = "2023-09-15T13:03:22.616Z";

const cache = new InMemoryCache({
typePolicies: {
Query: {
fields: {
date: {
read(existing) {
return new Date(existing || dateFromCache);
},
},
},
},
},
});

const client = new ApolloClient({
link: new ApolloLink(
(operation) =>
new Observable((observer) => {
observer.next({
data: {
// This raw string should be converted to a Date by the Query.date
// read function passed to InMemoryCache below.
date: dateFromNetwork,
// Make sure we don't accidentally return fields not mentioned in
// the query just because the result is incomplete.
ignored: "irrelevant to the subscribed query",
// Note the Query.missing field is, well, missing.
},
});
setTimeout(() => {
observer.complete();
}, 10);
})
),
cache,
});

const query = gql`
query {
date
missing
}
`;

const observable = client.watchQuery({
query,
fetchPolicy, // Varies with each test iteration
returnPartialData: true,
});

return new Promise<void>((resolve, reject) => {
subscribeAndCount(reject, observable, (handleCount, result) => {
let adjustedCount = handleCount;
if (
fetchPolicy === "network-only" ||
fetchPolicy === "no-cache" ||
fetchPolicy === "cache-only"
) {
// The network-only, no-cache, and cache-only fetch policies do not
// deliver a loading:true result initially, so we adjust the
// handleCount to skip that case.
++adjustedCount;
}

// The only fetch policy that does not re-read results from the cache is
// the "no-cache" policy. In this test, that means the Query.date field
// will remain as a raw string rather than being converted to a Date by
// the read function.
const expectedDateAfterResult =
fetchPolicy === "cache-only"
? new Date(dateFromCache)
: fetchPolicy === "no-cache"
? dateFromNetwork
: new Date(dateFromNetwork);

if (adjustedCount === 1) {
expect(result.loading).toBe(true);
expect(result.data).toEqual({
date: new Date(dateFromCache),
});
} else if (adjustedCount === 2) {
expect(result.loading).toBe(false);
expect(result.data).toEqual({
date: expectedDateAfterResult,
// The no-cache fetch policy does return extraneous fields from the
// raw network result that were not requested in the query, since
// the cache is not consulted.
...(fetchPolicy === "no-cache"
? {
ignored: "irrelevant to the subscribed query",
}
: null),
});

if (fetchPolicy === "no-cache") {
// The "no-cache" fetch policy does not receive updates from the
// cache, so we finish the test early (passing).
setTimeout(() => resolve(), 20);
} else {
cache.writeQuery({
query: gql`
query {
missing
}
`,
data: {
missing: "not missing anymore",
},
});
}
} else if (adjustedCount === 3) {
expect(result.loading).toBe(false);
expect(result.data).toEqual({
date: expectedDateAfterResult,
missing: "not missing anymore",
});

expect(cache.extract()).toEqual({
ROOT_QUERY: {
__typename: "Query",
// The cache-only fetch policy does not receive updates from the
// network, so it never ends up writing the date field into the
// cache explicitly, though Query.date can still be synthesized by
// the read function.
...(fetchPolicy === "cache-only"
? null
: {
// Make sure this field is stored internally as a raw string.
date: dateFromNetwork,
}),
// Written explicitly with cache.writeQuery above.
missing: "not missing anymore",
// The ignored field is never written to the cache, because it is
// not included in the query.
},
});

// Wait 20ms to give the test a chance to fail if there are unexpected
// additional results.
setTimeout(() => resolve(), 20);
} else {
reject(new Error(`Unexpected count ${adjustedCount}`));
}
});
});
}
);
});
23 changes: 13 additions & 10 deletions src/core/QueryInfo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,8 @@ export class QueryInfo {
"variables" | "fetchPolicy" | "errorPolicy"
>,
cacheWriteBehavior: CacheWriteBehavior
) {
): typeof result {
result = { ...result };
const merger = new DeepMerger();
const graphQLErrors = isNonEmptyArray(result.errors)
? result.errors.slice(0)
Expand Down Expand Up @@ -408,7 +409,10 @@ export class QueryInfo {
});

this.lastWrite = {
result,
// 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 },
Comment on lines +412 to +415
Copy link
Member

@phryneas phryneas Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to add: I'm definitely not challenging this defensive copy - 馃挴 agree!

variables: options.variables,
dmCount: destructiveMethodCounts.get(this.cache),
};
Expand Down Expand Up @@ -470,20 +474,19 @@ export class QueryInfo {
this.updateWatch(options.variables);
}

// If we're allowed to write to the cache, and we can read a
// complete result from the cache, update result.data to be the
// result from the cache, rather than the raw network result.
// Set without setDiff to avoid triggering a notify call, since
// we have other ways of notifying for this result.
// If we're allowed to write to the cache, update result.data to be
// the result as re-read from the cache, rather than the raw network
// result. Set without setDiff to avoid triggering a notify call,
// since we have other ways of notifying for this result.
this.updateLastDiff(diff, diffOptions);
if (diff.complete) {
result.data = diff.result;
}
result.data = diff.result;
});
} else {
this.lastWrite = void 0;
}
}

return result;
}

public markReady() {
Expand Down
2 changes: 1 addition & 1 deletion src/core/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1176,7 +1176,7 @@ export class QueryManager<TStore> {
// Use linkDocument rather than queryInfo.document so the
// operation/fragments used to write the result are the same as the
// ones used to obtain it from the link.
queryInfo.markResult(
result = queryInfo.markResult(
result,
linkDocument,
options,
Expand Down
113 changes: 1 addition & 112 deletions src/core/__tests__/QueryManager/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,6 @@ export function resetStore(qm: QueryManager<any>) {
}

describe("QueryManager", () => {
// Standard "get id from object" method.
const dataIdFromObject = (object: any) => {
if (object.__typename && object.id) {
return object.__typename + "__" + object.id;
}
return undefined;
};

// Helper method that serves as the constructor method for
// QueryManager but has defaults that make sense for these
// tests.
Expand Down Expand Up @@ -2224,107 +2216,6 @@ describe("QueryManager", () => {
}
);

itAsync(
"should not return stale data when we orphan a real-id node in the store with a real-id node",
(resolve, reject) => {
const query1 = gql`
query {
author {
name {
firstName
lastName
}
age
id
__typename
}
}
`;
const query2 = gql`
query {
author {
name {
firstName
}
id
__typename
}
}
`;
const data1 = {
author: {
name: {
firstName: "John",
lastName: "Smith",
},
age: 18,
id: "187",
__typename: "Author",
},
};
const data2 = {
author: {
name: {
firstName: "John",
},
id: "197",
__typename: "Author",
},
};
const reducerConfig = { dataIdFromObject };
const queryManager = createQueryManager({
link: mockSingleLink(
{
request: { query: query1 },
result: { data: data1 },
},
{
request: { query: query2 },
result: { data: data2 },
},
{
request: { query: query1 },
result: { data: data1 },
}
).setOnError(reject),
config: reducerConfig,
});

const observable1 = queryManager.watchQuery<any>({ query: query1 });
const observable2 = queryManager.watchQuery<any>({ query: query2 });

// I'm not sure the waiting 60 here really is required, but the test used to do it
return Promise.all([
observableToPromise(
{
observable: observable1,
wait: 60,
},
(result) => {
expect(result).toEqual({
data: data1,
loading: false,
networkStatus: NetworkStatus.ready,
});
}
),
observableToPromise(
{
observable: observable2,
wait: 60,
},
(result) => {
expect(result).toEqual({
data: data2,
loading: false,
networkStatus: NetworkStatus.ready,
});
}
),
]).then(resolve, reject);
}
);

itAsync(
"should return partial data when configured when we orphan a real-id node in the store with a real-id node",
(resolve, reject) => {
Expand Down Expand Up @@ -2519,9 +2410,7 @@ describe("QueryManager", () => {
loading: false,
networkStatus: NetworkStatus.ready,
data: {
info: {
a: "ay",
},
info: {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this result disappeared here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }
      }
    }
  }
})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for the explanation!

},
});
setTimeout(resolve, 100);
Expand Down