Merge pull request #2997 from rmosolgo/mutation-invalid-null-error

Add invalid null error for mutations

Author: Robert Mosolgo

lib/graphql.rb

@@ -91,13 +91,13 @@ def -@
 require "graphql/tracing"
 require "graphql/dig"
 require "graphql/execution"
+require "graphql/runtime_type_error"
+require "graphql/unresolved_type_error"
+require "graphql/invalid_null_error"
 require "graphql/schema"
 require "graphql/query"
 require "graphql/directive"
 require "graphql/execution"
-require "graphql/runtime_type_error"
-require "graphql/unresolved_type_error"
-require "graphql/invalid_null_error"
 require "graphql/types"
 require "graphql/relay"
 require "graphql/boolean_type"

lib/graphql/execution/interpreter/runtime.rb

@@ -254,7 +254,8 @@ def evaluate_selections(path, scoped_context, owner_object, owner_type, selectio
         def continue_value(path, value, field, is_non_null, ast_node)
           if value.nil?
             if is_non_null
-              err = field.owner::InvalidNullError.new(field.owner, field, value)
+              parent_type = field.owner_type
+              err = parent_type::InvalidNullError.new(parent_type, field, value)
               write_invalid_null_in_response(path, err)
             else
               write_in_response(path, nil)
@@ -311,7 +312,7 @@ def continue_field(path, value, field, type, ast_node, next_selections, is_non_n
               possible_types = query.possible_types(type)
 
               if !possible_types.include?(resolved_type)
-                parent_type = field.owner
+                parent_type = field.owner_type
                 err_class = type::UnresolvedTypeError
                 type_error = err_class.new(resolved_value, field, parent_type, resolved_type, possible_types)
                 schema.type_error(type_error, context)

lib/graphql/invalid_null_error.rb

@@ -28,5 +28,23 @@ def to_h
     def parent_error?
       false
     end
+
+    class << self
+      attr_accessor :parent_class
+
+      def subclass_for(parent_class)
+        subclass = Class.new(self)
+        subclass.parent_class = parent_class
+        subclass
+      end
+
+      def inspect
+        if name.nil? && parent_class.respond_to?(:mutation) && (mutation = parent_class.mutation)
+          "#{mutation.inspect}::#{parent_class.graphql_name}::InvalidNullError"
+        else
+          super
+        end
+      end
+    end
   end
 end

lib/graphql/schema/field.rb

@@ -36,9 +36,18 @@ class Field
       # @return [Symbol] The method on the type to look up
       attr_reader :resolver_method
 
-      # @return [Class] The type that this field belongs to
+      # @return [Class] The thing this field was defined on (type, mutation, resolver)
       attr_accessor :owner
 
+      # @return [Class] The GraphQL type this field belongs to. (For fields defined on mutations, it's the payload type)
+      def owner_type
+        @owner_type ||= if owner < GraphQL::Schema::Mutation
+          owner.payload_type
+        else
+          owner
+        end
+      end
+
       # @return [Symbol] the original name of the field, passed in by the user
       attr_reader :original_name
 

lib/graphql/schema/object.rb

@@ -74,7 +74,7 @@ class << self
         # Set up a type-specific invalid null error to use when this object's non-null fields wrongly return `nil`.
         # It should help with debugging and bug tracker integrations.
         def inherited(child_class)
-          child_class.const_set(:InvalidNullError, Class.new(GraphQL::InvalidNullError))
+          child_class.const_set(:InvalidNullError, GraphQL::InvalidNullError.subclass_for(child_class))
           super
         end
 

spec/dummy/test/application_system_test_case.rb

@@ -3,4 +3,14 @@
 
 class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
   driven_by :selenium, using: :chrome, screen_size: [1400, 1400]
+
+  teardown do
+    # Adapted from https://medium.com/@coorasse/catch-javascript-errors-in-your-system-tests-89c2fe6773b1
+    errors = page.driver.browser.manage.logs.get(:browser)
+    if errors.present?
+      errors.each do |error|
+        assert_nil "#{error.level}: #{error.message}"
+      end
+    end
+  end
 end

spec/graphql/schema/mutation_spec.rb

@@ -113,6 +113,17 @@
       response = Jazz::Schema.execute(query_str)
       assert_equal 2, response["errors"].length, "It should return two errors"
     end
+
+    it "raises a mutation-specific invalid null error" do
+      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"] }
+      if TESTING_INTERPRETER
+        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
   end
 
   describe ".null" do

spec/support/jazz.rb

@@ -776,6 +776,14 @@ def resolve
     end
   end
 
+  class ReturnInvalidNull < GraphQL::Schema::Mutation
+    field :int, Integer, null: false
+
+    def resolve
+      { int: nil }
+    end
+  end
+
   class Mutation < BaseObject
     field :add_ensemble, Ensemble, null: false do
       argument :input, EnsembleInput, required: true
@@ -796,6 +804,7 @@ class Mutation < BaseObject
     field :has_extras, mutation: HasExtras
     field :has_extras_stripped, mutation: HasExtrasStripped
     field :has_field_extras, mutation: HasFieldExtras, extras: [:lookahead]
+    field :return_invalid_null, mutation: ReturnInvalidNull
 
     def add_ensemble(input:)
       ens = Models::Ensemble.new(input.name)