Merge branch 'sh-optimize-groups-api-ee' into 'master'

Port from CE: Optimize API /groups/:id/projects by preloading assocations

See merge request gitlab-org/gitlab-ee!3604

app/models/group.rb

@@ -335,6 +335,14 @@ class Group < Namespace
     "#{parent.full_path}/#{path_was}"
   end
 
+  def group_member(user)
+    if group_members.loaded?
+      group_members.find { |gm| gm.user_id == user.id }
+    else
+      group_members.find_by(user_id: user)
+    end
+  end
+
   private
 
   def update_two_factor_requirement

app/models/project.rb

@@ -1124,7 +1124,11 @@ class Project < ActiveRecord::Base
   end
 
   def project_member(user)
-    project_members.find_by(user_id: user)
+    if project_members.loaded?
+      project_members.find { |member| member.user_id == user.id }
+    else
+      project_members.find_by(user_id: user)
+    end
   end
 
   def default_branch

app/models/user.rb

@@ -1021,7 +1021,11 @@ class User < ActiveRecord::Base
   end
 
   def notification_settings_for(source)
-    notification_settings.find_or_initialize_by(source: source)
+    if notification_settings.loaded?
+      notification_settings.find { |notification| notification.source == source }
+    else
+      notification_settings.find_or_initialize_by(source: source)
+    end
   end
 
   # Lazy load global notification setting

app/services/base_count_service.rb

@@ -9,11 +9,15 @@ class BaseCountService
   end
 
   def count
-    Rails.cache.fetch(cache_key, raw: raw?) { uncached_count }.to_i
+    Rails.cache.fetch(cache_key, cache_options) { uncached_count }.to_i
   end
 
-  def refresh_cache
-    Rails.cache.write(cache_key, uncached_count, raw: raw?)
+  def count_stored?
+    Rails.cache.read(cache_key).present?
+  end
+
+  def refresh_cache(&block)
+    Rails.cache.write(cache_key, block_given? ? yield : uncached_count, raw: raw?)
   end
 
   def uncached_count
@@ -31,4 +35,10 @@ class BaseCountService
   def cache_key
     raise NotImplementedError, 'cache_key must be implemented and return a String'
   end
+
+  # subclasses can override to add any specific options, such as
+  #   super.merge({ expires_in: 5.minutes })
+  def cache_options
+    { raw: raw? }
+  end
 end

app/services/projects/batch_count_service.rb

+# Service class for getting and caching the number of elements of several projects
+# Warning: do not user this service with a really large set of projects
+# because the service use maps to retrieve the project ids.
+module Projects
+  class BatchCountService
+    def initialize(projects)
+      @projects = projects
+    end
+
+    def refresh_cache
+      @projects.each do |project|
+        service = count_service.new(project)
+        unless service.count_stored?
+          service.refresh_cache { global_count[project.id].to_i }
+        end
+      end
+    end
+
+    def project_ids
+      @projects.map(&:id)
+    end
+
+    def global_count(project)
+      raise NotImplementedError, 'global_count must be implemented and return an hash indexed by the project id'
+    end
+
+    def count_service
+      raise NotImplementedError, 'count_service must be implemented and return a Projects::CountService object'
+    end
+  end
+end

app/services/projects/batch_forks_count_service.rb

+# Service class for getting and caching the number of forks of several projects
+# Warning: do not user this service with a really large set of projects
+# because the service use maps to retrieve the project ids
+module Projects
+  class BatchForksCountService < Projects::BatchCountService
+    def global_count
+      @global_count ||= begin
+        count_service.query(project_ids)
+                     .group(:forked_from_project_id)
+                     .count
+      end
+    end
+
+    def count_service
+      ::Projects::ForksCountService
+    end
+  end
+end

app/services/projects/batch_open_issues_count_service.rb

+# Service class for getting and caching the number of issues of several projects
+# Warning: do not user this service with a really large set of projects
+# because the service use maps to retrieve the project ids
+module Projects
+  class BatchOpenIssuesCountService < Projects::BatchCountService
+    def global_count
+      @global_count ||= begin
+        count_service.query(project_ids).group(:project_id).count
+      end
+    end
+
+    def count_service
+      ::Projects::OpenIssuesCountService
+    end
+  end
+end

app/services/projects/count_service.rb

@@ -11,6 +11,10 @@ module Projects
       @project = project
     end
 
+    def relation_for_count
+      self.class.query(@project.id)
+    end
+
     def cache_key_name
       raise(
         NotImplementedError,
@@ -21,5 +25,12 @@ module Projects
     def cache_key
       ['projects', 'count_service', VERSION, @project.id, cache_key_name]
     end
+
+    def self.query(project_ids)
+      raise(
+        NotImplementedError,
+        '"query" must be implemented and return an ActiveRecord::Relation'
+      )
+    end
   end
 end

app/services/projects/forks_count_service.rb

 module Projects
   # Service class for getting and caching the number of forks of a project.
   class ForksCountService < Projects::CountService
-    def relation_for_count
-      @project.forks
-    end
-
     def cache_key_name
       'forks_count'
     end
+
+    def self.query(project_ids)
+      # We can't directly change ForkedProjectLink to ForkNetworkMember here
+      # Nowadays, when a call using v3 to projects/:id/fork is made,
+      # the relationship to ForkNetworkMember is not updated
+      ForkedProjectLink.where(forked_from_project: project_ids)
+    end
   end
 end

app/services/projects/open_issues_count_service.rb

@@ -2,14 +2,14 @@ module Projects
   # Service class for counting and caching the number of open issues of a
   # project.
   class OpenIssuesCountService < Projects::CountService
-    def relation_for_count
-      # We don't include confidential issues in this number since this would
-      # expose the number of confidential issues to non project members.
-      @project.issues.opened.public_only
-    end
-
     def cache_key_name
       'open_issues_count'
     end
+
+    def self.query(project_ids)
+      # We don't include confidential issues in this number since this would
+      # expose the number of confidential issues to non project members.
+      Issue.opened.public_only.where(project: project_ids)
+    end
   end
 end

changelogs/unreleased/sh-optimize-groups-api.yml

+---
+title: Optimize API /groups/:id/projects by preloading associations
+merge_request:
+author:
+type: performance

lib/api/entities.rb

@@ -98,13 +98,29 @@ module API
     end
 
     class BasicProjectDetails < ProjectIdentity
-      expose :default_branch, :tag_list
+      include ::API::ProjectsRelationBuilder
+
+      expose :default_branch
+      # Avoids an N+1 query: https://github.com/mbleigh/acts-as-taggable-on/issues/91#issuecomment-168273770
+      expose :tag_list do |project|
+        # project.tags.order(:name).pluck(:name) is the most suitable option
+        # to avoid loading all the ActiveRecord objects but, if we use it here
+        # it override the preloaded associations and makes a query
+        # (fixed in https://github.com/rails/rails/pull/25976).
+        project.tags.map(&:name).sort
+      end
       expose :ssh_url_to_repo, :http_url_to_repo, :web_url
       expose :avatar_url do |project, options|
         project.avatar_url(only_path: false)
       end
       expose :star_count, :forks_count
       expose :last_activity_at
+
+      def self.preload_relation(projects_relation, options =  {})
+        projects_relation.preload(:project_feature, :route)
+                         .preload(namespace: [:route, :owner],
+                                  tags: :taggings)
+      end
     end
 
     class Project < BasicProjectDetails
@@ -156,7 +172,7 @@ module API
       expose :shared_runners_enabled
       expose :lfs_enabled?, as: :lfs_enabled
       expose :creator_id
-      expose :namespace, using: 'API::Entities::Namespace'
+      expose :namespace, using: 'API::Entities::NamespaceBasic'
       expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? }
       expose :import_status
       expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] }
@@ -166,7 +182,7 @@ module API
       expose :public_builds, as: :public_jobs
       expose :ci_config_path
       expose :shared_with_groups do |project, options|
