[v3] Complexity cost bug fixes

lib/graphql/analysis/ast/query_complexity.rb

@@ -44,6 +44,10 @@ def initialize(parent_type, field_definition, query, response_path)
           def own_complexity(child_complexity)
             @field_definition.calculate_complexity(query: @query, nodes: @nodes, child_complexity: child_complexity)
           end
+
+          def composite?
+            !empty?
+          end
         end
 
         def on_enter_field(node, parent, visitor)
@@ -145,35 +149,38 @@ def merged_max_complexity(query, inner_selections)
 
           # Add up the total cost for each unique field name's coalesced selections
           unique_field_keys.each_key.reduce(0) do |total, field_key|
-            composite_scopes = nil
-            field_cost = 0
-
-            # Collect composite selection scopes for further aggregation,
-            # leaf selections report their costs directly.
-            inner_selections.each do |inner_selection|
-              child_scope = inner_selection[field_key]
-              next unless child_scope
-
-              # Empty child scopes are leaf nodes with zero child complexity.
-              if child_scope.empty?
-                field_cost = child_scope.own_complexity(0)
-                field_complexity(child_scope, max_complexity: field_cost, child_complexity: nil)
+            # Collect all child scopes for this field key;
+            # all keys come with at least one scope.
+            child_scopes = inner_selections.filter_map { _1[field_key] }
+
+            # Compute maximum possible cost of child selections;
+            # composites merge their maximums, while leaf scopes are always zero.
+            # FieldsWillMerge validation assures all scopes are uniformly composite or leaf.
+            maximum_children_cost = if child_scopes.any?(&:composite?)
+              merged_max_complexity_for_scopes(query, child_scopes)
+            else
+              0
+            end
+
+            # Identify the maximum cost and scope among possibilities
+            maximum_cost = 0
+            maximum_scope = child_scopes.reduce(child_scopes.last) do |max_scope, possible_scope|
+              scope_cost = possible_scope.own_complexity(maximum_children_cost)
+              if scope_cost > maximum_cost
+                maximum_cost = scope_cost
+                possible_scope
               else
-                composite_scopes ||= []
-                composite_scopes << child_scope
+                max_scope
               end
             end
 
-            if composite_scopes
-              child_complexity = merged_max_complexity_for_scopes(query, composite_scopes)
-
-              # This is the last composite scope visited; assume it's representative (for backwards compatibility).
-              # Note: it would be more correct to score each composite scope and use the maximum possibility.
-              field_cost = composite_scopes.last.own_complexity(child_complexity)
-              field_complexity(composite_scopes.last, max_complexity: field_cost, child_complexity: child_complexity)
-            end
+            field_complexity(
+              maximum_scope,
+              max_complexity: maximum_cost,
+              child_complexity: maximum_children_cost,
+            )
 
-            total + field_cost
+            total + maximum_cost
           end
         end
       end

spec/graphql/analysis/ast/query_complexity_spec.rb

@@ -642,10 +642,109 @@ def field_complexity(scoped_type_complexity, max_complexity:, child_complexity:)
       field_complexities = reduce_result.first
 
       assert_equal({
-        ['cheese', 'id'] => { max_complexity: 1, child_complexity: nil },
-        ['cheese', 'flavor'] => { max_complexity: 1, child_complexity: nil },
+        ['cheese', 'id'] => { max_complexity: 1, child_complexity: 0 },
+        ['cheese', 'flavor'] => { max_complexity: 1, child_complexity: 0 },
         ['cheese'] => { max_complexity: 3, child_complexity: 2 },
       }, field_complexities)
     end
   end
+
+  describe "maximum of possible scopes regardless of selection order" do
+    class MaxOfPossibleScopes < GraphQL::Schema
+      class Cheese < GraphQL::Schema::Object
+        field :kind, String
+      end
+
+      module Producer
+        include GraphQL::Schema::Interface
+        field :cheese, Cheese, complexity: 5
+        field :name, String, complexity: 5
+      end
+
+      class Farm < GraphQL::Schema::Object
+        implements Producer
+        field :cheese, Cheese, complexity: 10
+        field :name, String, complexity: 10
+      end
+
+      class Entity < GraphQL::Schema::Union
+        possible_types Farm
+      end
+
+      class Query < GraphQL::Schema::Object
+        field :entity, Entity
+      end
+
+      def self.resolve_type
+        Farm
+      end
+
+      def self.cost(query_string)
+        GraphQL::Analysis::AST.analyze_query(
+          GraphQL::Query.new(self, query_string),
+          [GraphQL::Analysis::AST::QueryComplexity],
+        ).first
+      end
+
+      query(Query)
+      orphan_types(Producer)
+    end
+
+    it "uses maximum of merged composite fields, regardless of selection order" do
+      a = MaxOfPossibleScopes.cost(%|
+        {
+          entity {
+            ...on Producer { cheese { kind } }
+            ...on Farm { cheese { kind } }
+          }
+        }
+      |)
+
+      b = MaxOfPossibleScopes.cost(%|
+        {
+          entity {
+            ...on Farm { cheese { kind } }
+            ...on Producer { cheese { kind } }
+          }
+        }
+      |)
+
+      assert_equal 0, a - b
+    end
+
+    it "uses maximum of merged leaf fields, regardless of selection order" do
+      a = MaxOfPossibleScopes.cost(%|
+        {
+          entity {
+            ...on Producer { name }
+            ...on Farm { name }
+          }
+        }
+      |)
+
+      b = MaxOfPossibleScopes.cost(%|
+        {
+          entity {
+            ...on Farm { name }
+            ...on Producer { name }
+          }
+        }
+      |)
+
+      assert_equal 0, a - b
+    end
+
+    it "invalid mismatched scope types will still compute without error" do
+      cost = MaxOfPossibleScopes.cost(%|
+        {
+          entity {
+            ...on Farm { cheese { kind } }
+            ...on Producer { cheese: name }
+          }
+        }
+      |)
+
+      assert_equal 12, cost
+    end
+  end
 end