diff options
| -rw-r--r-- | app/graphql/resolvers/base_resolver.rb | 19 | ||||
| -rw-r--r-- | app/graphql/resolvers/concerns/resolves_pipelines.rb | 10 | ||||
| -rw-r--r-- | app/graphql/resolvers/issues_resolver.rb | 7 | ||||
| -rw-r--r-- | app/graphql/resolvers/project_resolver.rb | 4 | ||||
| -rw-r--r-- | app/graphql/types/base_field.rb | 34 | ||||
| -rw-r--r-- | app/graphql/types/issue_type.rb | 6 | ||||
| -rw-r--r-- | changelogs/unreleased/graphql-resolvers-complexity.yml | 6 | ||||
| -rw-r--r-- | spec/graphql/resolvers/base_resolver_spec.rb | 15 | ||||
| -rw-r--r-- | spec/graphql/resolvers/concerns/resolves_pipelines_spec.rb | 8 | ||||
| -rw-r--r-- | spec/graphql/resolvers/issues_resolver_spec.rb | 7 | ||||
| -rw-r--r-- | spec/graphql/resolvers/project_resolver_spec.rb | 8 | ||||
| -rw-r--r-- | spec/graphql/types/base_field_spec.rb | 26 |
12 files changed, 146 insertions, 4 deletions
diff --git a/app/graphql/resolvers/base_resolver.rb b/app/graphql/resolvers/base_resolver.rb index 063def75d38..31850c2cadb 100644 --- a/app/graphql/resolvers/base_resolver.rb +++ b/app/graphql/resolvers/base_resolver.rb @@ -9,5 +9,24 @@ module Resolvers end end end + + def self.resolver_complexity(args) + complexity = 1 + complexity += 1 if args[:sort] + complexity += 5 if args[:search] + + complexity + end + + def self.complexity_multiplier(args) + # When fetching many items, additional complexity is added to the field + # depending on how many items is fetched. For each item we add 1% of the + # original complexity - this means that loading 100 items (our default + # maxp_age_size limit) doubles the original complexity. + # + # Complexity is not increased when searching by specific ID(s), because + # complexity difference is minimal in this case. + [args[:iid], args[:iids]].any? ? 0 : 0.01 + end end end diff --git a/app/graphql/resolvers/concerns/resolves_pipelines.rb b/app/graphql/resolvers/concerns/resolves_pipelines.rb index 8fd26d85994..a166211fc18 100644 --- a/app/graphql/resolvers/concerns/resolves_pipelines.rb +++ b/app/graphql/resolvers/concerns/resolves_pipelines.rb @@ -19,6 +19,16 @@ module ResolvesPipelines description: "Filter pipelines by the sha of the commit they are run for" end + class_methods do + def resolver_complexity(args) + complexity = super + complexity += 2 if args[:sha] + complexity += 2 if args[:ref] + + complexity + end + end + def resolve_pipelines(project, params = {}) PipelinesFinder.new(project, context[:current_user], params).execute end diff --git a/app/graphql/resolvers/issues_resolver.rb b/app/graphql/resolvers/issues_resolver.rb index 54d32a688bf..1c3c24ad6dc 100644 --- a/app/graphql/resolvers/issues_resolver.rb +++ b/app/graphql/resolvers/issues_resolver.rb @@ -57,5 +57,12 @@ module Resolvers IssuesFinder.new(context[:current_user], args).execute end + + def self.resolver_complexity(args) + complexity = super + complexity += 2 if args[:labelName] + + complexity + end end end diff --git a/app/graphql/resolvers/project_resolver.rb b/app/graphql/resolvers/project_resolver.rb index ac7c9b0ce2e..2132447da5e 100644 --- a/app/graphql/resolvers/project_resolver.rb +++ b/app/graphql/resolvers/project_resolver.rb @@ -9,5 +9,9 @@ module Resolvers def resolve(full_path:) model_by_full_path(Project, full_path) end + + def self.complexity_multiplier(args) + 0 + end end end diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index 8c8b8a82d3e..15331129134 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -7,10 +7,40 @@ module Types DEFAULT_COMPLEXITY = 1 def initialize(*args, **kwargs, &block) - # complexity is already defaulted to 1, but let's make it explicit - kwargs[:complexity] ||= DEFAULT_COMPLEXITY + kwargs[:complexity] ||= field_complexity(kwargs[:resolver_class]) super(*args, **kwargs, &block) end + + private + + def field_complexity(resolver_class) + if resolver_class + field_resolver_complexity + else + DEFAULT_COMPLEXITY + end + end + + def field_resolver_complexity + # Complexity can be either integer or proc. If proc is used then it's + # called when computing a query complexity and context and query + # arguments are available for computing complexity. For resolvers we use + # proc because we set complexity depending on arguments and number of + # items which can be loaded. + proc do |ctx, args, child_complexity| + page_size = @max_page_size || ctx.schema.default_max_page_size + limit_value = [args[:first], args[:last], page_size].compact.min + + # Resolvers may add extra complexity depending on used arguments + complexity = child_complexity + self.resolver&.try(:resolver_complexity, args).to_i + + # Resolvers may add extra complexity depending on number of items being loaded. + multiplier = self.resolver&.try(:complexity_multiplier, args).to_f + complexity += complexity * limit_value * multiplier + + complexity.to_i + end + end end end diff --git a/app/graphql/types/issue_type.rb b/app/graphql/types/issue_type.rb index adb137dfee3..b21a226d07f 100644 --- a/app/graphql/types/issue_type.rb +++ b/app/graphql/types/issue_type.rb @@ -19,9 +19,11 @@ module Types null: false, resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(User, obj.author_id).find } - field :assignees, Types::UserType.connection_type, null: true + # Remove complexity when BatchLoader is used + field :assignees, Types::UserType.connection_type, null: true, complexity: 5 - field :labels, Types::LabelType.connection_type, null: true + # Remove complexity when BatchLoader is used + field :labels, Types::LabelType.connection_type, null: true, complexity: 5 field :milestone, Types::MilestoneType, null: true, resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchModelLoader.new(Milestone, obj.milestone_id).find } diff --git a/changelogs/unreleased/graphql-resolvers-complexity.yml b/changelogs/unreleased/graphql-resolvers-complexity.yml new file mode 100644 index 00000000000..503ffbd97f2 --- /dev/null +++ b/changelogs/unreleased/graphql-resolvers-complexity.yml @@ -0,0 +1,6 @@ +--- +title: 'GraphQL: improve evaluation of query complexity based on arguments and query + limits.' +merge_request: 28017 +author: +type: added diff --git a/spec/graphql/resolvers/base_resolver_spec.rb b/spec/graphql/resolvers/base_resolver_spec.rb index e3a34762b62..9982288e206 100644 --- a/spec/graphql/resolvers/base_resolver_spec.rb +++ b/spec/graphql/resolvers/base_resolver_spec.rb @@ -28,4 +28,19 @@ describe Resolvers::BaseResolver do expect(result).to eq(test: 1) end end + + it 'increases complexity based on arguments' do + field = Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, resolver_class: described_class, null: false, max_page_size: 1) + + expect(field.to_graphql.complexity.call({}, { sort: 'foo' }, 1)).to eq 3 + expect(field.to_graphql.complexity.call({}, { search: 'foo' }, 1)).to eq 7 + end + + it 'does not increase complexity when filtering by iids' do + field = Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, resolver_class: described_class, null: false, max_page_size: 100) + + expect(field.to_graphql.complexity.call({}, { sort: 'foo' }, 1)).to eq 6 + expect(field.to_graphql.complexity.call({}, { sort: 'foo', iid: 1 }, 1)).to eq 3 + expect(field.to_graphql.complexity.call({}, { sort: 'foo', iids: [1, 2, 3] }, 1)).to eq 3 + end end diff --git a/spec/graphql/resolvers/concerns/resolves_pipelines_spec.rb b/spec/graphql/resolvers/concerns/resolves_pipelines_spec.rb index ea7159eacf9..3140af27af5 100644 --- a/spec/graphql/resolvers/concerns/resolves_pipelines_spec.rb +++ b/spec/graphql/resolvers/concerns/resolves_pipelines_spec.rb @@ -46,6 +46,14 @@ describe ResolvesPipelines do expect(resolve_pipelines({}, {})).to be_empty end + it 'increases field complexity based on arguments' do + field = Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, resolver_class: resolver, null: false, max_page_size: 1) + + expect(field.to_graphql.complexity.call({}, {}, 1)).to eq 2 + expect(field.to_graphql.complexity.call({}, { sha: 'foo' }, 1)).to eq 4 + expect(field.to_graphql.complexity.call({}, { sha: 'ref' }, 1)).to eq 4 + end + def resolve_pipelines(args = {}, context = { current_user: current_user }) resolve(resolver, obj: project, args: args, ctx: context) end diff --git a/spec/graphql/resolvers/issues_resolver_spec.rb b/spec/graphql/resolvers/issues_resolver_spec.rb index 399a33dae75..bffcdbfe915 100644 --- a/spec/graphql/resolvers/issues_resolver_spec.rb +++ b/spec/graphql/resolvers/issues_resolver_spec.rb @@ -120,6 +120,13 @@ describe Resolvers::IssuesResolver do end end + it 'increases field complexity based on arguments' do + field = Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, resolver_class: described_class, null: false, max_page_size: 100) + + expect(field.to_graphql.complexity.call({}, {}, 1)).to eq 4 + expect(field.to_graphql.complexity.call({}, { labelName: 'foo' }, 1)).to eq 8 + end + def resolve_issues(args = {}, context = { current_user: current_user }) resolve(described_class, obj: project, args: args, ctx: context) end diff --git a/spec/graphql/resolvers/project_resolver_spec.rb b/spec/graphql/resolvers/project_resolver_spec.rb index d4990c6492c..4fdbb3aa43e 100644 --- a/spec/graphql/resolvers/project_resolver_spec.rb +++ b/spec/graphql/resolvers/project_resolver_spec.rb @@ -26,6 +26,14 @@ describe Resolvers::ProjectResolver do end end + it 'does not increase complexity depending on number of load limits' do + field1 = Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, resolver_class: described_class, null: false, max_page_size: 100) + field2 = Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, resolver_class: described_class, null: false, max_page_size: 1) + + expect(field1.to_graphql.complexity.call({}, {}, 1)).to eq 2 + expect(field2.to_graphql.complexity.call({}, {}, 1)).to eq 2 + end + def resolve_project(full_path) resolve(described_class, args: { full_path: full_path }) end diff --git a/spec/graphql/types/base_field_spec.rb b/spec/graphql/types/base_field_spec.rb index b5697ee5245..4fe426e2447 100644 --- a/spec/graphql/types/base_field_spec.rb +++ b/spec/graphql/types/base_field_spec.rb @@ -4,6 +4,18 @@ require 'spec_helper' describe Types::BaseField do context 'when considering complexity' do + let(:resolver) do + Class.new(described_class) do + def self.resolver_complexity(args) + 2 if args[:foo] + end + + def self.complexity_multiplier(args) + 0.01 + end + end + end + it 'defaults to 1' do field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true) @@ -15,5 +27,19 @@ describe Types::BaseField do expect(field.to_graphql.complexity).to eq 12 end + + it 'sets complexity depending on arguments for resolvers' do + field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, resolver_class: resolver, max_page_size: 100, null: true) + + expect(field.to_graphql.complexity.call({}, {}, 2)).to eq 4 + expect(field.to_graphql.complexity.call({}, { first: 50 }, 2)).to eq 3 + end + + it 'sets complexity depending on number load limits for resolvers' do + field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, resolver_class: resolver, max_page_size: 100, null: true) + + 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 + end end end |