Bug with empty arrays starting with client 3.4.0

[jheysen]

When receiving objects with different empty arrays, starting with 3.4.0 the arrays point to the same memory address when they are not the same object. It was tested against 3.3.21 where the problem doesn't occur, and the problem is still present in 3.4.9. In 3.4.x using fetch-policy no-cache doesn't show the problem since the new beheavour for the cache is avoided.

This poses a problem to applications that could push items to arrays after receiving them from network due to processing. Since the memory address returned for the different keys is the same for all empty arrays, lodash's cloneDeep doesn't do good eigther.

Intended outcome:
Different empty arrays shouldn't mix since they could represent different things.

Actual outcome:
Empty arrays all point to the same memory address and are indistinguishable from one another

How to reproduce the issue:

Trying to make a small repro in:
https://codesandbox.io/s/objective-pond-0jscd?file=/src/index.jsx

Versions
System:
OS: macOS 11.5.2
Binaries:
Node: 14.17.4 - ~/.nvm/versions/node/v14.17.4/bin/node
npm: 7.20.3 - ~/.nvm/versions/node/v14.17.4/bin/npm
Browsers:
Chrome: 92.0.4515.159
Safari: 14.1.2
npmPackages:
@apollo/client: ^3.4.9 => 3.4.9
apollo-angular: ^2.6.0 => 2.6.0
apollo3-cache-persist: ^0.8.0 => 0.8.0

[benjamn]

This is expected behavior due to cache result canonization (PR #7439).

This poses a problem to applications that could push items to arrays after receiving them from network due to processing.

The canonized result objects are frozen/immutable exactly so this can't happen. To modify one of these arrays you'd need to make a mutable copy, leaving the original [] reference unchanged.

Since the memory address returned for the different keys is the same for all empty arrays, lodash's cloneDeep doesn't do good eigther.

JSON.parse(JSON.stringify(result)) will give you back all the array/object diversity you could want. GraphQL result objects are JSON-serializable (and tree-shaped), unless you're using custom scalar values (or something like that), so JSON.stringify should be safe here.

You can also opt out of canonization by passing canonizeResults: false to useQuery:

const { loading, data } = useQuery(ALL_PEOPLE, {
  canonizeResults: false,
});

thanks to PR #8188. As a side note, I had to convince my team there would be some people who didn't want the memory savings and equality speedup of canonical results, so this issue makes me glad we went to the trouble of making this behavior configurable.

[jheysen]

The problem here is that for objects, the canonization would work for us, its just the empty arrays that give us loads of problems thus far

[benjamn]

In your reproduction, this code won't work because person and all its contents are immutable:

    if (person.pets?.length < 1) person.pets?.push("None");
    if (person.foods?.length < 1) person.foods?.push("No food");

This should throw an error in JS strict mode (inside any module), but will silently fail in script/sloppy mode. Either way, it doesn't do what you want.

In my previous comment, I was suggesting a style more like this:

const pets = person.pets?.slice(0) || [];
const foods = person.foods?.slice(0) || [];

// Now array mutation is safe:
if (pets.length < 1) pets.push("None");
if (foods.length < 1) foods.push("No food");

// Use pets and foods below...

Happy to look at other examples of problems you're having with empty arrays.

[KeithGillette]

We are experiencing the same problem with this changed cache result canonization behavior in our Apollo Angular project. One of our types contains deeply nested arrays of the form:

type Parent {
   _id: ID!
  …
  childList: [Child!]
} 

type Child {
   _id: ID!
  …
   childList: [Child!]
}

ChildCreate (
  parentId: ID!
  …
  childIndexPath: [Int!]!
): Child

Since our ChildCreate mutation returns a Child, we use the mutation update function in ApolloClient to insert the newly created Child into the correct position in the Parent.childList nested array by reading the ParentReadQuery via ApolloCache.readQuery, using cloneDeep to create a mutable copy, inserting the new Child from ChildCreate into the correct position in the nested arrays using the mutation variables, then writing the cloned query back via ApolloCache.writeQuery. This worked fine before the cache canonization, but now it can insert duplicate copies of the Child into empty arrays in nested the data structure.

While I confirm that passing canonizeResults: false in the ApolloCache.readQuery call corrects the issue, I remain somewhat confused why cloneDeep doesn't prevent the issue. I gather from @jheysen's original description is that cloneDeep just makes one mutable empty array shared by all array properties that don't have entries, since there's actually only one shared empty array in the cached query?

[jheysen]

Thats correct, cloneDeep detects thats all a reference to the same array and creates a deep copy of it, but uses the new copy as the reference for every reference is the cloned object… El El vie, 3 de sep. de 2021 a la(s) 17:48, Keith Gillette < ***@***.***> escribió:

…

We are experiencing the same problem with this changed cache result canonization behavior in our Apollo Angular project. One of our types contains deeply nested arrays of the form: type Parent { _id: ID! … childList: [Child!] } type Child { _id: ID! … childList: [Child!] } ChildCreate ( parentId: ID! … childIndexPath: [Int!]! ): Child Since our ChildCreate mutation returns a Child, we use the mutation update function in ApolloClient to insert the newly created Child into the correct position in the Parent.childList nested array by reading the ParentReadQuery via ApolloCache.readQuery, using cloneDeep to create a mutable copy, inserting the new Child from ChildCreate into the correct position in the nested arrays using the mutation variables, then writing the cloned query back via ApolloCache.writeQuery. This worked fine before the cache canonization, but now it can insert duplicate copies of the Child into empty arrays in nested the data structure. While I confirm that passing canonizeResults: false in the ApolloCache.readQuery call corrects the issue, I remain somewhat confused why cloneDeep doesn't prevent the issue. I gather from @jheysen <https://github.com/jheysen>'s original description is that cloneDeep just makes one mutable empty array shared by all array properties that don't have entries, since there's actually only one shared empty array in the cached query? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8717 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANTBZP2NESW3W33QLDLRP3UAE7D5ANCNFSM5C5W75KA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[benjamn]

Although I don't have a full solution yet, I wanted to let you (@jheysen and @KeithGillette) know that I do appreciate how canonization can cause problems for existing code that uses cloneDeep, and I'm confident we can find a way to keep object references stable without making all empty arrays === identical. My previous comments were proposing workarounds, but I agree there's something here that needs improvement/rethinking/fixing.

[benjamn]

@jheysen @KeithGillette Please have a look at #8822 when you have a chance. We're thinking about shipping that mitigation in a v3.4.x patch version, to remove the immediate surprise of canonization. Returning === objects when possible is still a goal for the cache, and canonization still works, but (with that PR) you'd have to opt in instead of opting out.