send resolver query keys as variables. (#138)

README.md

@@ -365,17 +365,17 @@ The `GraphQL::Stitching::HttpExecutable` class is provided as a simple executabl
 The stitching executor automatically batches subgraph requests so that only one request is made per location per generation of data. This is done using batched queries that combine all data access for a given a location. For example:
 
 ```graphql
-query MyOperation_2 {
-  _0_result: widgets(ids:["a","b","c"]) { ... } # << 3 Widget
-  _1_0_result: sprocket(id:"x") { ... } # << 1 Sprocket
-  _1_1_result: sprocket(id:"y") { ... } # << 1 Sprocket
-  _1_2_result: sprocket(id:"z") { ... } # << 1 Sprocket
+query MyOperation_2($_0_key:[ID!]!, $_1_0_key:ID!, $_1_1_key:ID!, $_1_2_key:ID!) {
+  _0_result: widgets(ids: $_0_key) { ... } # << 3 Widget
+  _1_0_result: sprocket(id: $_1_0_key) { ... } # << 1 Sprocket
+  _1_1_result: sprocket(id: $_1_1_key) { ... } # << 1 Sprocket
+  _1_2_result: sprocket(id: $_1_2_key) { ... } # << 1 Sprocket
 }
 ```
 
 Tips:
 
-* List queries (like the `widgets` selection above) are more compact for accessing multiple records, and are therefore preferable as stitching accessors.
+* List queries (like the `widgets` selection above) are generally preferable as resolver queries because they keep the batched document consistent regardless of set size, and make for smaller documents that parse and validate faster.
 * Assure that root field resolvers across your subgraph implement batching to anticipate cases like the three `sprocket` selections above.
 
 Otherwise, there's no developer intervention necessary (or generally possible) to improve upon data access. Note that multiple generations of data may still force the executor to return to a previous location for more data.

lib/graphql/stitching/composer.rb

@@ -546,22 +546,21 @@ def extract_resolvers(type_name, types_by_location)
                 raise ComposerError, "Resolver key at #{type_name}.#{field_name} must specify exactly one key."
               end
 
-              argument_name = key_selections[0].alias
-              argument_name ||= if field_candidate.arguments.size == 1
-                field_candidate.arguments.keys.first
-              elsif field_candidate.arguments[config.key]
-                config.key
+              argument = field_candidate.arguments[key_selections[0].alias]
+              argument ||= if field_candidate.arguments.size == 1
+                field_candidate.arguments.values.first
+              else
+                field_candidate.arguments[config.key]
               end
 
-              argument = field_candidate.arguments[argument_name]
               unless argument
                 raise ComposerError, "No resolver argument matched for #{type_name}.#{field_name}. " \
                   "Add an alias to the key that specifies its intended argument, ex: `arg:key`"
               end
 
               argument_structure = Util.flatten_type_structure(argument.type)
               if argument_structure.length != resolver_structure.length
-                raise ComposerError, "Mismatched input/output for #{type_name}.#{field_name}.#{argument_name} resolver. " \
+                raise ComposerError, "Mismatched input/output for #{type_name}.#{field_name}.#{argument.graphql_name} resolver. " \
                   "Arguments must map directly to results."
               end
 
@@ -582,7 +581,8 @@ def extract_resolvers(type_name, types_by_location)
                 type_name: resolver_type_name,
                 key: key_selections[0].name,
                 field: field_candidate.name,
-                arg: argument_name,
+                arg: argument.graphql_name,
+                arg_type_name: argument.type.unwrap.graphql_name,
                 list: resolver_structure.first.list?,
                 representations: config.representations,
               )

lib/graphql/stitching/executor/resolver_source.rb

@@ -6,6 +6,7 @@ class ResolverSource < GraphQL::Dataloader::Source
       def initialize(executor, location)
         @executor = executor
         @location = location
+        @variables = {}
       end
 
       def fetch(ops)
@@ -28,7 +29,7 @@ def fetch(ops)
             @executor.request.operation_name,
             @executor.request.operation_directives,
           )
-          variables = @executor.request.variables.slice(*variable_names)
+          variables = @variables.merge!(@executor.request.variables.slice(*variable_names))
           raw_result = @executor.request.supergraph.execute_at_location(@location, query_document, variables, @executor.request)
           @executor.query_count += 1
 
@@ -42,11 +43,11 @@ def fetch(ops)
       end
 
       # Builds batched resolver queries
-      # "query MyOperation_2_3($var:VarType) {
-      #   _0_result: list(keys:["a","b","c"]) { resolverSelections... }
-      #   _1_0_result: item(key:"x") { resolverSelections... }
-      #   _1_1_result: item(key:"y") { resolverSelections... }
-      #   _1_2_result: item(key:"z") { resolverSelections... }
+      # "query MyOperation_2_3($var:VarType, $_0_key:[ID!]!, $_1_0_key:ID!, $_1_1_key:ID!, $_1_2_key:ID!) {
+      #   _0_result: list(keys: $_0_key) { resolverSelections... }
+      #   _1_0_result: item(key: $_1_0_key) { resolverSelections... }
+      #   _1_1_result: item(key: $_1_1_key) { resolverSelections... }
+      #   _1_2_result: item(key: $_1_2_key) { resolverSelections... }
       # }"
       def build_document(origin_sets_by_operation, operation_name = nil, operation_directives = nil)
         variable_defs = {}
@@ -55,17 +56,21 @@ def build_document(origin_sets_by_operation, operation_name = nil, operation_dir
           resolver = op.resolver
 
           if resolver.list?
-            input = origin_set.each_with_index.reduce(String.new) do |memo, (origin_obj, index)|
-              memo << "," if index > 0
-              memo << build_key(resolver.key, origin_obj, as_representation: resolver.representations?)
-              memo
+            variable_name = "_#{batch_index}_key"
+
+            @variables[variable_name] = origin_set.map do |origin_obj|
+              build_key(resolver.key, origin_obj, as_representation: resolver.representations?)
             end
 
-            "_#{batch_index}_result: #{resolver.field}(#{resolver.arg}:[#{input}]) #{op.selections}"
+            variable_defs[variable_name] = "[#{resolver.arg_type_name}!]!"
+            "_#{batch_index}_result: #{resolver.field}(#{resolver.arg}:$#{variable_name}) #{op.selections}"
           else
             origin_set.map.with_index do |origin_obj, index|
-              input = build_key(resolver.key, origin_obj, as_representation: resolver.representations?)
-              "_#{batch_index}_#{index}_result: #{resolver.field}(#{resolver.arg}:#{input}) #{op.selections}"
+              variable_name = "_#{batch_index}_#{index}_key"
+              @variables[variable_name] = build_key(resolver.key, origin_obj, as_representation: resolver.representations?)
+
+              variable_defs[variable_name] = "#{resolver.arg_type_name}!"
+              "_#{batch_index}_#{index}_result: #{resolver.field}(#{resolver.arg}:$#{variable_name}) #{op.selections}"
             end
           end
         end
@@ -90,15 +95,19 @@ def build_document(origin_sets_by_operation, operation_name = nil, operation_dir
 
         doc << "{ #{query_fields.join(" ")} }"
 
-        return doc, variable_defs.keys
+        return doc, variable_defs.keys.tap do |names|
+          names.reject! { @variables.key?(_1) }
+        end
       end
 
       def build_key(key, origin_obj, as_representation: false)
-        key_value = JSON.generate(origin_obj[ExportSelection.key(key)])
         if as_representation
-          "{ __typename: \"#{origin_obj[ExportSelection.typename_node.alias]}\", #{key}: #{key_value} }"
+          {
+            "__typename" => origin_obj[ExportSelection.typename_node.alias],
+            key => origin_obj[ExportSelection.key(key)],
+          }
         else
-          key_value
+          origin_obj[ExportSelection.key(key)]
         end
       end
 

lib/graphql/stitching/resolver.rb

@@ -16,11 +16,14 @@ module Stitching
       # name of the root field to query.
       :field,
 
+      # specifies when the resolver is a list query.
+      :list,
+
       # name of the root field argument used to send the key.
       :arg,
 
-      # specifies when the resolver is a list query.
-      :list,
+      # type name of the root field argument used to send the key.
+      :arg_type_name,
 
       # specifies that keys should be sent as JSON representations with __typename and key.
       :representations,
@@ -35,8 +38,9 @@ def as_json
           type_name: type_name,
           key: key,
           field: field,
-          arg: arg,
           list: list,
+          arg: arg,
+          arg_type_name: arg_type_name,
           representations: representations,
         }.tap(&:compact!)
       end

lib/graphql/stitching/supergraph.rb

@@ -35,8 +35,9 @@ def from_definition(schema, executables:)
                 location: kwargs[:location],
                 key: kwargs[:key],
                 field: kwargs[:field],
-                arg: kwargs[:arg],
                 list: kwargs[:list] || false,
+                arg: kwargs[:arg],
+                arg_type_name: kwargs[:arg_type_name],
                 representations: kwargs[:representations] || false,
               )
             end
