Enable multiple execution errors for Fields defined to return a list … #2433
Conversation
| @@ -296,18 +296,6 @@ | |||
| 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.
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.
|
|
||
| describe "more than one ExecutionError on a field defined to return a list" do |
There was a problem hiding this comment.
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.
| @@ -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? | |||
There was a problem hiding this comment.
I think this is the correct logic for including (or not) the array of errors here.
This covers all scenarios:
- fields defined with
null: truewill not have these errors included here since as the comment below mentions they were already handled somewhere else. - fields defined with
null: falsewill have the list of errors included, regardless of whether they are defined to return a single type or a list of some type.
|
Currently the spec I added fails for the ruby 2.4.3 build in TravisCI. The actual results do not include the indices in the path. I am unable to replicate this failure locally. Does anyone have leads on what may be happening here? |
There was a problem hiding this comment.
Hi, thanks for taking this on! The change is looking great. It looks like a similar change will be required in interpreter/runtime.rb, which will be the default runtime in a future GraphQL-Ruby version (it has better performance and flexibility than execute.rb).
| 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.
adding the index to the path for list types.
|
Awesome, thanks so much for taking care of this 🎉 ! |
Fields defined to return a List of some type should be able to return an Array of GraphQL::ExecutionErrors instead.