Incorrect behavior on error handling of a mutation with multiple root fields

[khamusa]

Hello,

once again, thanks for this amazing work. I think I'd run into a bug regarding error handling when executing multiple mutation fields on the same mutation. Please consider:

mutation myDemoMutation {
  check_in(input: { id:"some-id"}) { ... }
  second_mutation: check_in(input: { id:"another-id" }) { ...   }
}

Now suppose the first field raises a not authorized GraphQL::ExecutionError exception. The api output is something along these lines:

{
  "data": null,
  "errors": [
    {
      "message": "You are not authorized to perform this action",
      "locations": [
        {
          "line": 8,
          "column": 3
        }
      ],
      "path": [
        "check_in"
      ]
    }
  ]
}

As you can see data is null, and hence no output is present for second_mutation. However, after inspecting the system state, I can verify that the mutation has been executed and the side-effects caused by it still persist.

In other words, the check_in operation of the second mutation executes successfully although the requester is unable to retrieve a response for it.

I believe the expected behavior should include the response for second_mutation under data, null for the first mutation, while also exposing the error for the first mutation under errors.

{
  "data": {
    check_in: null,
    second_mutation: { 
      # fields requested on the mutation field selection
    }
  },
  "errors": [
    {
      "message": "You are not authorized to perform this action",
      "locations": [
        {
          "line": 8,
          "column": 3
        }
      ],
      "path": [
        "check_in"
      ]
    }
  ]
}

