From 266aa7af37c634ca8a53ccb77d2512878b457883 Mon Sep 17 00:00:00 2001
From: Robert Mosolgo <rdmosolgo@gmail.com>
Date: Tue, 16 Jun 2020 17:09:02 -0400
Subject: [PATCH 1/4] Add invalid null error for mutations

---
 lib/graphql.rb                                  |  6 +++---
 lib/graphql/schema/member.rb                    |  1 +
 .../schema/member/has_invalid_null_error.rb     | 17 +++++++++++++++++
 lib/graphql/schema/mutation.rb                  |  1 +
 lib/graphql/schema/object.rb                    |  8 +-------
 spec/graphql/schema/mutation_spec.rb            |  7 +++++++
 spec/support/jazz.rb                            |  9 +++++++++
 7 files changed, 39 insertions(+), 10 deletions(-)
 create mode 100644 lib/graphql/schema/member/has_invalid_null_error.rb

diff --git a/lib/graphql.rb b/lib/graphql.rb
index db35c15e8c..6bea7c9c36 100644
--- a/lib/graphql.rb
+++ b/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"
diff --git a/lib/graphql/schema/member.rb b/lib/graphql/schema/member.rb
index e824d936f4..f2714d895f 100644
--- a/lib/graphql/schema/member.rb
+++ b/lib/graphql/schema/member.rb
@@ -4,6 +4,7 @@
 require 'graphql/schema/member/cached_graphql_definition'
 require 'graphql/schema/member/graphql_type_names'
 require 'graphql/schema/member/has_ast_node'
+require 'graphql/schema/member/has_invalid_null_error'
 require 'graphql/schema/member/has_path'
 require 'graphql/schema/member/has_unresolved_type_error'
 require 'graphql/schema/member/relay_shortcuts'
diff --git a/lib/graphql/schema/member/has_invalid_null_error.rb b/lib/graphql/schema/member/has_invalid_null_error.rb
new file mode 100644
index 0000000000..2334541e84
--- /dev/null
+++ b/lib/graphql/schema/member/has_invalid_null_error.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module GraphQL
+  class Schema
+    class Member
+      module HasInvalidNullError
+        # Set up a member-specific invalid null error
+        # to use when this member'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))
+          super
+        end
+      end
+    end
+  end
+end
diff --git a/lib/graphql/schema/mutation.rb b/lib/graphql/schema/mutation.rb
index 1f0201be8d..f9653dae7c 100644
--- a/lib/graphql/schema/mutation.rb
+++ b/lib/graphql/schema/mutation.rb
@@ -60,6 +60,7 @@ class Schema
     #
     class Mutation < GraphQL::Schema::Resolver
       extend GraphQL::Schema::Member::HasFields
+      extend GraphQL::Schema::Member::HasInvalidNullError
       extend GraphQL::Schema::Resolver::HasPayloadType
 
       class << self
diff --git a/lib/graphql/schema/object.rb b/lib/graphql/schema/object.rb
index 06a6be8980..f3edc16b6b 100644
--- a/lib/graphql/schema/object.rb
+++ b/lib/graphql/schema/object.rb
@@ -7,6 +7,7 @@ class Schema
     class Object < GraphQL::Schema::Member
       extend GraphQL::Schema::Member::AcceptsDefinition
       extend GraphQL::Schema::Member::HasFields
+      extend GraphQL::Schema::Member::HasInvalidNullError
 
       # @return [Object] the application object this type is wrapping
       attr_reader :object
@@ -71,13 +72,6 @@ def initialize(object, context)
       end
 
       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))
-          super
-        end
-
         def implements(*new_interfaces, **options)
           new_memberships = []
           new_interfaces.each do |int|
diff --git a/spec/graphql/schema/mutation_spec.rb b/spec/graphql/schema/mutation_spec.rb
index 4f7c4aee98..706aa21614 100644
--- a/spec/graphql/schema/mutation_spec.rb
+++ b/spec/graphql/schema/mutation_spec.rb
@@ -113,6 +113,13 @@
       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 ReturnInvalidNull.int"], response["errors"].map { |e| e["message"] }
+      assert_instance_of Jazz::ReturnInvalidNull::InvalidNullError, response.query.context.errors.first
+    end
   end
 
   describe ".null" do
diff --git a/spec/support/jazz.rb b/spec/support/jazz.rb
index aff685695f..9ef58ac399 100644
--- a/spec/support/jazz.rb
+++ b/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)

From b1582f1aefd4bec91d3321ae90a984029a9f54c4 Mon Sep 17 00:00:00 2001
From: Robert Mosolgo <rdmosolgo@gmail.com>
Date: Wed, 17 Jun 2020 08:14:44 -0400
Subject: [PATCH 2/4] use the payload type's non-null error

---
 lib/graphql/execution/interpreter/runtime.rb    |  5 +++--
 lib/graphql/schema/field.rb                     | 11 ++++++++++-
 lib/graphql/schema/member.rb                    |  1 -
 .../schema/member/has_invalid_null_error.rb     | 17 -----------------
 lib/graphql/schema/mutation.rb                  |  1 -
 lib/graphql/schema/object.rb                    |  8 +++++++-
 spec/graphql/schema/mutation_spec.rb            |  4 ++--
 7 files changed, 22 insertions(+), 25 deletions(-)
 delete mode 100644 lib/graphql/schema/member/has_invalid_null_error.rb

diff --git a/lib/graphql/execution/interpreter/runtime.rb b/lib/graphql/execution/interpreter/runtime.rb
index 8384d23348..80f88ace8e 100644
--- a/lib/graphql/execution/interpreter/runtime.rb
+++ b/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)
diff --git a/lib/graphql/schema/field.rb b/lib/graphql/schema/field.rb
index 5e3e0a295c..dfc3aa3f12 100644
--- a/lib/graphql/schema/field.rb
+++ b/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
 
