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

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

Merged
merged 6 commits into from
Jun 1, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jun 1, 2020

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.

> 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.
Comment on lines 536 to 538
if (process.env.NODE_ENV !== "production") {
warnAboutDataLoss(existingObject, incomingObject, property, getFieldValue);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this call is restricted to development, production minifiers should be able to prune virtually all of the code introduced by this PR.

Comment on lines 580 to 584
const fieldName = fieldNameFromStoreName(storeFieldName);
const typeDotName = `${parentType}.${fieldName}`;

if (warnings.has(typeDotName)) return;
warnings.add(typeDotName);
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is a fairly verbose warning, I wanted to make sure it's displayed at most once per Type.field.

benjamn added 5 commits June 1, 2020 17:01
Since cache.modify calls cache.merge, and we don't want cache.modify ever
to trigger "Cache data may be lost..." warnings, it's more convenient if
the StoreWriter is responsible for calling warnAboutDataLoss, rather than
always performing that check in cache.merge.
@benjamn benjamn merged commit 6cb48af into master Jun 1, 2020
@benjamn benjamn deleted the warn-when-clobbering-non-normalized-data branch June 1, 2020 22:53
benjamn added a commit that referenced this pull request Jun 1, 2020
This should have been part of #6372 (just merged).
benjamn added a commit that referenced this pull request Jun 16, 2020
Ever since the big refactoring in #6221 made the client more aggressive
about triggering network requests in response to incomplete cache data
(when using a cache-first FetchPolicy), folks testing the betas/RCs have
reported that feuding queries can end up triggering an endless loop of
unhelpful network requests.

This change does not solve the more general problem of queries competing
with each other to update the same data in incompatible ways (see #6372
for mitigation strategies), but I believe this commit will effectively put
a stop to most cases of endless network requests.

See my lengthy implementation comment for more details, since this is a
non-obvious solution to a very tricky problem.

Fixes #6307.
Fixes #6370.
Fixes #6434.
Fixes #6444.
benjamn added a commit that referenced this pull request Jun 16, 2020
…6448)

Ever since the big refactoring in #6221 made the client more aggressive
about triggering network requests in response to incomplete cache data
(when using a cache-first FetchPolicy), folks testing the betas/RCs have
reported that feuding queries can end up triggering an endless loop of
unhelpful network requests.

This change does not solve the more general problem of queries competing
with each other to update the same data in incompatible ways (see #6372
for mitigation strategies), but I believe this commit will effectively put
a stop to most cases of endless network requests.

See my lengthy implementation comment for more details, since this is a
non-obvious solution to a very tricky problem.

Fixes #6307.
Fixes #6370.
Fixes #6434.
Fixes #6444.
// If we're replacing every key of the existing object, then the
// existing data would be overwritten even if the objects were
// normalized, so warning would not be helpful here.
if (Object.keys(existing).every(
Copy link
Member

Choose a reason for hiding this comment

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

Does this check make sense when existing is an Array? It seems (though it's very likely I'm missing something) that this implies that this warning is skipped whenever incoming is at least as long as existing, which seems wrong because as said below arrays always need a custom merge function?

@jefbarn
Copy link

jefbarn commented Jul 30, 2020

@benjamn Is there a way to disable this warning for all array types? My API will always return merged results and I don't need a warning every time I add an item to an array. typePolicies is not usable because we're talking dozens of types and hundreds of fields.

@KenneyE
Copy link

KenneyE commented Aug 6, 2020

I'm running into a similar issue. I have an unregisterFromClass mutation that returns a full list of registeredClasses, which should replace whatever is in the cache. After unregistering from a single class, I receive this warning:

Cache data may be lost when replacing the registeredClasses field of a User object.

To address this problem (which is not a bug in Apollo Client), define a custom merge function for the User.registeredClasses field, so InMemoryCache can safely merge these objects:

  existing: [{"__ref":"ClassRegistration:334729"}]
  incoming: []

It appears everything is properly normalized. Existing and incoming are both correct. It seems like no warning is needed in this case.

@benjamn
Copy link
Member Author

benjamn commented Aug 7, 2020

@KenneyE You're right that no data about the ClassRegistration object will be lost by removing the reference from that array, but the presence of the reference in the array is a kind of information, too, which the cache can't assume is safe to discard.

However, since you know that this is fine/expected, you can silence the warning by providing a field policy like this:

new InMemoryCache({
  typePolicies: {
    User: {
      fields: {
        registeredClasses: {
          merge: false,
        },
      },
    },
  },
})

The merge: false shorthand is short for merge(_, incoming) { return incoming }, introduced in @apollo/client@3.1.0 by PR #6714.

@benjamn
Copy link
Member Author

benjamn commented Aug 7, 2020

@jefbarn There's not a good way to bulk-disable the warnings right now (except to use merge: false to keep things relatively short). We are currently considering ways to allow dynamic/lazy field policy generation, based on the __typename and field name, which might allow you to use less code to specify a bunch of similar field policies.

In your case, if the behavior is triggered by the type of the field being an array, you'd need to be able to determine the schema type from the __typename and field name, which isn't something the client automatically knows, but maybe you could come up with a naming convention, or extract a list of types/fields at build time.

@KenneyE
Copy link

KenneyE commented Aug 7, 2020

Thanks @benjamn! I was hoping to avoid having to add that for every array field, but it's a fair trade off. Appreciate the response.

@benjamn
Copy link
Member Author

benjamn commented Sep 24, 2020

I'm pretty excited about #7070, which makes it easier to configure merge: true (or any other merge behavior) on a type-by-type basis, rather than a field-by-field basis. There's still no general solution for arrays, but #7070 should make working with mergeable object types a lot easier.

benjamn added a commit that referenced this pull request Sep 24, 2020
benjamn added a commit that referenced this pull request Sep 24, 2020
benjamn added a commit that referenced this pull request Sep 24, 2020
@jpvajda jpvajda mentioned this pull request Aug 1, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants