Merged
Conversation
3 tasks
gmac
reviewed
Feb 2, 2026
| key = ast_selection.alias || ast_selection.name | ||
| current_exec_path << key | ||
| current_result_path << key | ||
| if paths_to_check.any? { |path_to_check| path_to_check[current_path_len] == key } |
Contributor
There was a problem hiding this comment.
Benchmark this. We took it out because we found we were slower trying to do fancy optimization checks rather than just traversing the tree. It also made the code a lot simpler. As a point of trivia, this bubbling algorithm was lifted almost verbatim from the stitching gem's shaper lib. Full-circle!
Contributor
There was a problem hiding this comment.
Actually, here: our final tuned traversal logic that ended up passing all internal tests is this... we had to make a bunch of small tweaks, nuanced to the point that I don't remember them all, and subtle enough that it'll probably take a long time to find all the quirks without a giant monolith to give you feedback. Might as well just get it tuned now. I also adjusted the design so that it could be run repeatedly on partial object subtrees (@defer experiment) while maintaining a shared internal cache across runs.
class State
#: Array[Hash[String, untyped]]
attr_reader :errors
#: error_path
attr_reader :actual_path
#: error_path
attr_reader :base_path
#: (?error_path) -> void
def initialize(base_path = EMPTY_ARRAY)
@base_path = base_path
@actual_path = []
@errors = []
end
#: -> error_path
def current_path
@base_path + @actual_path
end
end
#: (
#| executor: Executor,
#| invalidated_results: Hash[untyped, ExecutionError],
#| abstract_result_types: Hash[untyped, singleton(GraphQL::Schema::Object)],
#| ) -> void
def initialize(
executor:,
invalidated_results:,
abstract_result_types:
)
@executor = executor
@context = executor.context
@invalidated_results = invalidated_results
@abstract_result_types = abstract_result_types
end
#: (
#| singleton(GraphQL::Schema::Object),
#| Array[GraphQL::Language::Nodes::AbstractNode],
#| Hash[String, untyped],
#| ?Array[String | Integer]
#| ) -> [Hash[String, untyped]?, Array[error_hash]]
def format_object(parent_type, selections, data, base_path = EMPTY_ARRAY)
return [data, EMPTY_ARRAY] if @invalidated_results.empty?
state = State.new(base_path)
# Check if root data is invalidated (either inlined error or marked result)
if (err = @invalidated_results[data])
add_formatted_error(err, state)
return [nil, state.errors]
end
data = propagate_object_scope_errors(
data,
parent_type,
selections,
state,
)
[data, state.errors]
end
private
#: (untyped, singleton(GraphQL::Schema::Object), Array[GraphQL::Language::Nodes::AbstractNode], State) -> untyped
def propagate_object_scope_errors(raw_object, parent_type, selections, state)
return nil if raw_object.nil?
selections.each do |node|
case node
when GraphQL::Language::Nodes::Field
field_key = node.alias || node.name
state.actual_path << field_key
begin
node_type = @context.types.field(parent_type, node.name).type
named_type = node_type.unwrap
raw_value = raw_object.fetch(field_key, Executor::UNDEFINED)
# Aborted subtrees may have undefined fields that didn't execute.
# Ignore these rather than considering them invalid by the schema.
next if raw_value.equal?(Executor::UNDEFINED)
# Check for invalidated positions (inlined errors or marked results)
raw_object[field_key] = if (err = @invalidated_results[raw_value])
add_formatted_error(err, state)
nil
elsif node_type.list?
propagate_list_scope_errors(raw_value, node_type, node.selections, state)
elsif named_type.kind.leaf?
raw_value
else
propagate_object_scope_errors(raw_value, named_type, node.selections, state)
end
return nil if node_type.non_null? && raw_object[field_key].nil?
ensure
state.actual_path.pop
end
when GraphQL::Language::Nodes::InlineFragment
fragment_type = node.type ? @context.types.type(node.type.name) : parent_type
next unless result_of_type?(raw_object, parent_type, fragment_type)
result = propagate_object_scope_errors(raw_object, fragment_type, node.selections, state)
return nil if result.nil?
when GraphQL::Language::Nodes::FragmentSpread
fragment = @executor.fragments[node.name] #: as !nil
fragment_type = @context.types.type(fragment.type.name)
next unless result_of_type?(raw_object, parent_type, fragment_type)
result = propagate_object_scope_errors(raw_object, fragment_type, fragment.selections, state)
return nil if result.nil?
else
raise DocumentError, "Invalid selection node type"
end
end
raw_object
end
#: (Array[untyped]?, singleton(GraphQL::Schema::Member), Array[GraphQL::Language::Nodes::AbstractNode], State) -> Array[untyped]?
def propagate_list_scope_errors(raw_list, current_node_type, selections, state)
return nil if raw_list.nil?
item_node_type = Util.unwrap_non_null(current_node_type).of_type
named_type = item_node_type.unwrap
resolved_list = raw_list.map!.with_index do |raw_list_element, index|
state.actual_path << index
begin
# Check for invalidated positions (inlined errors or marked results)
result = if (err = @invalidated_results[raw_list_element])
add_formatted_error(err, state)
nil
elsif item_node_type.list?
propagate_list_scope_errors(raw_list_element, item_node_type, selections, state)
elsif named_type.kind.leaf?
raw_list_element
else
propagate_object_scope_errors(raw_list_element, named_type, selections, state)
end
return nil if result.nil? && item_node_type.non_null?
result
ensure
state.actual_path.pop
end
end
resolved_list
end
#: (untyped, singleton(GraphQL::Schema::Member), singleton(GraphQL::Schema::Member)) -> bool
def result_of_type?(result, current_type, inquiry_type)
# result_type must be concrete...
result_type = current_type.kind.abstract? ? @abstract_result_types[result] : current_type
raise ImplementationError, "No type annotation recorded for abstract result" if result_type.nil?
if inquiry_type.kind.abstract?
# abstract inquiry contains the concrete result type?
@context.types.possible_types(inquiry_type).include?(result_type)
else
# concrete result type matches the concrete inquiry type?
result_type == inquiry_type
end
end
Contributor
There was a problem hiding this comment.
Tests to pass:
class ErrorResultFormatterTest < Minitest::Test
TEST_RESOLVERS = {
"Node" => {
"id" => HashKeyResolver.new("id"),
"__type__" => ->(obj, ctx) { ctx.types.type(obj["__typename__"]) },
},
"Test" => {
"id" => HashKeyResolver.new("id"),
"req" => HashKeyResolver.new("req"),
"opt" => HashKeyResolver.new("opt"),
},
"Query" => {
"node" => HashKeyResolver.new("node"),
"test" => HashKeyResolver.new("test"),
"reqField" => HashKeyResolver.new("reqField"),
"anotherField" => HashKeyResolver.new("anotherField"),
},
}.freeze
def test_basic_object_structure
schema = "type Test { req: String! opt: String } type Query { test: Test }"
source = {
"test" => {
"req" => "yes",
"opt" => nil
}
}
expected = {
"data" => {
"test" => {
"req" => "yes",
"opt" => nil
}
}
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_errors_render_above_data_in_result
schema = "type Test { req: String! opt: String } type Query { test: Test }"
source = { "test" => { "req" => nil } }
assert_equal ["errors", "data"], exec_test(schema, "{ test { req } }", source).keys
end
def test_bubbles_null_for_single_object_scopes
schema = "type Test { req: String! opt: String } type Query { test: Test }"
source = {
"test" => {
"req" => nil,
"opt" => "yes"
},
}
expected = {
"data" => {
"test" => nil,
},
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.req",
"path" => ["test", "req"],
"locations" => [{ "line" => 1, "column" => 10 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_bubbles_null_for_nested_non_null_object_scopes
schema = "type Test { req: String! opt: String } type Query { test: Test! }"
source = {
"test" => {
"req" => nil,
"opt" => "yes"
}
}
expected = {
"data" => nil,
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.req",
"path" => ["test", "req"],
"locations" => [{ "line" => 1, "column" => 10 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_basic_list_structure
schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
source = {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => "yes", "opt" => "yes" },
],
}
expected = {
"data" => {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => "yes", "opt" => "yes" },
],
},
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_bubbles_null_for_list_elements
schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
source = {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => nil, "opt" => "yes" },
],
}
expected = {
"data" => {
"test" => [
{ "req" => "yes", "opt" => nil },
nil,
],
},
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.req",
"path" => ["test", 1, "req"],
"locations" => [{ "line" => 1, "column" => 10 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_bubbles_null_for_required_list_elements
schema = "type Test { req: String! opt: String } type Query { test: [Test!] }"
source = {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => nil, "opt" => "yes" },
]
}
expected = {
"data" => {
"test" => nil,
},
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.req",
"path" => ["test", 1, "req"],
"locations" => [{ "line" => 1, "column" => 10 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_bubbles_null_for_required_lists
schema = "type Test { req: String! opt: String } type Query { test: [Test!]! }"
source = {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => nil, "opt" => "yes" },
],
}
expected = {
"data" => nil,
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.req",
"path" => ["test", 1, "req"],
"locations" => [{ "line" => 1, "column" => 10 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_basic_nested_list_structure
schema = "type Test { req: String! opt: String } type Query { test: [[Test]] }"
source = {
"test" => [
[{ "req" => "yes", "opt" => nil }],
[{ "req" => "yes", "opt" => "yes" }],
],
}
expected = {
"data" => {
"test" => [
[{ "req" => "yes", "opt" => nil }],
[{ "req" => "yes", "opt" => "yes" }],
],
},
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_bubbles_null_for_nested_list_elements
schema = "type Test { req: String! opt: String } type Query { test: [[Test]] }"
source = {
"test" => [
[{ "req" => "yes", "opt" => nil }],
[{ "req" => nil, "opt" => "yes" }],
],
}
expected = {
"data" => {
"test" => [
[{ "req" => "yes", "opt" => nil }],
[nil],
],
},
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.req",
"path" => ["test", 1, 0, "req"],
"locations" => [{ "line" => 1, "column" => 10 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_bubbles_null_for_nested_required_list_elements
schema = "type Test { req: String! opt: String } type Query { test: [[Test!]] }"
source = {
"test" => [
[{ "req" => "yes", "opt" => nil }],
[{ "req" => nil, "opt" => "yes" }],
],
}
expected = {
"data" => {
"test" => [
[{ "req" => "yes", "opt" => nil }],
nil,
],
},
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.req",
"path" => ["test", 1, 0, "req"],
"locations" => [{ "line" => 1, "column" => 10 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_bubbles_null_for_inner_required_lists
schema = "type Test { req: String! opt: String } type Query { test: [[Test!]!] }"
source = {
"test" => [
[{ "req" => "yes", "opt" => nil }],
[{ "req" => nil, "opt" => "yes" }],
],
}
expected = {
"data" => {
"test" => nil,
},
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.req",
"path" => ["test", 1, 0, "req"],
"locations" => [{ "line" => 1, "column" => 10 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_bubbles_null_through_nested_required_list_scopes
schema = "type Test { req: String! opt: String } type Query { test: [[Test!]!]! }"
source = {
"test" => [
[{ "req" => "yes", "opt" => nil }],
[{ "req" => nil, "opt" => "yes" }],
],
}
expected = {
"data" => nil,
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.req",
"path" => ["test", 1, 0, "req"],
"locations" => [{ "line" => 1, "column" => 10 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_bubble_through_inline_fragment
schema = "type Test { req: String! opt: String } type Query { test: Test }"
source = {
"test" => {
"req" => nil,
"opt" => nil
},
}
expected = {
"data" => {
"test" => nil,
},
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.req",
"path" => ["test", "req"],
"locations" => [{ "line" => 1, "column" => 10 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_bubble_through_fragment_spreads
schema = "type Test { req: String! opt: String } type Query { test: Test }"
source = {
"test" => {
"req" => nil,
"opt" => nil
},
}
expected = {
"data" => {
"test" => nil,
},
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.req",
"path" => ["test", "req"],
"locations" => [{ "line" => 1, "column" => 10 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_inline_errors_in_null_positions_report
schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
source = {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => "yes", "opt" => GraphQL::Cardinal::ExecutionError.new("Not okay!") },
],
}
expected = {
"data" => {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => "yes", "opt" => nil },
],
},
"errors" => [{
"message" => "Not okay!",
"locations" => [{ "line" => 1, "column" => 14 }],
"path" => ["test", 1, "opt"],
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
inline_fragment_errors = [{
"message" => "Not okay!",
"locations" => [{ "line" => 1, "column" => 28 }],
"path" => ["test", 1, "opt"],
}]
result = exec_test(schema, "{ ...on Query { test { req opt } } }", source)
assert_equal expected["data"], result["data"]
assert_equal inline_fragment_errors, result["errors"]
fragment_errors = [{
"message" => "Not okay!",
"locations" => [{ "line" => 1, "column" => 59 }],
"path" => ["test", 1, "opt"],
}]
result = exec_test(schema, "{ ...Selection } fragment Selection on Query { test { req opt } }", source)
assert_equal expected["data"], result["data"]
assert_equal fragment_errors, result["errors"]
end
def test_abstract_fragments_on_concrete_results_interpret_type
schema = %|
interface Node {
id: ID!
}
type Test implements Node {
id: ID!
}
type Query {
node: Node
test: Test
}
|
query = %|
query {
test {
... on Node { id }
... NodeAttrs
}
}
fragment NodeAttrs on Node { id }
|
source = {
"test" => {},
}
expected = {
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.id",
"locations" => [{ "line" => 4, "column" => 31 }, { "line" => 8, "column" => 42 }],
"extensions" => { "code" => "INVALID_NULL" },
"path" => ["test", "id"],
}],
"data" => { "test" => nil },
}
assert_equal expected, exec_test(schema, query, source)
end
def test_concrete_fragments_on_abstract_results_interpret_type
schema = %|
interface Node {
id: ID!
}
type Test implements Node {
id: ID!
}
type Query {
node: Node
test: Test
}
|
query = %|
query {
node {
... on Test { id }
... TestAttrs
}
}
fragment TestAttrs on Test { id }
|
source = {
"node" => { "__typename__" => "Test" },
}
expected = {
"errors" => [{
"message" => "Cannot return null for non-nullable field Test.id",
"locations" => [{ "line" => 4, "column" => 31 }, { "line" => 8, "column" => 42 }],
"extensions" => { "code" => "INVALID_NULL" },
"path" => ["node", "id"],
}],
"data" => { "node" => nil },
}
assert_equal expected, exec_test(schema, query, source)
end
def test_inline_errors_in_non_null_positions_report_and_propagate
schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
source = {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => GraphQL::Cardinal::ExecutionError.new("Not okay!"), "opt" => nil },
],
}
expected = {
"data" => {
"test" => [
{ "req" => "yes", "opt" => nil },
nil,
],
},
"errors" => [{
"message" => "Not okay!",
"locations" => [{ "line" => 1, "column" => 10 }],
"path" => ["test", 1, "req"],
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_multiple_offenses_for_null_position_report_all_instances
schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
source = {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => "yes", "opt" => GraphQL::Cardinal::ExecutionError.new("Not okay!") },
{ "req" => "yes", "opt" => GraphQL::Cardinal::ExecutionError.new("Not okay!") },
],
}
expected = {
"data" => {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => "yes", "opt" => nil },
{ "req" => "yes", "opt" => nil },
],
},
"errors" => [{
"message" => "Not okay!",
"locations" => [{ "line" => 1, "column" => 14 }],
"path" => ["test", 1, "opt"],
}, {
"message" => "Not okay!",
"locations" => [{ "line" => 1, "column" => 14 }],
"path" => ["test", 2, "opt"],
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_multiple_offenses_for_non_null_position_without_intersecting_propagation_report_all_instances
schema = "type Test { req: String! opt: String } type Query { test: [Test] }"
source = {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => GraphQL::Cardinal::ExecutionError.new("Not okay!"), "opt" => "yes" },
{ "req" => GraphQL::Cardinal::ExecutionError.new("Not okay!"), "opt" => "yes" },
],
}
expected = {
"data" => {
"test" => [
{ "req" => "yes", "opt" => nil },
nil,
nil,
],
},
"errors" => [{
"message" => "Not okay!",
"locations" => [{ "line" => 1, "column" => 10 }],
"path" => ["test", 1, "req"],
}, {
"message" => "Not okay!",
"locations" => [{ "line" => 1, "column" => 10 }],
"path" => ["test", 2, "req"],
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_multiple_offenses_for_non_null_position_with_intersecting_propagation_report_first_instance
schema = "type Test { req: String! opt: String } type Query { test: [Test!] }"
source = {
"test" => [
{ "req" => "yes", "opt" => nil },
{ "req" => GraphQL::Cardinal::ExecutionError.new("first"), "opt" => "yes" },
{ "req" => GraphQL::Cardinal::ExecutionError.new("second"), "opt" => "yes" },
],
}
expected = {
"data" => {
"test" => nil,
},
"errors" => [{
"message" => "first",
"locations" => [{ "line" => 1, "column" => 10 }],
"path" => ["test", 1, "req"],
}],
}
assert_equal expected, exec_test(schema, "{ test { req opt } }", source)
end
def test_multiple_locations_for_duplicate_field_selections
schema = "type Query { reqField: String! }"
source = {
"reqField" => nil,
}
query = <<~GRAPHQL
{
reqField
reqField
}
GRAPHQL
expected = {
"data" => nil,
"errors" => [{
"message" => "Cannot return null for non-nullable field Query.reqField",
"extensions" => { "code" => "INVALID_NULL" },
"path" => ["reqField"],
"locations" => [
{ "line" => 2, "column" => 3 },
{ "line" => 3, "column" => 3 },
],
}],
}
assert_equal expected, exec_test(schema, query, source)
end
def test_multiple_locations_with_fragments
schema = "type Query { reqField: String! anotherField: String }"
source = {
"reqField" => nil,
"anotherField" => "value",
}
query = <<~GRAPHQL
{
reqField
...Fields
}
fragment Fields on Query {
reqField
anotherField
}
GRAPHQL
expected = {
"data" => nil,
"errors" => [{
"message" => "Cannot return null for non-nullable field Query.reqField",
"extensions" => { "code" => "INVALID_NULL" },
"path" => ["reqField"],
"locations" => [
{ "line" => 2, "column" => 3 },
{ "line" => 7, "column" => 3 },
],
}],
}
assert_equal expected, exec_test(schema, query, source)
end
def test_multiple_locations_with_inline_fragments
schema = "type Query { reqField: String! }"
source = {
"reqField" => nil,
}
query = <<~GRAPHQL
{
reqField
... on Query {
reqField
}
}
GRAPHQL
expected = {
"data" => nil,
"errors" => [{
"message" => "Cannot return null for non-nullable field Query.reqField",
"extensions" => { "code" => "INVALID_NULL" },
"path" => ["reqField"],
"locations" => [
{ "line" => 2, "column" => 3 },
{ "line" => 4, "column" => 5 },
],
}],
}
assert_equal expected, exec_test(schema, query, source)
end
def test_formats_errors_with_extensions
schema = "type Query { test: String! }"
source = {
"test" => GraphQL::Cardinal::ExecutionError.new("Not okay!", extensions: {
"code" => "TEST",
reason: "sorry",
})
}
expected = {
"data" => nil,
"errors" => [{
"message" => "Not okay!",
"locations" => [{ "line" => 1, "column" => 3 }],
"extensions" => { "code" => "TEST", "reason" => "sorry" },
"path" => ["test"],
}],
}
assert_equal expected, exec_test(schema, "{ test }", source)
end
def test_formats_error_message_for_non_null_list_items
schema = "type Test { req: String! } type Query { test: [Test!]! }"
source = {
"test" => [nil],
}
expected = {
"data" => nil,
"errors" => [{
"message" => "Cannot return null for non-nullable element of type 'Test!' for Query.test",
"path" => ["test", 0],
"locations" => [{ "line" => 1, "column" => 3 }],
"extensions" => { "code" => "INVALID_NULL" },
}],
}
assert_equal expected, exec_test(schema, "{ test { req } }", source)
end
Owner
Author
There was a problem hiding this comment.
Thanks for this awesome detailed spec. I adapted it and hacked in some fixes in 3b5825a.
One spec still evades me:
1) Failure:
ErrorResultFormatterTest#test_multiple_offenses_for_non_null_position_with_intersecting_propagation_report_first_instance [spec/graphql/execution/batching/errors_spec.rb:593]:
--- expected
+++ actual
@@ -1 +1 @@
-{"data" => {"test" => nil}, "errors" => [{"message" => "first", "locations" => [{"line" => 1, "column" => 10}], "path" => ["test", 1, "req"]}]}
+#<GraphQL::Query::Result @query=... @to_h={"errors" => [{"message" => "first", "locations" => [{"line" => 1, "column" => 10}], "path" => ["test", 1, "req"]}, {"message" => "second", "locations" => [{"line" => 1, "column" => 10}], "path" => ["test", "req"]}], "data" => {"test" => nil}}>
It looks like it expects the first error in the list to be present in the response, but not the second error.
I reviewed the "Handling Execution Errors" in the spec and didn't immediately find a rationale for it. What's the inspiration for that spec?
Contributor
There was a problem hiding this comment.
What's the inspiration for that spec?
Aaaah. Uncharted territory here... that test involves subtree abort sequences, which weren't in the POC. So what happens there is we have multiple failures across the breadth of the request which all executed at once. HOWEVER, the later failure would never have been discovered in a depth traversal because execution would have aborted during the first failed subtree. So, there are different ways you could handle this, which the spec never considers:
- Report all discovered errors, regardless of where they are based on execution order.
- Follow the spec implementation and only report errors expected from depth-first perspective.
I went with the second option in blind interests of making our full test suite pass verbatim, but you could certainly argue that the first option is more correct. I think the second is correct though, because frequently the entire breadth of a field will all fail with the same error (which we obviously don't want to report).
To make the second option work, we decoupled execution errors from reported errors – which actually makes execution errors WAY simpler: the executor can just report errors left and right with complete disregard for their "correctness". It over-reports. Then the error formatter pass combs through the error log and selects which errors to report in the result; and we tuned that to behave like a conventional depth execution engine for consistency with existing tests.
Like I said... uncharted territory. Breadth is weird. And fun! :)
Owner
Author
There was a problem hiding this comment.
Wow, cool -- I'll leave it as-is with the expectation that it might change in the future if I implement subtree termination.
gmac
reviewed
Feb 2, 2026
|
|
||
| def execute | ||
| @selected_operation = @document.definitions.first # TODO select named operation | ||
| isolated_steps = case @selected_operation.operation_type |
Contributor
There was a problem hiding this comment.
This needs to be wrapped in some kind of execute_with_directives block to address operation-level directives. Setup an issue if you want to get into how runtime directives work in breadth, or at least how we solved them.
Owner
Author
There was a problem hiding this comment.
Sure, I'll look into it when I get to supporting this 👍
gmac
reviewed
Feb 2, 2026
| attr_reader :ast_node, :ast_nodes, :key, :parent_type, :selections_step, :runner, :field_definition | ||
|
|
||
| def path | ||
| @path ||= [*@selections_step.path, @key].freeze |
Contributor
There was a problem hiding this comment.
Suggested change
| @path ||= [*@selections_step.path, @key].freeze | |
| @path ||= (@selections_step.path + @key).freeze |
Slightly faster...
Owner
Author
There was a problem hiding this comment.
Hmm, this 💥s in my code:
TypeError: no implicit conversion of String into Array
lib/graphql/execution/batching/field_resolve_step.rb:30:in 'Array#+'
lib/graphql/execution/batching/field_resolve_step.rb:30:in 'GraphQL::Execution::Batching::FieldResolveStep#path'
@selections_step.path is an Array of Strings, @key is another String. I need a new array including all members of the previous .path, plus @key in the end. I would have done:
@path ||= begin
new_path = @selections_step.path.dup
new_path << @key
new_path.freeze
end But I saw the [*..., @key] approach in graphql-breadth-exec and copied it 😅 . As far as I can tell it's doing what I want: creating exactly one new object.
Or, is there another way that + can be made to work?
Contributor
There was a problem hiding this comment.
Oh, ha, you’re not always using arrays. Right. Ours is always an array. Claude coached us that the dynamic spread created more objects than simple concatenation, so we’ve been updating the pattern where we see it. It’s not that big a deal. Micro-ops, though these are all the little corners that cut out garbage
Co-authored-by: Greg MacWilliam <greg.macwilliam@shopify.com>
Co-authored-by: Greg MacWilliam <greg.macwilliam@shopify.com>
Co-authored-by: Greg MacWilliam <greg.macwilliam@shopify.com>
…which isn't currently hooked up to Batching
Owner
Author
|
This is far from done but I think it's ready to merge because:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Taking a crack at #5507
Much inspiration taken from https://github.com/gmac/graphql-breadth-exec, as well as #5389 and the existing
Interpreter::Runtimemodule.I'm especially indebted to @gmac's work in https://github.com/gmac/graphql-breadth-exec for:
gather_selectionsto isolate mutations, it's perfect but I think it would have taken a while to click for me.I plan to iterate on this in "draft" state until it's basically implementing the GraphQL spec, then merge it and continue working on it.
Known TODOs:
SEED="61509" GRAPHQL_FUTURE=1 be rake test TEST=spec/graphql/execution/lazy_spec.rbArgument#original_keyword? See if that can be removedReview opt-in mechanisms and unify them somewhat ?I will do this later@skip/@includeloads:compatiblity