Assert matching response shapes in fields_will_merge

lib/graphql/static_validation/rules/fields_will_merge.rb

@@ -33,26 +33,19 @@ def on_field(node, _parent)
 
       private
 
-      def field_conflicts
-        @field_conflicts ||= Hash.new do |errors, field|
-          errors[field] = GraphQL::StaticValidation::FieldsWillMergeError.new(kind: :field, field_name: field)
-        end
-      end
-
-      def arg_conflicts
-        @arg_conflicts ||= Hash.new do |errors, field|
-          errors[field] = GraphQL::StaticValidation::FieldsWillMergeError.new(kind: :argument, field_name: field)
+      def conflicts
+        @conflicts ||= Hash.new do |h, error_type|
+          h[error_type] = Hash.new do |h2, field_name|
+            h2[field_name] = GraphQL::StaticValidation::FieldsWillMergeError.new(kind: error_type, field_name: field_name)
+          end
         end
       end
 
       def setting_errors
-        @field_conflicts = nil
-        @arg_conflicts = nil
-
+        @conflicts = nil
         yield
         # don't initialize these if they weren't initialized in the block:
-        @field_conflicts && @field_conflicts.each_value { |error| add_error(error) }
-        @arg_conflicts && @arg_conflicts.each_value { |error| add_error(error) }
+        @conflicts && @conflicts.each_value { |error_type| error_type.each_value { |error| add_error(error) } }
       end
 
       def conflicts_within_selection_set(node, parent_type)
@@ -221,7 +214,7 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false)
 
         if !are_mutually_exclusive
           if node1.name != node2.name
-            conflict = field_conflicts[response_key]
+            conflict = conflicts[:field][response_key]
 
             conflict.add_conflict(node1, node1.name)
             conflict.add_conflict(node2, node2.name)
@@ -230,7 +223,7 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false)
           end
 
           if !same_arguments?(node1, node2)
-            conflict = arg_conflicts[response_key]
+            conflict = conflicts[:argument][response_key]
 
             conflict.add_conflict(node1, GraphQL::Language.serialize(serialize_field_args(node1)))
             conflict.add_conflict(node2, GraphQL::Language.serialize(serialize_field_args(node2)))
@@ -239,13 +232,50 @@ def find_conflict(response_key, field1, field2, mutually_exclusive: false)
           end
         end
 
+        if !conflicts[:field].key?(response_key) &&
+            (t1 = field1.definition&.type) &&
+            (t2 = field2.definition&.type) &&
+            return_types_conflict?(t1, t2)
+
+          conflict = conflicts[:return_type][response_key]
+          conflict.add_conflict(node1, "`#{t1.to_type_signature}`")
+          conflict.add_conflict(node2, "`#{t2.to_type_signature}`")
+
+          @conflict_count += 1
+        end
+
         find_conflicts_between_sub_selection_sets(
           field1,
           field2,
           mutually_exclusive: are_mutually_exclusive,
         )
       end
 
+      def return_types_conflict?(type1, type2)
+        if type1.list?
+          if type2.list?
+            return_types_conflict?(type1.of_type, type2.of_type)
+          else
+            true
+          end
+        elsif type2.list?
+        elsif type1.non_null?
+          if type2.non_null?
+            return_types_conflict?(type1.of_type, type2.of_type)
+          else
+            true
+          end
+        elsif type2.non_null?
+          true
+        elsif !type1.kind.fields? && !type2.kind.fields?
+          type1 != type2
+        else
+          # One or more of these are composite types,
+          # their selections will be validated later on.
+          false
+        end
+      end
+
       def find_conflicts_between_sub_selection_sets(field1, field2, mutually_exclusive:)
         return if field1.definition.nil? ||
           field2.definition.nil? ||
