Merge branch 'ld-graphql-apply-look-ahead-to-group-resolvers' into 'master'

Add concern to add preloading to group resolvers

See merge request gitlab-org/gitlab!70034

app/graphql/resolvers/concerns/resolves_groups.rb

+# frozen_string_literal: true
+
+# Mixin for all resolver classes for type `Types::GroupType.connection_type`.
+module ResolvesGroups
+  extend ActiveSupport::Concern
+  include LooksAhead
+
+  def resolve_with_lookahead(**args)
+    apply_lookahead(resolve_groups(**args))
+  end
+
+  private
+
+  # The resolver should implement this method.
+  def resolve_groups(**args)
+    raise NotImplementedError
+  end
+
+  def preloads
+    {
+      contacts: [:contacts],
+      container_repositories_count: [:container_repositories],
+      custom_emoji: [:custom_emoji],
+      full_path: [:route],
+      organizations: [:organizations],
+      path: [:route],
+      dependency_proxy_blob_count: [:dependency_proxy_blobs],
+      dependency_proxy_blobs: [:dependency_proxy_blobs],
+      dependency_proxy_image_count: [:dependency_proxy_manifests],
+      dependency_proxy_image_ttl_policy: [:dependency_proxy_image_ttl_policy],
+      dependency_proxy_setting: [:dependency_proxy_setting]
+    }
+  end
+end

app/graphql/resolvers/groups_resolver.rb

@@ -2,6 +2,8 @@
 
 module Resolvers
   class GroupsResolver < BaseResolver
+    include ResolvesGroups
+
     type Types::GroupType, null: true
 
     argument :include_parent_descendants, GraphQL::Types::Boolean,
@@ -19,16 +21,12 @@ module Resolvers
 
     alias_method :parent, :object
 
-    def resolve(**args)
-      return [] unless parent.present?
-
-      find_groups(args)
-    end
-
     private
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def find_groups(args)
+    def resolve_groups(args)
+      return Group.none unless parent.present?
+
       GroupsFinder
         .new(context[:current_user], args.merge(parent: parent))
         .execute

app/graphql/resolvers/users/groups_resolver.rb

@@ -3,8 +3,8 @@
 module Resolvers
   module Users
     class GroupsResolver < BaseResolver
+      include ResolvesGroups
       include Gitlab::Graphql::Authorize::AuthorizeResource
-      include LooksAhead
 
       type Types::GroupType.connection_type, null: true
 
@@ -23,19 +23,14 @@ module Resolvers
         Preloaders::UserMaxAccessLevelInGroupsPreloader.new(nodes, current_user).execute
       end
 
-      def resolve_with_lookahead(**args)
-        return unless Feature.enabled?(:paginatable_namespace_drop_down_for_project_creation, current_user, default_enabled: :yaml)
-
-        apply_lookahead(Groups::UserGroupsFinder.new(current_user, object, args).execute)
+      def ready?(**args)
+        Feature.enabled?(:paginatable_namespace_drop_down_for_project_creation, current_user, default_enabled: :yaml)
       end
 
       private
 
-      def preloads
-        {
-          path: [:route],
-          full_path: [:route]
-        }
+      def resolve_groups(**args)
+        Groups::UserGroupsFinder.new(current_user, object, args).execute
       end
     end
   end

app/graphql/types/group_type.rb

@@ -34,6 +34,7 @@ module Types
           null: true,
           method: :project_creation_level_str,
           description: 'Permission level required to create projects in the group.'
+
     field :subgroup_creation_level,
           type: GraphQL::Types::String,
           null: true,
@@ -44,6 +45,7 @@ module Types
           type: GraphQL::Types::Boolean,
           null: true,
           description: 'Indicates if all users in this group are required to set up two-factor authentication.'
+
     field :two_factor_grace_period,
           type: GraphQL::Types::Int,
           null: true,
@@ -225,11 +227,11 @@ module Types
     end
 
     def dependency_proxy_image_count