Reference: graphql specs (https://facebook.github.io/graphql/#sec-Errors-and-Non-Nullability),

If an error is thrown while resolving a field, it should be treated as though the field returned null, and an error must be added to the "errors" list in the response. [ ... ] If all fields from the root of the request to the source of the error return Non-Null types, then the "data" entry in the response should be null.

Could you please confirm if my assumptions and interpretation of the specs are correct? If affirmative any chance we could have this fixed in upcoming releases?

[rmosolgo]

I think you're right -- this is a bug!

What's the return type of check_in ?

[rmosolgo]

Also, can you tell me a bit more about how this error is handled? For example, what kind of error is it and what technique is used to catch that error in GraphQL (eg, rescue_from, returning a ExecutionError, raising a ExecutionError ?)

[khamusa]

The first mutation is raising a Unauthorized < GraphQL::ExecutionError. However I just confirmed that all cases yield the same result:

• returning an instance of Unauthorized
• returning/raising an instance of GraphQL::ExecutionError

The check_in mutation payload type is an union type accepting either one of: Invitation or ValidationErrors.

Let me know if you need any more info

[rmosolgo]

Thanks, i'll take a look!

[rmosolgo]

I've written up a little test on this branch and it seems to pass: https://github.com/rmosolgo/graphql-ruby/compare/mutation-error-handling

Could you share some more info to help track this down? These would be helpful:

• Gem versions: graphql and any other related gems (eg graphql-batch)
• Source code for mutation (you can skip most of the resolve function, just show what values may be returned)
• Definitions of return types (IDL is ok too)

[khamusa]

Your spec clearly indicated the issue is not in the gem but in our code, so I've spent some time debuggin the issue. This definition causes the error:

field @mutation_name do
  type !payload_type
end  

while this does not:

field @mutation_name do
  type payload_type
end  

Conceptually we thought that when a field yield errors it is not actually resolving to null, but the null is rather a side-effect. Anyway, I guess it is pretty harmless to make it nullable and that solves our issue. Thank you for your help!

[rmosolgo]

It seems like an error in null propagation, and a pretty serious one. GraphQL is executing fields, but not telling you! Are you willing to share your gem versions?

[rmosolgo]

I updated my branch to test non-null fields and it passed, so I wonder if it's a difference between gem versions or some other factor!

[khamusa]

Sorry I omitted the gem versions.

    graphql (1.4.4)
    rspec-graphql_matchers (0.4.0)
      graphql (>= 0.9, < 2)

rspec-graphql_matchers is only available on test environment and does not mess with any graphql object. We're running ruby 2.2.4.

The resolver for the check_in mutation goes somewhat like this:

class Resolver < LetsGraph::MutationResolver
    def resolve # called by superclass #call and used as return value to #call

      # just forcing an error on the first mutation I was sending for testing purposes
      return GraphQL::ExecutionError.new 'Buuu!' if input[:id] == 'fail'
      
      @invitation =  Invitation.find input[:id] 
      # .... perform check_in
      @invitation
    end 
end

I also tested with an inline resolve function, same results, so I do not believe the issue to be related to the resolve.

Now the payload type is tricky, I'm autogenerating it from database columns (mapping db types to graphQL types).

Looking at your specs, the fundamental definition I see compared with my mutations is that I'm not using GraphQL::Relay::Mutation. It is somewhat autogenerated but it would translate to something along these lines:

GraphQL::ObjectType.define do
  field :check_in do
      type !Mutations::Invitations::CheckIn::PayloadType # Union of two common object types
      argument :input, !Mutations::Invitations::CheckIn::InputType
      resolve Mutations::Invitations::CheckIn::Resolver.new
  end
end

The generated "signature" for the mutation is check_in(input: CheckInInput!): CheckInResult!.

Maybe the difference in using return_field :shipEdge, !Ship.edge_type vs. my type !Mutations::Invitations::CheckIn::PayloadType could be the cause of the specs passing?

[rmosolgo]

Thanks, that's really helpful. I think you're right about the difference being GraphQL::Relay::Mutation.

Just to confirm: field :check_in belongs to the root Mutation type, right? (That's how it was in the first example, just want to double-check.)

[khamusa]

Yes!

[rmosolgo]

Thanks for all the details! I've replicated the issue here: https://github.com/rmosolgo/graphql-ruby/tree/mutation-field-error-test

If a Mutation field is non-null and that field returns nil, the nil is propagated to the top-level "data" key, as specified in "Errors and Non-Nullability":

If all fields from the root of the request to the source of the error return Non-Null types, then the "data" entry in the response should be null.

I thought that "Normal and Serial Execution" would specify that "execution halts if a field raises a field error", but I don't see such a specification. Do you?

I've asked for clarification here: graphql/graphql-spec#277

[khamusa]

If all fields from the root of the request to the source of the error return Non-Null types, then the "data" entry in the response should be null.

I am under the impression this statement is assuming a single root field with a Non-null path to a child with errors...

I'm more convinced that spec-wise the correct result should be data: null if a Non-Null root mutation field yields an error, because of:

Since Non-Null type fields cannot be null (the check_in field), field errors are propagated to be handled by the parent field (in our case the root mutation). If the parent field may be null ("data" may be null after all) then it resolves to null, otherwise if it is a Non-Null type, the field error is further propagated to it’s parent field (we are already at the root, so we can't propagate it any further, we are left with no other option).

If we do accept the above interpretation, for consistency we have to consider that serial execution should halt. Executing further fields with side-effects while also denying the user the possibility of asserting its results would be extremely confusing.

I also see a logical argument on the fact that enforcing serial execution for mutations does mean assuming dependency between the multiple root fields in the first place... I can think of quite a few use cases where halting the execution would be far more useful than allowing fields to execute independently.

A possible way to support both use cases would be to allow execution of further fields if the field yielding the error is of nullable return type, but halting execution if it is not nullable. Doing otherwise would yield a response that violates the schema. I wonder if the client could have voice on this decision too.

Finally:

... given the unluckiness of 13 ...

😂

[rmosolgo]

Agreed, here's a fix: #575

[rmosolgo]

Thanks for your help tracking this down! It seems like others agree over at graphql/graphql-spec#277. This fix is released in 1.4.5!

[khamusa]

Thanks to you, I'm actually glad to be of help. We'll upgrade right away!