Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable multiple execution errors for Fields defined to return a list … #2433

Merged
merged 3 commits into from
Aug 21, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/graphql/execution/execute.rb
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ def continue_resolve_field(raw_value, field_type, field_ctx)
raw_value.path = field_ctx.path
query.context.errors.push(raw_value)
when Array
if !field_type.list?
if field_type.non_null?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the correct logic for including (or not) the array of errors here.

This covers all scenarios:

  • fields defined with null: true will not have these errors included here since as the comment below mentions they were already handled somewhere else.
  • fields defined with null: false will have the list of errors included, regardless of whether they are defined to return a single type or a list of some type.

# List type errors are handled above, this is for the case of fields returning an array of errors
list_errors = raw_value.each_with_index.select { |value, _| value.is_a?(GraphQL::ExecutionError) }
if list_errors.any?
Expand Down
6 changes: 3 additions & 3 deletions lib/graphql/execution/interpreter/runtime.rb
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,9 @@ def continue_value(path, value, field, is_non_null, 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 do |v|
v.path ||= path
v.ast_node ||= ast_node
value.each_with_index do |error, index|
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding the index to the path for list types.

error.ast_node ||= ast_node
error.path ||= path + (field.type.list? ? [index] : [])
end
write_execution_errors_in_response(path, value)
HALT
Expand Down
32 changes: 18 additions & 14 deletions spec/graphql/execution_error_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -295,19 +295,7 @@

describe "more than one ExecutionError" do
let(:query_string) { %|{ multipleErrorsOnNonNullableField} |}
it "the errors are inserted into the errors key and the data is nil even for a NonNullable field " do

# I Think the path here is _wrong_, since this is not an array field:
Copy link
Contributor Author

@stanishev stanishev Aug 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment makes sense to me because I submitted the PR that introduced this test. The comment is correct (and thank you sincerely for pointing this out).

I've taken the liberty to remove the comment because I see that you have already corrected the spec and I think that at this point the comment is no longer necessary. To someone else reading this without the context we have, I think it may just be confusing without adding much.

# expected_result = {
# "data"=>nil,
# "errors"=>
# [{"message"=>"This is an error message for some error.",
# "locations"=>[{"line"=>1, "column"=>3}],
# "path"=>["multipleErrorsOnNonNullableField", 0]},
# {"message"=>"This is another error message for a different error.",
# "locations"=>[{"line"=>1, "column"=>3}],
# "path"=>["multipleErrorsOnNonNullableField", 1]}]
# }
it "the errors are inserted into the errors key and the data is nil even for a NonNullable field" do
expected_result = {
"data"=>nil,
"errors"=>
Expand All @@ -320,6 +308,22 @@
}
assert_equal(expected_result, result)
end
end

describe "more than one ExecutionError on a field defined to return a list" do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more test, similar to the earlier one, but this time the return type is a list type and so the path includes the list indices as well.

let(:query_string) { %|{ multipleErrorsOnNonNullableListField} |}
it "the errors are inserted into the errors key and the data is nil even for a NonNullable field" do
expected_result = {
"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]},
{"message"=>"The second error message for a field defined to return a list of strings.",
"locations"=>[{"line"=>1, "column"=>3}],
"path"=>["multipleErrorsOnNonNullableListField", 1]}],
}
assert_equal(expected_result, result)
end
end
end
end
1 change: 1 addition & 0 deletions spec/graphql/introspection/schema_type_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
{"name"=>"maybeNull"},
{"name"=>"milk"},
{"name"=>"multipleErrorsOnNonNullableField"},
{"name"=>"multipleErrorsOnNonNullableListField"},
{"name"=>"root"},
{"name"=>"searchDairy"},
{"name"=>"tracingScalar"},
Expand Down
8 changes: 8 additions & 0 deletions spec/support/dummy/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,14 @@ def multiple_errors_on_non_nullable_field
]
end

field :multiple_errors_on_non_nullable_list_field, [String], null: false
def multiple_errors_on_non_nullable_list_field
[
GraphQL::ExecutionError.new("The first error message for a field defined to return a list of strings."),
GraphQL::ExecutionError.new("The second error message for a field defined to return a list of strings.")
]
end

field :execution_error_with_options, Integer, null: true
def execution_error_with_options
GraphQL::ExecutionError.new("Permission Denied!", options: { "code" => "permission_denied" })
Expand Down