Exception in onCompleted silently fails other functions

[cihadturhan]

Describe the bug I discovered it the hard way. If there is an error thrown inside onCompleted method, it will be suppressed. And the worst thing is, onError and update functions will be never called. Code Example

Mutation(
   options: MutationOptions(
    documentNode: gql(createSyncCodeMutation),
    update: (cache, result){
      // ** This will NEVER be called! **
     },
     onError: (OperationException exception){
      // ** This will NEVER be called even there is an error! **
     },
     onCompleted: (dynamic result) {
      // ** if result is NULL, the following will throw error **
      // ** and you will not know there is an exception here **
       if (result.data) {
          return;
       }
     }
  ),
)

Workaround Null-checking result inside onCompleted fixes

Mutation(
   options: MutationOptions(
    documentNode: gql(createSyncCodeMutation),
    update: (cache, result){
      // ** This WILL be called! **
     },
     onError: (OperationException exception){
      // ** This WILL be called too **
     },
     onCompleted: (dynamic result) {
       // Check if result is not null to prevent exception
       if(result == null){
         return;
       }
       if (result.data) {
          return;
       }
     }
  ),
)

graphql_flutter: ^3.1.0-beta.4

[micimize]

hmm, this is likely due to this: https://github.com/zino-app/graphql-flutter/blob/65d6069f43bafa4dbc001daf156e4c2cb6097653/packages/graphql/lib/src/core/observable_query.dart#L234-L236

Will take some consideration – am not quite sure what the best way to handle this is is.

[micimize]

The way we should solve this is likely through wrappers in MutationCallbackHandler but we need to be cognizant of how apollo handles these issues in our solution. Questions we should consider:

• should onCompleted be called on error?
• looking at apollo, it looks like they actually throw the exception. Does that make sense?
• if not, is our callback order correct? It seems natural to call onError first (#680)– why doesn't apollo?
• should errors in onCompleted and update override existing errors, or maybe we have a new wrapper class?

I'm going to close the outstanding PR (as it's outdated b/c of v4) and related issue and consolidate here. This is an important/major issue. Ideal solution is unclear, but wrapping callbacks is the natural first pass I think