keyFields mismatch with query leads to hard Invariant Violation crash #6673

berekuk · 2020-07-22T13:18:41Z

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 (?).

The text was updated successfully, but these errors were encountered:

benjamn · 2020-07-22T14:45:44Z

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.

berekuk · 2020-07-22T15:02:17Z

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.

benjamn · 2020-07-22T15:26:51Z

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 our Observables, so that this behavior is never triggered.

vfonic · 2020-09-08T10:13:55Z

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...

benjamn · 2020-09-09T17:32:21Z

@vfonic If you don't know why you're upgrading ("zero new functionality" is absurd, though I assume you just mean you don't see value in any of the new functionality), any amount of migration effort is going to feel like a chore.

We are not going to slow down the pace of development, but we will continue to have extended prerelease periods before major releases, allowing anyone to try out what we're planning. The beta/RC period for AC3 was nearly a year long! Here's the current v3.2.0 PR, with beta releases you can already try (though it's just a minor release).

More feedback on the consequences of any breaking changes (especially if they were not intended to be breaking) is always welcome. And if you're happy with AC2, maybe try taking a more gradual approach to updating to AC3? I realize it's a lot, but we really want you to end up appreciating the value of what you're doing, rather than getting frustrated.

vfonic · 2020-09-09T20:10:03Z

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 call writeQuery 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?

benjamn · 2020-09-10T22:16:09Z

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).

@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!

Antonio-Laguna · 2020-09-17T07:36:08Z

I've just had this issue. If the schema of a query changes with what was in cache, the whole app blows with this error. Is there any way to catch this?

Even if this is a fatal error there's still a chance to ignore the cache and fetch again before going nuke on the app. Right?

sparebytes · 2020-12-09T17:08:18Z

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.

macrozone · 2021-01-13T14:58:33Z

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.

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

AoLi12306 · 2021-02-25T23:46:44Z

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.

RituSomani · 2021-05-05T13:46:50Z

I'm facing similar issue. Is there something to do with Schema

ts-ign0re · 2021-05-13T14:19:01Z

Yeah, same error

jonixj · 2021-05-24T21:40:33Z

almost a year since this was reported, still no update?

wpride · 2021-06-10T04:22:08Z

A better error message would be really nice here.

jorbuedo · 2021-08-19T15:39:46Z

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.

jorbuedo · 2021-08-19T15:55:51Z

Could be as simple as adding the response to the message there:

apollo-client/src/cache/inmemory/policies.ts

Line 1092 in e5c411a

    
           invariant(!strict, `Missing field '${responseName}' while computing key fields`);

I did that locally to find out the problem.

Should help with `cache.identify`-related cases of issue #6673.

#6673 (comment)

benjamn · 2021-08-20T20:43:39Z

A couple updates:

After Allow @directive and $variable strings in field policy keyArgs: ["arg", "@dir", "$var"] arrays #8678, this exception should no longer happen (ever) for keyArgs, since the function for handling keyArgs (computeFieldKeyObject) is now separate from the function for handling keyFields (computeKeyFieldsObject), and never throws
Improve keyFields error behavior when primary key fields are missing #8679 ensures cache.identify never throws when key fields are missing, but merely returns undefined (as usual)
I took @jorbuedo's suggestion from keyFields mismatch with query leads to hard Invariant Violation crash #6673 (comment) and added the offending arguments object to the error message (also in Improve keyFields error behavior when primary key fields are missing #8679)

Thanks for everyone's patience. Please have a look at those PRs if you're curious/impatient!

jorbuedo · 2021-08-20T21:03:45Z

Looks good, thanks for the updates!

In case you are curious, my use case was using Contentful cms. Each content model inherits from Entry, so I added:

possibleTypes: {
    Entry: [...listOfComponents],
},
typePolicies: {
  Entry: {
    keyFields: ['sys', ['id']],
  },
}

Then when querying for some hyperlink that was supposed to be of a certain type, having a fragment that expanded the id... it just broke when the CMS unexpectadly returned a different component.

Catching errors early is good, but having changes on the CMS blow up the site would be no good. So thanks for removing the exception!

Should help with `cache.identify`-related cases of issue #6673.

#6673 (comment)

#8679) * Make `cache.identify` return undefined instead of throwing, to help with `cache.identify`-related cases of issue #6673. * Improve error message when `computeKeyFieldsObject` throws: #6673 (comment)

adamyonk · 2021-09-29T14:39:27Z

I get that this truly is a "fatal" sort of error, but when this happens in a useQuery the state of the app gets stuck on that query being in the loading: true state with no error. And react error boundaries don't seem to be able to catch this. Is there some way to get at this error so I can move the app into a safer error state?

alicerocheman · 2021-10-21T09:37:31Z

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);

dleavitt · 2022-06-17T23:43:07Z

@benjamn nice job on this, it's now very easy to figure out how to resolve these when they occur!

A couple additional questions:

1. Is there any way to handle/catch these invariant violations?

It would be great to be able to show error UI, do reporting, etc. Ideally they should never happen in prod, but in practice it can be hard to know you've tested every possibility in highly polymorphic queries (e.g. an interface that's implemented by dozens of concrete types).

2. Could there be an option to mimic keyFields: false rather than throwing the invariant error if a keyField is missing?

