Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v3] Complexity cost bug fixes #4843

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 31 additions & 24 deletions lib/graphql/analysis/ast/query_complexity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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|
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This switches to always hill-climb for the maximum possibility among scopes, regardless if they are leaf or composite scopes.

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(
Copy link
Contributor Author

@gmac gmac Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lastly, always trigger exactly one hook per field with the resolved maximums among scopes, costs, and children costs. Before leaf scopes would receive nil as their child_complexity – now they receive 0. This feels more inline with the own_complexity call that has always sent zero child cost to leaf scopes. If the hook cares to differentiate, it can always inspect the scope to determine if there are child selections.

maximum_scope,
max_complexity: maximum_cost,
child_complexity: maximum_children_cost,
)

total + field_cost
total + maximum_cost
end
end
end
Expand Down
103 changes: 101 additions & 2 deletions spec/graphql/analysis/ast/query_complexity_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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