-      group.dependency_proxy_manifests.count
+      group.dependency_proxy_manifests.size
     end
 
     def dependency_proxy_blob_count
-      group.dependency_proxy_blobs.count
+      group.dependency_proxy_blobs.size
     end
 
     def dependency_proxy_total_size

app/models/group.rb

@@ -56,6 +56,9 @@ class Group < Namespace
   has_many :boards
   has_many :badges, class_name: 'GroupBadge'
 
+  has_many :organizations, class_name: 'CustomerRelations::Organization', inverse_of: :group
+  has_many :contacts, class_name: 'CustomerRelations::Contact', inverse_of: :group
+
   has_many :cluster_groups, class_name: 'Clusters::Group'
   has_many :clusters, through: :cluster_groups, class_name: 'Clusters::Cluster'
 
@@ -757,14 +760,6 @@ class Group < Namespace
     Timelog.in_group(self)
   end
 
-  def organizations
-    ::CustomerRelations::Organization.where(group_id: self.id)
-  end
-
-  def contacts
-    ::CustomerRelations::Contact.where(group_id: self.id)
-  end
-
   def dependency_proxy_image_ttl_policy
     super || build_dependency_proxy_image_ttl_policy
   end

spec/graphql/resolvers/concerns/resolves_groups_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ResolvesGroups do
+  include GraphqlHelpers
+  include AfterNextHelpers
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:groups) { create_pair(:group) }
+
+  let_it_be(:resolver) do
+    Class.new(Resolvers::BaseResolver) do
+      include ResolvesGroups
+      type Types::GroupType, null: true
+    end
+  end
+
+  let_it_be(:query_type) do
+    query_factory do |query|
+      query.field :groups,
+                  Types::GroupType.connection_type,
+                  null: true,
+                  resolver: resolver
+    end
+  end
+
+  let_it_be(:lookahead_fields) do
+    <<~FIELDS
+      contacts { nodes { id } }
+      containerRepositoriesCount
+      customEmoji { nodes { id } }
+      fullPath
+      organizations { nodes { id } }
+      path
+      dependencyProxyBlobCount
+      dependencyProxyBlobs { nodes { fileName } }
+      dependencyProxyImageCount
+      dependencyProxyImageTtlPolicy { enabled }
+      dependencyProxySetting { enabled }
+    FIELDS
+  end
+
+  it 'avoids N+1 queries on the fields marked with lookahead' do
+    group_ids = groups.map(&:id)
+
+    allow_next(resolver).to receive(:resolve_groups).and_return(Group.id_in(group_ids))
+    # Prevent authorization queries from affecting the test.
+    allow(Ability).to receive(:allowed?).and_return(true)
+
+    single_group_query = ActiveRecord::QueryRecorder.new do
+      data = query_groups(limit: 1)
+      expect(data.size).to eq(1)
+    end
+
+    multi_group_query = -> {
+      data = query_groups(limit: 2)
+      expect(data.size).to eq(2)
+    }
+
+    expect { multi_group_query.call }.not_to exceed_query_limit(single_group_query)
+  end
+
+  def query_groups(limit:)
+    query_string = "{ groups(first: #{limit}) { nodes { id #{lookahead_fields} } } }"
+
+    data = execute_query(query_type, graphql: query_string)
+
+    graphql_dig_at(data, :data, :groups, :nodes)
+  end
+end

spec/models/group_spec.rb

@@ -37,6 +37,8 @@ RSpec.describe Group do
     it { is_expected.to have_many(:daily_build_group_report_results).class_name('Ci::DailyBuildGroupReportResult') }
     it { is_expected.to have_many(:group_callouts).class_name('Users::GroupCallout').with_foreign_key(:group_id) }
     it { is_expected.to have_many(:bulk_import_exports).class_name('BulkImports::Export') }
+    it { is_expected.to have_many(:contacts).class_name('CustomerRelations::Contact') }
+    it { is_expected.to have_many(:organizations).class_name('CustomerRelations::Organization') }
 
     describe '#members & #requesters' do
       let(:requester) { create(:user) }