Identify written results using processed StoreObject in StoreWriter#processSelectionSet
#8996
Conversation
The test changes were necessary because the expected keyFields identification exception is now thrown after field traversal, allowing missing field errors to be logged during traversal that previously were silently hidden.
src/cache/inmemory/key-extractor.ts
Outdated
| // Usually the extracted value will be a scalar value, since most primary | ||
| // key fields are scalar, but just in case we get an object or an array, we | ||
| // need to do some normalization of the order of (nested) keys. | ||
| if (isNonNullObject(value)) { | ||
| if (Array.isArray(value)) { | ||
| return value.map(normalize) as any; | ||
| } | ||
| return collectSpecifierPaths( | ||
| Object.keys(value).sort(), | ||
| path => extractKeyPath(value, path), | ||
| ); |
There was a problem hiding this comment.
This sorting of the keys of unexpected leaf objects is the part of this PR that fixes #8855, by ensuring both keyFields and keyArgs extraction produces canonical ID strings, even if the KeySpecifier array does not explicitly determine an order for all object keys.
That said, it's probably not a great idea in general to use objects or arrays as keyFields values, since they can change size and shape without warning, and entity ID strings are supposed to be relatively stable and cheap to compute.
src/cache/inmemory/writeToStore.ts
Outdated
| const [id, keyObject] = policies.identify(result, { | ||
| selectionSet, | ||
| fragmentMap: context.fragmentMap, | ||
| storeObject: incoming, | ||
| readField, | ||
| }); | ||
|
|
||
| // If dataId was not provided, fall back to the id just generated by | ||
| // policies.identify. | ||
| dataId = dataId || id; | ||
|
|
||
| // Write any key fields that were used during identification, even if | ||
| // they were not mentioned in the original query. | ||
| if (keyObject) { | ||
| // TODO Reverse the order of the arguments? | ||
| incoming = context.merge(incoming, keyObject); | ||
| } |
There was a problem hiding this comment.
I would love to remove the keyObject logic here, but we have a number of tests where a key field (often id) is provided in the written data but not mentioned in the query/fragment, and this logic makes sure we write those key fields anyway (even though write execution is primarily driven by the structure of the query/fragment). We could warn about such situations (like we do in the opposite case, for fields missing from the result object but mentioned in the query/fragment), but I don't think we can change the behavior without a major version bump (that is, AC4).
benjamn
added
2021-11
👩🏭 refactor
💸 caching
🥚 backwards-compatible
There was a problem hiding this comment.
As per usual, I’m not happy about the number of callbacks/higher-order functions in these changes, but I can tolerate it! Overall though it seems like a cleaner solution than the former one. I think some more accurate types would make these files easier to reason about.
benjamn
force-pushed
the
identify-written-results-using-StoreObject-in-processSelectionSet
branch
from
abccacc
to
280ab72
Compare
benjamn
deleted the
identify-written-results-using-StoreObject-in-processSelectionSet
branch
This PR supersedes #8860, accomplishing the same goals and passing the same tests, using a different (but backwards-compatible) approach to identifying written result objects.
The crux of this approach is that
keyFieldsextraction duringpolicies.identifydoes not need to worry about field aliases if it operates on the fully de-aliasedStoreObjectthat's about to be written into theEntityStore, rather than operating on the raw result object (which still contains aliases).With this shift from identifying raw result objects to identifying processed
StoreObjects, theAliasMaplogic that previously complicatedkeyFieldsFnFromSpecifieris no longer necessary. That logic was essentially duplicating effort also performed byprocessSelectionSetduring its recursive traversal of the selection set, so this new approach should be computationally simpler and faster, as well as easier to understand.If you have any custom
keyFieldsfunctions in thetypePoliciesyou pass toInMemoryCache, the raw result object will still be passed as the first argument, to ensure backwards compatibility. However, you may want to consider using theKeyFieldsContextobject passed as the second argument to yourkeyFieldsfunction, specificallycontext.storeObjectandcontext.readField("someField", context.storeObject), instead of reading fields from the raw result object.If you're using the more declarative
keyFields: [...key field names...]syntax, you shouldn't need to change anything, since this syntax is now implemented in terms ofcontext.storeObjectbehind the scenes.As a bonus, it may finally be possible to use synthetic client-only fields as
keyFields(potentially fixing #8939), since those fields are now extracted in a way that respects customreadfunctions, though I'm not sure I can recommend this as a best practice, sincekeyFieldsshould ideally be cheap scalar fields. If you can think of any compelling use cases for this pattern, please drop a comment below.