Merge pull request #4052 from rmosolgo/list-each-error-handling

Rescue errors raised by calling .each on list values

lib/graphql/execution/interpreter/runtime.rb

@@ -754,11 +754,16 @@ def continue_field(path, value, owner_type, field, current_type, ast_node, next_
             response_list = GraphQLResultArray.new(result_name, selection_result)
             response_list.graphql_non_null_list_items = inner_type.non_null?
             set_result(selection_result, result_name, response_list)
-
+            result_was_set = false
             idx = 0
-            begin
+            list_value = begin
               value.each do |inner_value|
                 break if dead_result?(response_list)
+                if !result_was_set
+                  # Don't set the result unless `.each` is successful
+                  set_result(selection_result, result_name, response_list)
+                  result_was_set = true
+                end
                 next_path = path.dup
                 next_path << idx
                 this_idx = idx
@@ -772,6 +777,13 @@ def continue_field(path, value, owner_type, field, current_type, ast_node, next_
                   resolve_list_item(inner_value, inner_type, next_path, ast_node, field, owner_object, arguments, this_idx, response_list, next_selections, owner_type)
                 end
               end
+              # Maybe the list was empty and the block was never called.
+              if !result_was_set
+                set_result(selection_result, result_name, response_list)
+                result_was_set = true
+              end
+
+              response_list
             rescue NoMethodError => err
               # Ruby 2.2 doesn't have NoMethodError#receiver, can't check that one in this case. (It's been EOL since 2017.)
               if err.name == :each && (err.respond_to?(:receiver) ? err.receiver == value : true)
@@ -781,9 +793,17 @@ def continue_field(path, value, owner_type, field, current_type, ast_node, next_
                 # This was some other NoMethodError -- let it bubble to reveal the real error.
                 raise
               end
+            rescue GraphQL::ExecutionError, GraphQL::UnauthorizedError => ex_err
+              ex_err
+            rescue StandardError => err
+              begin
+                query.handle_or_reraise(err)
+              rescue GraphQL::ExecutionError => ex_err
+                ex_err
+              end
             end
 
-            response_list
+            continue_value(path, list_value, owner_type, field, inner_type.non_null?, ast_node, result_name, selection_result)
           else
             raise "Invariant: Unhandled type kind #{current_type.kind} (#{current_type})"
           end

spec/graphql/execution/errors_spec.rb

@@ -51,6 +51,11 @@ class ErrorBGrandchildClass < ErrorBChildClass; end
       err.value
     end
 
+    class ErrorList < Array
+      def each
+        raise ErrorB
+      end
+    end
 
     class Thing < GraphQL::Schema::Object
       def self.authorized?(obj, ctx)
@@ -157,6 +162,12 @@ def thing
       def non_nullable_array
         [nil]
       end
+
+      field :error_in_each, [Int]
+
+      def error_in_each
+        ErrorList.new
+      end
     end
 
     query(Query)
@@ -309,5 +320,13 @@ def non_nullable_array
         assert_equal({ "data" => nil, "errors" => [expected_error] }, res)
       end
     end
+
+    describe "when .each on a list type raises an error" do
+      it "rescues it properly" do
+        res = ErrorsTestSchema.execute("{ __typename errorInEach }")
+        expected_error = { "message" => "boom!", "locations"=>[{"line"=>1, "column"=>14}], "path"=>["errorInEach"] }
+        assert_equal({ "data" => { "__typename" => "Query", "errorInEach" => nil }, "errors" => [expected_error] }, res)
+      end
+    end
   end
 end