Use deterministic order for type policy keyFields and field policy keyArgs

[rohandhruva]

When using @typePolicy and @fieldPolicy, I noticed a few cache misses because the keyFields and keyArgs were not in the same order. When generating a CacheKey in the CacheResolver, the generated key was bar+foo, whereas the type policy generated it as foo+bar.

This PR uses deterministic sorting in both the keyFields of a type policy and keyArgs of a field policy.

[BoD]

I think we can just use List. Technically SortedSet makes more sense, but I'd rather avoid the JVM-only type.

Note: the equivalent TypePolicy in com.apollographql.cache.normalized.api also needs to be a List instead of a Set (that's a breaking change but that's all right at the moment).

[BoD]

If we use a List I think it'd make more sense to keep the user-defined order set in @typePolicy.

[BoD]

I think this works only if the key arguments of a field have the same names as the key fields of the type. So for example:

extend type User @typePolicy(keyFields: "firstName lastName")
extend type Query @fieldPolicy(forField: "user", keyArgs: "firstName lastName")

{
  user(firstName: "john", lastName: "smith") {
    firstName
    lastName
  } 
}

• The CacheKeyGenerator will return User:john+smith
• The CacheResolver will return User:john+smith ✅

But

extend type User @typePolicy(keyFields: "firstName lastName")
extend type Query @fieldPolicy(forField: "user", keyArgs: "a b")

{
  user(b: "john", a: "smith") {
    firstName
    lastName
  } 
}

• The CacheKeyGenerator will return User:john+smith
• The CacheResolver will return User:smith+john ❌

---

In the end I think it would make sense to use the same orders as defined in @typePolicy and @fieldPolicy. But that isn't currently possible because that information is lost in compilation! Instead, for the key args, we use the order in which they are passed to the field. So in the example above we must select user(firstName: "john", lastName: "smith") and not user(lastName: "smith", firstName: "john").

But this soon will be possible, because for the key args, I intend to not use CompiledField but a Map generated in the Cache class. With that we'll have the order information.

[BoD]

Thanks a lot for opening this. In summary:

• using Set for @typePolicy is indeed wrong. Let's use List
• I don't think we can really sort, because that assumes fields and args have the same names. Instead the order in which the args are passed is used... which is definitely not great but we'll be able to fix that a bit later.

[rohandhruva]

Thanks a lot for opening this. In summary:

• using Set for @typePolicy is indeed wrong. Let's use List
• I don't think we can really sort, because that assumes fields and args have the same names. Instead the order in which the args are passed is used... which is definitely not great but we'll be able to fix that a bit later.

Thanks for the review and explanation.

If I am understanding it right, did you want me to close out this PR and then open a new one that changes it from a Set to a List?

[BoD]

If I am understanding it right, did you want me to close out this PR and then open a new one that changes it from a Set to a List?

I think this can be done in this PR - or a new one if you prefer :)

[BoD]

Thanks again for having opened this. In the end, I needed to change the Sets to Lists in #250 so this one can be closed.