This could also be the default behavior when catching and not re-raising one of these invariant errors.

Like, I might some queries where I just want to opt out of caching behavior for some types.. Let's say I'm trying to get a list of all commenters on a blog post. I'd love to be able to do like post { id name comments { user { id name } } rather than having to make sure I'm asking for the keyFields on comments, which in this query is just an empty edge. Obvs not super compelling in this particular case but important in the "interface with dozens of concrete types" scenario.

berekuk changed the title ~~Invalid keyFields lead to hard Invariant Violation crash~~ keyFields mismatch with query lead to hard Invariant Violation crash Jul 22, 2020

berekuk changed the title ~~keyFields mismatch with query lead to hard Invariant Violation crash~~ keyFields mismatch with query leads to hard Invariant Violation crash Jul 22, 2020

benjamn added this to the Post 3.0 milestone Jul 22, 2020

benjamn added the 🐞 bug label Jul 22, 2020

benjamn self-assigned this Sep 29, 2020

hallee mentioned this issue Jan 11, 2021

Normalize fields for all guides queries Quartz/content#32

Merged

brainkim added the 💸 caching label May 13, 2021

benjamn added a commit that referenced this issue Aug 20, 2021

Make cache.identify return undefined instead of throwing.

cd89150

Should help with `cache.identify`-related cases of issue #6673.

benjamn added a commit that referenced this issue Aug 20, 2021

Improve error message when computeKeyFieldsObject throws.

30857d7

#6673 (comment)

benjamn mentioned this issue Aug 20, 2021

Improve keyFields error behavior when primary key fields are missing #8679

Merged

benjamn added a commit that referenced this issue Aug 25, 2021

Make cache.identify return undefined instead of throwing.

751802a

Should help with `cache.identify`-related cases of issue #6673.

benjamn added a commit that referenced this issue Aug 25, 2021

Improve error message when computeKeyFieldsObject throws.

e1b737f

#6673 (comment)

jpvajda removed this from the Post 3.0 milestone Aug 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keyFields mismatch with query leads to hard Invariant Violation crash #6673

keyFields mismatch with query leads to hard Invariant Violation crash #6673

berekuk commented Jul 22, 2020 •

edited

Loading

benjamn commented Jul 22, 2020 •

edited

Loading

berekuk commented Jul 22, 2020

benjamn commented Jul 22, 2020

vfonic commented Sep 8, 2020 •

edited

Loading

benjamn commented Sep 9, 2020 •

edited

Loading

vfonic commented Sep 9, 2020

benjamn commented Sep 10, 2020

Antonio-Laguna commented Sep 17, 2020 •

edited

Loading

sparebytes commented Dec 9, 2020 •

edited

Loading

macrozone commented Jan 13, 2021 •

edited

Loading

AoLi12306 commented Feb 25, 2021

RituSomani commented May 5, 2021

ts-ign0re commented May 13, 2021

jonixj commented May 24, 2021

wpride commented Jun 10, 2021

jorbuedo commented Aug 19, 2021

jorbuedo commented Aug 19, 2021 •

edited

Loading

benjamn commented Aug 20, 2021

jorbuedo commented Aug 20, 2021

adamyonk commented Sep 29, 2021

alicerocheman commented Oct 21, 2021 •

edited

Loading

dleavitt commented Jun 17, 2022 •

edited

Loading

keyFields mismatch with query leads to hard Invariant Violation crash #6673

keyFields mismatch with query leads to hard Invariant Violation crash #6673

Comments

berekuk commented Jul 22, 2020 • edited Loading

benjamn commented Jul 22, 2020 • edited Loading

berekuk commented Jul 22, 2020

benjamn commented Jul 22, 2020

vfonic commented Sep 8, 2020 • edited Loading

benjamn commented Sep 9, 2020 • edited Loading

vfonic commented Sep 9, 2020

benjamn commented Sep 10, 2020

Antonio-Laguna commented Sep 17, 2020 • edited Loading

sparebytes commented Dec 9, 2020 • edited Loading

macrozone commented Jan 13, 2021 • edited Loading

AoLi12306 commented Feb 25, 2021

RituSomani commented May 5, 2021

ts-ign0re commented May 13, 2021

jonixj commented May 24, 2021

wpride commented Jun 10, 2021

jorbuedo commented Aug 19, 2021

jorbuedo commented Aug 19, 2021 • edited Loading

benjamn commented Aug 20, 2021

jorbuedo commented Aug 20, 2021

adamyonk commented Sep 29, 2021

alicerocheman commented Oct 21, 2021 • edited Loading

dleavitt commented Jun 17, 2022 • edited Loading

berekuk commented Jul 22, 2020 •

edited

Loading

benjamn commented Jul 22, 2020 •

edited

Loading

vfonic commented Sep 8, 2020 •

edited

Loading

benjamn commented Sep 9, 2020 •

edited

Loading

Antonio-Laguna commented Sep 17, 2020 •

edited

Loading

sparebytes commented Dec 9, 2020 •

edited

Loading

macrozone commented Jan 13, 2021 •

edited

Loading

jorbuedo commented Aug 19, 2021 •

edited

Loading

alicerocheman commented Oct 21, 2021 •

edited

Loading

dleavitt commented Jun 17, 2022 •

edited

Loading