Refetch queries on underfetch

[steelbrain]

Situation

When an object is mounted, and a mutation is executed that returns the same ID as the mounted object but has less fields, loading is returned as false but data is an empty object as returned from this line in apollo-client::QueryManager.

An internal error is generated of course with the message template Cannot find field $FIELDS on Object but that's swallowed and never shown to the user in the code referenced above.

A repro for this error can be found at stevehollaar/react-apollo-error-template@mutate-bug

Possible fixes

There are multiple fixes that were evaluated for this, each with it's own downside

• Automatically "fill in" the missing fields in outgoing mutations based on the types of results currently being watched, so in case we get a result from server that has ID that's already mounted, we will have all the fields we need. This would've caused a lot of over-fetching and issues in large codebases.
• Do not automatically update the mounted components, like relay. Let users do it the ...fragment way (which fills in the fields automatically) or specify a manual store updater.
• After a mutation that causes issues is executed, detect the errors and refetch the query

The last one seemed to be the most straight forward and simple solutions with no side effects so that's what I've done here.

This PR fixes apollographql/apollo-client#3267 #1314

Credits

Thanks @stevehollaar for his repro-case and constant support in this quest

Todo

• Fix breaking tests
• Add new tests

[apollo-cla]

@steelbrain: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[jonas-app]

@steelbrain thanks for your investigation 👍
Sounds like a useable workaround for us.
Will try it in our sample project tomorrow.
apollographql/apollo-client#3267 (comment)

[jonas-app]

@steelbrain We successfully tested the workaround in our projects. 💯

[mlcohen]

@steelbrain @jonas-arkulpa This is fantastic! We've been running into this issue and have been debating how to resolve the problem. I agree that automatically "filling in" missing fields is the wrong way to go. We've gone down the path of adding fields causing errors and applying fragments. Not great 😕. Your solution to instead refetch data is a good option, however, I wonder if the query manager could be further refactored so that an optional hook/plugin could be provided to the QM. The QM could then call the hook to delegate how best to handle a detected error. I say this since in our particular scenario, the detected field error pops up not because of direct GQL mutation but, rather, from our system running one or more queries whenever our app receives pushed system events (via websocket). Depending on the circumstance, we could either use a default refetch action (like your solution provides) or in turn perform our own custom error handling that best serves our purposes. Thoughts?

For more detail of the problem we have run into, please see apollographql/apollo-client#2920 (comment)

[steelbrain]

@mlcohen I also have a branch in apollo-client that returns/exposes partial errors to the codebase so they can be handled but I am not sure if we should expose them to the React component.

The fact that Apollo React Query component returns an object with properties such as loading and data makes it work to our advantage that we can just add it to that object instead of changing function invocation signature.

Thoughts?

Edit: We could also add a prop to the Query component with name noRefetchOnPartial so component could handle the behavior themselves with partialErrors

[mlcohen]

@steelbrain Yeah, I can certainly see your point directly exposing the error to the React component. We were thinking about updating the apollo client's query manager directly instead of indirectly via a React component. Applying the hook directly against the underlying apollo client instance would provide a way for the system to "globally" handle detected field errors. This means that React components that use apollo client would not be exposed to these types of errors. The global default would be for the QM to do an automatic refetch.

Your idea of adding a noRefetchOnPartial prop also sounds intriguing. Perhaps both options are viable. A "global" approach by directly adding a hook the apollo client's QM and a more isolated approach that overrides the global approach when a noRefetchOnPartial prop is supplied. Hmm 🤔

[steelbrain]

Because including partialError in React requires that we also expose it in QueryManager I think we should do both.

Other than refetching or handling it on a component-level basis, I am curious how do you intend to handle it directly in Apollo Client? Unless, you are not using React for rendering UI.

[mlcohen]

In our application we perform GQL queries and mutation through react-apollo's graphql HOC and directly via the apollo client instance calling its query and mutate methods. Most of the time we use the graphql HOC for our React components but where we receive pushed system events, we have actions that use the apollo client instance. Based on these two approaches, it seems that if the partial field is exposed in the result, we could handle the error by doing a refetch in our own way. It would be ideal if the graphql HOC and apollo client's query and mutate could perform a data refetch just like what you're doing in the new Query component. Of course that likely means duplicating the logic in three places to do the same thing. The common point among the three is within the query manager's getCurrentQueryResult method. Perhaps a hook within the QM to handle partial errors isn't necessary since a data refetch seems to suffice, but logic that is more centrally focused to handle partial errors could help.

[peisenmann]

If you're calling data.refetch(), then you're going to be hitting the network and forcibly updating the local data state to the latest state on the server, right? Even if the user has done a mutation on a small portion of a larger data structure, the user doesn't necessarily want the rest of the cache updated.

What I'd consider the expected behavior is for the response of the mutation to simply overlay/merge into the existing cache, and then fire renders for all subscribers from the current cache state, not from the network response.

[steelbrain]

@peisenmann While merging is possible, it's also possible that other fields may have changed
by mutation and without refetching you are showing outdated data to the user. This is use-case specific tho.

Either way that cannot be done in react-apollo, it'd have to be done in apollo-cache-inmemory or at least apollo-client. This is the least side effective and straight forward way I've evaluated to be (see first post for other methods considered/tried)

[peisenmann]

@steelbrain A perfectly valid point, for sure. I'd expect in that case, if you wanted to check for those other fields, you would have them in the query part of your mutation or you would use refetchQueries to trigger the other, bigger query to be re-invoked. By excluding those fields from my mutation, I'm signaling that I don't want them updated at this time.

In an application I'm working on, when a user queues up local changes that are not ready to be persisted to the server yet, we use a writeFragment to write the data back into the Apollo cache. This keeps all views up to date with the user's changes, as long as no rogue refetches are called.

As you've noted, I will take my "expected behavior" points up with the other apollo-* libs. My request for this thread is not to introduce automatic refetches (or at least to allow me to specify noRefetchOnError or something of that nature).

[idris]

@peisenmann see the original issue (apollographql/apollo-client#3267). Normally the mutation payload does get merged into the cache. The code in this PR only gets activated when the mutation payload has under-fetched data, and a component is trying to render with that under-fetched data. In that case you cannot simply merge with the cache because the cache doesn't have what you need.

[peisenmann]

@idris Ah, okay, so the original full query is not even populated in the first place. @steelbrain Apologies for wasting your time. Thanks for the PR!

[dan-weaver]

Edit: I think I’ve actually run into a different issue not necessarily related to this PR. I apologize and have reproduced it and will create a new issue from it.

@idris I'm not following what your saying but am desperate to understand this behavior better as I keep running into it. @peisenmann I think this occurs in the case that the original full query is there. Someone correct me if I'm wrong, but that is the behavior I am seeing.

So I'm probably overlooking something and am going to look foolish once you explain @idris, but what do you mean here exactly:

The code in this PR only gets activated when the mutation payload has under-fetched data, and a component is trying to render with that under-fetched data. In that case you cannot simply merge with the cache because the cache doesn't have what you need.

Why is that not possible? You should be able to theoretically merge any fields that were in that mutation payload into the cache and not disturb the ones that it didn't fetch no?

[felixk42]

Would really love to see this PR merged, it's been consistently a serious deal breaking bug for at least 10 months now...

[hwillson]

@felixk42 I'm close to getting this merged in, but this change could have implications on a few edge cases. Regardless, you should see more updates here today.

[jonas-app]

@hwillson thanks for your work & your blog post!

As this issue is the biggest blocker for (not only) our projects I think it clearly needs more thoughts putting into it. We are really happy to finally have a usable workaround, but it clearly is just that, a workaround.

[hwillson]

Thanks @jonas-arkulpa!

Just a quick heads up for everyone here. I cleaned things up a bit, ran some more tests, and have had a few more discussions about this approach. While this PR does fix the original issue, we're still a bit concerned about the potential performance impact this could have on larger applications. We're currently digging a bit deeper to see if this can be resolved in another way, but we haven't ruled this potential solution out yet. Sorry for the delay all - this issue has been around for a while, and we can't wait to get it fixed, but we just want to make sure the fix doesn't cause more problems. Thanks for your patience!

[idris]

@hwillson we (me and my team, original authors of this PR) would be happy to contribute to those discussions and provide input. Happy to do that here or in Slack. IMO the more in-the-open the discussion, the quicker we can poke holes and land on something that makes sense.

[pleunv]

I don't know why this keeps flying under the radar or if it is deemed not important enough, it's just rather frustrating that something that so drastically changes behavior (and even breaks apps) on a minor version bump gets so little attention.
Is there anything that us users can do to help move this forward?

[cagdastulek]

+1

[dajvido]

I wonder, what is the reason to deconstruct this variable in here when you don't use it later anywhere.

[danilobuerger]

@dajvido To remove it from opts: See something like https://codeburst.io/use-es2015-object-rest-operator-to-omit-properties-38a3ecffe90

[hwillson]

Thanks very much for your work on this @steelbrain! We're going to move ahead with getting this merged, but we've flipped things around a bit so the partial refetch functionality is disabled by default. That will de-risk this PR a bit, and help us get this out sooner than later. If anyone in this PR objects, definitely let me know. Otherwise, the current plan is to get this launched tomorrow. Thanks again for working on this!

[justinanastos]

🎉

[mcmar]

Please let me know when a new version of react-apollo@2.x.x is released. I really need to get my hands on this ASAP.

[hwillson]

@mcmar It will be released very soon. The plan was to release it today, but we've hit a bit of a snag with a few dependency issues (unrelated to this PR). They should be resolved shortly, and the release will be pushed out right after (current guesstimate - early am tomorrow). Thanks!

[hwillson]

React Apollo 2.2.0 is now available. Thanks!

[cagdastulek]

Great!

I upgraded to 2.2.0 and rerun my tests. Unfortunately, the issue I reported above with a test case was not resolved. Let me know if it is not really something this PR could be able to solve. I will submit it as a separate issue, accordingly.

Thanks.

[jonas-app]

@cagdastulek Have you enabled partial refetch?
https://github.com/apollographql/apollo-client/pull/3945/files

[cagdastulek]

Yes, it didn't help either.

[steelbrain]

For the people that were still having issues with this PR merged, I've opened up a followup PR that should take care of the remaining issues: #2493

[vreality64]

@steelbrain @hwillson
I think this code makes #556 situation. (Bug)

Let's assume using Query component with network-only policy. Query response is always will be cached. It means Object.keys(currentResult.data) !== 0, right?

If then, fetchPolicy is ignored. It will always consume cached result. In other words, if something is already cached, fetchPolicy is ignored

[tnrich]

@steelbrain and others, I was thinking it might be nice if this edge case could be detected and the user could be alerted of what is going on without them having to figure it out on their own and find this PR.

I think a short message in the console could be really helpful. Something like:

An error occurred while trying to refetch the query XX from the cache. If you want this query to refetch in this instance please use the partialRefetch: true prop, or if you don't please explicitly pass a disablePartialRefetch: true. More info: #2003 (and a link to the docs)

What do people think about that? IMO it's much better than getting the confusing no error, no data situation.

[heyitsaamir]

A question related to this, when the query is being refetched, the loading flag is flipped to true for the main query, which shows a spinner in our case. Is there a recommended way of handling this? Thanks for all the work that went into this fix!