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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache does not merge fragments correctly #8370

Closed
chrbala opened this issue Jun 11, 2021 · 3 comments · Fixed by #8372
Closed

Cache does not merge fragments correctly #8370

chrbala opened this issue Jun 11, 2021 · 3 comments · Fixed by #8372

Comments

@chrbala
Copy link

chrbala commented Jun 11, 2021

Intended outcome:
Complex fragment merges work as though they were all specified in a single large query

Actual outcome:
Fragment merges can result in the cache dropping fields, which ends up returning an empty value from useQuery when returnPartialData is off, and misses fields when returnPartialData is on.

How to reproduce the issue:
https://github.com/chrbala/apollo-bug-repro/tree/data-missing

As the repo is presented, the data comes back empty, and there is a warning about cache data getting lost. Note that the data comes back correctly from the "server", but is corrupted in the cache.

  • If returnPartialData is used, the query returns a value, but the text field is missing from the data.
  • If the text field on line 86 is replaced with __typename (the same as the contents of ItemFragment), the query succeeds. This is likely because there are no conflicts for the item.
  • If ...ValueFragment is removed from line 82, the query succeeds. This is likely because there are no conflicts for the item.
  • If the id field is removed from line 84, the query succeeds. This is likely because the item is included in the parent's item in the cache and is no longer normalized.

