Skip to content

Commit f0406ad

Browse files
gmacgmac
committed
fix planner correctness bugs.
1 parent 01737e8 commit f0406ad

4 files changed

Lines changed: 140 additions & 8 deletions

File tree

‎lib/graphql/stitching/planner.rb‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ def add_step(
9191
path.each { entrypoint << "/" << _1 }
9292

9393
step = @steps_by_entrypoint[entrypoint]
94-
next_index = step ? parent_index : @planning_index += 1
94+
next_index = step ? step.index : @planning_index += 1
9595

9696
unless selections.empty?
9797
selections = extract_locale_selections(location, parent_type, next_index, selections, path, variables)
@@ -110,6 +110,7 @@ def add_step(
110110
resolver: resolver,
111111
)
112112
else
113+
step.variables.merge!(variables)
113114
step.selections.concat(selections)
114115
step
115116
end
@@ -142,7 +143,8 @@ def build_root_entrypoints
142143
# A.2) Partition mutation fields by consecutive location for serial execution.
143144
partitions = []
144145
each_field_in_scope(parent_type, @request.operation.selections) do |node|
145-
next_location = @supergraph.locations_by_type_and_field[parent_type.graphql_name][node.name].first
146+
locations = @supergraph.locations_by_type_and_field[parent_type.graphql_name][node.name] || SUPERGRAPH_LOCATIONS
147+
next_location = locations.first
146148

147149
if partitions.none? || partitions.last.location != next_location
148150
partitions << ScopePartition.new(location: next_location, selections: [])
@@ -256,7 +258,8 @@ def extract_locale_selections(
256258
fragment_type = node.type ? @supergraph.memoized_schema_types[node.type.name] : parent_type
257259
next unless @supergraph.locations_by_type[fragment_type.graphql_name].include?(current_location)
258260

259-
is_same_scope = fragment_type == parent_type
261+
extract_node_directive_variables(node, locale_variables)
262+
is_same_scope = fragment_type == parent_type && node.directives.empty?
260263
selection_set = is_same_scope ? locale_selections : []
261264
extract_locale_selections(current_location, fragment_type, parent_index, node.selections, path, locale_variables, selection_set)
262265

@@ -269,14 +272,17 @@ def extract_locale_selections(
269272
fragment = @request.fragment_definitions[node.name]
270273
next unless @supergraph.locations_by_type[fragment.type.name].include?(current_location)
271274

275+
extract_node_directive_variables(node, locale_variables)
276+
extract_node_directive_variables(fragment, locale_variables)
272277
requires_typename = true
273278
fragment_type = @supergraph.memoized_schema_types[fragment.type.name]
274-
is_same_scope = fragment_type == parent_type
279+
directives = [*fragment.directives, *node.directives]
280+
is_same_scope = fragment_type == parent_type && directives.empty?
275281
selection_set = is_same_scope ? locale_selections : []
276282
extract_locale_selections(current_location, fragment_type, parent_index, fragment.selections, path, locale_variables, selection_set)
277283

278284
unless is_same_scope
279-
locale_selections << GraphQL::Language::Nodes::InlineFragment.new(type: fragment.type, selections: selection_set)
285+
locale_selections << GraphQL::Language::Nodes::InlineFragment.new(type: fragment.type, directives: directives, selections: selection_set)
280286
end
281287

282288
else
@@ -362,13 +368,30 @@ def extract_node_variables(node_with_args, variable_definitions)
362368
extract_node_variables(argument.value, variable_definitions)
363369
when GraphQL::Language::Nodes::VariableIdentifier
364370
variable_definitions[argument.value.name] ||= @request.variable_definitions[argument.value.name]
371+
when Array
372+
extract_value_variables(argument.value, variable_definitions)
365373
end
366374
end
367375

368376
if node_with_args.respond_to?(:directives)
369-
node_with_args.directives.each do |directive|
370-
extract_node_variables(directive, variable_definitions)
371-
end
377+
extract_node_directive_variables(node_with_args, variable_definitions)
378+
end
379+
end
380+
381+
def extract_value_variables(value, variable_definitions)
382+
case value
383+
when GraphQL::Language::Nodes::InputObject
384+
extract_node_variables(value, variable_definitions)
385+
when GraphQL::Language::Nodes::VariableIdentifier
386+
variable_definitions[value.name] ||= @request.variable_definitions[value.name]
387+
when Array
388+
value.each { extract_value_variables(_1, variable_definitions) }
389+
end
390+
end
391+
392+
def extract_node_directive_variables(node_with_directives, variable_definitions)
393+
node_with_directives.directives.each do |directive|
394+
extract_node_variables(directive, variable_definitions)
372395
end
373396
end
374397

‎test/graphql/stitching/planner/plan_fragments_test.rb‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,5 +156,6 @@ def test_plans_repeat_selections_and_fragments_into_coalesced_groupings
156156
assert_equal "bravo", third.location
157157
assert_equal ["namespace", "test", "nest"], third.path
158158
assert_equal "{ b }", third.selections
159+
assert_equal second.step, third.after
159160
end
160161
end

‎test/graphql/stitching/planner/plan_root_operations_test.rb‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,27 @@ def test_plans_subscription_operations_for_single_field
127127
}
128128
end
129129

130+
def test_plans_mutation_typename_on_supergraph
131+
document = %|
132+
mutation {
133+
__typename
134+
}
135+
|
136+
137+
plan = GraphQL::Stitching::Request.new(@supergraph, document).plan
138+
139+
assert_equal 1, plan.ops.length
140+
assert_keys plan.ops[0].as_json, {
141+
after: 0,
142+
location: "__super",
143+
operation_type: "mutation",
144+
selections: %|{ __typename }|,
145+
path: [],
146+
if_type: nil,
147+
resolver: nil,
148+
}
149+
end
150+
130151
def test_raises_for_subscription_operations_with_multiple_fields
131152
document = %|
132153
subscription {

‎test/graphql/stitching/planner/plan_variables_test.rb‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,29 @@ def test_extracts_variables_from_inline_input_objects
8383
assert_equal expected_vars, plan.ops[1].variables
8484
end
8585

86+
def test_extracts_variables_from_list_arguments
87+
query = %|
88+
query($id: ID, $childIds: [ID!]) {
89+
widget(ids: [$id], input: { name: "test", children: [$childIds] }) {
90+
id
91+
}
92+
}
93+
|
94+
95+
supergraph = compose_definitions({
96+
"widgets" => %|
97+
input MakeWidgetInput { name: String children: [[ID!]] }
98+
type Widget { id: ID! }
99+
type Query { widget(ids: [ID!], input: MakeWidgetInput): Widget }
100+
|,
101+
})
102+
103+
plan = GraphQL::Stitching::Request.new(supergraph, query).plan
104+
105+
assert_equal 1, plan.ops.length
106+
assert_equal({ "id" => "ID", "childIds" => "[ID!]" }, plan.ops[0].variables)
107+
end
108+
86109
def test_extracts_variables_for_input_object_fragments
87110
mutation = %|
88111
mutation($newWidget: MakeWidgetInput!, $newSprocket: MakeSprocketInput!, $lang: String) {
@@ -117,6 +140,22 @@ def test_extracts_variables_from_inline_fragments
117140
assert_equal expected_vars, plan.ops[0].variables
118141
end
119142

143+
def test_extracts_variables_from_inline_fragment_directives
144+
query = %|
145+
query($wid: ID!, $show: Boolean!) {
146+
thing(id: $wid) { ...on Widget @dir(show: $show) { id name } }
147+
}
148+
|
149+
150+
plan = GraphQL::Stitching::Request.new(@supergraph, query).plan
151+
152+
assert_equal 1, plan.ops.length
153+
154+
expected_vars = { "wid" => "ID!", "show" => "Boolean!" }
155+
assert_equal expected_vars, plan.ops[0].variables
156+
assert_equal %|{ thing(id: $wid) { ... on Widget @dir(show: $show) { id name } _export___typename: __typename } }|, plan.ops[0].selections
157+
end
158+
120159
def test_extracts_variables_from_fragment_spreads
121160
query = %|
122161
query($wid: ID!, $lang: String) {
@@ -132,4 +171,52 @@ def test_extracts_variables_from_fragment_spreads
132171
expected_vars = { "wid" => "ID!", "lang" => "String" }
133172
assert_equal expected_vars, plan.ops[0].variables
134173
end
174+
175+
def test_extracts_variables_from_fragment_spread_directives
176+
query = %|
177+
query($wid: ID!, $show: Boolean!) {
178+
thing(id: $wid) { ...WidgetAttrs @dir(show: $show) }
179+
}
180+
fragment WidgetAttrs on Widget { id name }
181+
|
182+
183+
plan = GraphQL::Stitching::Request.new(@supergraph, query).plan
184+
185+
assert_equal 1, plan.ops.length
186+
187+
expected_vars = { "wid" => "ID!", "show" => "Boolean!" }
188+
assert_equal expected_vars, plan.ops[0].variables
189+
assert_equal %|{ thing(id: $wid) { ... on Widget @dir(show: $show) { id name } _export___typename: __typename } }|, plan.ops[0].selections
190+
end
191+
192+
def test_merges_variables_from_coalesced_steps
193+
alpha = %|
194+
type Test { id: ID! a(lang: String): String x(unit: String): Int }
195+
type Query { testA(id: ID!): Test! @stitch(key: "id") }
196+
|
197+
198+
bravo = %|
199+
type Test { id: ID! b: String }
200+
type Namespace { test: Test }
201+
type Query {
202+
namespace: Namespace!
203+
testB(id: ID!): Test! @stitch(key: "id")
204+
}
205+
|
206+
207+
query = %|
208+
query($lang: String, $unit: String) {
209+
namespace {
210+
test { a(lang: $lang) }
211+
test { x(unit: $unit) }
212+
}
213+
}
214+
|
215+
216+
supergraph = compose_definitions({ "alpha" => alpha, "bravo" => bravo })
217+
plan = GraphQL::Stitching::Request.new(supergraph, query).plan
218+
219+
assert_equal 2, plan.ops.length
220+
assert_equal({ "lang" => "String", "unit" => "String" }, plan.ops[1].variables)
221+
end
135222
end

0 commit comments

Comments
 (0)