Detect dangling references in executeSelectionSet. #5830
Conversation
Previously, when a Reference object was encountered in the cache but the entity it referred to had been removed from the cache, we would muddle along and likely end up reporting missing fields within the object, without explicitly reporting that the object itself was missing. Now, such errors will be thrown by diffQueryAgainstStore, unless returnPartialData:true is specified, like missing field errors. Should help with #5797, though a reproduction would be helpful.
benjamn
force-pushed
the
detect-dangling-references
branch
from
0132f7c
to
99d34a4
Compare
| !context.policies.rootTypenamesById[objectOrReference.__ref] && | ||
| !context.store.has(objectOrReference.__ref)) { | ||
| return { | ||
| result: {}, |
There was a problem hiding this comment.
After playing around with various options here, including null, undefined (or no result key), and an empty object, this {} seems to be the most convenient behavior. I may be rationalizing my choice, but I think it makes sense, because a dangling reference must have pointed to an object, and now we don't know anything about the contents of that object, so it's as if it's still there but empty/useless.
There was a problem hiding this comment.
Using {} is definitely more convenient; I'm a tad apprehensive about using an empty object for results due to the issues we had in React Apollo around this: apollographql/react-apollo#3388. This isn't an apples to apples comparison of course, and the real problem in RA's case was that {} wasn't being used consistently. All of this to say, as long as we're using {} consistently then I think we should be okay.
| @@ -198,6 +186,17 @@ export class StoreReader { | |||
| objectOrReference, | |||
| context, | |||
| }: ExecSelectionSetOptions): ExecResult { | |||
| if (isReference(objectOrReference) && | |||
| !context.policies.rootTypenamesById[objectOrReference.__ref] && | |||
There was a problem hiding this comment.
This check avoids reporting dangling references to ROOT_QUERY or ROOT_MUTATION, since those objects are never written explicitly by the developer, and are almost never removed from the cache after first being created. Instead, we report missing fields within those root objects.
| !context.policies.rootTypenamesById[objectOrReference.__ref] && | ||
| !context.store.has(objectOrReference.__ref)) { | ||
| return { | ||
| result: {}, |
There was a problem hiding this comment.
Using {} is definitely more convenient; I'm a tad apprehensive about using an empty object for results due to the issues we had in React Apollo around this: apollographql/react-apollo#3388. This isn't an apples to apples comparison of course, and the real problem in RA's case was that {} wasn't being used consistently. All of this to say, as long as we're using {} consistently then I think we should be okay.
Previously, when a
Referenceobject was encountered in the cache but the entity it referred to had been removed from the cache, we would muddle along and likely end up reporting missing fields within the object, without explicitly reporting that the object itself was missing.With these changes, such errors will be thrown by
diffQueryAgainstStore, unlessreturnPartialData: trueis specified, just like missing field errors.Should help with #5797, though a reproduction would be helpful.