diff options
-rw-r--r-- | app/graphql/gitlab_schema.rb | 1 | ||||
-rw-r--r-- | app/graphql/types/base_field.rb | 19 | ||||
-rw-r--r-- | app/graphql/types/merge_request_type.rb | 2 | ||||
-rw-r--r-- | app/graphql/types/mutation_type.rb | 2 | ||||
-rw-r--r-- | app/graphql/types/permission_types/merge_request.rb | 4 | ||||
-rw-r--r-- | app/graphql/types/project_type.rb | 4 | ||||
-rw-r--r-- | app/graphql/types/repository_type.rb | 6 | ||||
-rw-r--r-- | app/graphql/types/tree/tree_type.rb | 6 | ||||
-rw-r--r-- | lib/gitlab/graphql/authorize.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/graphql/calls_gitaly.rb | 15 | ||||
-rw-r--r-- | lib/gitlab/graphql/calls_gitaly/instrumentation.rb | 40 | ||||
-rw-r--r-- | lib/gitlab/graphql/mount_mutation.rb | 5 | ||||
-rw-r--r-- | spec/graphql/gitlab_schema_spec.rb | 4 | ||||
-rw-r--r-- | spec/graphql/types/base_field_spec.rb | 59 | ||||
-rw-r--r-- | spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb | 23 | ||||
-rw-r--r-- | spec/requests/api/graphql_spec.rb | 31 |
16 files changed, 207 insertions, 16 deletions
diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index 5615909c4ec..152ebb930e2 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -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 diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index dd0d9105df6..efeee4a7a4d 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -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 diff --git a/app/graphql/types/merge_request_type.rb b/app/graphql/types/merge_request_type.rb index 577ccd48ef8..6734d4761c2 100644 --- a/app/graphql/types/merge_request_type.rb +++ b/app/graphql/types/merge_request_type.rb @@ -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 diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 6ef1d816b7c..bc5fb709522 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -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 diff --git a/app/graphql/types/permission_types/merge_request.rb b/app/graphql/types/permission_types/merge_request.rb index 13995d3ea8f..d877fc177d2 100644 --- a/app/graphql/types/permission_types/merge_request.rb +++ b/app/graphql/types/permission_types/merge_request.rb @@ -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 diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index c25688ab043..13be71c26ee 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -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 diff --git a/app/graphql/types/repository_type.rb b/app/graphql/types/repository_type.rb index 5987467e1ea..b024eca61fc 100644 --- a/app/graphql/types/repository_type.rb +++ b/app/graphql/types/repository_type.rb @@ -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 diff --git a/app/graphql/types/tree/tree_type.rb b/app/graphql/types/tree/tree_type.rb index b947713074e..fbdc1597461 100644 --- a/app/graphql/types/tree/tree_type.rb +++ b/app/graphql/types/tree/tree_type.rb @@ -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 diff --git a/lib/gitlab/graphql/authorize.rb b/lib/gitlab/graphql/authorize.rb index f8d0208e275..e83b567308b 100644 --- a/lib/gitlab/graphql/authorize.rb +++ b/lib/gitlab/graphql/authorize.rb @@ -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 diff --git a/lib/gitlab/graphql/calls_gitaly.rb b/lib/gitlab/graphql/calls_gitaly.rb new file mode 100644 index 00000000000..40cd74a34f2 --- /dev/null +++ b/lib/gitlab/graphql/calls_gitaly.rb @@ -0,0 +1,15 @@ +# 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 diff --git a/lib/gitlab/graphql/calls_gitaly/instrumentation.rb b/lib/gitlab/graphql/calls_gitaly/instrumentation.rb new file mode 100644 index 00000000000..fbd5e348c7d --- /dev/null +++ b/lib/gitlab/graphql/calls_gitaly/instrumentation.rb @@ -0,0 +1,40 @@ +# 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 diff --git a/lib/gitlab/graphql/mount_mutation.rb b/lib/gitlab/graphql/mount_mutation.rb index 9048967d4e1..b10e963170a 100644 --- a/lib/gitlab/graphql/mount_mutation.rb +++ b/lib/gitlab/graphql/mount_mutation.rb @@ -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 diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb index d36e428a8ee..93b86b9b812 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -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 diff --git a/spec/graphql/types/base_field_spec.rb b/spec/graphql/types/base_field_spec.rb index 0d3c3e37daf..77ef8933717 100644 --- a/spec/graphql/types/base_field_spec.rb +++ b/spec/graphql/types/base_field_spec.rb @@ -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 diff --git a/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb b/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb new file mode 100644 index 00000000000..d93ce464a92 --- /dev/null +++ b/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb @@ -0,0 +1,23 @@ +# 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 diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index 656d6f8b50b..67371cb35b6 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -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 |