StoreWriter: Fragment selection not fully merged with other selections

[czabaj]

Sorry for opening another issue, I searched carefully but did not found a similar issue.

I encountered this bug when I attempt to mimic Relay style components and fragment organization, where each component defines its data needs as its own set of fragments, which parent screen component compose into screen query to gather all necessary data in one request. This could lead to queries with many overlapping fragments, some of which are deeply nested.

One such screen query ended in a weird state, where loading is false, but errors and data are both undefined. After some travel in the call stack, I found a silent "Can't find field..." error in readFromStore (described in #6375). Curiously, the field was returned from the server but was not stored in the cache.

After some more traveling in the call stack, I found out, that the field is lost in StoreWriter#writeToStore. I don't know the precise cause yet, but the problem is caused by fragment selection nested in another fragment selection -- the Query and parent Fragment selections are processed first and entities are stored in EntityStore, but then the nested fragment for an entity (with identity - id field - which seems to be related), is processed and its limited selection overwrites some entities. As the fragment selection does not fully overlap with other selections, the fragment simply omits some fields from the EntityStore.

I tried setting the merge functions for cache typePolicies per documentation in hope that it is matter of merge functions, but even setting merge: true for all types in query had no effect and even if it did, it would still smell like a bug in implementation to me.

I created a minimal reproducible example (MRE) in CodeSandbox. It is a failed test there and I hope the comments in code will help explain all the necessary details.

Intended outcome:

All fragment selections in a single query are merged into the EntityStore during StoreWriter#writeToStore, thus the EntityStore contains all the data returned from the server.

Actual outcome:

Deeply nested fragments in some cases (see a minimal reproducible example for details) overwrites record for a certain entity in EntityStore, which, in case the fragment selection does not fully overlap with other selections, leads to (silent) "Can't find field..." error in readFromStore.

How to reproduce the issue:

Open the MRE https://codesandbox.io/s/apollo-cache-error-bnu9w?file=/src/index.test.js

Versions

  System:
    OS: macOS 11.2.3
  Binaries:
    Node: 14.16.0 - ~/.nvm/versions/node/v14.16.0/bin/node
    Yarn: 1.22.10 - ~/.nvm/versions/node/v14.16.0/bin/yarn
    npm: 7.7.4 - ~/.nvm/versions/node/v14.16.0/bin/npm
  Browsers:
    Chrome: 90.0.4430.85
    Edge: 90.0.818.42
    Firefox: 85.0.2
    Safari: 14.0.3
  npmPackages:
    @apollo/client: 3.3.15 => 3.3.15
    apollo-server-errors: ^2.4.2 => 2.5.0

[benjamn]

@czabaj The problem here is that your OrderItem.rewards fields is a list, and you seem to be expecting the existing elements to get merged with incoming elements according to their indexes (same-index elements get merged).

However, that merging is not something the cache can safely do by default, since the positions of the elements in the two arrays don't give the cache any information about the logical identities of the objects.

Instead, since you seem to know it's safe to merge these array elements by index, you can/should configure that behavior with a merge function for the OrderItem.rewards field:

new InMemoryCache({
  typePolicies: {
    OrderItem: {
      fields: {
        rewards: {
          merge(existing: any[] | undefined, incoming: any[], { mergeObjects }) {
            if (existing) {
              // Merge object elements that have the same index:
              const merged = existing.map((e, i) => mergeObjects(e, incoming[i]));
              // In case incoming.length > existing.length:
              const additional = incoming.slice(existing.length);
              return [ ...merged, ...additional ];
            }
            return incoming;
          },
        },
      },
    },
  },
})

I've opened a PR with an adaptation of your regression test (thanks!), showing both the initial failure and also that adding the merge function above fixes the test. Happy to answer questions about the details if any of this is unclear.

While the merge function approach will work, a simpler approach would be to follow this unofficial rule of thumb: Make sure any objects used as list elements can be normalized (that is, the objects have a __typename and id, or you've configured some other keyFields). If you follow this rule, all your array elements will have IDs, so the positions of the (reference) objects in the array won't matter for the merging of the objects, since the cache can safely and automatically merge entity objects that have the same ID.

You still might want to define a merge function for OrderItem.rewards to control how the arrays of references are combined (if you don't want incoming to replace existing), but the implementation of that merge function should be easier, since you won't have to call mergeObjects on the array elements.

[czabaj]

Thank you for your very nice explanation! The "unofficial rule of thumb" is very helpful for future API design. Coincidentally, I assign a synthetic id to the Reward in my business logic, but, to be able to make the ids truly unique, I concatenate its type with its parent OrderItem.id which I do outside of the Apollo. I'm thinking about create of an id @client field on Rewards, but it seems to me that accessing the parent of Rewards is practically impossible in read field policy function and I'm unsure, whether the read field policy is processed before the field is stored into the cache? I know that deprecated LocalState processes data before it is stored into the cache, but as the read function is technically part of the cache, creating a @client field backed by read function and used in keyField policy might not work, or will it?

Anyway, using the merge function for the field is brilliant. I'm also think about setting keyFields: false for the Order as I don't need to access it apart from the parents and it has tree like relation (Application -> Order -> OrderItem[] -> Rewards[]) i.e. there is no cyclic dependency and not other Application contains the same Order, so storing everything unnormalized inside Order might theoretically work, but I tried it and the app crashed, so it's not that simple.

[czabaj]

As the behavior proved not to be a bug, I'm closing this issue.

[benjamn]

creating a @client field backed by read function and used in keyField policy might not work, or will it?

@czabaj I've given this some thought before, and while I think it might not be fundamentally impossible, I can confirm it will not work right now. Specifically, if you try to define a read function for a key field, the field value that ends up in the ID will just be whatever was in the query result. To help catch mistakes, cache.identify(obj) throws if any of the keyFields: [...] fields you specify are not present in the result object, but that only happens if you configure keyFields.