Commit 2c42884c authored by Jan Provaznik's avatar Jan Provaznik

Fix graphql resolver complexity

Since 1.9 graphql always sets complexity for resolvers, which
caused that our default resolver complexity proc wasn't used.

Unless complexity is set explicitly then graphql now uses complexity
`1`. To avoid this behavior, we use `complexity` method to set
complexity to `0`, then when instantiating a field we use our
default complexity proc if complexity is `0`.
parent becfc8bc
...@@ -28,6 +28,10 @@ module Resolvers ...@@ -28,6 +28,10 @@ module Resolvers
end end
end end
def self.complexity
0
end
def self.resolver_complexity(args, child_complexity:) def self.resolver_complexity(args, child_complexity:)
complexity = 1 complexity = 1
complexity += 1 if args[:sort] complexity += 1 if args[:sort]
......
...@@ -9,7 +9,7 @@ module Types ...@@ -9,7 +9,7 @@ module Types
def initialize(*args, **kwargs, &block) def initialize(*args, **kwargs, &block)
@calls_gitaly = !!kwargs.delete(:calls_gitaly) @calls_gitaly = !!kwargs.delete(:calls_gitaly)
@constant_complexity = !!kwargs[:complexity] @constant_complexity = !!kwargs[:complexity]
kwargs[:complexity] ||= field_complexity(kwargs[:resolver_class]) kwargs[:complexity] = field_complexity(kwargs[:resolver_class], kwargs[:complexity])
@feature_flag = kwargs[:feature_flag] @feature_flag = kwargs[:feature_flag]
kwargs = check_feature_flag(kwargs) kwargs = check_feature_flag(kwargs)
...@@ -51,7 +51,9 @@ module Types ...@@ -51,7 +51,9 @@ module Types
args args
end end
def field_complexity(resolver_class) def field_complexity(resolver_class, current)
return current if current.present? && current > 0
if resolver_class if resolver_class
field_resolver_complexity field_resolver_complexity
else else
...@@ -66,22 +68,30 @@ module Types ...@@ -66,22 +68,30 @@ module Types
# proc because we set complexity depending on arguments and number of # proc because we set complexity depending on arguments and number of
# items which can be loaded. # items which can be loaded.
proc do |ctx, args, child_complexity| proc do |ctx, args, child_complexity|
next base_complexity unless resolver_complexity_enabled?(ctx)
# Resolvers may add extra complexity depending on used arguments # Resolvers may add extra complexity depending on used arguments
complexity = child_complexity + self.resolver&.try(:resolver_complexity, args, child_complexity: child_complexity).to_i complexity = child_complexity + self.resolver&.try(:resolver_complexity, args, child_complexity: child_complexity).to_i
complexity += 1 if calls_gitaly? complexity += 1 if calls_gitaly?
complexity += complexity * connection_complexity_multiplier(ctx, args)
field_defn = to_graphql complexity.to_i
end
end
def resolver_complexity_enabled?(ctx)
ctx.fetch(:graphql_resolver_complexity_flag) { |key| ctx[key] = Feature.enabled?(:graphql_resolver_complexity) }
end
if field_defn.connection? def connection_complexity_multiplier(ctx, args)
# Resolvers may add extra complexity depending on number of items being loaded. # Resolvers may add extra complexity depending on number of items being loaded.
field_defn = to_graphql
return 0 unless field_defn.connection?
page_size = field_defn.connection_max_page_size || ctx.schema.default_max_page_size page_size = field_defn.connection_max_page_size || ctx.schema.default_max_page_size
limit_value = [args[:first], args[:last], page_size].compact.min limit_value = [args[:first], args[:last], page_size].compact.min
multiplier = self.resolver&.try(:complexity_multiplier, args).to_f multiplier = self.resolver&.try(:complexity_multiplier, args).to_f
complexity += complexity * limit_value * multiplier limit_value * multiplier
end
complexity.to_i
end
end end
end end
end end
...@@ -92,5 +92,10 @@ module Resolvers ...@@ -92,5 +92,10 @@ module Resolvers
complexity complexity
end end
# https://gitlab.com/gitlab-org/gitlab/issues/205312
def self.complexity_multiplier(args)
0.001
end
end end
end end
...@@ -17,7 +17,8 @@ module Types ...@@ -17,7 +17,8 @@ module Types
Types::DesignManagement::DesignType.connection_type, Types::DesignManagement::DesignType.connection_type,
null: false, null: false,
resolver: Resolvers::DesignManagement::DesignsResolver, resolver: Resolvers::DesignManagement::DesignsResolver,
description: 'All designs for the design collection' description: 'All designs for the design collection',
complexity: 5
field :versions, field :versions,
Types::DesignManagement::VersionType.connection_type, Types::DesignManagement::VersionType.connection_type,
......
...@@ -46,7 +46,15 @@ describe Types::BaseField do ...@@ -46,7 +46,15 @@ describe Types::BaseField do
expect(field.to_graphql.complexity).to eq 12 expect(field.to_graphql.complexity).to eq 12
end end
context 'when field has a resolver proc' do context 'when field has a resolver' do
context 'when a valid complexity is already set' do
let(:field) { described_class.new(name: 'test', type: GraphQL::STRING_TYPE.connection_type, resolver_class: resolver, complexity: 2, max_page_size: 100, null: true) }
it 'uses this complexity' do
expect(field.to_graphql.complexity).to eq 2
end
end
context 'and is a connection' do context 'and is a connection' do
let(:field) { described_class.new(name: 'test', type: GraphQL::STRING_TYPE.connection_type, resolver_class: resolver, max_page_size: 100, null: true) } let(:field) { described_class.new(name: 'test', type: GraphQL::STRING_TYPE.connection_type, resolver_class: resolver, max_page_size: 100, null: true) }
...@@ -59,6 +67,17 @@ describe Types::BaseField do ...@@ -59,6 +67,17 @@ describe Types::BaseField do
expect(field.to_graphql.complexity.call({}, { first: 1 }, 2)).to eq 2 expect(field.to_graphql.complexity.call({}, { first: 1 }, 2)).to eq 2
expect(field.to_graphql.complexity.call({}, { first: 1, foo: true }, 2)).to eq 4 expect(field.to_graphql.complexity.call({}, { first: 1, foo: true }, 2)).to eq 4
end end
context 'when graphql_resolver_complexity is disabled' do
before do
stub_feature_flags(graphql_resolver_complexity: false)
end
it 'sets default field complexity' do
expect(field.to_graphql.complexity.call({}, {}, 2)).to eq 1
expect(field.to_graphql.complexity.call({}, { first: 50 }, 2)).to eq 1
end
end
end end
context 'and is not a connection' do context 'and is not a connection' do
......
...@@ -152,4 +152,52 @@ describe 'GraphQL' do ...@@ -152,4 +152,52 @@ describe 'GraphQL' do
end end
end end
end end
describe 'resolver complexity' do
let_it_be(:project) { create(:project, :public) }
let(:query) do
graphql_query_for(
'project',
{ 'fullPath' => project.full_path },
query_graphql_field(resource, {}, 'edges { node { iid } }')
)
end
before do
stub_const('GitlabSchema::DEFAULT_MAX_COMPLEXITY', 6)
stub_feature_flags(graphql_resolver_complexity: true)
end
context 'when fetching single resource' do
let(:resource) { 'issues(first: 1)' }
it 'processes the query' do
post_graphql(query)
expect(graphql_errors).to be_nil
end
end
context 'when fetching too many resources' do
let(:resource) { 'issues(first: 100)' }
it 'returns an error' do
post_graphql(query)
expect_graphql_errors_to_include(/which exceeds max complexity/)
end
context 'when graphql_resolver_complexity is disabled' do
before do
stub_feature_flags(graphql_resolver_complexity: false)
end
it 'processes the query' do
post_graphql(query)
expect(graphql_errors).to be_nil
end
end
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment