apollo-client: keyFields mismatch with query leads to hard Invariant Violation crash
Intended outcome:
Specifying unknown fields in keyFields
(or failing to ask for all keyFields
in queries) should result in a catchable exception which doesn’t affect the entire server process.
Actual outcome:
When I pass an unknown field name to keyFields
in InMemoryCache configuration, the entire node process crashes down with Invariant Violation
error.
Here’s the stacktrace I’m getting with the repo below:
/Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/zen-observable/lib/Observable.js:65
throw e;
^
Invariant Violation: Missing field 'foo' while computing key fields
at new InvariantError (/Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/ts-invariant/lib/invariant.js:16:28)
at Object.invariant (/Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/ts-invariant/lib/invariant.js:28:15)
at /Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:3788:130
at Array.forEach (<anonymous>)
at computeKeyObject (/Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:3777:15)
at /Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:3740:13
at Policies.identify (/Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:3454:33)
at StoreWriter.processSelectionSet (/Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:4448:27)
at StoreWriter.processFieldValue (/Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:4536:21)
at /Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:4485:47
at Set.forEach (<anonymous>)
at StoreWriter.processSelectionSet (/Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:4471:17)
at StoreWriter.writeToStore (/Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:4420:29)
at InMemoryCache.write (/Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:4659:37)
at InMemoryCache.ApolloCache.writeQuery (/Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:3321:21)
at /Users/berekuk/coding/tmp/apollo-crash/apollo-crash/node_modules/@apollo/client/apollo-client.cjs.js:2359:31
My attempts to pin down this problem lead me to setTimeout
call in zen-observable’s hostReportError and to invariant(…) call in computeKeyObject in apollo-client.
How to reproduce the issue:
Here’s my repo which reproduces the problem: https://github.com/berekuk/bug-apollo-keyFields-crash
It uses Next.js since that’s how I discovered this issue and since I didn’t want to implement a custom Express.js server just for this bugreport.
I also tried to reproduce it with a simple node script which used apolloClient.watchQuery
but didn’t succeed. useQuery
probably does something differently, but I’m not sure what exactly.
Versions
See https://github.com/berekuk/bug-apollo-keyFields-crash/blob/master/package.json:
"@apollo/client": "^3.0.2",
"graphql": "^15.3.0",
PS: Besides this specific issue, I’d very much prefer for apollo-client never to do something like this on non-fatal errors. I’m worried, though, because it seems like apollo-client calls invariant()
quite often, and though I’m not familiar with ts-invariant and zen-observable APIs, it probably behaves like a hard non-easily-catchable assertion in all those cases (?).
About this issue
- Original URL
- State: open
- Created 4 years ago
- Reactions: 13
- Comments: 23 (7 by maintainers)
Commits related to this issue
- Make cache.identify return undefined instead of throwing. Should help with `cache.identify`-related cases of issue #6673. — committed to apollographql/apollo-client by benjamn 3 years ago
- Improve error message when computeKeyFieldsObject throws. https://github.com/apollographql/apollo-client/issues/6673#issuecomment-902031088 — committed to apollographql/apollo-client by benjamn 3 years ago
- Make cache.identify return undefined instead of throwing. Should help with `cache.identify`-related cases of issue #6673. — committed to apollographql/apollo-client by benjamn 3 years ago
- Improve error message when computeKeyFieldsObject throws. https://github.com/apollographql/apollo-client/issues/6673#issuecomment-902031088 — committed to apollographql/apollo-client by benjamn 3 years ago
- Improve `keyFields` error behavior when primary key fields are missing (#8679) * Make `cache.identify` return undefined instead of throwing, to help with `cache.identify`-related cases of issue #6... — committed to apollographql/apollo-client by benjamn 3 years ago
The big issue with this is how difficult it is to track down the offending query as the stack trace and message give no clue as to which one it is except for the field name.
I’m completely ok with this mistake causing an exception which I (or my framework, e.g. Next.js) would have to catch explicitly.
I’m definitely NOT ok with this error bringing my entire production website down, as it happened today 😦 One page/query which I wrote incorrectly or forgot about during a refactoring or apollo-client upgrade (it worked fine in 2.6) shouldn’t have this effect.
Actually, I believe no kind of code except for syntax errors should have this effect, and I’m really strugging to understand why it’s acceptable to intentionally write
setTimeout(() => { throw new Error() })
in any npm library.I’m just burning through my second day of upgrading from apollo client v2 to v3.
Just came here to say that what you did with apollo client v3 wasted so many hours of so many people with zero new functionality. I still didn’t manage to get v3 to fully work, and the above error is just one of the errors with this major version upgrade.
Please consider making less breaking changes for the future major update. This is just too much…
have the same issue, it just does not tell you at all where this is coming from. it just crashes the whole server (because of SSR).
Edit: i brute force checked every Query, but i did not found any query where the field is missing. Also i checked the cache an every object there has the field i want to use
This error message is ridiculously useless, unless you’ve only got 1 or 2 entities.
I’ve got many, and all their keyFields are “id”!
PLEASE add the entity name to the error message, please, pretty pretty please… 🥺
As a workaround, I did this (currently line 1813 of node_modules/@apollo/client/cache/cache.cjs.js):
__DEV__ ? globals.invariant(!strict, "Missing field '" + responseName + "' while computing key fields of "+ JSON.stringify(response)) : globals.invariant(!strict, 4);
I was facing similar issue, this happened to me because I was using “id” + “column B” as the primary key, however during fetching, I only fetched “id”, after also fetched “column B”, error has gone.
A better error message would be really nice here.
@vfonic I’m glad you mentioned this, because we do have an automated transform that can handle most of the
import
changes, which I now realize was never mentioned in the migration guide (fixed now). It’s not perfect, but it’s safe to run even if you’ve already migrated some/most of your imports, since it avoids changing any imports that have already been migrated. Sorry that wasn’t more clearly advertised before!Thanks for kind words. I appreciate it!
I’m just trying to keep up the pace so I don’t end up stuck in some prehistoric version with borderline impossible upgrade path.
I added a CSS-in-JS package and it didn’t play nicely with the other packages I had so I tried to upgrade whichever packages stopped working or started having issues (in the process I also upgraded typescript to v4 so that probably broke quite some things). One such package was apollo-client so I chose to upgrade it.
In the end, after I got it to work, I realized the new cache improvements, that I read about, made it slower (at least when using
.writeQuery
+ ‘network-only’) than in v2 so much so that the UI wasn’t getting updated on time (I callwriteQuery
and redirect immediately after, the redirect redirects back as it reads old data from the cache and figures something’s missing; this used to work). I don’t know if I set up something wrong. If I get the time, I’ll try to create a small repo with repro steps.One last thing and then I’ll finish my blabering: a simple codemod could make this shuffling between v2 and v3 much easier. (apollo-client => @apollo/client, react-apollo => @apollo/client, graphql-tag => @apollo/client). These are the only changes I’ve read about and noticed while upgrading.
I understand there must be so many changes, but as a user, I didn’t see them. I prefer Angular v50 (or React Native), than @apollo/client v3 that has so many changes that it’s too difficult to upgrade (AngularJS => Angular). Perhaps having beta releases for a year long is not for the better?
A couple updates:
keyArgs
, since the function for handlingkeyArgs
(computeFieldKeyObject
) is now separate from the function for handlingkeyFields
(computeKeyFieldsObject
), and never throwscache.identify
never throws when key fields are missing, but merely returns undefined (as usual)Thanks for everyone’s patience. Please have a look at those PRs if you’re curious/impatient!
Same here. Is there any workaround to at least debug what’s the offending entity? As far as I can tell, all of them have the keyFields.
almost a year since this was reported, still no update?
Ok, I think we agree on all of that. We will dig into this and make sure that we’re providing appropriate
observer.error
handlers for all of ourObservable
s, so that this behavior is never triggered.I would challenge your assumption that this kind of mistake should be “non-fatal.” If an ID can’t be computed for some instances of an object type, those deficient objects will effectively not be compatible with the rest of the objects of that type in your data graph. That seems like a pretty serious problem that you would want to catch (ideally in development) and fix!
I agree that it should be possible to catch and handle this kind of error at runtime (and I would have thought that was how it worked—so there’s definitely a bug if that’s not the case), but it shouldn’t be something you can easily ignore.