Merge pull request #3320 from rmosolgo/dataloader-args-fix

Improve argument handling with Dataloader

guides/dataloader/overview.md

@@ -127,8 +127,6 @@ class FollowUser < GraphQL::Schema::Mutation
 end
 ```
 
-__Caveat:__ The current implementation loads all `loads:` objects in the _same fiber_, so data will be fetched sequentially.
-
 ## Data Sources
 
 To implement batch-loading data sources, see the {% internal_link "Sources guide", "/dataloader/sources" %}.

lib/graphql.rb

@@ -128,6 +128,7 @@ def match?(pattern)
 require "graphql/filter"
 require "graphql/internal_representation"
 require "graphql/static_validation"
+require "graphql/dataloader"
 require "graphql/introspection"
 
 require "graphql/analysis_error"
@@ -148,7 +149,6 @@ def match?(pattern)
 require "graphql/unauthorized_error"
 require "graphql/unauthorized_field_error"
 require "graphql/load_application_object_failed_error"
-require "graphql/dataloader"
 require "graphql/deprecation"
 
 module GraphQL

lib/graphql/execution/interpreter/arguments_cache.rb

@@ -6,17 +6,21 @@ class Interpreter
       class ArgumentsCache
         def initialize(query)
           @query = query
+          @dataloader = query.context.dataloader
           @storage = Hash.new do |h, ast_node|
             h[ast_node] = Hash.new do |h2, arg_owner|
               h2[arg_owner] = Hash.new do |h3, parent_object|
-                # First, normalize all AST or Ruby values to a plain Ruby hash
-                args_hash = prepare_args_hash(ast_node)
-                # Then call into the schema to coerce those incoming values
-                args = arg_owner.coerce_arguments(parent_object, args_hash, query.context)
+                dataload_for(ast_node, arg_owner, parent_object) do |kwarg_arguments|
+                  h3[parent_object] = @query.schema.after_lazy(kwarg_arguments) do |resolved_args|
+                    h3[parent_object] = resolved_args
+                  end
+                end
 
-                h3[parent_object] = @query.schema.after_lazy(args) do |resolved_args|
-                  # when this promise is resolved, update the cache with the resolved value
-                  h3[parent_object] = resolved_args
+                if !h3.key?(parent_object)
+                  # TODO should i bother putting anything here?
+                  h3[parent_object] = NO_ARGUMENTS
+                else
+                  h3[parent_object]
                 end
               end
             end
@@ -25,6 +29,25 @@ def initialize(query)
 
         def fetch(ast_node, argument_owner, parent_object)
           @storage[ast_node][argument_owner][parent_object]
+          # If any jobs were enqueued, run them now,
+          # since this might have been called outside of execution.
+          # (The jobs are responsible for updating `result` in-place.)
+          @dataloader.run
+          # Ack, the _hash_ is updated, but the key is eventually
+          # overridden with an immutable arguments instance.
+          # The first call queues up the job,
+          # then this call fetches the result.
+          # TODO this should be better, find a solution
+          # that works with merging the runtime.rb code
+          @storage[ast_node][argument_owner][parent_object]
+        end
+
+        # @yield [Interpreter::Arguments, Lazy<Interpreter::Arguments>] The finally-loaded arguments
+        def dataload_for(ast_node, argument_owner, parent_object, &block)
+          # First, normalize all AST or Ruby values to a plain Ruby hash
+          args_hash = self.class.prepare_args_hash(@query, ast_node)
+          argument_owner.coerce_arguments(parent_object, args_hash, @query.context, &block)
+          nil
         end
 
         private
@@ -33,35 +56,35 @@ def fetch(ast_node, argument_owner, parent_object)
 
         NO_VALUE_GIVEN = Object.new
 
-        def prepare_args_hash(ast_arg_or_hash_or_value)
+        def self.prepare_args_hash(query, ast_arg_or_hash_or_value)
           case ast_arg_or_hash_or_value
           when Hash
             if ast_arg_or_hash_or_value.empty?
               return NO_ARGUMENTS
             end
             args_hash = {}
             ast_arg_or_hash_or_value.each do |k, v|
-              args_hash[k] = prepare_args_hash(v)
+              args_hash[k] = prepare_args_hash(query, v)
             end
             args_hash
           when Array
-            ast_arg_or_hash_or_value.map { |v| prepare_args_hash(v) }
+            ast_arg_or_hash_or_value.map { |v| prepare_args_hash(query, v) }
           when GraphQL::Language::Nodes::Field, GraphQL::Language::Nodes::InputObject, GraphQL::Language::Nodes::Directive
             if ast_arg_or_hash_or_value.arguments.empty?
               return NO_ARGUMENTS
             end
             args_hash = {}
             ast_arg_or_hash_or_value.arguments.each do |arg|
-              v = prepare_args_hash(arg.value)
+              v = prepare_args_hash(query, arg.value)
               if v != NO_VALUE_GIVEN
                 args_hash[arg.name] = v
               end
             end
             args_hash
           when GraphQL::Language::Nodes::VariableIdentifier
-            if @query.variables.key?(ast_arg_or_hash_or_value.name)
-              variable_value = @query.variables[ast_arg_or_hash_or_value.name]
-              prepare_args_hash(variable_value)
+            if query.variables.key?(ast_arg_or_hash_or_value.name)
+              variable_value = query.variables[ast_arg_or_hash_or_value.name]
+              prepare_args_hash(query, variable_value)
             else
               NO_VALUE_GIVEN
             end

lib/graphql/execution/interpreter/resolve.rb

@@ -25,6 +25,12 @@ def self.resolve_all(results, dataloader)
         #
         # @return [void]
         def self.resolve(results, dataloader)
+          # There might be pending jobs here that _will_ write lazies
+          # into the result hash. We should run them out, so we
+          # can be sure that all lazies will be present in the result hashes.
+          # A better implementation would somehow interleave (or unify)
+          # these approaches.
+          dataloader.run
           next_results = []
           while results.any?
             result_value = results.shift
@@ -35,17 +41,16 @@ def self.resolve(results, dataloader)
               results.concat(result_value)
               next
             elsif result_value.is_a?(Lazy)
-              result_value = result_value.value
-            end
-
-            if result_value.is_a?(Lazy)
-              # Since this field returned another lazy,
-              # add it to the same queue
-              results << result_value
-            elsif result_value.is_a?(Hash) || result_value.is_a?(Array)
-              # Add these values in wholesale --
-              # they might be modified by later work in the dataloader.
-              next_results << result_value
+              loaded_value = result_value.value
+              if loaded_value.is_a?(Lazy)
+                # Since this field returned another lazy,
+                # add it to the same queue
+                results << loaded_value
+              elsif loaded_value.is_a?(Hash) || loaded_value.is_a?(Array)
+                # Add these values in wholesale --
+                # they might be modified by later work in the dataloader.
+                next_results << loaded_value
+              end
             end
           end
 

lib/graphql/execution/interpreter/runtime.rb

@@ -196,13 +196,21 @@ def evaluate_selection(path, result_name, field_ast_nodes_or_ast_node, scoped_co
             object = authorized_new(field_defn.owner, object, context, next_path)
           end
 
-          begin
-            kwarg_arguments = arguments(object, field_defn, ast_node)
-          rescue GraphQL::ExecutionError, GraphQL::UnauthorizedError => e
-            continue_value(next_path, e, owner_type, field_defn, return_type.non_null?, ast_node)
-            return
+          total_args_count = field_defn.arguments.size
+          if total_args_count == 0
+            kwarg_arguments = GraphQL::Execution::Interpreter::Arguments::EMPTY
+            evaluate_selection_with_args(kwarg_arguments, field_defn, next_path, ast_node, field_ast_nodes, scoped_context, owner_type, object, is_eager_field)
+          else
+            # TODO remove all arguments(...) usages?
+            @query.arguments_cache.dataload_for(ast_node, field_defn, object) do |resolved_arguments|
+              evaluate_selection_with_args(resolved_arguments, field_defn, next_path, ast_node, field_ast_nodes, scoped_context, owner_type, object, is_eager_field)
+            end
           end
+        end
 
+        def evaluate_selection_with_args(kwarg_arguments, field_defn, next_path, ast_node, field_ast_nodes, scoped_context, owner_type, object, is_eager_field)  # rubocop:disable Metrics/ParameterLists
+          context.scoped_context = scoped_context
+          return_type = field_defn.type
           after_lazy(kwarg_arguments, owner: owner_type, field: field_defn, path: next_path, ast_node: ast_node, scoped_context: context.scoped_context, owner_object: object, arguments: kwarg_arguments) do |resolved_arguments|
             if resolved_arguments.is_a?(GraphQL::ExecutionError) || resolved_arguments.is_a?(GraphQL::UnauthorizedError)
               continue_value(next_path, resolved_arguments, owner_type, field_defn, return_type.non_null?, ast_node)
@@ -283,9 +291,10 @@ def evaluate_selection(path, result_name, field_ast_nodes_or_ast_node, scoped_co
             # (Subselections of this mutation will still be resolved level-by-level.)
             if is_eager_field
               Interpreter::Resolve.resolve_all([field_result], @dataloader)
+            else
+              # Return this from `after_lazy` because it might be another lazy that needs to be resolved
+              field_result
             end
-
-            nil
           end
         end
 

lib/graphql/query.rb

@@ -251,12 +251,18 @@ def irep_selection
     # @param parent_object [GraphQL::Schema::Object]
     # @return Hash{Symbol => Object}
     def arguments_for(ast_node, definition, parent_object: nil)
+      if interpreter?
+        arguments_cache.fetch(ast_node, definition, parent_object)
+      else
+        arguments_cache[ast_node][definition]
+      end
+    end
+
+    def arguments_cache
       if interpreter?
         @arguments_cache ||= Execution::Interpreter::ArgumentsCache.new(self)
-        @arguments_cache.fetch(ast_node, definition, parent_object)
       else
         @arguments_cache ||= ArgumentsCache.build(self)
-        @arguments_cache[ast_node][definition]
       end
     end
 

lib/graphql/query/context.rb

@@ -157,7 +157,7 @@ def initialize(query:, schema: query.schema, values:, object:)
       end
 
       def dataloader
-        @dataloader ||= query.multiplex.dataloader
+        @dataloader ||= query.multiplex ? query.multiplex.dataloader : schema.dataloader_class.new
       end
 
       # @api private

lib/graphql/query/null_context.rb

@@ -9,10 +9,11 @@ def visible_field?(t); true; end
         def visible_type?(t); true; end
       end
 
-      attr_reader :schema, :query, :warden
+      attr_reader :schema, :query, :warden, :dataloader
 
       def initialize
         @query = nil
+        @dataloader = GraphQL::Dataloader::NullDataloader.new
         @schema = GraphQL::Schema.new
         @warden = NullWarden.new(
           GraphQL::Filter.new,
@@ -36,7 +37,7 @@ def instance
           @instance = self.new
         end
 
-        def_delegators :instance, :query, :schema, :warden, :interpreter?
+        def_delegators :instance, :query, :schema, :warden, :interpreter?, :dataloader
       end
     end
   end

lib/graphql/schema/argument.rb

@@ -236,6 +236,67 @@ def prepare_value(obj, value, context: nil)
         end
       end
 
+      # @api private
+      def coerce_into_values(parent_object, values, context, argument_values)
+        arg_name = graphql_name
+        arg_key = keyword
+        has_value = false
+        default_used = false
+        if values.key?(arg_name)
+          has_value = true
+          value = values[arg_name]
+        elsif values.key?(arg_key)
+          has_value = true
+          value = values[arg_key]
+        elsif default_value?
+          has_value = true
+          value = default_value
+          default_used = true
+        end
+
+        if has_value
+          loaded_value = nil
+          coerced_value = context.schema.error_handler.with_error_handling(context) do
+            type.coerce_input(value, context)
+          end
+
+          # TODO this should probably be inside after_lazy
+          if loads && !from_resolver?
+            loaded_value = if type.list?
+              loaded_values = coerced_value.map { |val| owner.load_application_object(self, loads, val, context) }
+              context.schema.after_any_lazies(loaded_values) { |result| result }
+            else
+              owner.load_application_object(self, loads, coerced_value, context)
+            end
+          end
+
+          coerced_value = if loaded_value
+            loaded_value
+          else
+            coerced_value
+          end
+
+          # If this isn't lazy, then the block returns eagerly and assigns the result here
+          # If it _is_ lazy, then we write the lazy to the hash, then update it later
+          argument_values[arg_key] = context.schema.after_lazy(coerced_value) do |coerced_value|
+            owner.validate_directive_argument(self, coerced_value)
+            prepared_value = context.schema.error_handler.with_error_handling(context) do
+              prepare_value(parent_object, coerced_value, context: context)
+            end
+
+            # TODO code smell to access such a deeply-nested constant in a distant module
+            argument_values[arg_key] = GraphQL::Execution::Interpreter::ArgumentValue.new(
+              value: prepared_value,
+              definition: self,
+              default_used: default_used,
+            )
+          end
+        else
+          # has_value is false
+          owner.validate_directive_argument(self, nil)
+        end
+      end
+
       private
 
       def validate_input_type(input_type)

lib/graphql/schema/field.rb

@@ -60,6 +60,10 @@ def introspection?
         @introspection
       end
 
+      def inspect
+        "#<#{self.class} #{path}#{arguments.any? ? "(...)" : ""}: #{type.to_type_signature}>"
+      end
+
       alias :mutation :resolver
 
       # @return [Boolean] Apply tracing to this field? (Default: skip scalars, this is the override value)

lib/graphql/schema/input_object.rb

@@ -214,8 +214,12 @@ def coerce_input(value, ctx)
           arguments = coerce_arguments(nil, value, ctx)
 
           ctx.schema.after_lazy(arguments) do |resolved_arguments|
-            input_obj_instance = self.new(resolved_arguments, ruby_kwargs: resolved_arguments.keyword_arguments, context: ctx, defaults_used: nil)
-            input_obj_instance.prepare
+            if resolved_arguments.is_a?(GraphQL::Error)
+              raise resolved_arguments
+            else
+              input_obj_instance = self.new(resolved_arguments, ruby_kwargs: resolved_arguments.keyword_arguments, context: ctx, defaults_used: nil)
+              input_obj_instance.prepare
+            end
           end
         end