@@ -138,8 +139,9 @@ def to_definition
                 location: resolver.location,
                 key: resolver.key,
                 field: resolver.field,
-                arg: resolver.arg,
                 list: resolver.list || nil,
+                arg: resolver.arg,
+                arg_type_name: resolver.arg_type_name,
                 representations: resolver.representations || nil,
               }.tap(&:compact!)) if existing.nil?
             end

lib/graphql/stitching/supergraph/resolver_directive.rb

@@ -10,6 +10,7 @@ class ResolverDirective < GraphQL::Schema::Directive
       argument :key, String, required: true
       argument :field, String, required: true
       argument :arg, String, required: true
+      argument :arg_type_name, String, required: true
       argument :list, Boolean, required: false
       argument :representations, Boolean, required: false
       repeatable true

test/graphql/stitching/composer/configuration_test.rb

@@ -50,6 +50,7 @@ def test_perform_with_static_resolver_config
           field: "productA",
           key: "id",
           arg: "id",
+          arg_type_name: "ID",
           list: false,
           representations: false,
         ),
@@ -59,6 +60,7 @@ def test_perform_with_static_resolver_config
           field: "productB",
           key: "id",
           arg: "key",
+          arg_type_name: "ID",
           list: false,
           representations: false,
         ),

test/graphql/stitching/composer/merge_resolvers_test.rb

@@ -15,6 +15,7 @@ def test_creates_resolver_map
           key: "id",
           field: "a",
           arg: "id",
+          arg_type_name: "ID",
           list: false,
           representations: false,
           type_name: "Test"
@@ -24,6 +25,7 @@ def test_creates_resolver_map
           key: "id",
           field: "b",
           arg: "ids",
+          arg_type_name: "ID",
           list: true,
           representations: false,
           type_name: "Test"

test/graphql/stitching/executor/executor_test.rb

@@ -20,14 +20,14 @@ def mock_execs(source, returns, operation_name: nil, variables: nil)
       alpha: {
         schema: GraphQL::Schema.from_definition(alpha),
         executable: -> (req, src, vars) {
-          results << { location: "alpha", source: src }
+          results << { location: "alpha", source: src, variables: vars }
           { "data" => returns.shift }
         },
       },
       bravo: {
         schema: GraphQL::Schema.from_definition(bravo),
         executable: -> (req, src, vars) {
-          results << { location: "bravo", source: src }
+          results << { location: "bravo", source: src, variables: vars }
           { "data" => returns.shift }
         },
       },
@@ -46,17 +46,24 @@ def mock_execs(source, returns, operation_name: nil, variables: nil)
   def test_with_batching
     req = %|{ featured { name } }|
 
-    expected1 = %|
+    expected_source1 = %|
       query{ featured { _export_id: id _export___typename: __typename } }
     |
