Merge pull request #5301 from fisxoj/mn-nullable-array-element-type-errors

Improve nullable array element type error reporting

Author: rmosolgo

lib/graphql/execution/interpreter/runtime.rb

@@ -475,9 +475,10 @@ def continue_value(value, field, is_non_null, ast_node, result_name, selection_r
             if is_non_null
               set_result(selection_result, result_name, nil, false, is_non_null) do
                 # 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
+                is_from_array = selection_result.is_a?(GraphQLResultArray)
+                parent_type = is_from_array ? 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, ast_node)
+                err = parent_type::InvalidNullError.new(parent_type, field, ast_node, is_from_array: is_from_array)
                 schema.type_error(err, context)
               end
             else

lib/graphql/invalid_null_error.rb

@@ -12,11 +12,24 @@ class InvalidNullError < GraphQL::Error
     # @return [GraphQL::Language::Nodes::Field] the field where the error occurred
     attr_reader :ast_node
 
-    def initialize(parent_type, field, ast_node)
+    # @return [Boolean] indicates an array result caused the error
+    attr_reader :is_from_array
+
+    def initialize(parent_type, field, ast_node, is_from_array: false)
       @parent_type = parent_type
       @field = field
       @ast_node = ast_node
-      super("Cannot return null for non-nullable field #{@parent_type.graphql_name}.#{@field.graphql_name}")
+      @is_from_array = is_from_array
+
+      # For List elements, identify the non-null error is for an
+      # element and the required element type so it's not ambiguous
+      # whether it was caused by a null instead of the list or a
+      # null element.
+      if @is_from_array
+        super("Cannot return null for non-nullable element of type '#{@field.type.of_type.of_type.to_type_signature}' for #{@parent_type.graphql_name}.#{@field.graphql_name}")
+      else
+        super("Cannot return null for non-nullable field #{@parent_type.graphql_name}.#{@field.graphql_name}")
+      end
     end
 
     class << self

spec/graphql/execution/errors_spec.rb

@@ -307,7 +307,7 @@ 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 element of type 'String!' for Query.nonNullableArray",
           "path" => ["nonNullableArray", 0],
           "locations" => [{ "line" => 1, "column" => 3 }]
         }
@@ -317,7 +317,7 @@ def non_nullable_array
       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 element of type 'String!' for Query.nonNullableArray",
           "path" => ["nonNullableArray", 0],
           "locations" => [{ "line" => 1, "column" => 3 }]
         }

spec/graphql/execution/interpreter_spec.rb

@@ -501,7 +501,7 @@ def execute_multiplex(multiplex:)
         }
       }
       GRAPHQL
-      assert_equal ["Cannot return null for non-nullable field Query.find"], res["errors"].map { |e| e["message"] }
+      assert_equal ["Cannot return null for non-nullable element of type 'Entity!' for Query.find"], res["errors"].map { |e| e["message"] }
     end
 
     it "works with lists of unions" do

spec/graphql/schema/list_spec.rb

@@ -116,6 +116,12 @@ def echoes(items:)
         def nil_echoes(items:)
           items.first[:items]
         end
+
+        field :invalid_result, [String], null: false
+
+        def invalid_result
+          ["A", "B", nil]
+        end
       end
 
       query(Query)
@@ -127,6 +133,12 @@ def nil_echoes(items:)
       assert_equal [expected_error], res["errors"].map { |e| e["message"] }
     end
 
+    it "reports when an element must be non-nil" do
+      res = ListValidationSchema.execute "{ invalidResult }"
+      expected_error = "Cannot return null for non-nullable element of type 'String!' for Query.invalidResult"
+      assert_equal [expected_error], res["errors"].map { |e| e["message"] }
+    end
+
     it "works with #valid_input?" do
       assert ListValidationSchema::Item.to_list_type.valid_isolated_input?(["A", "B"])
       refute ListValidationSchema::Item.to_list_type.valid_isolated_input?(["A", "B", "C"])