diff --git a/lib/graphql/execution/field_resolve_step.rb b/lib/graphql/execution/field_resolve_step.rb index eef8e8db85..06284bffcd 100644 --- a/lib/graphql/execution/field_resolve_step.rb +++ b/lib/graphql/execution/field_resolve_step.rb @@ -49,6 +49,7 @@ def append_selection(ast_node) end def value + return nil if @selections_step.killed query = @selections_step.query set_current_field query.current_trace.begin_execute_field(@field_definition, @arguments, @field_results, query) @@ -79,7 +80,9 @@ def sync(lazy) end def call + return nil if @selections_step.killed set_current_field if @field_definition + if @enqueued_authorization enqueue_next_steps elsif @finish_extension_idx @@ -102,24 +105,64 @@ def call set_current_field(nil) end - def add_graphql_error(err) + def add_graphql_error(result, key, err, return_type: @field_definition.type) err.path = path if err.ast_node.nil? err.ast_nodes = ast_nodes end - @selections_step.query.context.add_error(err) + @runner.add_finalizer(@selections_step.query, result, key, err) + if !err.is_a?(GraphQL::Execution::Skip) + field_type = return_type + should_propagate_null = field_type.non_null? + while (should_propagate_null == false && field_type.kind.wraps?) + field_type = field_type.of_type + should_propagate_null = field_type.non_null? + end + if should_propagate_null + propagate_nulls + end + end err end + def propagate_nulls + propagating_null = true + highest_nulled_depth = path.size + highest_list_depth = nil + current_field_step = self + while current_field_step + return_type = current_field_step.field_definition.type + if propagating_null && return_type.non_null? + highest_nulled_depth = current_field_step.path.size + else + propagating_null = false + end + + if return_type.list? + highest_list_depth = current_field_step.path.size + end + + current_field_step = current_field_step.selections_step.field_resolve_step + end + + if highest_list_depth.nil? || highest_nulled_depth <= highest_list_depth + kill_field_step = self + while kill_field_step && highest_nulled_depth <= kill_field_step.path.size + kill_field_step.selections_step.killed = true + kill_field_step = kill_field_step.selections_step.field_resolve_step + end + end + end + def build_errors_result(errors, single_error) first_error = errors.nil? ? single_error : errors.pop - @field_results = error_instance_array(@selections_step.objects.size, first_error) + @field_results = [first_error] + @results = [@selections_step.results.first] if errors errors.each do |e| - add_graphql_error(e) + add_graphql_error(@results.first, key, e) end end - @results ||= @selections_step.results build_results end @@ -226,7 +269,7 @@ def execute_field authorized_results << @results[i] end rescue GraphQL::ExecutionError => exec_err - add_graphql_error(exec_err) + add_graphql_error(@results[i], key, exec_err) end end i += 1 @@ -331,12 +374,12 @@ def execute_field end end rescue GraphQL::ExecutionError => err - add_graphql_error(err) + build_errors_result(nil, err) rescue StandardError => stderr begin @selections_step.query.handle_or_reraise(stderr, field: @field_definition, arguments: @arguments, object: nil) rescue GraphQL::ExecutionError => err - add_graphql_error(err) + add_graphql_error(@results[0], key, err) end end @@ -458,13 +501,13 @@ def build_results end def finish_leaf_result(result_h, key, field_result, return_type, ctx) - final_field_result = build_leaf_result(field_result, return_type, ctx, false) + final_field_result = build_leaf_result(result_h, key, field_result, return_type, ctx, false) @directive_finalizers&.each { |f| @runner.add_finalizer(ctx.query, result_h, key, f) } result_h[@key] = final_field_result end - def build_leaf_result(field_result, return_type, ctx, is_from_array) + def build_leaf_result(result_h, result_key, field_result, return_type, ctx, is_from_array) if field_result.nil? if return_type.non_null? add_non_null_error(is_from_array) @@ -473,7 +516,7 @@ def build_leaf_result(field_result, return_type, ctx, is_from_array) end elsif field_result.is_a?(Finalizer) if field_result.is_a?(GraphQL::RuntimeError) - add_graphql_error(field_result) + add_graphql_error(result_h, result_key, field_result, return_type: return_type) else field_result.path = path @runner.add_finalizer(ctx.query, result_h, key, field_result) @@ -484,7 +527,11 @@ def build_leaf_result(field_result, return_type, ctx, is_from_array) end inner_type = return_type.of_type - field_result.map { |item| build_leaf_result(item, inner_type, ctx, true) } + result_a = Array.new(field_result.size) + field_result.each_with_index do |item, idx| + result_a[idx] = build_leaf_result(result_a, idx, item, inner_type, ctx, true) + end + result_a else return_type.coerce_result(field_result, ctx) end @@ -526,6 +573,7 @@ def enqueue_next_steps query.current_trace.objects(obj_type, next_objects, ctx) @runner.add_step(SelectionsStep.new( path: path, + field_resolve_step: self, parent_type: obj_type, selections: @next_selections, objects: next_objects, @@ -538,6 +586,7 @@ def enqueue_next_steps query.current_trace.objects(@static_type, @all_next_objects, ctx) @runner.add_step(SelectionsStep.new( path: path, + field_resolve_step: self, parent_type: @static_type, selections: @next_selections, objects: @all_next_objects, @@ -558,7 +607,11 @@ def authorized_finished(step) def add_non_null_error(is_from_array) err = @parent_type::InvalidNullError.new(@parent_type, @field_definition, ast_nodes, is_from_array: is_from_array, path: path) - @runner.schema.type_error(err, @selections_step.query.context) + nn_result = @runner.schema.type_error(err, @selections_step.query.context) + if nn_result.nil? + propagate_nulls + end + nn_result end def set_current_field(new_value = @field_definition) @@ -576,7 +629,7 @@ def build_graphql_result(graphql_result, key, field_result, return_type, is_nn, end elsif field_result.is_a?(Finalizer) graphql_result[key] = if field_result.is_a?(GraphQL::RuntimeError) - add_graphql_error(field_result) + add_graphql_error(graphql_result, key, field_result) else field_result.path = path @runner.add_finalizer(@selections_step.query, graphql_result, key, field_result) diff --git a/lib/graphql/execution/finalize.rb b/lib/graphql/execution/finalize.rb index f9e3d607b0..850b000168 100644 --- a/lib/graphql/execution/finalize.rb +++ b/lib/graphql/execution/finalize.rb @@ -33,14 +33,15 @@ def initialize(query, data, runner) targets.each_with_index do |target, idx| if target.is_a?(Hash) - if target[key].equal?(err) + value_at_key = target[key] + if value_at_key.equal?(err) tf = @finalizers[target] ||= {}.compare_by_identity tf[key] = err @finalizers_count += 1 - elsif (arr = target[key]).is_a?(Array) - arr.each_with_index do |el, idx| + elsif value_at_key.is_a?(Array) + value_at_key.each_with_index do |el, idx| if el.equal?(err) - tf = @finalizers[arr] ||= {}.compare_by_identity + tf = @finalizers[value_at_key] ||= {}.compare_by_identity tf[idx] = err @finalizers_count += 1 end @@ -170,9 +171,7 @@ def check_object_result(result_h, parent_type, ast_selections) end def check_list_result(result_arr, inner_type, ast_selections) - inner_type_non_null = false - if inner_type.non_null? - inner_type_non_null = true + if (inner_type_non_null = inner_type.non_null?) inner_type = inner_type.of_type end diff --git a/lib/graphql/execution/prepare_object_step.rb b/lib/graphql/execution/prepare_object_step.rb index c566fdb7ee..59ac39fef4 100644 --- a/lib/graphql/execution/prepare_object_step.rb +++ b/lib/graphql/execution/prepare_object_step.rb @@ -73,6 +73,10 @@ def call @field_resolve_step.set_current_field(nil) end + def add_field_error(err) + @field_resolve_step.add_graphql_error(@graphql_result, @key, err) + end + def authorize if @field_resolve_step.was_scoped && !@resolved_type.reauthorize_scoped_objects @authorized_value = @object @@ -96,13 +100,13 @@ def authorize create_result end rescue GraphQL::RuntimeError => err - @graphql_result[@key] = @field_resolve_step.add_graphql_error(err) + @graphql_result[@key] = add_field_error(err) rescue StandardError => err query ||= @field_resolve_step.selections_step.query begin query.handle_or_reraise(err, field: @field_resolve_step.field_definition, arguments: @field_resolve_step.arguments, object: @object) # rubocop:disable Development/ContextIsPassedCop rescue GraphQL::RuntimeError => err - @graphql_result[@key] = @field_resolve_step.add_graphql_error(err) + @graphql_result[@key] = add_field_error(err) end end @@ -120,13 +124,13 @@ def create_result elsif @is_non_null @graphql_result[@key] = @field_resolve_step.add_non_null_error(@is_from_array) else - @graphql_result[@key] = @field_resolve_step.add_graphql_error(@authorization_error) + @graphql_result[@key] = add_field_error(@authorization_error) end rescue GraphQL::RuntimeError => err if @is_non_null @graphql_result[@key] = @field_resolve_step.add_non_null_error(@is_from_array) else - @graphql_result[@key] = @field_resolve_step.add_graphql_error(err) + @graphql_result[@key] = add_field_error(err) end end end diff --git a/lib/graphql/execution/runner.rb b/lib/graphql/execution/runner.rb index bdb5078779..1a40003c7a 100644 --- a/lib/graphql/execution/runner.rb +++ b/lib/graphql/execution/runner.rb @@ -298,6 +298,7 @@ def begin_execute(isolated_steps, results, query, root_type, root_value) if query.query? isolated_steps[0] << SelectionsStep.new( parent_type: root_type, + field_resolve_step: nil, selections: selected_operation.selections, objects: objects, results: [data], @@ -317,6 +318,7 @@ def begin_execute(isolated_steps, results, query, root_type, root_value) isolated_steps << [SelectionsStep.new( clobber: false, # `data` is being shared among several selections steps parent_type: root_type, + field_resolve_step: field_resolve_step, selections: field_resolve_step.ast_nodes || Array(field_resolve_step.ast_node), objects: objects, results: [data], @@ -332,6 +334,7 @@ def begin_execute(isolated_steps, results, query, root_type, root_value) end isolated_steps[0] << SelectionsStep.new( parent_type: root_type, + field_resolve_step: nil, selections: selected_operation.selections, objects: objects, results: [data], @@ -355,6 +358,7 @@ def begin_execute(isolated_steps, results, query, root_type, root_value) results << { "data" => data } isolated_steps[0] << SelectionsStep.new( parent_type: resolved_type, + field_resolve_step: nil, selections: selected_operation.selections, objects: objects, results: [data], @@ -372,6 +376,7 @@ def begin_execute(isolated_steps, results, query, root_type, root_value) results << { "data" => list_result } isolated_steps[0] << SelectionsStep.new( parent_type: inner_type, + field_resolve_step: nil, selections: selected_operation.selections, objects: root_value, results: list_result, @@ -420,6 +425,7 @@ def run_isolated_scalar(type, partial) selections = partial.ast_nodes dummy_ss = SelectionsStep.new( parent_type: nil, + field_resolve_step: nil, selections: selections, objects: nil, results: nil, diff --git a/lib/graphql/execution/selections_step.rb b/lib/graphql/execution/selections_step.rb index 800b413227..4999593818 100644 --- a/lib/graphql/execution/selections_step.rb +++ b/lib/graphql/execution/selections_step.rb @@ -2,8 +2,9 @@ module GraphQL module Execution class SelectionsStep - def initialize(parent_type:, selections:, objects:, results:, runner:, query:, path:, clobber: true) + def initialize(parent_type:, field_resolve_step:, selections:, objects:, results:, runner:, query:, path:, clobber: true) @path = path + @field_resolve_step = field_resolve_step @parent_type = parent_type @selections = selections @runner = runner @@ -12,10 +13,13 @@ def initialize(parent_type:, selections:, objects:, results:, runner:, query:, p @query = query @graphql_objects = nil @all_selections = nil + @killed = false @clobber = clobber end - attr_reader :path, :query, :objects, :results + attr_reader :path, :query, :objects, :results, :field_resolve_step + + attr_accessor :killed def graphql_objects @graphql_objects ||= @objects.map do |obj| diff --git a/lib/graphql/execution_error.rb b/lib/graphql/execution_error.rb index 4ccbb001ef..b79d05fc94 100644 --- a/lib/graphql/execution_error.rb +++ b/lib/graphql/execution_error.rb @@ -26,6 +26,10 @@ def initialize(message, ast_node: nil, ast_nodes: nil, options: nil, extensions: end def finalize_graphql_result(query, result_data, key) + add_this_error = !query.context.errors.any? { |e| e.eql?(self) } + if add_this_error + query.context.add_error(self) + end if ast_node.is_a?(GraphQL::Language::Nodes::Directive) # This is for backwards compatibility ... what does the spec say? result_data.delete(key) diff --git a/spec/graphql/authorization_spec.rb b/spec/graphql/authorization_spec.rb index 138d693500..985fd00d21 100644 --- a/spec/graphql/authorization_spec.rb +++ b/spec/graphql/authorization_spec.rb @@ -620,11 +620,7 @@ def auth_execute(*args, **kwargs) it "halts on unauthorized mutations" do query = "mutation { doUnauthorizedStuff(input: {}) { __typename } }" res = auth_execute(query, context: { unauthorized_mutation: true }) - if TESTING_EXEC_NEXT - assert_equal({}, res["data"]) - else - assert_nil res["data"].fetch("doUnauthorizedStuff") - end + assert_nil res["data"].fetch("doUnauthorizedStuff") assert_raises GraphQL::RequiredImplementationMissingError do auth_execute(query) end diff --git a/spec/graphql/execution/interpreter_spec.rb b/spec/graphql/execution/interpreter_spec.rb index 0a3c629d08..686f090507 100644 --- a/spec/graphql/execution/interpreter_spec.rb +++ b/spec/graphql/execution/interpreter_spec.rb @@ -746,7 +746,10 @@ class Thing < GraphQL::Schema::Object end class Query < GraphQL::Schema::Object - field :things, Thing.connection_type, null: false, resolve_static: true + field :things, Thing.connection_type, resolve_static: true + field :other_things, Thing.connection_type, resolve_static: :things + field :non_null_things, Thing.connection_type, null: false, resolve_static: :things + field :non_null_other_things, Thing.connection_type, null: false, resolve_static: :things def self.things(context) [{title: "a"}, {title: "b"}, {title: "c"}] @@ -756,6 +759,18 @@ def things self.class.things(context) end + def other_things + self.class.things(context) + end + + def non_null_other_things + self.class.things(context) + end + + def non_null_things + self.class.things(context) + end + field :thing, Thing, null: false, resolve_static: true def self.thing(context) @@ -775,18 +790,37 @@ class Schema < GraphQL::Schema end end + it "works on different branches" do + res = ConnectionErrorTest::Schema.execute("{ things { nodes { title } } otherThings { nodes { title } } }") + assert_equal({ "things" => nil, "otherThings" => nil }, res["data"]) + assert_equal [["things", "nodes", 0, "title"], ["otherThings", "nodes", 0, "title"]], res["errors"].map { |e| e["path"] } + assert_equal 2, res.context[:authorized_calls] + end + + it "Does non-null propagation across branches" do + res = ConnectionErrorTest::Schema.execute("{ nonNullThings { nodes { title } } nonNullOtherThings { nodes { title } } }") + assert_nil res.fetch("data") + expected_error_paths = if_exec_next([ + ["nonNullThings", "nodes", 0, "title"] + ], [ + ["nonNullThings", "nodes", 0, "title"], + ["nonNullOtherThings", "nodes", 0, "title"] + ]) + assert_equal expected_error_paths, res["errors"].map { |e| e["path"] } + assert_equal if_exec_next(1, 2), res.context[:authorized_calls] + end + it "returns only 1 error and stops resolving fields after that" do res = ConnectionErrorTest::Schema.execute("{ things { nodes { title } } }") - assert_equal 1, res["errors"].size + assert_equal [["things", "nodes", 0, "title"]], res["errors"].map { |e| e["path"] } assert_equal 1, res.context[:authorized_calls] res = ConnectionErrorTest::Schema.execute("{ things { edges { node { title } } } }") - assert_equal 1, res["errors"].size + assert_equal [["things", "edges", 0, "node", "title"]], res["errors"].map { |e| e["path"] } assert_equal 1, res.context[:authorized_calls] res = ConnectionErrorTest::Schema.execute("{ thing { title body } }") - exec_next_TODO "should abort other branches in this case" - assert_equal 1, res["errors"].size + assert_equal [["thing", "title"]], res["errors"].map { |e| e["path"] } assert_equal 1, res.context[:authorized_calls] end end diff --git a/spec/graphql/schema/validator_spec.rb b/spec/graphql/schema/validator_spec.rb index 9d5385401a..be8d9c57d5 100644 --- a/spec/graphql/schema/validator_spec.rb +++ b/spec/graphql/schema/validator_spec.rb @@ -141,11 +141,13 @@ def validate(obj, ctx, value) { "validated" => nil }, ] }, - "errors" => [ + "errors" => if_exec_next([ + {"message"=>"value must be greater than 5", "locations"=>[{"line"=>1, "column"=>10}], "path"=>["list", 0, "validated"]}, + ],[ {"message"=>"value must be greater than 5", "locations"=>[{"line"=>1, "column"=>10}], "path"=>["list", 0, "validated"]}, {"message"=>"value must be greater than 5", "locations"=>[{"line"=>1, "column"=>10}], "path"=>["list", 1, "validated"]}, {"message"=>"value must be greater than 5", "locations"=>[{"line"=>1, "column"=>10}], "path"=>["list", 2, "validated"]}, - ] + ]) } assert_equal expected_response, res end