From a90880bdaabcc19e1212f05aeb979ffea9878848 Mon Sep 17 00:00:00 2001 From: Vlad Stanishev Date: Mon, 19 Aug 2019 16:27:37 -0400 Subject: [PATCH 1/3] Enable multiple execution errors for Fields defined to return a list of types. --- lib/graphql/execution/execute.rb | 2 +- spec/graphql/execution_error_spec.rb | 30 +++++++++++-------- .../graphql/introspection/schema_type_spec.rb | 1 + spec/support/dummy/schema.rb | 8 +++++ 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/lib/graphql/execution/execute.rb b/lib/graphql/execution/execute.rb index d96ba02087..5ea1e1ff5b 100644 --- a/lib/graphql/execution/execute.rb +++ b/lib/graphql/execution/execute.rb @@ -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? # 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? diff --git a/spec/graphql/execution_error_spec.rb b/spec/graphql/execution_error_spec.rb index 7c7e5135ef..0710893e5e 100644 --- a/spec/graphql/execution_error_spec.rb +++ b/spec/graphql/execution_error_spec.rb @@ -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: - # 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]}] - # } 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 + 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"=>"This is the first error message for a field defined to return a list of types.", + "locations"=>[{"line"=>1, "column"=>3}], + "path"=>["multipleErrorsOnNonNullableListField", 0]}, + {"message"=>"This is the second error message for a field defined to return a list of types.", + "locations"=>[{"line"=>1, "column"=>3}], + "path"=>["multipleErrorsOnNonNullableListField", 1]}], + } + assert_equal(expected_result, result) + end + end + end end diff --git a/spec/graphql/introspection/schema_type_spec.rb b/spec/graphql/introspection/schema_type_spec.rb index f12659a8d7..e97323cec7 100644 --- a/spec/graphql/introspection/schema_type_spec.rb +++ b/spec/graphql/introspection/schema_type_spec.rb @@ -37,6 +37,7 @@ {"name"=>"maybeNull"}, {"name"=>"milk"}, {"name"=>"multipleErrorsOnNonNullableField"}, + {"name"=>"multipleErrorsOnNonNullableListField"}, {"name"=>"root"}, {"name"=>"searchDairy"}, {"name"=>"tracingScalar"}, diff --git a/spec/support/dummy/schema.rb b/spec/support/dummy/schema.rb index d1ced3453e..1fc73d4e29 100644 --- a/spec/support/dummy/schema.rb +++ b/spec/support/dummy/schema.rb @@ -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("This is the first error message for a field defined to return a list of types."), + GraphQL::ExecutionError.new("This is the second error message for a field defined to return a list of types.") + ] + end + field :execution_error_with_options, Integer, null: true def execution_error_with_options GraphQL::ExecutionError.new("Permission Denied!", options: { "code" => "permission_denied" }) From 04daa523b5c659d076f14ecde8e6f27e839c04e4 Mon Sep 17 00:00:00 2001 From: Vlad Stanishev Date: Tue, 20 Aug 2019 16:42:19 -0400 Subject: [PATCH 2/3] Fixing the multiple_errors_on_non_nullable_list_field example the dummy schema. --- spec/graphql/execution_error_spec.rb | 4 ++-- spec/support/dummy/schema.rb | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/spec/graphql/execution_error_spec.rb b/spec/graphql/execution_error_spec.rb index 0710893e5e..6c7536428f 100644 --- a/spec/graphql/execution_error_spec.rb +++ b/spec/graphql/execution_error_spec.rb @@ -295,7 +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 + 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"=> @@ -311,7 +311,7 @@ describe "more than one ExecutionError on a field defined to return a list" do let(:query_string) { %|{ multipleErrorsOnNonNullableListField} |} - it "the errors are inserted into the errors key and the data is nil even for a NonNullable field " do + 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"=> diff --git a/spec/support/dummy/schema.rb b/spec/support/dummy/schema.rb index 1fc73d4e29..604720b9e0 100644 --- a/spec/support/dummy/schema.rb +++ b/spec/support/dummy/schema.rb @@ -406,10 +406,11 @@ def multiple_errors_on_non_nullable_field field :multiple_errors_on_non_nullable_list_field, [String], null: false def multiple_errors_on_non_nullable_list_field - [ - GraphQL::ExecutionError.new("This is the first error message for a field defined to return a list of types."), - GraphQL::ExecutionError.new("This is the second error message for a field defined to return a list of types.") - ] + err1 = GraphQL::ExecutionError.new("This is the first error message for a field defined to return a list of types.") + err1.path = ["multipleErrorsOnNonNullableListField", 0] + err2 = GraphQL::ExecutionError.new("This is the second error message for a field defined to return a list of types.") + err2.path = ["multipleErrorsOnNonNullableListField", 1] + [err1, err2] end field :execution_error_with_options, Integer, null: true From 1f68d14f3acd3509e6482edceb024f09a663743a Mon Sep 17 00:00:00 2001 From: Vlad Stanishev Date: Wed, 21 Aug 2019 10:44:49 -0400 Subject: [PATCH 3/3] Properly fixing runtime.rb to handle returning lists of errors. --- lib/graphql/execution/interpreter/runtime.rb | 6 +++--- spec/graphql/execution_error_spec.rb | 4 ++-- spec/support/dummy/schema.rb | 9 ++++----- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/graphql/execution/interpreter/runtime.rb b/lib/graphql/execution/interpreter/runtime.rb index 49270e3ea2..0bf3fbbf13 100644 --- a/lib/graphql/execution/interpreter/runtime.rb +++ b/lib/graphql/execution/interpreter/runtime.rb @@ -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| + error.ast_node ||= ast_node + error.path ||= path + (field.type.list? ? [index] : []) end write_execution_errors_in_response(path, value) HALT diff --git a/spec/graphql/execution_error_spec.rb b/spec/graphql/execution_error_spec.rb index 6c7536428f..72a21afeb5 100644 --- a/spec/graphql/execution_error_spec.rb +++ b/spec/graphql/execution_error_spec.rb @@ -315,10 +315,10 @@ expected_result = { "data"=>nil, "errors"=> - [{"message"=>"This is the first error message for a field defined to return a list of types.", + [{"message"=>"The first error message for a field defined to return a list of strings.", "locations"=>[{"line"=>1, "column"=>3}], "path"=>["multipleErrorsOnNonNullableListField", 0]}, - {"message"=>"This is the second error message for a field defined to return a list of types.", + {"message"=>"The second error message for a field defined to return a list of strings.", "locations"=>[{"line"=>1, "column"=>3}], "path"=>["multipleErrorsOnNonNullableListField", 1]}], } diff --git a/spec/support/dummy/schema.rb b/spec/support/dummy/schema.rb index 604720b9e0..641064d10a 100644 --- a/spec/support/dummy/schema.rb +++ b/spec/support/dummy/schema.rb @@ -406,11 +406,10 @@ def multiple_errors_on_non_nullable_field field :multiple_errors_on_non_nullable_list_field, [String], null: false def multiple_errors_on_non_nullable_list_field - err1 = GraphQL::ExecutionError.new("This is the first error message for a field defined to return a list of types.") - err1.path = ["multipleErrorsOnNonNullableListField", 0] - err2 = GraphQL::ExecutionError.new("This is the second error message for a field defined to return a list of types.") - err2.path = ["multipleErrorsOnNonNullableListField", 1] - [err1, err2] + [ + 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