Guided error formatter

Author: gmac

lib/graphql/cardinal/executor.rb

@@ -5,13 +5,12 @@
 require_relative "./executor/authorization"
 require_relative "./executor/hot_paths"
 require_relative "./executor/response_hash"
-require_relative "./executor/error_formatting"
+require_relative "./executor/error_formatter"
 
 module GraphQL
   module Cardinal
     class Executor
       include HotPaths
-      include ErrorFormatting
 
       TYPENAME_FIELD = "__typename"
       TYPENAME_FIELD_RESOLVER = TypenameResolver.new
@@ -28,7 +27,6 @@ def initialize(schema, resolvers, document, root_object, variables: {}, context:
         @context = context
         @data = {}
         @errors = []
-        @path = []
         @exec_queue = []
         @exec_count = 0
         @context[:query] = @query
@@ -69,7 +67,7 @@ def perform
           execute_scope(@exec_queue.shift) until @exec_queue.empty?
         end
 
-        response = { "data" => @errors.empty? ? @data : format_inline_errors(@data, @errors) }
+        response = { "data" => @errors.empty? ? @data : ErrorFormatter.new(@query, @data, @errors).perform }
         response["errors"] = @errors.map(&:to_h) unless @errors.empty?
         response
       end
@@ -78,14 +76,13 @@ def perform
 
       def execute_scope(exec_scope)
         unless exec_scope.fields
-          lazy_field_keys = []
           exec_scope.fields = execution_fields_by_key(exec_scope.parent_type, exec_scope.selections)
           exec_scope.fields.each_value do |exec_field|
-            @path.push(exec_field.key)
             parent_type = exec_scope.parent_type
             parent_sources = exec_scope.sources
             field_name = exec_field.name
 
+            exec_field.scope = exec_scope
             exec_field.type = @query.get_field(parent_type, field_name).type
             value_type = exec_field.type.unwrap
 
@@ -94,71 +91,61 @@ def execute_scope(exec_scope)
               if field_name == TYPENAME_FIELD
                 field_resolver = TYPENAME_FIELD_RESOLVER
               else
-                raise NotImplementedError, "No field resolver for `#{parent_type.graphql_name}.#{field_name}`"
+                raise NotImplementedError, "No field resolver for '#{parent_type.graphql_name}.#{field_name}'"
               end
             end
 
-            resolved_sources = if !field_resolver.authorized?(@context)
-              @errors << AuthorizationError.new(type_name: parent_type.graphql_name, field_name: field_name, path: @path.dup, base: true)
+            exec_field.result = if !field_resolver.authorized?(@context)
+              @errors << AuthorizationError.new(type_name: parent_type.graphql_name, field_name: field_name, path: exec_field.path, base: true)
               Array.new(parent_sources.length, @errors.last)
             elsif !Authorization.can_access_type?(value_type, @context)
-              @errors << AuthorizationError.new(type_name: value_type.graphql_name, path: @path.dup, base: true)
+              @errors << AuthorizationError.new(type_name: value_type.graphql_name, path: exec_field.path, base: true)
               Array.new(parent_sources.length, @errors.last)
             else
               begin
                 @tracers.each { _1.before_resolve_field(parent_type, field_name, parent_sources.length, @context) }
                 field_resolver.resolve(parent_sources, exec_field.arguments(@variables), @context, exec_scope)
               rescue StandardError => e
-                report_exception(error: e)
-                @errors << InternalError.new(path: @path.dup, base: true)
+                report_exception(error: e, field: exec_field)
+                @errors << InternalError.new(path: exec_field.path, base: true)
                 Array.new(parent_sources.length, @errors.last)
               ensure
                 @tracers.each { _1.after_resolve_field(parent_type, field_name, parent_sources.length, @context) }
                 @exec_count += 1
               end
             end
-
-            if resolved_sources.is_a?(Promise)
-              exec_field.promise = resolved_sources
-              lazy_field_keys << exec_field.key
-            else
-              resolve_execution_field(exec_scope, exec_field, resolved_sources, lazy_field_keys)
-              lazy_field_keys.clear
-            end
-
-            @path.pop
           end
         end
 
-        if exec_scope.lazy_fields_pending?
+        if exec_scope.lazy_fields?
           if exec_scope.lazy_fields_ready?
-            exec_scope.method(:lazy_exec!).call # << noop for loaders that have already run
+            exec_scope.send(:lazy_exec!) # << noop for loaders that have already run
             exec_scope.fields.each_value do |exec_field|
-              next unless exec_field.promise
-
-              @path.push(exec_field.key)
-              resolve_execution_field(exec_scope, exec_field, exec_field.promise.value)
-              @path.pop
+              sources = exec_field.result.is_a?(Promise) ? exec_field.result.value : exec_field.result
+              resolve_execution_field(exec_field, sources)
             end
           else
             # requeue the scope to wait on others that haven't built fields yet
             @exec_queue << exec_scope
           end
+        else
+          exec_scope.fields.each_value do |exec_field|
+            resolve_execution_field(exec_field, exec_field.result)
+          end
         end
 
         nil
       end
 
-      def resolve_execution_field(exec_scope, exec_field, resolved_sources, lazy_field_keys = nil)
-        parent_sources = exec_scope.sources
-        parent_responses = exec_scope.responses
+      def resolve_execution_field(exec_field, resolved_sources)
+        parent_sources = exec_field.scope.sources
+        parent_responses = exec_field.scope.responses
         field_key = exec_field.key
-        field_name = exec_field.name
         field_type = exec_field.type
         return_type = field_type.unwrap
 
         if resolved_sources.length != parent_sources.length
-          report_exception("Incorrect number of results resolved. Expected #{parent_sources.length}, got #{resolved_sources.length}")
+          report_exception("Incorrect number of results resolved. Expected #{parent_sources.length}, got #{resolved_sources.length}", field: exec_field)
           resolved_sources = Array.new(parent_sources.length, nil)
         end
 
@@ -168,9 +155,7 @@ def resolve_execution_field(exec_scope, exec_field, resolved_sources, lazy_field
           next_responses = []
           resolved_sources.each_with_index do |source, i|
             # DANGER: HOT PATH!
-            response = parent_responses[i]
-            lazy_field_keys.each { |k| response[k] = nil } if lazy_field_keys && !lazy_field_keys.empty?
-            response[field_key] = build_composite_response(field_type, source, next_sources, next_responses)
+            parent_responses[i][field_key] = build_composite_response(exec_field, field_type, source, next_sources, next_responses)
           end
 
           if return_type.kind.abstract?
@@ -184,7 +169,7 @@ def resolve_execution_field(exec_scope, exec_field, resolved_sources, lazy_field
             next_sources.each_with_index do |source, i|
               # DANGER: HOT PATH!
               impl_type = type_resolver.call(source, @context)
-              next_sources_by_type[impl_type] << (field_name == TYPENAME_FIELD ? impl_type.graphql_name : source)
+              next_sources_by_type[impl_type] << (exec_field.name == TYPENAME_FIELD ? impl_type.graphql_name : source)
               next_responses_by_type[impl_type] << next_responses[i].tap { |r| r.typename = impl_type.graphql_name }
             end
 
@@ -193,7 +178,7 @@ def resolve_execution_field(exec_scope, exec_field, resolved_sources, lazy_field
             next_sources_by_type.each do |impl_type, impl_type_sources|
               # check concrete type access only once per resolved type...
               unless Authorization.can_access_type?(impl_type, @context)
-                @errors << AuthorizationError.new(type_name: impl_type.graphql_name, path: @path.dup, base: true)
+                @errors << AuthorizationError.new(type_name: impl_type.graphql_name, path: exec_field.path, base: true)
                 impl_type_sources = Array.new(impl_type_sources.length, @errors.last)
               end
 
@@ -204,7 +189,8 @@ def resolve_execution_field(exec_scope, exec_field, resolved_sources, lazy_field
                 responses: next_responses_by_type[impl_type],
                 loader_cache: loader_cache,
                 loader_group: loader_group,
-                parent: exec_scope,
+                path: exec_field.path,
+                parent: exec_field.scope,
               )
             end
 
@@ -215,17 +201,16 @@ def resolve_execution_field(exec_scope, exec_field, resolved_sources, lazy_field
               selections: exec_field.selections,
               sources: next_sources,
               responses: next_responses,
-              parent: exec_scope,
+              path: exec_field.path,
+              parent: exec_field.scope,
             )
           end
         else
           # build leaf results
           resolved_sources.each_with_index do |val, i|
             # DANGER: HOT PATH!
-            response = parent_responses[i]
-            lazy_field_keys.each { |k| response[k] = nil } if lazy_field_keys && !lazy_field_keys.empty?
-            response[field_key] = if val.nil? || val.is_a?(StandardError)
-              build_missing_value(field_type, val)
+            parent_responses[i][field_key] = if val.nil? || val.is_a?(StandardError)
+              build_missing_value(exec_field, field_type, val)
             elsif return_type.kind.scalar?
               coerce_scalar_value(return_type, val)
             elsif return_type.kind.enum?
@@ -254,7 +239,7 @@ def execution_fields_by_key(parent_type, selections, map: Hash.new { |h, k| h[k]
             fragment = @query.fragments[node.name]
             fragment_type = @query.get_type(fragment.type.name)
             if @query.possible_types(fragment_type).include?(parent_type)
-              execution_fields_by_key(parent_type, node.selections, map: map)
+              execution_fields_by_key(parent_type, fragment.selections, map: map)
             end
 
           else
@@ -286,9 +271,9 @@ def if_argument?(bool_arg)
         end
       end
 
-      def report_exception(message = nil, error: nil, path: @path.dup)
+      def report_exception(message = nil, error: nil, field: nil)
         # todo: add real error reporting...
-        puts "Error at #{path.join(".")}: #{message || error&.message}"
+        puts "Error at #{field.path.join(".")}: #{message || error&.message}" if field
         puts error.backtrace.join("\n") if error
       end
     end

…ql/cardinal/executor/error_formatting.rb …hql/cardinal/executor/error_formatter.rblib/graphql/cardinal/executor/error_formatting.rb renamed to lib/graphql/cardinal/executor/error_formatter.rb lib/graphql/cardinal/executor/error_formatting.rb renamed to lib/graphql/cardinal/executor/error_formatter.rb

@@ -3,35 +3,51 @@
 
 module GraphQL::Cardinal
   class Executor
-    module ErrorFormatting
-      private
+    class ErrorFormatter
+      def initialize(query, data, errors)
+        @query = query
+        @data = data
+        @target_paths = errors.map(&:path).tap(&:compact!).tap(&:uniq!)
+        @selection_path = []
+        @actual_path = []
+      end
+
+      def perform
+        return @data if @target_paths.empty?
 
-      def format_inline_errors(data, _errors)
-        # todo: make this smarter to only traverse down actual error paths
-        @path = []
         propagate_object_scope_errors(
-          data,
+          @data,
           @query.root_type_for_operation(@query.selected_operation.operation_type),
           @query.selected_operation.selections,
         )
       end
 
+      private
+
       def propagate_object_scope_errors(raw_object, parent_type, selections)
         return nil if raw_object.nil?
 
         selections.each do |node|
           case node
           when GraphQL::Language::Nodes::Field
-            field_name = node.alias || node.name
-            @path << field_name
+            field_key = node.alias || node.name
+
+            return raw_object unless @target_paths.any? do |target_path|
+              target_path[@selection_path.length] == field_key && @selection_path.each_with_index.all? do |part, i|
+                part == target_path[i]
+              end
+            end
+
+            @selection_path << field_key
+            @actual_path << field_key
 
             begin
               node_type = @query.get_field(parent_type, node.name).type
               named_type = node_type.unwrap
-              raw_value = raw_object[field_name]
+              raw_value = raw_object[field_key]
 
-              raw_object[field_name] = if raw_value.is_a?(ExecutionError)
-                raw_value.replace_path(@path.dup) unless raw_value.base_error?
+              raw_object[field_key] = if raw_value.is_a?(ExecutionError)
+                raw_value.replace_path(@actual_path.dup) unless raw_value.base_error?
                 nil
               elsif node_type.list?
                 node_type = node_type.of_type while node_type.non_null?
@@ -42,9 +58,10 @@ def propagate_object_scope_errors(raw_object, parent_type, selections)
                 propagate_object_scope_errors(raw_value, named_type, node.selections)
               end
 
-              return nil if node_type.non_null? && raw_object[field_name].nil?
+              return nil if node_type.non_null? && raw_object[field_key].nil?
             ensure
-              @path.pop
+              @selection_path.pop
+              @actual_path.pop
             end
 
           when GraphQL::Language::Nodes::InlineFragment
@@ -79,7 +96,7 @@ def propagate_list_scope_errors(raw_list, current_node_type, selections)
         contains_null = false
 
         resolved_list = raw_list.map!.with_index do |raw_list_element, index|
-          @path << index
+          @actual_path << index
 
           begin
             result = if next_node_type.list?
@@ -97,7 +114,7 @@ def propagate_list_scope_errors(raw_list, current_node_type, selections)
 
             result
           ensure
-            @path.pop
+            @actual_path.pop
           end
         end
 

lib/graphql/cardinal/executor/execution_field.rb

@@ -4,20 +4,28 @@ module GraphQL::Cardinal
   class Executor
     class ExecutionField
       attr_reader :key, :node
-      attr_accessor :type, :promise
+      attr_accessor :scope, :type, :result
 
-      def initialize(key)
+      def initialize(key, scope = nil)
         @key = key.freeze
+        @scope = scope
+        @name = nil
         @node = nil
         @nodes = nil
+        @type = nil
+        @result = nil
         @arguments = nil
-        @promise = nil
+        @path = nil
       end
 
       def name
         @name ||= @node.name.freeze
       end
 
+      def path
+        @path ||= (@scope ? [*@scope.path, @key] : []).freeze
+      end
+
       def add_node(n)
         if !@node
           @node = n

lib/graphql/cardinal/executor/execution_scope.rb

@@ -3,7 +3,7 @@
 module GraphQL::Cardinal
   class Executor
     class ExecutionScope
-      attr_reader :parent_type, :selections, :sources, :responses, :parent
+      attr_reader :parent_type, :selections, :sources, :responses, :path, :parent
       attr_accessor :fields
 
       def initialize(
@@ -13,6 +13,7 @@ def initialize(
         responses:,
         loader_cache: nil,
         loader_group: nil,
+        path: [],
         parent: nil
       )
         @parent_type = parent_type
@@ -21,6 +22,7 @@ def initialize(
         @responses = responses
         @loader_cache = loader_cache
         @loader_group = loader_group
+        @path = path.freeze
         @parent = parent
         @fields = nil
       end
@@ -30,9 +32,8 @@ def defer(loader_class, keys:, group: nil)
         loader.load(keys)
       end
 
-      # does any field in this scope have a pending promise?
-      def lazy_fields_pending?
-        @fields&.each_value&.any? { _1.promise&.pending? } || false
+      def lazy_fields?
+        @fields&.each_value&.any? { _1.result.is_a?(Promise) } || false
       end
 
       # is this scope ungrouped, or have all scopes in the group built their fields?
@@ -47,7 +48,7 @@ def loader_cache
       end
 
       def lazy_exec!
-        loader_cache.each_value { |loader| loader.method(:lazy_exec!).call }
+        loader_cache.each_value { |loader| loader.send(:lazy_exec!) }
       end
     end
   end