Address reviewer comments58409-increase-graphql-complexity-for-fields-that-make-gitaly-calls

- Remove Gitaly call check for fields that have a constant complexity
declared
- Add associated test

6 files changed, 34 insertions, 25 deletions

diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb
index 6b377e88e16..efeee4a7a4d 100644
--- a/app/graphql/types/base_field.rb
+++ b/app/graphql/types/base_field.rb
@@ -8,6 +8,7 @@ module Types
 
     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)
@@ -23,6 +24,10 @@ module Types
       @calls_gitaly
     end
 
+    def constant_complexity?
+      @constant_complexity
+    end
+
     private
 
     def field_complexity(resolver_class)
diff --git a/app/graphql/types/tree/tree_type.rb b/app/graphql/types/tree/tree_type.rb
index 2023abc13f9..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
 
diff --git a/lib/gitlab/graphql/calls_gitaly.rb b/lib/gitlab/graphql/calls_gitaly.rb
index f75941e269f..40cd74a34f2 100644
--- a/lib/gitlab/graphql/calls_gitaly.rb
+++ b/lib/gitlab/graphql/calls_gitaly.rb
@@ -2,8 +2,8 @@
 
 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.
+    # 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
 
diff --git a/lib/gitlab/graphql/calls_gitaly/instrumentation.rb b/lib/gitlab/graphql/calls_gitaly/instrumentation.rb
index e2733a1416f..fbd5e348c7d 100644
--- a/lib/gitlab/graphql/calls_gitaly/instrumentation.rb
+++ b/lib/gitlab/graphql/calls_gitaly/instrumentation.rb
@@ -5,9 +5,11 @@ module Gitlab
     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 && type_object.respond_to?(:calls_gitaly?)
+          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
 
@@ -25,12 +27,11 @@ module Gitlab
         end
 
         def calls_gitaly_check(type_object, calls)
-          return if type_object.calls_gitaly?
           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 add `calls_gitaly: true` to the field declaration")
+          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
diff --git a/spec/graphql/types/base_field_spec.rb b/spec/graphql/types/base_field_spec.rb
index 10913e530cf..77ef8933717 100644
--- a/spec/graphql/types/base_field_spec.rb
+++ b/spec/graphql/types/base_field_spec.rb
@@ -96,10 +96,19 @@ describe Types::BaseField do
         expect(field.base_complexity).to eq Types::BaseField::DEFAULT_COMPLEXITY
       end
 
-      it 'is overridden by declared complexity value' do
-        field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, calls_gitaly: true, complexity: 12)
+      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
+          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
diff --git a/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb b/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb
index 92d200bfd4e..d93ce464a92 100644
--- a/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb
+++ b/spec/lib/gitlab/graphql/calls_gitaly/instrumentation_spec.rb
@@ -4,25 +4,19 @@ require 'spec_helper'
 describe Gitlab::Graphql::CallsGitaly::Instrumentation do
   subject { described_class.new }
 
-  context 'when considering complexity' do
-    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) }
+  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
+    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 'does not raise an error if calls_gitaly is true' do
-          expect { subject.send(:calls_gitaly_check, gitaly_field, 1) }.not_to raise_error
-        end
-
-        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(/please add `calls_gitaly: true`/)
-        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