Merge branch '62706-graphql-complexity-values-are-incorrectly-doubled' into 'master'

Reduce GraphQL complexity for non-connection fields

Closes #62706

See merge request gitlab-org/gitlab-ce!29165

5 files changed, 43 insertions, 27 deletions

diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb
index a374851e835..dd0d9105df6 100644
--- a/app/graphql/types/base_field.rb
+++ b/app/graphql/types/base_field.rb
@@ -29,15 +29,18 @@ module Types
       # 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, child_complexity: child_complexity).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
+        field_defn = to_graphql
+
+        if field_defn.connection?
+          # Resolvers may add extra complexity depending on number of items being loaded.
+          page_size   = field_defn.connection_max_page_size || ctx.schema.default_max_page_size
+          limit_value = [args[:first], args[:last], page_size].compact.min
+          multiplier  = self.resolver&.try(:complexity_multiplier, args).to_f
+          complexity += complexity * limit_value * multiplier
+        end
 
         complexity.to_i
       end
diff --git a/spec/graphql/resolvers/base_resolver_spec.rb b/spec/graphql/resolvers/base_resolver_spec.rb
index 9982288e206..c162fdbbb47 100644
--- a/spec/graphql/resolvers/base_resolver_spec.rb
+++ b/spec/graphql/resolvers/base_resolver_spec.rb
@@ -29,18 +29,20 @@ describe Resolvers::BaseResolver do
     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)
+  context 'when field is a connection' do
+    it 'increases complexity based on arguments' do
+      field = Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE.connection_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
+      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)
+    it 'does not increase complexity when filtering by iids' do
+      field = Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE.connection_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
+      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
 end
diff --git a/spec/graphql/resolvers/issues_resolver_spec.rb b/spec/graphql/resolvers/issues_resolver_spec.rb
index bffcdbfe915..798fe00de97 100644
--- a/spec/graphql/resolvers/issues_resolver_spec.rb
+++ b/spec/graphql/resolvers/issues_resolver_spec.rb
@@ -121,7 +121,7 @@ describe Resolvers::IssuesResolver do
   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)
+    field = Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE.connection_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
diff --git a/spec/graphql/resolvers/namespace_projects_resolver_spec.rb b/spec/graphql/resolvers/namespace_projects_resolver_spec.rb
index 395e08081d3..20e197e9f73 100644
--- a/spec/graphql/resolvers/namespace_projects_resolver_spec.rb
+++ b/spec/graphql/resolvers/namespace_projects_resolver_spec.rb
@@ -57,7 +57,7 @@ describe Resolvers::NamespaceProjectsResolver, :nested_groups do
   end
 
   it 'has an high complexity regardless of arguments' do
-    field = Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE, resolver_class: described_class, null: false, max_page_size: 100)
+    field = Types::BaseField.new(name: 'test', type: GraphQL::STRING_TYPE.connection_type, resolver_class: described_class, null: false, max_page_size: 100)
 
     expect(field.to_graphql.complexity.call({}, {}, 1)).to eq 24
     expect(field.to_graphql.complexity.call({}, { include_subgroups: true }, 1)).to eq 24
diff --git a/spec/graphql/types/base_field_spec.rb b/spec/graphql/types/base_field_spec.rb
index a7fb156d9a8..0d3c3e37daf 100644
--- a/spec/graphql/types/base_field_spec.rb
+++ b/spec/graphql/types/base_field_spec.rb
@@ -28,18 +28,29 @@ 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)
+    context 'when field has a resolver proc' do
+      context 'and is a connection' do
+        let(:field) { described_class.new(name: 'test', type: GraphQL::STRING_TYPE.connection_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 arguments for resolvers' do
+          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)
+        it 'sets complexity depending on number load limits for resolvers' do
+          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
 
-      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
+      context 'and is not a connection' do
+        it 'sets complexity as normal' 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 2
+          expect(field.to_graphql.complexity.call({}, { first: 50 }, 2)).to eq 2
+        end
+      end
     end
   end
 end