diff --git a/lib/graphql/schema/member.rb b/lib/graphql/schema/member.rb
index f2714d895f..e824d936f4 100644
--- a/lib/graphql/schema/member.rb
+++ b/lib/graphql/schema/member.rb
@@ -4,7 +4,6 @@
 require 'graphql/schema/member/cached_graphql_definition'
 require 'graphql/schema/member/graphql_type_names'
 require 'graphql/schema/member/has_ast_node'
-require 'graphql/schema/member/has_invalid_null_error'
 require 'graphql/schema/member/has_path'
 require 'graphql/schema/member/has_unresolved_type_error'
 require 'graphql/schema/member/relay_shortcuts'
diff --git a/lib/graphql/schema/member/has_invalid_null_error.rb b/lib/graphql/schema/member/has_invalid_null_error.rb
deleted file mode 100644
index 2334541e84..0000000000
--- a/lib/graphql/schema/member/has_invalid_null_error.rb
+++ /dev/null
@@ -1,17 +0,0 @@
-# frozen_string_literal: true
-
-module GraphQL
-  class Schema
-    class Member
-      module HasInvalidNullError
-        # Set up a member-specific invalid null error
-        # to use when this member'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))
-          super
-        end
-      end
-    end
-  end
-end
diff --git a/lib/graphql/schema/mutation.rb b/lib/graphql/schema/mutation.rb
index f9653dae7c..1f0201be8d 100644
--- a/lib/graphql/schema/mutation.rb
+++ b/lib/graphql/schema/mutation.rb
@@ -60,7 +60,6 @@ class Schema
     #
     class Mutation < GraphQL::Schema::Resolver
       extend GraphQL::Schema::Member::HasFields
-      extend GraphQL::Schema::Member::HasInvalidNullError
       extend GraphQL::Schema::Resolver::HasPayloadType
 
       class << self
diff --git a/lib/graphql/schema/object.rb b/lib/graphql/schema/object.rb
index f3edc16b6b..06a6be8980 100644
--- a/lib/graphql/schema/object.rb
+++ b/lib/graphql/schema/object.rb
@@ -7,7 +7,6 @@ class Schema
     class Object < GraphQL::Schema::Member
       extend GraphQL::Schema::Member::AcceptsDefinition
       extend GraphQL::Schema::Member::HasFields
-      extend GraphQL::Schema::Member::HasInvalidNullError
 
       # @return [Object] the application object this type is wrapping
       attr_reader :object
@@ -72,6 +71,13 @@ def initialize(object, context)
       end
 
       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))
+          super
+        end
+
         def implements(*new_interfaces, **options)
           new_memberships = []
           new_interfaces.each do |int|
diff --git a/spec/graphql/schema/mutation_spec.rb b/spec/graphql/schema/mutation_spec.rb
index 706aa21614..40ca4bc8ab 100644
--- a/spec/graphql/schema/mutation_spec.rb
+++ b/spec/graphql/schema/mutation_spec.rb
@@ -117,8 +117,8 @@
     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 ReturnInvalidNull.int"], response["errors"].map { |e| e["message"] }
-      assert_instance_of Jazz::ReturnInvalidNull::InvalidNullError, response.query.context.errors.first
+      assert_equal ["Cannot return null for non-nullable field ReturnInvalidNullPayload.int"], response["errors"].map { |e| e["message"] }
+      assert_instance_of Jazz::ReturnInvalidNull.payload_type::InvalidNullError, response.query.context.errors.first
     end
   end
 

From 2cb8174073d7cb4012bb65506b81c7a298da5d60 Mon Sep 17 00:00:00 2001
From: Robert Mosolgo <rdmosolgo@gmail.com>
Date: Wed, 17 Jun 2020 09:43:22 -0400
Subject: [PATCH 3/4] Add logging for any JS error

---
 spec/dummy/test/application_system_test_case.rb | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/spec/dummy/test/application_system_test_case.rb b/spec/dummy/test/application_system_test_case.rb
index 93fada67a8..95c5cfceed 100644
--- a/spec/dummy/test/application_system_test_case.rb
+++ b/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

From d759092cd01d8a3d7a8f09e90248d1f07a38ce93 Mon Sep 17 00:00:00 2001
From: Robert Mosolgo <rdmosolgo@gmail.com>
Date: Wed, 17 Jun 2020 11:14:34 -0400
Subject: [PATCH 4/4] improve debug output for error class

---
 lib/graphql/invalid_null_error.rb    | 18 ++++++++++++++++++
 lib/graphql/schema/object.rb         |  2 +-
 spec/graphql/schema/mutation_spec.rb |  6 +++++-
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/lib/graphql/invalid_null_error.rb b/lib/graphql/invalid_null_error.rb
index f949e3351c..ea49a02dde 100644
--- a/lib/graphql/invalid_null_error.rb
+++ b/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
diff --git a/lib/graphql/schema/object.rb b/lib/graphql/schema/object.rb
index 06a6be8980..aec96e43aa 100644
--- a/lib/graphql/schema/object.rb
+++ b/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
 
diff --git a/spec/graphql/schema/mutation_spec.rb b/spec/graphql/schema/mutation_spec.rb
index 40ca4bc8ab..ef99b12684 100644
--- a/spec/graphql/schema/mutation_spec.rb
+++ b/spec/graphql/schema/mutation_spec.rb
@@ -118,7 +118,11 @@
       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"] }
-      assert_instance_of Jazz::ReturnInvalidNull.payload_type::InvalidNullError, response.query.context.errors.first
+      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