Non-nullable fields that raise an ExecutionError cause an uncaught exception

[cjoudrey]

While investigating an issue I noticed that when using graphql-batch, a non-nullable field that raises a GraphQL::ExecutionError causes an uncaught GraphQL::InvalidNullError exception.

I have yet to find the exact commit in graphql-ruby that caused this regression, but it's reproducible with the following diff:

diff --git a/test/batch_test.rb b/test/batch_test.rb
index daf48f0..4234c92 100644
--- a/test/batch_test.rb
+++ b/test/batch_test.rb
@@ -79,6 +79,22 @@ class GraphQL::BatchTest < Minitest::Test
     assert_equal ["Product/123"], queries
   end

+  def test_non_null_that_raises
+    query_string = <<-GRAPHQL
+      {
+        product(id: "1") {
+          id
+          nonNullButRaises {
+            id
+          }
+        }
+      }
+    GRAPHQL
+    result = Schema.execute(query_string)
+  end
+
   def test_batched_association_preload
     query_string = <<-GRAPHQL
       {
diff --git a/test/support/schema.rb b/test/support/schema.rb
index c6eb843..aa97340 100644
--- a/test/support/schema.rb
+++ b/test/support/schema.rb
@@ -55,6 +55,13 @@ ProductType = GraphQL::ObjectType.define do
       Promise.all([query]).then { query.value.size }
     }
   end
+
+  field :nonNullButRaises do
+    type !ImageType
+    resolve -> (_, _, _) {
+      raise GraphQL::ExecutionError, 'Error'
+    }
+  end

At first I thought it was graphql-ruby that didn't handle this case correctly, but then I tried it out and it worked as expected.

I believe in this case you'd expect result to be something along the lines of:

{
  "data": {
    "products": null
  },
  "errors": [
    ... 
  ]
}

@dylanahsmith any idea? If not, I can take a look later this week.

[dylanahsmith]

Yup, this was caused by https://github.com/rmosolgo/graphql-ruby/pull/174/files#diff-a66f6e348162887250fe0e5939b3a11bL23 which handles the InvalidNullError in FieldResolution#result instead of in FieldResolution#get_finished_value

If I move that rescue into FieldResolution#get_finished_value then the test passes

diff --git a/lib/graphql/query/serial_execution/field_resolution.rb b/lib/graphql/query/serial_execution/field_resolution.rb
index c4daffe..349ae76 100644
--- a/lib/graphql/query/serial_execution/field_resolution.rb
+++ b/lib/graphql/query/serial_execution/field_resolution.rb
@@ -15,17 +15,8 @@ module GraphQL

         def result
           result_name = irep_node.name
-          begin
-            raw_value = get_raw_value
-            { result_name => get_finished_value(raw_value) }
-          rescue GraphQL::InvalidNullError => err
-            if field.type.kind.non_null?
-              raise(err)
-            else
-              err.parent_error? || execution_context.add_error(err)
-              {result_name => nil}
-            end
-          end
+          raw_value = get_raw_value
+          { result_name => get_finished_value(raw_value) }
         end

         private
@@ -41,7 +32,16 @@ module GraphQL

           strategy_class = GraphQL::Query::SerialExecution::ValueResolution.get_strategy_for_kind(field.type.kind)
           result_strategy = strategy_class.new(raw_value, field.type, target, parent_type, irep_node, execution_context)
-          result_strategy.result
+          begin
+            result_strategy.result
+          rescue GraphQL::InvalidNullError => err
+            if field.type.kind.non_null?
+              raise(err)
+            else
+              err.parent_error? || execution_context.add_error(err)
+              nil
+            end
+          end
         end

I will write up a PR to propose that change upstream, since I would prefer to not monkey patch yet another method.

[rmosolgo]

I'm interested in any upstream changes that would make this less likely in the future, but I'm not sure what they would be :( do you have any insight into that? (Of course, feel free to discuss over there if you prefer)

[dylanahsmith]

I opened rmosolgo/graphql-ruby#296 upstream.

Once we get this issue fixed, we should definitely include that regression test.

[dylanahsmith]

@cjoudrey could you create a PR with your regression test now that the upstream PR has been merged and released?

[cjoudrey]

Absolutely!