-        SharedGroup.represent(project.project_group_links.all, options)
+        SharedGroup.represent(project.project_group_links, options)
       end
       expose :only_allow_merge_if_pipeline_succeeds
       expose :repository_storage, if: lambda { |_project, options| options[:current_user].try(:admin?) }
@@ -178,6 +194,18 @@ module API
       expose :approvals_before_merge, if: ->(project, _) { project.feature_available?(:merge_request_approvers) }
 
       expose :statistics, using: 'API::Entities::ProjectStatistics', if: :statistics
+
+      def self.preload_relation(projects_relation, options =  {})
+        super(projects_relation).preload(:group)
+                                .preload(project_group_links: :group,
+                                         fork_network: :root_project,
+                                         forked_project_link: :forked_from_project,
+                                         forked_from_project: [:route, :forks, namespace: :route, tags: :taggings])
+      end
+
+      def self.forks_counting_projects(projects_relation)
+        projects_relation + projects_relation.map(&:forked_from_project).compact
+      end
     end
 
     class ProjectStatistics < Grape::Entity
@@ -690,9 +718,11 @@ module API
       expose :created_at
     end
 
-    class Namespace < Grape::Entity
+    class NamespaceBasic < Grape::Entity
       expose :id, :name, :path, :kind, :full_path, :parent_id
+    end
 
+    class Namespace < NamespaceBasic
       expose :members_count_with_descendants, if: -> (namespace, opts) { expose_members_count_with_descendants?(namespace, opts) } do |namespace, _|
         namespace.users_with_descendants.count
       end
@@ -758,7 +788,7 @@ module API
           if options.key?(:project_members)
             (options[:project_members] || []).find { |member| member.source_id == project.id }
           else
-            project.project_members.find_by(user_id: options[:current_user].id)
+            project.project_member(options[:current_user])
           end
         end
 
@@ -767,11 +797,25 @@ module API
             if options.key?(:group_members)
               (options[:group_members] || []).find { |member| member.source_id == project.namespace_id }
             else
-              project.group.group_members.find_by(user_id: options[:current_user].id)
+              project.group.group_member(options[:current_user])
             end
           end
         end
       end
+
+      def self.preload_relation(projects_relation, options = {})
+        relation = super(projects_relation, options)
+
+        unless options.key?(:group_members)
+          relation = relation.preload(group: [group_members: [:source, user: [notification_settings: :source]]])
+        end
+
+        unless options.key?(:project_members)
+          relation = relation.preload(project_members: [:source, user: [notification_settings: :source]])
+        end
+
+        relation
+      end
     end
 
     class LabelBasic < Grape::Entity

lib/api/groups.rb

@@ -61,6 +61,13 @@ module API
         groups
       end
 
+      def find_group_projects(params)
+        group = find_group!(params[:id])
+        projects = GroupProjectsFinder.new(group: group, current_user: current_user, params: project_finder_params).execute
+        projects = reorder_projects(projects)
+        paginate(projects)
+      end
+
       def present_groups(params, groups)
         options = {
           with: Entities::Group,
@@ -203,11 +210,10 @@ module API
         use :pagination
       end
       get ":id/projects" do
-        group = find_group!(params[:id])
-        projects = GroupProjectsFinder.new(group: group, current_user: current_user, params: project_finder_params).execute
-        projects = reorder_projects(projects)
+        projects = find_group_projects(params)
         entity = params[:simple] ? Entities::BasicProjectDetails : Entities::Project
-        present paginate(projects), with: entity, current_user: current_user
+
+        present entity.prepare_relation(projects), with: entity, current_user: current_user
       end
 
       desc 'Get a list of subgroups in this group.' do

lib/api/projects.rb

@@ -85,11 +85,11 @@ module API
         projects = projects.with_statistics if params[:statistics]
         projects = projects.with_issues_enabled if params[:with_issues_enabled]
         projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled]
+        projects = paginate(projects)
 
         if current_user
