fix(InternalRepresentation) keep children separate by static type

rmosolgo · rmosolgo · commit c9d2933f07d7 · 2016-11-02T12:07:33.000-04:00
diff --git a/lib/graphql/analysis/analyze_query.rb b/lib/graphql/analysis/analyze_query.rb
@@ -32,8 +32,10 @@ def analyze_query(query, analyzers)
     def reduce_node(irep_node, reducer_states)
       visit_analyzers(:enter, irep_node, reducer_states)
 
-      irep_node.children.each do |name, child_irep_node|
-        reduce_node(child_irep_node, reducer_states)
+      irep_node.typed_children.each do |type_defn, children|
+        children.each do |name, child_irep_node|
+          reduce_node(child_irep_node, reducer_states)
+        end
       end
 
       visit_analyzers(:leave, irep_node, reducer_states)
diff --git a/lib/graphql/internal_representation/node.rb b/lib/graphql/internal_representation/node.rb
@@ -3,20 +3,22 @@
 module GraphQL
   module InternalRepresentation
     class Node
-      def initialize(parent:, ast_node: nil, return_type: nil, name: nil, definition_name: nil, definitions: {}, children: {}, spreads: [], directives: Set.new, included: true)
-        # Make sure these are kept in sync with #dup
+      def initialize(parent:, ast_node: nil, return_type: nil, name: nil, definition_name: nil, definitions: {}, spreads: [], directives: Set.new, included: true, typed_children: Hash.new {|h, k| h[k] = {} })
         @ast_node = ast_node
         @return_type = return_type
         @name = name
         @definition_name = definition_name
         @parent = parent
         @definitions = definitions
-        @children = children
         @spreads = spreads
         @directives = directives
         @included = included
+        @typed_children = typed_children
       end
 
+      # @return [Hash{GraphQL::BaseType => Hash{String => Node}] Children for each type condition
+      attr_reader :typed_children
+
       # Note: by the time this gets out of the Rewrite phase, this will be empty -- it's emptied out when fragments are merged back in
       # @return [Array<GraphQL::InternalRepresentation::Node>] Fragment names that were spread in this node
       attr_reader :spreads
