useMutation onCompleted executed when errors present

[agiveygives]

Intended outcome:

I have an onCompleted callback defined on the options of useMutation. I only want onCompleted to execute when no errors are present; so, I've set defaultOptions: { query: { errorPolicy: 'all' }, mutate: { errorPolicy: 'all' } } on the ApolloClient.

This is the expected functionality that is described on the docs

Actual outcome:

When a GraphqlError occurs, the onCompleted function is still executed with null data.

How to reproduce the issue:

This is the gist of our setup. I'm working on reproducing the issue on a small app as well, and will post the links on here once that is up and running.

...

const client = new ApolloClient({
  link: ApolloLink.from([
    onError(({ operation, forward, response }) => {
      // Do stuff to handle errors

      forward(operation);
    }),
    new HttpLink({
      uri: `http://localhost:3000/api/graphql`,
      credentials: 'include'
    })
  ]),
  cache: new InMemoryCache(),
  defaultOptions: {
    query: { errorPolicy: 'all' },
    mutate: { errorPolicy: 'all' }
  }
});

...

const rootElement = document.getElementById('root');

ReactDOM.render(
  <React.StrictMode>
    <ApolloProvider client={client}>
      <App />
    </ApolloProvider>
  </React.StrictMode>,

  rootElement
);

...

const MyComponent = () => {
  const [mutation, { loading, error }] = useMutation(
    GQL_MUTATION,
    {
      onCompleted: (data) => {
        console.log('onCompleted executed with:');
        console.log(data);
      }
    }
  );

  return (
    <>
      <div>{JSON.stringify(error)}</div>
      <button
        disabled={loading}
        onClick={() => mutation()}
      >
        Execute Mutation
      </button>
    </>
  );
};

Versions

"@apollo/client": "^3.5.6"

System:
    OS: macOS 11.6.4
  Binaries:
    Node: 16.13.1 - ~/.nvm/versions/node/v16.13.1/bin/node
    Yarn: 1.22.17 - /usr/local/bin/yarn
    npm: 8.1.2 - ~/.nvm/versions/node/v16.13.1/bin/npm
  Browsers:
    Chrome: 99.0.4844.51
    Safari: 15.3

[jeanlucmongrain]

I'm on 3.5.10 and experienced this exact same behavior

[shamshad17]

I have also faced the same on version 3.5.10.
onCompleted function also gets called with data=null along with onError whenever there is an error in query or mutation. It's breaking my application after update.
Earlier I was using v3.3.4 and It was working fine.

[gmathieu]

I just ran into this using 3.6.4.

useMutation's onCompleted appears to be called on any successful network response regardless of errors, data, or errorPolicy. I looked back through the history and it seems this has always been the case.

apollo-client/src/react/hooks/useMutation.ts

Line 104 in 67a13b5

ref.current.options?.onCompleted?.(response.data!);

By comparison useQuery's has some level of protection:

apollo-client/src/react/hooks/useQuery.ts

Lines 505 to 513 in 67a13b5

	private handleErrorOrCompleted(result: ApolloQueryResult<TData>) {
	if (!result.loading) {
	if (result.error) {
	this.onError(result.error);
	} else if (result.data) {
	this.onCompleted(result.data);
	}
	}
	}

The inconsistent behavior is surprising especially since the documentation doesn't reflect reality:

A callback function that's called when your mutation successfully completes with zero errors (or if errorPolicy is ignore and partial data is returned).

This function is passed the mutation's result data.

[aelfannir]

Same issue in 3.7.1

[abd30590]

I think what happens is normal, when you get a null data response, that's doesn't mean that you have errors, it's your response!
but if there is some errors and you don't want the the onComplete to be executed then remove the option errorPolicy: 'all'
or set it to errorPolicy: 'none' then if you have errors, the onError callback will be executed instead of onComplete

[jerelmiller]

Hey all 👋

Given that the documentation states that onCompleted should be called with zero errors, I would agree this should be classified as a bug. I can't guarantee a timeline on a fix, but if any of you wants to submit a PR to fix the issue, we'd be happy to look at it!

---

@abd30590 from what I understand with the GraphQL spec response format, null data with no errors is an impossible state. I find this section on data to be relevant here (emphasis mine):