-    expected2 = %|
-      query{
-        _0_0_result: product(id:"1") { name }
-        _0_1_result: product(id:"2") { name }
-        _0_2_result: product(id:"3") { name }
+    expected_source2 = %|
+      query($_0_0_key:ID!,$_0_1_key:ID!,$_0_2_key:ID!){
+        _0_0_result: product(id:$_0_0_key) { name }
+        _0_1_result: product(id:$_0_1_key) { name }
+        _0_2_result: product(id:$_0_2_key) { name }
       }
     |
 
+    expected_vars1 = {}
+    expected_vars2 = {
+      "_0_0_key" => "1",
+      "_0_1_key" => "2",
+      "_0_2_key" => "3",
+    }
+
     execs = mock_execs(req, [
       {
         "featured" => [
@@ -75,22 +82,29 @@ def test_with_batching
     assert_equal 2, execs.length
 
     assert_equal "bravo", execs[0][:location]
-    assert_equal squish_string(expected1), execs[0][:source]
+    assert_equal squish_string(expected_source1), execs[0][:source]
+    assert_equal expected_vars1, execs[0][:variables]
 
     assert_equal "alpha", execs[1][:location]
-    assert_equal squish_string(expected2), execs[1][:source]
+    assert_equal squish_string(expected_source2), execs[1][:source]
+    assert_equal expected_vars2, execs[1][:variables]
   end
 
   def test_with_operation_name_and_directives
     req = %|query Test @inContext(lang: "EN") { featured { name } }|
 
-    expected1 = %|
+    expected_source1 = %|
       query Test_1 @inContext(lang: "EN") { featured { _export_id: id _export___typename: __typename } }
     |
-    expected2 = %|
-      query Test_2 @inContext(lang: "EN") { _0_0_result: product(id:"1") { name } }
+    expected_source2 = %|
+      query Test_2($_0_0_key:ID!) @inContext(lang: "EN") { _0_0_result: product(id:$_0_0_key) { name } }
     |
 
+    expected_vars1 = {}
+    expected_vars2 = {
+      "_0_0_key" => "1",
+    }
+
     execs = mock_execs(req, [
       { "featured" => [{ "_export_id" => "1", "_export___typename" => "Product" }] },
       { "_0_0_result" => { "name" => "Potato" } },
@@ -99,9 +113,11 @@ def test_with_operation_name_and_directives
     assert_equal 2, execs.length
 
     assert_equal "bravo", execs[0][:location]
-    assert_equal squish_string(expected1), execs[0][:source]
+    assert_equal squish_string(expected_source1), execs[0][:source]
+    assert_equal expected_vars1, execs[0][:variables]
 
     assert_equal "alpha", execs[1][:location]
-    assert_equal squish_string(expected2), execs[1][:source]
+    assert_equal squish_string(expected_source2), execs[1][:source]
+    assert_equal expected_vars2, execs[1][:variables]
   end
 end

test/graphql/stitching/executor/resolver_source_test.rb

@@ -17,6 +17,7 @@ def setup
         location: "products",
         field: "storefronts",
         arg: "ids",
+        arg_type_name: "ID",
         key: "id",
         list: true,
         type_name: "Storefront"
@@ -35,6 +36,7 @@ def setup
         location: "products",
         field: "product",
         arg: "upc",
+        arg_type_name: "ID",
         key: "upc",
         list: false,
         type_name: "Product"
@@ -52,10 +54,10 @@ def test_builds_document_for_operation_batch
     query_document, variable_names = @source.build_document(@origin_sets_by_operation)
 
     expected = %|
-      query($lang:String!,$currency:Currency!){
-        _0_result: storefronts(ids:["7","8"]) { name(lang:$lang) }
-        _1_0_result: product(upc:"abc") { price(currency:$currency) }
-        _1_1_result: product(upc:"xyz") { price(currency:$currency) }
+      query($lang:String!,$_0_key:[ID!]!,$currency:Currency!,$_1_0_key:ID!,$_1_1_key:ID!){
+        _0_result: storefronts(ids:$_0_key) { name(lang:$lang) }
+        _1_0_result: product(upc:$_1_0_key) { price(currency:$currency) }
+        _1_1_result: product(upc:$_1_1_key) { price(currency:$currency) }
       }
     |
 
@@ -67,10 +69,10 @@ def test_builds_document_with_operation_name
     query_document, variable_names = @source.build_document(@origin_sets_by_operation, "MyOperation")
 
     expected = %|
-      query MyOperation_2_3($lang:String!,$currency:Currency!){
-        _0_result: storefronts(ids:["7","8"]) { name(lang:$lang) }
-        _1_0_result: product(upc:"abc") { price(currency:$currency) }
-        _1_1_result: product(upc:"xyz") { price(currency:$currency) }
+      query MyOperation_2_3($lang:String!,$_0_key:[ID!]!,$currency:Currency!,$_1_0_key:ID!,$_1_1_key:ID!){
+        _0_result: storefronts(ids:$_0_key) { name(lang:$lang) }
+        _1_0_result: product(upc:$_1_0_key) { price(currency:$currency) }
+        _1_1_result: product(upc:$_1_1_key) { price(currency:$currency) }
       }
     |
 
@@ -86,10 +88,10 @@ def test_builds_document_with_operation_directives
     )
 
     expected = %|
-      query MyOperation_2_3($lang:String!,$currency:Currency!) @inContext(lang: "EN") {
-        _0_result: storefronts(ids:["7","8"]) { name(lang:$lang) }
-        _1_0_result: product(upc:"abc") { price(currency:$currency) }
-        _1_1_result: product(upc:"xyz") { price(currency:$currency) }
+      query MyOperation_2_3($lang:String!,$_0_key:[ID!]!,$currency:Currency!,$_1_0_key:ID!,$_1_1_key:ID!) @inContext(lang: "EN") {
+        _0_result: storefronts(ids:$_0_key) { name(lang:$lang) }
+        _1_0_result: product(upc:$_1_0_key) { price(currency:$currency) }
+        _1_1_result: product(upc:$_1_1_key) { price(currency:$currency) }
       }
     |