@@ -56,8 +58,15 @@ def initialize(parent:, ast_node: nil, return_type: nil, name: nil, definition_n
       # @return [GraphQL::BaseType]
       attr_reader :return_type
 
-      # @return [Array<GraphQL::Query::Node>]
-      attr_reader :children
+      # @deprecated Use {#typed_children} instead
+      # @return [Hash{String => Node}]
+      def children
+        if typed_children.size > 1
+          raise("This node has children of different types, use #typed_children")
+        else
+          typed_children.values.first || {}
+        end
+      end
 
       # @return [Boolean] false if every field for this node included `@skip(if: true)`
       attr_accessor :included
@@ -97,8 +106,12 @@ def path
       def inspect(indent = 0)
         own_indent = " " * indent
         self_inspect = "#{own_indent}<Node #{name} #{skipped? ? "(skipped)" : ""}(#{definition_name}: {#{definitions.keys.join("|")}} -> #{return_type})>"
-        if children.any?
-          self_inspect << " {\n#{children.values.map { |n| n.inspect(indent + 2)}.join("\n")}\n#{own_indent}}"
+        if typed_children.any?
+          self_inspect << " {"
+          typed_children.each do |type_defn, children|
+            self_inspect << "\n#{own_indent}  #{type_defn} => (#{children.keys.join(",")})"
+          end
+          self_inspect << "\n#{own_indent}}"
         end
         self_inspect
       end
diff --git a/lib/graphql/internal_representation/rewrite.rb b/lib/graphql/internal_representation/rewrite.rb
@@ -45,10 +45,11 @@ def validate(context)
         visitor[Nodes::Field].enter << ->(ast_node, prev_ast_node) {
           parent_node = @nodes.last
           node_name = ast_node.alias || ast_node.name
+          owner_type = context.parent_type_definition.unwrap
           # This node might not be novel, eg inside an inline fragment
           # but it could contain new type information, which is captured below.
           # (StaticValidation ensures that merging fields is fair game)
-          node = parent_node.children[node_name] ||= begin
+          node = parent_node.typed_children[owner_type][node_name] ||= begin
             Node.new(
               return_type: context.type_definition && context.type_definition.unwrap,
               ast_node: ast_node,
@@ -58,8 +59,7 @@ def validate(context)
               included: false, # may be set to true on leaving the node
             )
           end
-          object_type = context.parent_type_definition.unwrap
-          node.definitions[object_type] = context.field_definition
+          node.definitions[owner_type] = context.field_definition
           @nodes.push(node)
           @parent_directives.push([])
         }
@@ -176,8 +176,10 @@ def validate(context)
       # Merge the children from `fragment_node` into `parent_node`.
       # This is an implementation of "fragment inlining"
       def deep_merge(parent_node, fragment_node, included)
-        fragment_node.children.each do |name, child_node|
-          deep_merge_child(parent_node, name, child_node, included)
+        fragment_node.typed_children.each do |type_defn, children|
+          children.each do |name, child_node|
+            deep_merge_child(parent_node, type_defn, name, child_node, included)
+          end
         end
       end
 
@@ -186,29 +188,29 @@ def deep_merge(parent_node, fragment_node, included)
       # - If the spread was included, first-level children should be included if _either_ node was included
       # - If the spread was _not_ included, first-level children should be included if _a pre-existing_ node was included
       #   (A copied node should be excluded)
-      def deep_merge_child(parent_node, name, node, extra_included)
-        child_node = parent_node.children[name]
+      def deep_merge_child(parent_node, type_defn, name, node, extra_included)
+        child_node = parent_node.typed_children[type_defn][name]
         previously_included = child_node.nil? ? false : child_node.included?
         next_included = extra_included ? (previously_included || node.included?) : previously_included
 
         if child_node.nil?
-          child_node = parent_node.children[name] = node.dup
+          child_node = parent_node.typed_children[type_defn][name] = node.dup
         end
 
         child_node.definitions.merge!(node.definitions)
 
         child_node.included = next_included
 
-
-
-        node.children.each do |merge_child_name, merge_child_node|
-          deep_merge_child(child_node, merge_child_name, merge_child_node, node.included)
+        node.typed_children.each do |type_defn, children|
+          children.each do |merge_child_name, merge_child_node|
+            deep_merge_child(child_node, type_defn, merge_child_name, merge_child_node, node.included)
+          end
         end
       end
 
       # return true if node or _any_ children have a fragment spread
       def any_fragment_spreads?(node)
-        node.spreads.any? || node.children.any? { |name, node| any_fragment_spreads?(node) }
+        node.spreads.any? || node.typed_children.any? { |type_defn, children| children.any? { |name, node| any_fragment_spreads?(node) } }
       end
 
       # pop off own directives,
diff --git a/lib/graphql/query/serial_execution/selection_resolution.rb b/lib/graphql/query/serial_execution/selection_resolution.rb
@@ -3,15 +3,36 @@ class Query
     class SerialExecution
       module SelectionResolution
         def self.resolve(target, current_type, irep_node, execution_context)
-          irep_node.children.each_with_object({}) do |(name, irep_node), memo|
-            if irep_node.included? && irep_node.definitions.any? { |potential_type, field_defn| GraphQL::Execution::Typecast.compatible?(current_type, potential_type, execution_context.query.context) }
-              field_result = execution_context.strategy.field_resolution.new(
-                irep_node,
-                current_type,
-                target,
-                execution_context
-              ).result
-              memo.merge!(field_result)
+          selection_result = {}
+          irep_node.typed_children.each do |type_defn, typed_children|
+            type_child_result = {}
+            compat = GraphQL::Execution::Typecast.compatible?(current_type, type_defn, execution_context.query.context)
+            if compat
+              typed_children.each do |name, irep_node|
+                applies = irep_node.included? && irep_node.definitions.any? { |potential_type, field_defn| GraphQL::Execution::Typecast.compatible?(current_type, potential_type, execution_context.query.context) }
+                if applies
+                  field_result = execution_context.strategy.field_resolution.new(
+                    irep_node,
+                    current_type,
+                    target,
+                    execution_context
+                  ).result
+                  type_child_result.merge!(field_result)
+                end
+              end
+            end
+            deeply_merge(selection_result, type_child_result)
+          end
+          selection_result
+        end
+
+        def self.deeply_merge(complete_result, typed_result)
+          typed_result.each do |key, value|
+            if value.is_a?(Hash)
+              (complete_result[key] ||= {}).merge!(value)
+            else
+              # TODO: we can avoid running this twice
+              complete_result[key] = value
             end
           end
         end
diff --git a/spec/graphql/internal_representation/rewrite_spec.rb b/spec/graphql/internal_representation/rewrite_spec.rb
@@ -19,18 +19,20 @@
       }
     }
     |}