There appears to be a very broad issue (#8063) that shares this outcome, but this seems to be one of likely many root causes, so I thought it was appropriate to open a unique issue here.

Versions
System:
OS: macOS Mojave 10.14.6
Binaries:
Node: 12.13.1 - /usr/local/bin/node
Yarn: 1.22.10 - /usr/local/bin/yarn
npm: 6.12.1 - /usr/local/bin/npm
Browsers:
Chrome: 91.0.4472.77
Firefox: 89.0
Safari: 14.1
npmPackages:
@apollo/client: ^3.3.19 => 3.3.20

@benjamn benjamn self-assigned this Jun 11, 2021
benjamn added a commit that referenced this issue Jun 11, 2021
Fixes #8370 by postponing all context.store.merge updates until after
all processSelectionSet work is done, performing the store updates at
the end of StoreWriter#writeToStore, whereas previously those updates
happened at the end of each StoreWriter#processSelectionSet call.

The postponement of store updates gives the StoreWriter a chance to
merge together multiple partial objects representing the same logical
object (possibly matched by different fragments along different query
paths), before running merge functions with applyMerges.

Previously, those partial objects would be merged separately into
context.store, so later objects might replace earlier objects in cases
where the objects are unidentifiable/non-normalized, and there is no
merge function (or merge:true) to force the objects to merge.

Of course, you could configure merge:true to explicily permit merging
unidentified objects with a given __typename. Configuring merge:true for
the 'Container' type seems to fixes the reproduction in #8370. But this
explicit configuration should not be necessary when we know the objects
in question represent the same logical object, because they were derived
from the same original result object (same ID, not necessarily ===).

In other words, this PR should wipe out a whole class of situations
where we used to show "Cache data may be lost..." warnings, since we can
now safely merge the fields of unidentifiable objects that share the
same original result object, without requiring an explicit merge:true
configuration to permit that mixing of fields.
benjamn added a commit that referenced this issue Jun 11, 2021
Fixes #8370 by postponing all context.store.merge updates until after
all processSelectionSet work is done, performing the store updates at
the end of StoreWriter#writeToStore, whereas previously those updates
happened at the end of each StoreWriter#processSelectionSet call.

The postponement of store updates gives the StoreWriter a chance to
merge together multiple partial objects representing the same logical
object (possibly matched by different fragments along different query
paths), before running merge functions with applyMerges.

Previously, those partial objects would be merged separately into
context.store, so later objects might replace earlier objects in cases
where the objects are unidentifiable/non-normalized, and there is no
merge function (or merge:true) to force the objects to merge.

Of course, you could configure merge:true to explicily permit merging
unidentified objects with a given __typename. Configuring merge:true for
the 'Container' type seems to fixes the reproduction in #8370. But this
explicit configuration should not be necessary when we know the objects
in question represent the same logical object, because they were derived
from the same original result object (same ID, not necessarily ===).

In other words, this PR should wipe out a whole class of situations
where we used to show "Cache data may be lost..." warnings, since we can
now safely merge the fields of unidentifiable objects that share the
same original result object, without requiring an explicit merge:true
configuration to permit that mixing of fields.
@benjamn
Copy link
Member

benjamn commented Jun 11, 2021

@chrbala Please see #8372 for a solution that fixes your reproduction (thanks!) without any additional configuration of field merge functions.


⚠️ The text below was going to be my original answer, before I realized we really shouldn't have to display the Cache data may be lost... error in this case, since the Container objects in question are derived from the same original result object (along different query paths), which means merging the fields back together should be safe, without requiring a merge: true configuration.

Though it's not the right answer to this issue, I'm leaving this generic advice here, in case it's useful to anybody:

If you take a look at the browser console when running your reproduction, you'll see a descriptive warning that helps explain the problem (implemented in #6372):

Cache data may be lost when replacing the value field of a Item object.

To address this problem (which is not a bug in Apollo Client), either ensure all objects of type Container have an ID or a custom merge function, or define a custom merge function for the Item.value field, so InMemoryCache can safely merge these objects:

  existing: {"__typename":"Container","text":"Hello World"}
  incoming: {"__typename":"Container","value":{"__typename":"Value"}}

For more information about these options, please refer to the documentation:

  * Ensuring entity objects have IDs: https://go.apollo.dev/c/generating-unique-identifiers
  * Defining custom merge functions: https://go.apollo.dev/c/merging-non-normalized-objects

Though I recommend reading that documentation, here's the most likely way you'll want to fix this problem:

diff --git a/src/index.jsx b/src/index.jsx
index a302376..a399766 100644
--- a/src/index.jsx
+++ b/src/index.jsx
@@ -111,7 +111,13 @@ function App() {
 }
 
 const client = new ApolloClient({
-  cache: new InMemoryCache({}),
+  cache: new InMemoryCache({
+    typePolicies: {
+      Container: {
+        merge: true,
+      },
+    },
+  }),
   link,
 });

Alternatively, you could make sure that your { __typename: "Container" } objects also have an id field, so the cache can tell if two Container objects have the same logical identity, which allows their fields to be merged automatically.

@benjamn benjamn added this to the Release 3.4 milestone Jun 11, 2021
@chrbala
Copy link
Author

chrbala commented Jun 11, 2021

Great to see a fix already in the works! As an an aside, originally I had arrays in the structure, which did not show the Cache data may be lost when replacing the value field of a Item object. warning until I swapped the array for a simple type reference.

type Container {
  text: String!
  value: [Value!]!
}

@hwillson
Copy link
Member

Fixed in #8372 - thanks!

@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
bengotow added a commit to dagster-io/dagster that referenced this issue Feb 1, 2023
…n health (#12030)

### Summary & Motivation

This fix is somewhat unsatisfactory but it appears that the use of React
fragments in the query for partition health was confusing Apollo
client's cache, such that it was unable to fetch and return the
partition health data.

I am attempting to narrow down a repro case that we can both report
upstream and use to make sure that we are not doing anything similar
elsewhere in Dagit.

Possible related upstream issue:
apollographql/apollo-client#8370

### How I Tested These Changes

Co-authored-by: bengotow <[email protected]>
jamiedemaria pushed a commit to dagster-io/dagster that referenced this issue Feb 1, 2023
…n health (#12030)

### Summary & Motivation

This fix is somewhat unsatisfactory but it appears that the use of React
fragments in the query for partition health was confusing Apollo
client's cache, such that it was unable to fetch and return the
partition health data.

I am attempting to narrow down a repro case that we can both report
upstream and use to make sure that we are not doing anything similar
elsewhere in Dagit.

Possible related upstream issue:
apollographql/apollo-client#8370

### How I Tested These Changes

Co-authored-by: bengotow <[email protected]>
Ramshackle-Jamathon pushed a commit to dagster-io/dagster that referenced this issue Feb 2, 2023
…n health (#12030)

### Summary & Motivation

This fix is somewhat unsatisfactory but it appears that the use of React
fragments in the query for partition health was confusing Apollo
client's cache, such that it was unable to fetch and return the
partition health data.

I am attempting to narrow down a repro case that we can both report
upstream and use to make sure that we are not doing anything similar
elsewhere in Dagit.

Possible related upstream issue:
apollographql/apollo-client#8370

### How I Tested These Changes

Co-authored-by: bengotow <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants