Mutations ignore partial results and the cache never gets updated

[knedev42]

I've been migrating a large codebase to the newest version of apollo for a while now and keep stumbling into bugs & undocumented behavior. The latest one being a pretty serious issue for us, because it basically makes the cache useless and prevents us from trusting it that entities will be updated after a mutation.

If a mutation passes successfully and returns data, but one of the optional subfields in the returned data has thrown an error in it's resolver, the default behavior of apollo server is to set that particular field to null and to record the error that was thrown in the errors array. That's completely fine, because it gives us access to the "partial" data and also gives us visibility into the error that was thrown for a particular subfield (I'm using errorPolicy: 'all'). However the cache never gets updated and the entire result of the mutation is ignored, even though the mutation was actually successful and a valid entity was returned (even though an optional deeply nested subfield was null, because it threw an error during it's resolution from let's say a 3rd party API).

I believe the offending line is

apollo-client/src/core/QueryManager.ts

Line 1083 in ed7c5bb

if (!graphQLResultHasError(mutation.result)) {

This is a very naive approach of handling mutation results. It does not respect responses with partial data and field errors in them, nor any errorPolicy. There should be an additional check to see if there is any returned data, not only if errors exist, because sometimes they are just field resolution errors from a deeply nested subtype of the returned supertype.

Intended outcome:

To have mutations that pass successfully and return data be respected, regardless of whether a deeply nested subtype failed one of it's field resolutions and returned null for it.

Actual outcome:

Mutations that pass successfully and return data are ignored if there's an irrelevant field error.

Versions

npmPackages:
@apollo/client: ^3.1.3 => 3.1.3

[knedev42]

As a workaround I was forced to do this on the server. It's swallowing field resolution errors when the operation is a mutation and there's partial data alongside some field errors.

  formatResponse: (response, meta) => {
    if (shouldIgnoreMutationErrorsWithPartialData(response, meta)) {
      return { data: response.data };
    }

    return response;
  }

I believe no one should be forced to employ workarounds like this, just because their graphql client can't deal with partial data.
Also not everyone has control over their APIs.

[benjamn]

@knedev42 Thanks for pointing this out.

I've aligned the errorPolicy behavior of mutations and subscriptions with the existing behavior for queries in #7003, which is available for testing in @apollo/client@3.2.0-rc.0, if you want to give that a try.

I also don't blame you for finding the default errorPolicy (none) counter-intuitive, but it's not something we can change without a major version bump. Just to be clear, with the changes in #7003, you will need to specify a non-default policy like errorPolicy: "all" to have mutation results written into the cache when there are errors.

You might also consider passing an update function to the client.mutate call to handle the result explicitly, since mutation results in GraphQL often need a bit of processing to propagate their consequences accurately. Sometimes it's enough to update any returned entity objects in the cache, but that automatic behavior definitely won't handle all mutations properly.

[knedev42]

Thanks, I'll be able to give it a try early next week :)

[knedev42]

@benjamn sorry for taking a bit longer in order to try it out, I just got swarmed with other work.

So handling partial responses seems to work fine. But now there's a different problem. If the mutation returns an actual error, the update function gets called and the promise gets resolved. Now the promise getting resolved isn't that big of a deal at least for us, since we're not actually relying on it. But the update function shouldn't really be called when the mutation failed 🤷

I skimmed through the code and I believe that's because the check is only for whether there is data in the response. However when you throw from a mutation in apollo-server, the data property does get populated, but the keys in it (the mutations that were called) are null. So I think the check for a successful mutation should be - there's data and any of the keys at the root of data are not null. Otherwise it's just unusable, without guarding in every single update function (also the promise resolving is a bit odd, but it is what it is). Or if we should guard against mutation failures in our update functions, then that should probably be thoroughly documented and people should be aware of it.

Thanks again, and sorry for the small delay :)

[knedev42]

Just wanted to let you know that I tried the latest version again (v3.3.19) and the behavior is still the same as my last comment - mutations that fail completely (throw an error) call the update function and the promise is getting resolved.