+
     it "produces a tree of nodes" do
       op_node = rewrite_result["getCheeses"]
 
-      assert_equal 2, op_node.children.length
+      root_children = op_node.typed_children[DairyAppQueryType]
+      assert_equal 2, root_children.length
       assert_equal DairyAppQueryType, op_node.return_type
-      first_field = op_node.children.values.first
-      assert_equal 3, first_field.children.length
+      first_field = root_children.values.first
+      assert_equal 3, first_field.typed_children[CheeseType].length
       assert_equal [DairyAppQueryType], first_field.definitions.keys
       assert_equal CheeseType, first_field.return_type
 
-      second_field = op_node.children.values.last
-      assert_equal 1, second_field.children.length
+      second_field = root_children.values.last
+      assert_equal 1, second_field.typed_children[CheeseType].length
       assert_equal [DairyAppQueryType], second_field.definitions.keys
       assert_equal CheeseType, second_field.return_type
       assert second_field.inspect.is_a?(String)
@@ -47,8 +49,8 @@
     |}
 
     it "gets dynamic field definitions" do
-      cheese_field = rewrite_result[nil].children["cheese"]
-      typename_field = cheese_field.children["typename"]
+      cheese_field = rewrite_result[nil].typed_children[DairyAppQueryType]["cheese"]
+      typename_field = cheese_field.typed_children[CheeseType]["typename"]
       assert_equal "__typename", typename_field.definitions.values.first.name
       assert_equal "__typename", typename_field.definition_name
     end
@@ -125,22 +127,91 @@
     it "puts all fragment members as children" do
       op_node = rewrite_result[nil]
 
-      cheese_field = op_node.children["cheese"]
-      assert_equal ["id1", "id2", "fatContent", "origin", "similarCow", "flavor"], cheese_field.children.keys
+      cheese_field = op_node.typed_children[DairyAppQueryType]["cheese"]
+      assert_equal ["id1", "id2", "fatContent", "similarCow", "flavor"], cheese_field.typed_children[CheeseType].keys
+      assert_equal ["fatContent", "origin"], cheese_field.typed_children[EdibleInterface].keys
       # Merge:
-      similar_cow_field = cheese_field.children["similarCow"]
-      assert_equal ["similarCowSource", "fatContent", "similarCheese", "id"], similar_cow_field.children.keys
+      similar_cow_field = cheese_field.typed_children[CheeseType]["similarCow"]
+      assert_equal ["similarCowSource", "fatContent", "similarCheese", "id"], similar_cow_field.typed_children[CheeseType].keys
       # Deep merge:
-      similar_sheep_field = similar_cow_field.children["similarCheese"]
-      assert_equal ["flavor", "source"], similar_sheep_field.children.keys
+      similar_sheep_field = similar_cow_field.typed_children[CheeseType]["similarCheese"]
+      assert_equal ["flavor", "source"], similar_sheep_field.typed_children[CheeseType].keys
 
-      assert_equal [EdibleInterface], cheese_field.children["origin"].definitions.keys
-      assert_equal [CheeseType, EdibleInterface], cheese_field.children["fatContent"].definitions.keys
-      assert_equal [CheeseType], cheese_field.children["flavor"].definitions.keys
+      assert_equal [EdibleInterface], cheese_field.typed_children[EdibleInterface]["origin"].definitions.keys
+      assert_equal [EdibleInterface], cheese_field.typed_children[EdibleInterface]["fatContent"].definitions.keys
+      assert_equal [CheeseType], cheese_field.typed_children[CheeseType]["fatContent"].definitions.keys
+      assert_equal [CheeseType], cheese_field.typed_children[CheeseType]["flavor"].definitions.keys
 
       # nested spread inside fragment definition:
       cheese_2_field = op_node.children["cheese2"].children["similarCheese"]
       assert_equal ["id", "fatContent"], cheese_2_field.children.keys
     end
   end