-          projects = projects.includes(:route, :taggings, namespace: :route)
-          project_members = current_user.project_members
-          group_members = current_user.group_members
+          project_members = current_user.project_members.preload(:source, user: [notification_settings: :source])
+          group_members = current_user.group_members.preload(:source, user: [notification_settings: :source])
         end
 
         options = options.reverse_merge(
@@ -101,7 +101,7 @@ module API
         )
         options[:with] = Entities::BasicProjectDetails if params[:simple]
 
-        present paginate(projects), options
+        present options[:with].prepare_relation(projects, options), options
       end
     end
 

lib/api/projects_relation_builder.rb

+module API
+  module ProjectsRelationBuilder
+    extend ActiveSupport::Concern
+
+    module ClassMethods
+      def prepare_relation(projects_relation, options = {})
+        projects_relation = preload_relation(projects_relation, options)
+        execute_batch_counting(projects_relation)
+        projects_relation
+      end
+
+      def preload_relation(projects_relation, options =  {})
+        projects_relation
+      end
+
+      def forks_counting_projects(projects_relation)
+        projects_relation
+      end
+
+      def batch_forks_counting(projects_relation)
+        ::Projects::BatchForksCountService.new(forks_counting_projects(projects_relation)).refresh_cache
+      end
+
+      def batch_open_issues_counting(projects_relation)
+        ::Projects::BatchOpenIssuesCountService.new(projects_relation).refresh_cache
+      end
+
+      def execute_batch_counting(projects_relation)
+        batch_forks_counting(projects_relation)
+        batch_open_issues_counting(projects_relation)
+      end
+    end
+  end
+end

spec/models/project_spec.rb

@@ -352,7 +352,6 @@ describe Project do
 
     it { is_expected.to delegate_method(:empty_repo?).to(:repository) }
     it { is_expected.to delegate_method(:members).to(:team).with_prefix(true) }
-    it { is_expected.to delegate_method(:count).to(:forks).with_prefix(true) }
     it { is_expected.to delegate_method(:name).to(:owner).with_prefix(true).with_arguments(allow_nil: true) }
   end
 
@@ -2844,7 +2843,7 @@ describe Project do
     it 'returns the number of forks' do
       project = build(:project)
 
-      allow(project.forks).to receive(:count).and_return(1)
+      expect_any_instance_of(Projects::ForksCountService).to receive(:count).and_return(1)
 
       expect(project.forks_count).to eq(1)
     end

spec/requests/api/groups_spec.rb

@@ -445,6 +445,20 @@ describe API::Groups do
 
         expect(response).to have_gitlab_http_status(404)
       end
+
+      it 'avoids N+1 queries' do
+        get api("/groups/#{group1.id}/projects", admin)
+
+        control_count = ActiveRecord::QueryRecorder.new do
+          get api("/groups/#{group1.id}/projects", admin)
+        end.count
+
+        create(:project, namespace: group1)
+
+        expect do
+          get api("/groups/#{group1.id}/projects", admin)
+        end.not_to exceed_query_limit(control_count)
+      end
     end
 
     context 'when using group path in URL' do

spec/requests/api/projects_spec.rb

@@ -891,8 +891,7 @@ describe API::Projects do
           'path' => user.namespace.path,
           'kind' => user.namespace.kind,
           'full_path' => user.namespace.full_path,
-          'parent_id' => nil,
-          'plan' => nil
+          'parent_id' => nil
         })
       end
 

spec/services/projects/count_service_spec.rb

@@ -4,9 +4,18 @@ describe Projects::CountService do
   let(:project) { build(:project, id: 1) }
   let(:service) { described_class.new(project) }
 
-  describe '#relation_for_count' do
+  describe '.query' do
     it 'raises NotImplementedError' do
       expect { service.relation_for_count }.to raise_error(NotImplementedError)
+      expect { described_class.query(project.id) }.to raise_error(NotImplementedError)
+    end
+  end
+
+  describe '#relation_for_count' do
+    it 'calls the class method query with the project id' do
+      expect(described_class).to receive(:query).with(project.id)
+
+      service.relation_for_count
     end
   end