Commit 2e746803 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Merge branch '58409-increase-graphql-complexity-for-fields-that-make-gitaly-calls' into 'master'

Increase GraphQL complexity for fields that make Gitaly Calls

Closes #58409

See merge request gitlab-org/gitlab-ce!28814
parents f845a081 675c9b9f
......@@ -13,6 +13,7 @@ class GitlabSchema < GraphQL::Schema
use BatchLoader::GraphQL
use Gitlab::Graphql::Authorize
use Gitlab::Graphql::Present
use Gitlab::Graphql::CallsGitaly
use Gitlab::Graphql::Connections
use Gitlab::Graphql::GenericTracing
......
......@@ -7,18 +7,34 @@ module Types
DEFAULT_COMPLEXITY = 1
def initialize(*args, **kwargs, &block)
@calls_gitaly = !!kwargs.delete(:calls_gitaly)
@constant_complexity = !!kwargs[:complexity]
kwargs[:complexity] ||= field_complexity(kwargs[:resolver_class])
super(*args, **kwargs, &block)
end
def base_complexity
complexity = DEFAULT_COMPLEXITY
complexity += 1 if calls_gitaly?
complexity
end
def calls_gitaly?
@calls_gitaly
end
def constant_complexity?
@constant_complexity
end
private
def field_complexity(resolver_class)
if resolver_class
field_resolver_complexity
else
DEFAULT_COMPLEXITY
base_complexity
end
end
......@@ -31,6 +47,7 @@ module Types
proc do |ctx, args, child_complexity|
# 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 += 1 if calls_gitaly?
field_defn = to_graphql
......
......@@ -43,7 +43,7 @@ module Types
field :allow_collaboration, GraphQL::BOOLEAN_TYPE, null: true
field :should_be_rebased, GraphQL::BOOLEAN_TYPE, method: :should_be_rebased?, null: false
field :rebase_commit_sha, GraphQL::STRING_TYPE, null: true
field :rebase_in_progress, GraphQL::BOOLEAN_TYPE, method: :rebase_in_progress?, null: false
field :rebase_in_progress, GraphQL::BOOLEAN_TYPE, method: :rebase_in_progress?, null: false, calls_gitaly: true
field :merge_commit_message, GraphQL::STRING_TYPE, method: :default_merge_commit_message, null: true, deprecation_reason: "Renamed to defaultMergeCommitMessage"
field :default_merge_commit_message, GraphQL::STRING_TYPE, null: true
field :merge_ongoing, GraphQL::BOOLEAN_TYPE, method: :merge_ongoing?, null: false
......
......@@ -9,6 +9,6 @@ module Types
mount_mutation Mutations::AwardEmojis::Add
mount_mutation Mutations::AwardEmojis::Remove
mount_mutation Mutations::AwardEmojis::Toggle
mount_mutation Mutations::MergeRequests::SetWip
mount_mutation Mutations::MergeRequests::SetWip, calls_gitaly: true
end
end
......@@ -10,8 +10,8 @@ module Types
abilities :read_merge_request, :admin_merge_request,
:update_merge_request, :create_note
permission_field :push_to_source_branch, method: :can_push_to_source_branch?
permission_field :remove_source_branch, method: :can_remove_source_branch?
permission_field :push_to_source_branch, method: :can_push_to_source_branch?, calls_gitaly: true
permission_field :remove_source_branch, method: :can_remove_source_branch?, calls_gitaly: true
permission_field :cherry_pick_on_current_merge_request, method: :can_cherry_pick_on_current_merge_request?
permission_field :revert_on_current_merge_request, method: :can_revert_on_current_merge_request?
end
......
......@@ -26,7 +26,7 @@ module Types
field :web_url, GraphQL::STRING_TYPE, null: true
field :star_count, GraphQL::INT_TYPE, null: false
field :forks_count, GraphQL::INT_TYPE, null: false
field :forks_count, GraphQL::INT_TYPE, null: false, calls_gitaly: true # 4 times
field :created_at, Types::TimeType, null: true
field :last_activity_at, Types::TimeType, null: true
......@@ -40,7 +40,7 @@ module Types
field :lfs_enabled, GraphQL::BOOLEAN_TYPE, null: true
field :merge_requests_ff_only_enabled, GraphQL::BOOLEAN_TYPE, null: true
field :avatar_url, GraphQL::STRING_TYPE, null: true, resolve: -> (project, args, ctx) do
field :avatar_url, GraphQL::STRING_TYPE, null: true, calls_gitaly: true, resolve: -> (project, args, ctx) do
project.avatar_url(only_path: false)
end
......
......@@ -6,9 +6,9 @@ module Types
authorize :download_code
field :root_ref, GraphQL::STRING_TYPE, null: true
field :empty, GraphQL::BOOLEAN_TYPE, null: false, method: :empty?
field :root_ref, GraphQL::STRING_TYPE, null: true, calls_gitaly: true
field :empty, GraphQL::BOOLEAN_TYPE, null: false, method: :empty?, calls_gitaly: true
field :exists, GraphQL::BOOLEAN_TYPE, null: false, method: :exists?
field :tree, Types::Tree::TreeType, null: true, resolver: Resolvers::TreeResolver
field :tree, Types::Tree::TreeType, null: true, resolver: Resolvers::TreeResolver, calls_gitaly: true
end
end
......@@ -7,7 +7,7 @@ module Types
graphql_name 'Tree'
# Complexity 10 as it triggers a Gitaly call on each render
field :last_commit, Types::CommitType, null: true, complexity: 10, resolve: -> (tree, args, ctx) do
field :last_commit, Types::CommitType, null: true, complexity: 10, calls_gitaly: true, resolve: -> (tree, args, ctx) do
tree.repository.last_commit_for_path(tree.sha, tree.path)
end
......@@ -15,9 +15,9 @@ module Types
Gitlab::Graphql::Representation::TreeEntry.decorate(obj.trees, obj.repository)
end
field :submodules, Types::Tree::SubmoduleType.connection_type, null: false
field :submodules, Types::Tree::SubmoduleType.connection_type, null: false, calls_gitaly: true
field :blobs, Types::Tree::BlobType.connection_type, null: false, resolve: -> (obj, args, ctx) do
field :blobs, Types::Tree::BlobType.connection_type, null: false, calls_gitaly: true, resolve: -> (obj, args, ctx) do
Gitlab::Graphql::Representation::TreeEntry.decorate(obj.blobs, obj.repository)
end
# rubocop: enable Graphql/AuthorizeTypes
......
......@@ -8,7 +8,7 @@ module Gitlab
extend ActiveSupport::Concern
def self.use(schema_definition)
schema_definition.instrument(:field, Instrumentation.new, after_built_ins: true)
schema_definition.instrument(:field, Gitlab::Graphql::Authorize::Instrumentation.new, after_built_ins: true)
end
end
end
......
# frozen_string_literal: true
module Gitlab
module Graphql
# Wraps the field resolution to count Gitaly calls before and after.
# Raises an error if the field calls Gitaly but hadn't declared such.
module CallsGitaly
extend ActiveSupport::Concern
def self.use(schema_definition)
schema_definition.instrument(:field, Gitlab::Graphql::CallsGitaly::Instrumentation.new, after_built_ins: true)
end
end
end
end
# frozen_string_literal: true
module Gitlab
module Graphql
module CallsGitaly
class Instrumentation
# Check if any `calls_gitaly: true` declarations need to be added
# Do nothing if a constant complexity was provided
def instrument(_type, field)
type_object = field.metadata[:type_class]
return field unless type_object.respond_to?(:calls_gitaly?)
return field if type_object.constant_complexity? || type_object.calls_gitaly?
old_resolver_proc = field.resolve_proc
gitaly_wrapped_resolve = -> (typed_object, args, ctx) do
previous_gitaly_call_count = Gitlab::GitalyClient.get_request_count
result = old_resolver_proc.call(typed_object, args, ctx)
current_gitaly_call_count = Gitlab::GitalyClient.get_request_count
calls_gitaly_check(type_object, current_gitaly_call_count - previous_gitaly_call_count)
result
end
field.redefine do
resolve(gitaly_wrapped_resolve)
end
end
def calls_gitaly_check(type_object, calls)
return if calls < 1
# Will inform you if there needs to be `calls_gitaly: true` as a kwarg in the field declaration
# if there is at least 1 Gitaly call involved with the field resolution.
error = RuntimeError.new("Gitaly is called for field '#{type_object.name}' on #{type_object.owner.try(:name)} - please either specify a constant complexity or add `calls_gitaly: true` to the field declaration")
Gitlab::Sentry.track_exception(error)
end
end
end
end
end
......@@ -6,11 +6,12 @@ module Gitlab
extend ActiveSupport::Concern
class_methods do
def mount_mutation(mutation_class)
def mount_mutation(mutation_class, **custom_kwargs)
# Using an underscored field name symbol will make `graphql-ruby`
# standardize the field name
field mutation_class.graphql_name.underscore.to_sym,
mutation: mutation_class
mutation: mutation_class,
**custom_kwargs
end
end
end
......
......@@ -21,6 +21,10 @@ describe GitlabSchema do
expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::Present::Instrumentation))
end
it 'enables using gitaly call checker' do
expect(field_instrumenters).to include(instance_of(::Gitlab::Graphql::CallsGitaly::Instrumentation))
end
it 'has the base mutation' do
expect(described_class.mutation).to eq(::Types::MutationType.to_graphql)
end
......
......@@ -22,6 +22,24 @@ describe Types::BaseField do
expect(field.to_graphql.complexity).to eq 1
end
describe '#base_complexity' do
context 'with no gitaly calls' do
it 'defaults to 1' do
field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true)
expect(field.base_complexity).to eq 1
end
end
context 'with a gitaly call' do
it 'adds 1 to the default value' do
field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, calls_gitaly: true)
expect(field.base_complexity).to eq 2
end
end
end
it 'has specified value' do
field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, complexity: 12)
......@@ -52,5 +70,46 @@ describe Types::BaseField do
end
end
end
context 'calls_gitaly' do
context 'for fields with a resolver' do
it 'adds 1 if true' do
with_gitaly_field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, resolver_class: resolver, null: true, calls_gitaly: true)
without_gitaly_field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, resolver_class: resolver, null: true)
base_result = without_gitaly_field.to_graphql.complexity.call({}, {}, 2)
expect(with_gitaly_field.to_graphql.complexity.call({}, {}, 2)).to eq base_result + 1
end
end
context 'for fields without a resolver' do
it 'adds 1 if true' do
field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, calls_gitaly: true)
expect(field.to_graphql.complexity).to eq 2
end
end
it 'defaults to false' do
field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true)
expect(field.base_complexity).to eq Types::BaseField::DEFAULT_COMPLEXITY
end
context 'with declared constant complexity value' do
it 'has complexity set to that constant' do
field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, complexity: 12)
expect(field.to_graphql.complexity).to eq 12
end
it 'does not raise an error even with Gitaly calls' do
allow(Gitlab::GitalyClient).to receive(:get_request_count).and_return([0, 1])
field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, complexity: 12)
expect(field.to_graphql.complexity).to eq 12
end
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Graphql::CallsGitaly::Instrumentation do
subject { described_class.new }
describe '#calls_gitaly_check' do
let(:gitaly_field) { Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, calls_gitaly: true) }
let(:no_gitaly_field) { Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, calls_gitaly: false) }
context 'if there are no Gitaly calls' do
it 'does not raise an error if calls_gitaly is false' do
expect { subject.send(:calls_gitaly_check, no_gitaly_field, 0) }.not_to raise_error
end
end
context 'if there is at least 1 Gitaly call' do
it 'raises an error if calls_gitaly: is false or not defined' do
expect { subject.send(:calls_gitaly_check, no_gitaly_field, 1) }.to raise_error(/specify a constant complexity or add `calls_gitaly: true`/)
end
end
end
end
......@@ -131,4 +131,35 @@ describe 'GraphQL' do
end
end
end
describe 'testing for Gitaly calls' do
let(:project) { create(:project, :repository) }
let(:user) { create(:user) }
let(:query) do
graphql_query_for('project', { 'fullPath' => project.full_path }, %w(id))
end
before do
project.add_developer(user)
end
it_behaves_like 'a working graphql query' do
before do
post_graphql(query, current_user: user)
end
end
context 'when Gitaly is called' do
before do
allow(Gitlab::GitalyClient).to receive(:get_request_count).and_return(1, 2)
end
it "logs a warning that the 'calls_gitaly' field declaration is missing" do
expect(Gitlab::Sentry).to receive(:track_exception).once
post_graphql(query, current_user: user)
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