7.1.1 Data

The data entry in the response will be the result of the execution of the requested operation. If the operation was a query, this output will be an object of the query root operation type; if the operation was a mutation, this output will be an object of the mutation root operation type.

If an error was raised before execution begins, the data entry should not be present in the result.

If an error was raised during the execution that prevented a valid response, the data entry in the response should be null.

I'd expect the only time you'd ever see onCompleted called when your server returns errors is with errorPolicy: 'none' since that strips out all errors from the response. This aligns with our documentation.

A callback function that's called when your mutation successfully completes with zero errors (or if errorPolicy is ignore and partial data is returned).

[CH-PatrickWalmsley]

+1 on this issue. It's a difficult bug for library users to work around due to the lack of errors + data in the onError and onComplete handlers. Thanks!

[sbassin]

Same issue in 3.8.3. I've verified that invoking the mutation via Apollo sandbox works just fine. My other mutations give me non-null data when there are no errors (i.e. error is also null).

Does anybody know what could lead to this scenario? It does seem like an illegal state according to my limited understanding of the documentation, but I suspect that there's something I could fix on my end to get this working and it's just not obvious to me at this point.

[adrienharnay]

Hi,

I have been using @apollo/client for a few years, and with

mutate: {
  errorPolicy: 'all',
},

onCompleted has always been invoked when calling a mutation that resulted in a GraphQL error. My understanding regarding the docs was that onCompleted would not be called when the mutation would result in a network error.

