Merge pull request #5257 from rmosolgo/fix-invalid-null-error-handling

Improve default InvalidNullError handling

Author: rmosolgo

lib/graphql/execution/interpreter/runtime.rb

@@ -473,7 +473,7 @@ def continue_value(value, field, is_non_null, ast_node, result_name, selection_r
                 # When this comes from a list item, use the parent object:
                 parent_type = selection_result.is_a?(GraphQLResultArray) ? selection_result.graphql_parent.graphql_result_type : selection_result.graphql_result_type
                 # This block is called if `result_name` is not dead. (Maybe a previous invalid nil caused it be marked dead.)
-                err = parent_type::InvalidNullError.new(parent_type, field, value)
+                err = parent_type::InvalidNullError.new(parent_type, field, value, ast_node)
                 schema.type_error(err, context)
               end
             else

lib/graphql/invalid_null_error.rb

@@ -2,7 +2,7 @@
 module GraphQL
   # Raised automatically when a field's resolve function returns `nil`
   # for a non-null field.
-  class InvalidNullError < GraphQL::RuntimeTypeError
+  class InvalidNullError < GraphQL::Error
     # @return [GraphQL::BaseType] The owner of {#field}
     attr_reader :parent_type
 
@@ -12,23 +12,17 @@ class InvalidNullError < GraphQL::RuntimeTypeError
     # @return [nil, GraphQL::ExecutionError] The invalid value for this field
     attr_reader :value
 
-    def initialize(parent_type, field, value)
+    # @return [GraphQL::Language::Nodes::Field] the field where the error occurred
+    attr_reader :ast_node
+
+    def initialize(parent_type, field, value, ast_node)
       @parent_type = parent_type
       @field = field
       @value = value
+      @ast_node = ast_node
       super("Cannot return null for non-nullable field #{@parent_type.graphql_name}.#{@field.graphql_name}")
     end
 
-    # @return [Hash] An entry for the response's "errors" key
-    def to_h
-      { "message" => message }
-    end
-
-    # @deprecated always false
-    def parent_error?
-      false
-    end
-
     class << self
       attr_accessor :parent_class
 

lib/graphql/schema.rb

@@ -1298,7 +1298,10 @@ def unauthorized_field(unauthorized_error)
       def type_error(type_error, ctx)
         case type_error
         when GraphQL::InvalidNullError
-          ctx.errors << type_error
+          execution_error = GraphQL::ExecutionError.new(type_error.message, ast_node: type_error.ast_node)
+          execution_error.path = ctx[:current_path]
+
+          ctx.errors << execution_error
         when GraphQL::UnresolvedTypeError, GraphQL::StringEncodingError, GraphQL::IntegerEncodingError
           raise type_error
         when GraphQL::IntegerDecodingError

spec/graphql/execution/errors_spec.rb

@@ -307,15 +307,19 @@ def non_nullable_array
       it "outputs the appropriate error message when using non-interpreter schema" do
         res = ErrorsTestSchemaWithoutInterpreter.execute("{ nonNullableArray }")
         expected_error = {
-          "message" => "Cannot return null for non-nullable field Query.nonNullableArray"
+          "message" => "Cannot return null for non-nullable field Query.nonNullableArray",
+          "path" => ["nonNullableArray", 0],
+          "locations" => [{ "line" => 1, "column" => 3 }]
         }
         assert_equal({ "data" => nil, "errors" => [expected_error] }, res)
       end
 
       it "outputs the appropriate error message when using interpreter schema" do
         res = ErrorsTestSchema.execute("{ nonNullableArray }")
         expected_error = {
-          "message" => "Cannot return null for non-nullable field Query.nonNullableArray"
+          "message" => "Cannot return null for non-nullable field Query.nonNullableArray",
+          "path" => ["nonNullableArray", 0],
+          "locations" => [{ "line" => 1, "column" => 3 }]
         }
         assert_equal({ "data" => nil, "errors" => [expected_error] }, res)
       end

spec/graphql/execution/multiplex_spec.rb

@@ -114,7 +114,11 @@ def multiplex(*a, **kw)
         },
         {
           "data"=>{"invalidNestedNull"=>{"value" => 2,"nullableNestedSum" => nil}},
-          "errors"=>[{"message"=>"Cannot return null for non-nullable field LazySum.nestedSum"}],
+          "errors"=>[{
+            "message"=>"Cannot return null for non-nullable field LazySum.nestedSum",
+            "path"=>["invalidNestedNull", "nullableNestedSum", "nestedSum"],
+            "locations"=>[{"line"=>5, "column"=>11}],
+          }],
         },
         {
           "errors" => [{

spec/graphql/non_null_type_spec.rb

@@ -7,7 +7,11 @@
       query_string = %|{ cow { name cantBeNullButIs } }|
       result = Dummy::Schema.execute(query_string)
       assert_equal({"cow" => nil }, result["data"])
-      assert_equal([{"message"=>"Cannot return null for non-nullable field Cow.cantBeNullButIs"}], result["errors"])
+      assert_equal([{
+        "message" => "Cannot return null for non-nullable field Cow.cantBeNullButIs",
+        "path" => ["cow", "cantBeNullButIs"],
+        "locations" => [{"line" => 1, "column" => 14}],
+      }], result["errors"])
     end
 
     it "propagates the null up to the next nullable field" do
@@ -26,7 +30,11 @@
       |
       result = Dummy::Schema.execute(query_string)
       assert_nil(result["data"])
-      assert_equal([{"message"=>"Cannot return null for non-nullable field DeepNonNull.nonNullInt"}], result["errors"])
+      assert_equal([{
+        "message" => "Cannot return null for non-nullable field DeepNonNull.nonNullInt",
+        "path" => ["nn1", "nn2", "nn3", "nni3"],
+        "locations" => [{"line" => 8, "column" => 15}],
+      }], result["errors"])
     end
 
     describe "when type_error is configured to raise an error" do

spec/graphql/query/executor_spec.rb

@@ -147,7 +147,9 @@
           "data" => { "cow" => nil },
           "errors" => [
             {
-              "message" => "Cannot return null for non-nullable field Cow.cantBeNullButIs"
+              "message" => "Cannot return null for non-nullable field Cow.cantBeNullButIs",
+              "path" => ["cow", "cantBeNullButIs"],
+              "locations" => [{ "line" => 1, "column" => 28 }]
             }
           ]
         }

spec/graphql/schema/mutation_spec.rb

@@ -113,9 +113,6 @@
       query_str = "mutation { returnInvalidNull { int } }"
       response = Jazz::Schema.execute(query_str)
       assert_equal ["Cannot return null for non-nullable field ReturnInvalidNullPayload.int"], response["errors"].map { |e| e["message"] }
-      error = response.query.context.errors.first
-      assert_instance_of Jazz::ReturnInvalidNull.payload_type::InvalidNullError, error
-      assert_equal "Jazz::ReturnInvalidNull::ReturnInvalidNullPayload::InvalidNullError", error.class.inspect
     end
   end