@@ -344,7 +374,7 @@ def find_fields_and_fragments(selections, owner_type:, parents:, fields:, fragme
             fields << Field.new(node, definition, owner_type, parents)
           when GraphQL::Language::Nodes::InlineFragment
             fragment_type = node.type ? context.warden.get_type(node.type.name) : owner_type
-            find_fields_and_fragments(node.selections, parents: [*parents, fragment_type], owner_type: owner_type, fields: fields, fragment_spreads: fragment_spreads) if fragment_type
+            find_fields_and_fragments(node.selections, parents: [*parents, fragment_type], owner_type: fragment_type, fields: fields, fragment_spreads: fragment_spreads) if fragment_type
           when GraphQL::Language::Nodes::FragmentSpread
             fragment_spreads << FragmentSpread.new(node.name, parents)
           end

lib/graphql/static_validation/rules/fields_will_merge_error.rb

@@ -26,7 +26,13 @@ def conflicts
       end
 
       def add_conflict(node, conflict_str)
-        return if nodes.include?(node)
+        # Can't use `.include?` here because AST nodes implement `#==`
+        # based on string value, not including location. But sometimes,
+        # identical nodes conflict because of their differing return types.
+        if nodes.any? { |n| n == node && n.line == node.line && n.col == node.col }
+          # already have an error for this node
+          return
+        end
 
         @nodes << node
         @conflicts << conflict_str

spec/graphql/execution/interpreter_spec.rb

@@ -41,11 +41,6 @@ def lazy_sym
         Box.new(value: object.sym)
       end
 
-      field :null_union_field_test, Integer, null: false
-      def null_union_field_test
-        1
-      end
-
       field :always_cached_value, Integer, null: false
       def always_cached_value
         raise "should never be called"
@@ -61,11 +56,6 @@ def expansion
         Query::EXPANSIONS.find { |e| e.sym == @object.expansion_sym }
       end
 
-      field :null_union_field_test, Integer
-      def null_union_field_test
-        nil
-      end
-
       field :parent_class_name, String, null: false, extras: [:parent]
 
       def parent_class_name(parent:)
@@ -484,23 +474,6 @@ def self.trace(event, data)
       assert_equal Hash, res["data"].class
       assert_equal Array, res["data"]["findMany"].class
     end
-
-    it "works with union lists that have members of different kinds, with different nullabilities" do
-      res = InterpreterTest::Schema.execute <<-GRAPHQL
-      {
-        findMany(ids: ["RAV", "Dark Confidant"]) {
-          ... on Expansion {
-            nullUnionFieldTest
-          }
-          ... on Card {
-            nullUnionFieldTest
-          }
-        }
-      }
-      GRAPHQL
-
-      assert_equal [1, nil], res["data"]["findMany"].map { |f| f["nullUnionFieldTest"] }
-    end
   end
 
   describe "duplicated fields" do

spec/graphql/static_validation/rules/fields_will_merge_spec.rb

@@ -49,7 +49,7 @@
 
       interface Pet {
         name(surname: Boolean = false): String!
-        nickname: String
+        nickname: String!
         toys: [Toy!]!
       }
 
@@ -63,15 +63,15 @@
 
       type Dog implements Pet & Mammal & Canine {
         name(surname: Boolean = false): String!
-        nickname: String
+        nickname: String!
         doesKnowCommand(dogCommand: PetCommand): Boolean!
         barkVolume: Int!
         toys: [Toy!]!
       }
 
       type Cat implements Pet & Mammal & Feline {
         name(surname: Boolean = false): String!
-        nickname: String
+        nickname: String!
         doesKnowCommand(catCommand: PetCommand): Boolean!
         meowVolume: Int!
         toys: [Toy!]!
@@ -998,4 +998,47 @@
       refute_match %r/\$arg12/, error_messages.first
     end
   end
+
+  describe "Conflicting leaf typed fields" do
+    it "adds an error" do
+      schema = GraphQL::Schema.from_definition(<<-GRAPHQL)
+      interface Thing {
+        name: String
+      }
+
+      type Dog implements Thing {
+        spots: Boolean
+      }
+
+      type Jaguar implements Thing {
+        spots: Int
+      }
+
+      type Query {
+        thing: Thing
+      }
+      GRAPHQL
+
+      query_str = <<-GRAPHQL
+      {
+        thing {
+          ... on Dog { spots }
+          ... on Jaguar { spots }
+        }
+      }
+      GRAPHQL
+
+      res = schema.validate(query_str)
+      expected_error = {
+        "message"=>"Field 'spots' has a return_type conflict: `Boolean` or `Int`?",
+        "locations"=>[{"line"=>3, "column"=>24}, {"line"=>4, "column"=>27}],
+        "path"=>[],
+        "extensions"=>
+          {"code"=>"fieldConflict",
+           "fieldName"=>"spots",
+           "conflicts"=>"`Boolean` or `Int`"}
+        }
+      assert_equal [expected_error], res.map(&:to_h)
+    end
+  end
 end