Fix cache key generation for arrays by calling orderIndependentKey fo…

[kyum1n]

…r each element

[apollo-cla]

@kyum1n: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[designatednerd]

Hi! Is there a bug you can point to or a test case you can add to explain what's happening here?

[kyum1n]

I did not create an issue but I guess the diff is not self-explanatory, sorry. There is a bug in the caching when the query contains an array of objects.

When no cacheKeyForObject is provied to the ApolloStore, a cache key will be generated based on the field path. Since dictionaries have no defined order, the function orderIndependentKey is used to sort the entries by their keys to ensure that the cache key is always the same.

Until now this function had two cases:

1. the passed object's value is a JSONObject: In this case the function was called on its value again.
2. "else": Which just created a "key:value"-string

But if the value is an array, then the 2nd case would just use the default description of that array. The problem becomes visible when this array contains JSONObjects with more than one key because in that case the order of the keys is undefined and the resulting key is not the same anymore for the multiple calls of the same request.

Let's say we have an object with an array with a single object like this:

{ 
    "someArray": [
        {
            "key1": "value1",
            "key2": "value2"
        }
    ]
}

orderIndependentKey will currently return randomly either
"someArray": "["key1": "value1", "key2": "value2"]"
or
"someArray": "["key2": "value2", "key1": "value1"]"
while my suggested fix would always return the first.

Currently the cache only randomly hits the cached key and thus is unreliable for queries with arrays and also stores the same data under different cache keys because of that undefined order.

[designatednerd]

OK that makes way more sense. Can you please add a test for this so I don't accidentally break it in the future? 😃

[kyum1n]

Unfortunately I am not familiar with the testing setup here and how I would go about adding a query with an array-param.

[designatednerd]

Oof, yeah there's not anything in the StarWars API that takes inputs as an array. There is in the GitHub API, but that'll require a bunch more bootstrapping. I'll PR something that adds a query with an array, and then you can pull that in and use it in a test. Sound good?

[kyum1n]

Sure, I could try that.

[designatednerd]

@kyum1n - Is it sufficient for the test to have any array, or does it need to be an array of JSON objects? I can definitely get you the first from the GitHub API, but it's going to be much more complicated to get the second.

[kyum1n]

We need an array of JSON objects. Its the keys of these objects that are currently stored in a random order.

[designatednerd]

Oof, yeah that'd need a different schema that supports that. Is the schema you're using public?

[kyum1n]

No, unfortunately not.

[designatednerd]

🤔 - OK, if you can confirm this fix works in your codebase, I'll merge this PR make an issue to come back to this later for testing when I can dig up an API that takes JSON objects.

Does that work for you?

[kyum1n]

Yes, sounds good. And yes, this fix solved our caching problems.

[designatednerd]

OK, #1304 will track figuring out how to test this, but I'll try to get this out in a patch.