+
+  describe "nested fields on typed fragments" do
+    let(:result) { DummySchema.execute(query_string) }
+    let(:query_string) {%|
+    {
+      allDairy {
+        __typename
+
+        ... on Milk {
+          selfAsEdible {
+            milkInlineOrigin: origin
+          }
+        }
+
+        ... on Cheese {
+          selfAsEdible {
+            cheeseInlineOrigin: origin
+          }
+        }
+
+        ... on Edible {
+          selfAsEdible {
+            edibleInlineOrigin: origin
+          }
+        }
+
+        ... {
+          ... on Edible {
+            selfAsEdible {
+              untypedInlineOrigin: origin
+            }
+          }
+        }
+        ...milkFields
+        ...cheeseFields
+      }
+    }
+
+    fragment cheeseFields on Cheese {
+      selfAsEdible {
+        cheeseFragmentOrigin: origin
+      }
+    }
+    fragment milkFields on Milk {
+      selfAsEdible {
+        milkFragmentOrigin: origin
+      }
+    }
+    |}
+
+    it "distinguishes between nested fields with the same name on different typed fragments" do
+      all_dairy = result["data"]["allDairy"]
+      cheeses = all_dairy.select { |d| d["__typename"] == "Cheese" }
+      milks = all_dairy.select { |d| d["__typename"] == "Milk" }
+
+      # Make sure all the data is there:
+      assert_equal 3, cheeses.length
+      assert_equal 1, milks.length
+
+      cheeses.each do |cheese|
+        assert_equal ["cheeseInlineOrigin", "cheeseFragmentOrigin", "edibleInlineOrigin", "untypedInlineOrigin"], cheese["selfAsEdible"].keys
+      end
+      milks.each do |milk|
+        assert_equal ["milkInlineOrigin", "milkFragmentOrigin", "edibleInlineOrigin", "untypedInlineOrigin"], milk["selfAsEdible"].keys
+      end
+    end
+  end
 end
diff --git a/spec/graphql/introspection/type_type_spec.rb b/spec/graphql/introspection/type_type_spec.rb
@@ -18,6 +18,7 @@
     {"name"=>"id",          "isDeprecated" => false, "type" => { "kind" => "NON_NULL", "name" => nil, "ofType" => { "name" => "Int"}}},
     {"name"=>"nullableCheese", "isDeprecated"=>false, "type"=>{ "kind" => "OBJECT",  "name" => "Cheese", "ofType"=>nil}},
     {"name"=>"origin",      "isDeprecated" => false, "type" => { "kind" => "NON_NULL", "name" => nil, "ofType" => { "name" => "String"}}},
+    {"name"=>"selfAsEdible", "isDeprecated"=>false, "type"=>{"kind"=>"INTERFACE", "name"=>"Edible", "ofType"=>nil}},
     {"name"=>"similarCheese", "isDeprecated"=>false, "type"=>{ "kind" => "OBJECT", "name"=>"Cheese", "ofType"=>nil}},
     {"name"=>"source",      "isDeprecated" => false, "type" => { "kind" => "NON_NULL", "name" => nil, "ofType" => { "name" => "DairyAnimal"}}},
   ]}
@@ -49,6 +50,7 @@
           {"type"=>{"kind"=>"LIST","name"=>nil, "ofType"=>{"name"=>"String"}}},
           {"type"=>{"kind"=>"NON_NULL","name"=>nil, "ofType"=>{"name"=>"ID"}}},
           {"type"=>{"kind"=>"NON_NULL","name"=>nil, "ofType"=>{"name"=>"String"}}},
+          {"type"=>{"kind"=>"INTERFACE", "name"=>"Edible", "ofType"=>nil}},
           {"type"=>{"kind"=>"ENUM","name"=>"DairyAnimal", "ofType"=>nil}},
         ]
       },
diff --git a/spec/graphql/schema/reduce_types_spec.rb b/spec/graphql/schema/reduce_types_spec.rb
@@ -10,9 +10,9 @@ def reduce_types(types)
       "Cheese" => CheeseType,
       "Float" => GraphQL::FLOAT_TYPE,
       "String" => GraphQL::STRING_TYPE,
+      "Edible" => EdibleInterface,
       "DairyAnimal" => DairyAnimalEnum,
       "Int" => GraphQL::INT_TYPE,
-      "Edible" => EdibleInterface,
       "AnimalProduct" => AnimalProductInterface,
       "LocalProduct" => LocalProductInterface,
     }
diff --git a/spec/support/dairy_app.rb b/spec/support/dairy_app.rb
@@ -16,6 +16,7 @@ class NoSuchDairyError < StandardError; end
   description "Something you can eat, yum"
   field :fatContent, !types.Float, "Percentage which is fat"
   field :origin, !types.String, "Place the edible comes from"
+  field :selfAsEdible, EdibleInterface, resolve: ->(o, a, c) { o }
 end
 
 AnimalProductInterface = GraphQL::InterfaceType.define do
