Never merge fields in cache unless objects have same identity. #5603

benjamn · 2019-11-21T19:20:26Z

These changes realize the consequences of the following extremely important principles:

In order to accumulate GraphQL data over time, the InMemoryCache must be able to merge the fields of different response objects together, as those objects are written to the cache.
However, merging the fields of two objects is allowable only when those objects are known to have the same identity.

If we cannot determine the identities of both objects (using __typename and id, or keyFields), or their identities are different, we should never merge their fields together, because doing so risks combining unrelated fields into the same logical entity object, resulting in an object that could never have been returned by the GraphQL server.
Even if two objects have the same identity, which allows us to merge their top-level fields, we should not assume we can recursively merge the values of fields that have the same name, unless those values are also identifiable objects with matching identities. Otherwise, we must replace the existing value with the incoming value, without attempting to combine them.

Exception A: if the existing value and the incoming value are deeply equal to each other, then we can safely reuse the existing value to avoid needlessly altering references in the cache.

Exception B: The application developer can provide custom field merge functions, e.g. for paginating array-valued fields. Since this level of customization was not possible in Apollo Client 2.x, some amount of automatic merging was necessary, but we can do the right thing now that the developer has more control.

I am happy that I did not have to update very many tests as a result of these changes, but the principles above are so important that I absolutely would have corrected as many tests as necessary to accommodate these changes. 🍳:feelsgood: ✨

These changes realize the consequences of the following extremely important principles: 1. In order to accumulate GraphQL data over time, the InMemoryCache must be able to merge the fields of different response objects together, as those objects are written to the cache. 2. However, merging the fields of two objects is allowable only when those objects have the same identity. If we cannot determine the identities of both objects (using __typename and id, or keyFields), or their identities are different, we should never merge their fields together, because doing so risks combining unrelated fields into the same logical entity object, resulting in an object that could never have been returned by the GraphQL server. 3. Even if two objects have the same identity, which allows us to merge their top-level fields, we should not assume we can recursively merge the values of fields that have the same name, unless those values are also identifiable objects with matching identities. Otherwise, we must replace the existing value with the incoming value, without attempting to combine them. Exception A: if the existing value and the incoming value are deeply equal to each other, then we can safely reuse the existing value to avoid needlessly altering references in the cache. Exception B: The application developer can provide custom field merge functions, e.g. for paginating array-valued fields. Since this level of customization was not possible in Apollo Client 2.x, some amount of automatic merging was necessary, but we can do the right thing now that the developer has more control. I am happy that I did not have to update very many tests as a result of these changes, but the principles above are so important that I absolutely would have thrown away the entire test suite if these changes had demanded more drastic action.

benjamn · 2019-11-21T19:43:02Z

src/cache/inmemory/writeToStore.ts

-    // If this property was overridden by a custom merge function, then
-    // its merged value has already been determined, so we should return
-    // incoming without recursively merging it into existing.
-    return incoming;


This logic was previously necessary to avoid re-merging data that had already been handled by custom merge functions. Now that we are no longer recursively merging data in the storeObjectReconciler function, there's no risk of double merging.

benjamn · 2019-11-21T19:44:42Z

src/cache/inmemory/writeToStore.ts

