Array containing only errors is nullified

[gmac]

Describe the bug

When resolving an array, the entire field is nullified when it contains only errors. I believe it should return an array with only the error positions nullified. For example, I believe this current behavior is incorrect:

def test_array(obj, args, ctx)
  [
    GraphQL::ExecutionError.new('Invalid'),
    GraphQL::ExecutionError.new('Not found')
  ]
end

# Results:
{
  data: { testArray: nil },
  errors: [
    { path: ['testArray', 0], message: 'Invalid', ... },
    { path: ['testArray', 1], message: 'Not found', ... },
  ],
}

I would expect this to return a valid array with two empty positions as the testArray data value:

{
  data: { testArray: [nil, nil] },
  errors: [
    { path: ['testArray', 0], message: 'Invalid', ... },
    { path: ['testArray', 1], message: 'Not found', ... },
  ],
}

Versions

graphql version: 1.11, schema built using GraphQL::Schema.from_definition

Steps to reproduce

1. Setup the following schema:

type Query {
  testArray: [String]
}

2. When resolving testArray, return the following:

[
  GraphQL::ExecutionError.new('Invalid'),
  GraphQL::ExecutionError.new('Not found')
]

Expected behavior

I would expect this to result in:

{
  data: { testArray: [nil, nil] },
  errors: [
    { path: ['testArray', 0], message: 'Invalid', ... },
    { path: ['testArray', 1], message: 'Not found', ... },
  ],
}

Actual behavior

While the errors are reported correctly, the testArray field is nullified rather than returning an array with empty positions.

{
  data: { testArray: nil },
  errors: [
    { path: ['testArray', 0], message: 'Invalid', ... },
    { path: ['testArray', 1], message: 'Not found', ... },
  ],
}

Additional context

Everything works correctly as long as the array contains at least one valid result in addition to other errors. For example, returning nil for one array position produces the expected results:

[
  nil,
  GraphQL::ExecutionError.new('Not found')
]

This outcome is as I would expect:

{
  data: { testArray: [nil, nil] },
  errors: [
    { path: ['testArray', 1], message: 'Not found', ... },
  ],
}

[rmosolgo]

Hey, thanks for the detailed writeup!

I think the "bug" here is because GraphQL-Ruby technically supports returning multiple errors from fields (this isn't allowed by the spec, but GraphQL-Ruby had it first 😖 ). The code for this is here:

graphql-ruby/lib/graphql/execution/interpreter/runtime.rb

Lines 265 to 276 in af617b9

	elsif value.is_a?(GraphQL::ExecutionError)
	value.path ||= path
	value.ast_node ||= ast_node
	write_execution_errors_in_response(path, [value])
	HALT
	elsif value.is_a?(Array) && value.any? && value.all? { |v| v.is_a?(GraphQL::ExecutionError) }
	value.each_with_index do |error, index|
	error.ast_node ||= ast_node
	error.path ||= path + (field.type.list? ? [index] : [])
	end
	write_execution_errors_in_response(path, value)
	HALT

If you want to take a crack at improving it, you might add a !field.type.list? to the clause that removes the entire array of errors. That code was meant to handle multiple errors from a non-list field, but it seems like it's wrongly hitting list fields that return results containing only errors.

I think if you skip that clause for list fields, the errors would get handled later on, as in your example of mixed error/non-error results. Want to give it a shot?

[gmac]

Thanks for the info! I timeboxed exploration to an hour and it looks like this is a bit contentious given that tests already verify the current behavior for list types: https://github.com/rmosolgo/graphql-ruby/blob/master/spec/graphql/execution_error_spec.rb#L310-L324.

Also, it looks like the problem may be a bit more involved than just opting list types out of the all-errors clause. Adding in the !field.type.list? check changes the execution result of the test linked above to this:

{"data"=>nil, "errors"=>[{"message"=>"The first error message for a field defined to return a list of strings.", "locations"=>[{"line"=>1, "column"=>3}], "path"=>["multipleErrorsOnNonNullableListField", 0]}]}

Now it looks like it's only reporting a single error for the whole array field, and still nullifying the result. In the meantime, I've worked around this with an array helper in my own app code that manually maps arrays errors:

def map_array_errors(context, array, path: nil)
  array.each_with_index.map do |item, idx|
    item = yield(item, idx) if block_given?
    if item.is_a?(GraphQL::ExecutionError)
      item.path = [path.presence || context[:current_path], idx].flatten
      context.add_error(item)
      nil
    else
      item
    end
  end
end

While it's not ideal to always pipe arrays through that to match the implementation in Node, it's not the end of the world and it avoids breaking changes here. A good approach could be to put this old behavior on a setting that is disabled by default – then legacy users could opt into the old behavior, while GraphQL Ruby would follow the GraphQL spec by default. I leave it to your discretion if this is something worth adjusting in a major future release, of if it's best just to let the old implementation roll and close this out.

[rmosolgo]

This will be released in 1.13.0! #3656