diff options
| -rw-r--r-- | app/graphql/gitlab_schema.rb | 1 | ||||
| -rw-r--r-- | app/graphql/types/base_field.rb | 22 | ||||
| -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 | 30 | ||||
| -rw-r--r-- | spec/graphql/gitlab_schema_spec.rb | 4 | ||||
| -rw-r--r-- | spec/graphql/types/base_field_spec.rb | 18 | ||||
| -rw-r--r-- | spec/requests/api/graphql_spec.rb | 31 |
8 files changed, 99 insertions, 24 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 95db116d6f9..64bc7e6474f 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -21,6 +21,18 @@ module Types complexity end + def calls_gitaly_check(calls) + return if @calls_gitaly + + # Will inform you if :calls_gitaly should be true or false based on the number of Gitaly calls + # involved with the request. + if calls > 0 + raise "Gitaly is called for field '#{name}' - please add `calls_gitaly: true` to the field declaration" + end + rescue => e + Gitlab::Sentry.track_exception(e) + end + private def field_complexity(resolver_class) @@ -54,15 +66,5 @@ module Types complexity.to_i end end - - def calls_gitaly_check - # Will inform you if :calls_gitaly should be true or false based on the number of Gitaly calls - # involved with the request. - if @calls_gitaly && Gitlab::GitalyClient.get_request_count == 0 - raise "Gitaly is called for field '#{name}' - please add `calls_gitaly: true` to the field declaration" - end - rescue => e - Gitlab::Sentry.track_exception(e) - end end end 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..f75941e269f --- /dev/null +++ b/lib/gitlab/graphql/calls_gitaly.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + # Allow fields to declare permissions their objects must have. The field + # will be set to nil unless all required permissions are present. + 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..ca54e12c049 --- /dev/null +++ b/lib/gitlab/graphql/calls_gitaly/instrumentation.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module CallsGitaly + class Instrumentation + # Check if any `calls_gitaly: true` declarations need to be added + def instrument(_type, field) + type_object = field.metadata[:type_class] + return field unless type_object && type_object.respond_to?(:calls_gitaly_check) + + old_resolver_proc = field.resolve_proc + wrapped_proc = gitaly_wrapped_resolve(old_resolver_proc, type_object) + field.redefine { resolve(wrapped_proc) } + end + + def gitaly_wrapped_resolve(old_resolver_proc, type_object) + proc do |parent_typed_object, args, ctx| + previous_gitaly_call_count = Gitlab::GitalyClient.get_request_count + + old_resolver_proc.call(parent_typed_object, args, ctx) + + current_gitaly_call_count = Gitlab::GitalyClient.get_request_count + type_object.calls_gitaly_check(current_gitaly_call_count - previous_gitaly_call_count) + end + end + end + 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 d7360872508..0be83ea60c4 100644 --- a/spec/graphql/types/base_field_spec.rb +++ b/spec/graphql/types/base_field_spec.rb @@ -106,26 +106,18 @@ describe Types::BaseField do let(:no_gitaly_field) { described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, calls_gitaly: false) } context 'if there are no Gitaly calls' do - before do - allow(Gitlab::GitalyClient).to receive(:get_request_count).and_return(0) - end - it 'does not raise an error if calls_gitaly is false' do - expect { no_gitaly_field.send(:calls_gitaly_check) }.not_to raise_error - end - - it 'raises an error if calls_gitaly: true appears' do - expect { gitaly_field.send(:calls_gitaly_check) }.to raise_error(/please add `calls_gitaly: true`/) + expect { no_gitaly_field.send(:calls_gitaly_check, 0) }.not_to raise_error end end context 'if there is at least 1 Gitaly call' do - before do - allow(Gitlab::GitalyClient).to receive(:get_request_count).and_return(1) + it 'does not raise an error if calls_gitaly is true' do + expect { gitaly_field.send(:calls_gitaly_check, 1) }.not_to raise_error end - it 'does not raise an error if calls_gitaly is true' do - expect { gitaly_field.send(:calls_gitaly_check) }.not_to raise_error + it 'raises an error if calls_gitaly: is false or not defined' do + expect { no_gitaly_field.send(:calls_gitaly_check, 1) }.to raise_error(/please add `calls_gitaly: true`/) end end end diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index 656d6f8b50b..d78b17827a6 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(forksCount)) + 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 |