-      // Incoming references can be merged with existing non-reference data
-      // if the existing data appears to be of a compatible type.
-      store.set(
-        incoming.__ref,
-        this.merge(
-          existing,
-          store.get(incoming.__ref),


👋 to this terrible hack

benjamn · 2019-11-21T19:46:40Z

src/cache/inmemory/writeToStore.ts

-    if (Array.isArray(incoming)) {
-      if (!Array.isArray(existing)) return incoming;
-      if (existing.length > incoming.length) {
-        // Allow the incoming array to truncate the existing array, if the
-        // incoming array is shorter.
-        return this.merge(


Most arrays in GraphQL results contain a sequence of entity references, which are already merged according to their identities, so it was always a little questionable to be recursively merging arrays of non-reference data.

benjamn · 2019-11-21T20:03:32Z

src/cache/inmemory/writeToStore.ts

-    }
+    invariant(
+      !isReference(existing) || isReference(incoming),
+      `Store error: the application attempted to write an object with no provided id but the store already contains an id of ${existing.__ref} for this object.`,


I don't love this error, and it definitely confuses people (see #2510), but fixing it by including the necessary ID fields in the query seems to be worthwhile in virtually every situation (as opposed to disabling the error). Open to discussion!

Right - I think having it is better than not as well, so I'm all for keeping it.

hwillson

This looks great @benjamn! We might want to add a quick note about this in the changelog breaking changes section, but hopefully this won't impact many people (and the fact that you had to change very few tests for this has me very optimistic).

One consequence of #5603 is that replacing non-normalized data in the cache can result in loss of useful data. In almost every case, the right solution is to make sure the data can be normalized, or (if that isn't possible) to define a custom merge function for the replaced field, within the parent TypePolicy. It turns out we can give a very detailed warning in all such situations, so that's what this commit does. Looking at the output for our test suite, every warning is legitimate and worth fixing. I will resolve the warnings and test failures that our test suite generates in subsequent commits. TODO Update the documentation URLs after #5677 is merged.

One consequence of #5603 is that replacing non-normalized data in the cache can result in loss of useful data. In almost every case, the right solution is to make sure the data can be normalized, or (if that isn't possible) to define a custom merge function for the replaced field, within the parent TypePolicy. It turns out we can give a very detailed warning in all such situations, so that's what this commit does. Looking at the output for our test suite, every warning is legitimate and worth fixing. I will resolve the warnings and test failures that our test suite generates in subsequent commits.

> This is a revival of PR #5833, which has accumulated too many merge conflicts over the last few months to be worth rebasing. One consequence of #5603 is that replacing non-normalized data in the cache can result in loss of useful data, which is preferable to mistakenly merging unrelated objects, but not ideal. In almost every case, the right solution is to make sure the data can be normalized, or (if that isn't possible) to define a custom `merge` function for the field in question, within the parent type policy. It turns out we can give a very detailed warning about such situations in development, and that's what this commit does. For example: Cache data may be lost when replacing the d field of a Query object. To address this problem (which is not a bug in Apollo Client), either ensure all objects of type D have IDs, or define a custom merge function for the Query.d field, so InMemoryCache can safely merge these objects: existing: {"__typename":"D","e":4} incoming: {"__typename":"D","h":{"__typename":"H","i":7}} 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 Looking at the output for our test suite, every warning seems legitimate and worth fixing. I will resolve the warnings in subsequent commits.

Solves infinite loops when more than one query with different fields on an unidentified object Fixes apollographql#6370

benjamn added 🐞 bug 🚨 high-priority 🧞‍♂️ enhancement 🧩 implementation-detail labels Nov 21, 2019

benjamn added this to the Release 3.0 milestone Nov 21, 2019

benjamn requested review from hwillson and jbaxleyiii November 21, 2019 19:20

benjamn self-assigned this Nov 21, 2019

benjamn mentioned this pull request Nov 21, 2019

Release 3.0 #5116

Merged

31 tasks

benjamn commented Nov 21, 2019

View reviewed changes

hwillson approved these changes Nov 21, 2019

View reviewed changes

Mention PR #5603 in CHANGELOG.md.

25af540

benjamn merged commit 7dad1cf into release-3.0 Nov 21, 2019

benjamn mentioned this pull request Jan 7, 2020

[3.0] Nested object inside the cache gets overwritten by subsequent queries #5762

Closed

benjamn deleted the merge-fields-only-when-same-entity-identity branch January 7, 2020 17:45

benjamn mentioned this pull request Jan 23, 2020

Warn when clobbering non-normalized data in the cache. #5833

Closed

4 tasks

michael-land mentioned this pull request Apr 10, 2020

3.0.0-beta.43 Missing cache result fields warning, cache not updated after mutation #6136

Closed

benjamn mentioned this pull request Jun 1, 2020

Warn when clobbering non-normalized data in the cache. #6372

Merged

thomassuckow pushed a commit to thomassuckow/apollo-client that referenced this pull request Jun 13, 2020

Partially backpeddle apollographql#5603

d006ce1

Solves infinite loops when more than one query with different fields on an unidentified object Fixes apollographql#6370

benjamn mentioned this pull request Jun 15, 2020

Two queries loop forever when requesting different parts of same object #6370

Closed

chenesan mentioned this pull request Aug 9, 2020

[AC3] Hard to migrate to v3 due to lots of queries with non-normalized data type #6808

Closed

nlokare mentioned this pull request Sep 18, 2020

Optional Apollo Client configuration to make type policies in the cache merge by default apollographql/apollo-feature-requests#263

Closed

benjamn mentioned this pull request Oct 20, 2020

When more than one useQuery hook is used network requests double #7196

Closed

bchrobot mentioned this pull request Nov 3, 2021

feat: update apollo-client politics-rewired/Spoke#1027

Merged

9 tasks

github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Never merge fields in cache unless objects have same identity. #5603

Never merge fields in cache unless objects have same identity. #5603

benjamn commented Nov 21, 2019 •

edited

Loading

benjamn Nov 21, 2019 •

edited

Loading

benjamn Nov 21, 2019

benjamn Nov 21, 2019

benjamn Nov 21, 2019

hwillson Nov 21, 2019

hwillson left a comment

Never merge fields in cache unless objects have same identity. #5603

Never merge fields in cache unless objects have same identity. #5603

Conversation

benjamn commented Nov 21, 2019 • edited Loading

benjamn Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

benjamn Nov 21, 2019

Choose a reason for hiding this comment

benjamn Nov 21, 2019

Choose a reason for hiding this comment

benjamn Nov 21, 2019

Choose a reason for hiding this comment

hwillson Nov 21, 2019

Choose a reason for hiding this comment

hwillson left a comment

Choose a reason for hiding this comment

benjamn commented Nov 21, 2019 •

edited

Loading

benjamn Nov 21, 2019 •

edited

Loading