Now, since 3.8.x, onCompleted is no longer called upon GraphQL or network error, and I see no mention of changes on onCompleted anywhere in the changelog, which would have broken my application if I would not have specific tests for @apollo/client code in my application code (I've been burnt to many times by silent breaking changes).

This change has been brought by this commit: e3d611d which fixes onError but breaks the "normal" behavior of onCompleted in a minor version. @jerelmiller might be worth mentioning it in the releases even if we don't release a major version, what do you think?

Edit: to avoid breaking changes, what about introducing onSuccess alongside onCompleted and onError, and removing onCompleted in the next breaking release? It feels to me onCompleted should be called regardless of success or failure (the mutation will always become completed at some point).

[jerelmiller]

Hey @adrienharnay 👋

I'll do my best to respond to a few of your comments here.

onCompleted has always been invoked when calling a mutation that resulted in a GraphQL error. My understanding regarding the docs was that onCompleted would not be called when the mutation would result in a network error.

This was actually not quite the behavior specified in our documentation and as such, we classified it as a bug. If you take a look at our docs on the onCompleted callback, it specifies the following:

A callback function that's called when your mutation successfully completes with zero errors (or if errorPolicy is ignore and partial data is returned).

This function is passed the mutation's result data and any options passed to the mutation.

Note here how this says "zero errors", not "zero network errors". Given that the client behaved differently than specified, we opted to fix this behavior to match the documented behavior.

When we try and determine how to treat an issue, we typically look to our docs first to see how we are communicating the behavior. As such we treat our documentation as our source of truth when determining whether something is a bug or not. This was one of those cases where our documentation was different than the behavior, so we treated it as a bug fix and released it in a minor version.

---

Now, since 3.8.x, onCompleted is no longer called upon GraphQL or network error, and I see no mention of changes on onCompleted anywhere in the changelog

This change was communicated both in our 3.8.0 tagged release and in our 3.8.0 patch changes entry with the following:

#11103 e3d611daf Thanks @caylahamann! - Fixes a bug in useMutation so that onError is called when an error is returned from the request with errorPolicy set to 'all'.

Perhaps we could have called this out a bit better, so apologies if this change wasn't as clearly stated as it could have been. Let us know if you have recommendations on how we could have called this out in a way that was more understandable!

---

which would have broken my application if I would not have specific tests for @apollo/client code in my application code (I've been burnt to many times by silent breaking changes).

Unfortunately this is the downside to managing a library with such a large user base. What one set of users may see as a bug in behavior, others may come to rely on that behavior in their apps as a feature of the library. That means regardless of what decision we make, we always make someone unhappy. Just to reiterate, we viewed this as a buggy behavior because our documentation communicated a different behavior, hence why the minor release felt appropriate. I sincerely apologize this resulted in a breaking change to you!

---

Edit: to avoid breaking changes, what about introducing onSuccess alongside onCompleted and onError, and removing onCompleted in the next breaking release? It feels to me onCompleted should be called regardless of success or failure (the mutation will always become completed at some point).

We will definitely take this into consideration! I can't guarantee we will do something like this, but can absolutely understand how the name of this callback can be confusing.

If you don't mind me asking, what do you typically use these callbacks for? I'm trying to gather feedback on how devs use these callbacks so we can better communicate how/when to use them.

---

I appreciate the feedback and apologies again for what ended up as a breaking change to you!

[jerelmiller]

Hey all 👋

I just realized this issue was fixed along with #11103 and released in 3.8.0, which means we can close this issue. Please feel free to open a new issue if other problems arise. Thanks!

[jerelmiller]

@sbassin apologies, just saw your reply as well. If you're still seeing different behavior in a 3.8.x release, do you mind opening a new issue with a reproduction? It's not clear to me what might be happening. Thanks!

[adrienharnay]

Hi @jerelmiller, thank you for your answer.

I don't agree with the strategy that the documentation was correct but not the code (for - to my knowledge - at least 3 years), so the change is not a breaking change. It is still a breaking change in my opinion.

As for the changelog, the commit was documented yes, but the commit was not only about onError and also introduced the change about onCompleted, which was not documented. This is my main issue with the changelog.

As for how we use onCompleted, here is a simplified example:

onCompleted: (data) => {
  if (!data) {
    setState('error');
  } else {
    setState('success');
  }
}

[jerelmiller]

Hey @adrienharnay,

First off, again I want to apologize for the fact that you experienced a breaking change in your app due to this bug fix. It was not the intention, nor did I realize people relied on this behavior.

I don't agree with the strategy that the documentation was correct but not the code

I'm not sure I agree, otherwise we'd never be able to fix bugs since a bug fix is a change in behavior from the perspective of the code. On top of that, devs aren't reading our code for correctness and behavior (typically), they are reading our documentation and I believe the documentation accurately represents the intention of the original API design. I also believe the documentation reflects the original intent because of the type of that function:

  onCompleted?: (data: TData, clientOptions?: BaseMutationOptions) => void;

Note here how data is TData, not TData | undefined, hence why this should have never been possible:

onCompleted: (data) => {
  if (!data) {
    console.log('this should be impossible, according to the type')
  }
}

This is further supported by this issue itself, which was opened in March 2022, and with plenty of followup comments describing that the expected behavior was that onCompleted should have been called only when there were no errors.

(for - to my knowledge - at least 3 years), so the change is not a breaking change. It is still a breaking change in my opinion.

I can absolutely understand why you feel this way from the outside looking-in. Given the amount of time this behavior existed, it appears as if we intentionally left this behavior in there.

The unfortunate reality (or fortunate depending on how you look at it 😆) of maintaining a heavily utilized project such as Apollo Client, especially with a team that is as small is ours, is that things just can slip through the cracks unintentionally. A year ago we had over 600+ open issues and we've managed to bring that to ~470 today. It is just not possible to address every issue and move the project forward in a meaningful way, despite our best intentions.

As for the changelog, the commit was documented yes, but the commit was not only about onError and also introduced the change about onCompleted, which was not documented. This is my main issue with the changelog.

This is a fair criticism. Given that I approved that PR, including the changelog verbiage, this one is on me.

I'll see if I can make an update to the existing changelog entry that better reflects the underlying change to try and help others that may come across that changelog entry.

---

At this point we are unlikely to reverse the behavior for the various reasons I've stated above. If the 3.8.x upgrade is too breaking for your app because of this change, I'd recommend staying on the latest 3.7.x version and update your use of onCompleted to move the if (!error) logic into the onError callback. Once those changes are made, you should be safe to upgrade back to 3.8.x.

Appreciate the response!

[adrienharnay]

Thank you for taking the time to answer, I appreciate the work you are doing on Apollo, supporting many apps. I do agree that the types were wrong for some time now (we actually patched them in our codebase).
Updating the change log would be useful for other users that might rely on this!
Have a great day

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.