-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"=> | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
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:
null: true
will not have these errors included here since as the comment below mentions they were already handled somewhere else.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.