Merge branch 'sh-optimize-groups-api-10-2' into '10-2-stable-patch-5'

app/models/group.rb

@@ -303,6 +303,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

@@ -978,7 +978,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

@@ -12,8 +12,12 @@ class BaseCountService
     Rails.cache.fetch(cache_key, raw: raw?) { 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

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

@@ -80,16 +80,37 @@ module API
       expose :group_access, as: :group_access_level
     end
 
-    class BasicProjectDetails < Grape::Entity
-      expose :id, :description, :default_branch, :tag_list
-      expose :ssh_url_to_repo, :http_url_to_repo, :web_url
+    class ProjectIdentity < Grape::Entity
+      expose :id, :description
       expose :name, :name_with_namespace
       expose :path, :path_with_namespace
+      expose :created_at
+    end
+
+    class BasicProjectDetails < ProjectIdentity
+      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 :created_at, :last_activity_at
+      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
@@ -141,7 +162,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] }
@@ -151,7 +172,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 :request_access_enabled
@@ -159,6 +180,18 @@ module API
       expose :printing_merge_request_link_enabled
 
       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
@@ -622,9 +655,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
@@ -684,7 +719,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
 
@@ -693,11 +728,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
@@ -833,17 +882,24 @@ module API
       expose :id, :sha, :ref, :status
     end
 
-    class Job < Grape::Entity
+    class JobBasic < Grape::Entity
       expose :id, :status, :stage, :name, :ref, :tag, :coverage
       expose :created_at, :started_at, :finished_at
       expose :duration
       expose :user, with: User
-      expose :artifacts_file, using: JobArtifactFile, if: -> (job, opts) { job.artifacts? }
       expose :commit, with: Commit
-      expose :runner, with: Runner
       expose :pipeline, with: PipelineBasic
     end
 
+    class Job < JobBasic
+      expose :artifacts_file, using: JobArtifactFile, if: -> (job, opts) { job.artifacts? }
+      expose :runner, with: Runner
+    end
+
+    class JobBasicWithProject < JobBasic
+      expose :project, with: ProjectIdentity
+    end
+
     class Trigger < Grape::Entity
       expose :id
       expose :token, :description

lib/api/groups.rb

@@ -25,24 +25,7 @@ module API
         optional :statistics, type: Boolean, default: false, desc: 'Include project statistics'
       end
 
-      def present_groups(groups, options = {})
-        options = options.reverse_merge(
-          with: Entities::Group,
-          current_user: current_user
-        )
-
-        groups = groups.with_statistics if options[:statistics]
-        present paginate(groups), options
-      end
-    end
-
-    resource :groups do
-      include CustomAttributesEndpoints
-
-      desc 'Get a groups list' do
-        success Entities::Group
-      end
-      params do
+      params :group_list_params do
         use :statistics_params
         optional :skip_groups, type: Array[Integer], desc: 'Array of group ids to exclude from list'
         optional :all_available, type: Boolean, desc: 'Show all group that you have access to'
@@ -52,19 +35,54 @@ module API
         optional :sort, type: String, values: %w[asc desc], default: 'asc', desc: 'Sort by asc (ascending) or desc (descending)'
         use :pagination
       end
-      get do
+
+      def find_groups(params)
         find_params = {
           all_available: params[:all_available],
-          owned: params[:owned],
-          custom_attributes: params[:custom_attributes]
+          custom_attributes: params[:custom_attributes],
+          owned: params[:owned]
         }
+        find_params[:parent] = find_group!(params[:id]) if params[:id]
 
         groups = GroupsFinder.new(current_user, find_params).execute
         groups = groups.search(params[:search]) if params[:search].present?
         groups = groups.where.not(id: params[:skip_groups]) if params[:skip_groups].present?
         groups = groups.reorder(params[:order_by] => params[:sort])
 
-        present_groups groups, statistics: params[:statistics] && current_user.admin?
+        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,
+          current_user: current_user,
+          statistics: params[:statistics] && current_user.admin?
+        }
+
+        groups = groups.with_statistics if options[:statistics]
+        present paginate(groups), options
+      end
+    end
+
+    resource :groups do
+      include CustomAttributesEndpoints
+
+      desc 'Get a groups list' do
+        success Entities::Group
+      end
+      params do
+        use :group_list_params
+      end
+      get do
+        groups = find_groups(params)
+        present_groups params, groups
       end
 
       desc 'Create a group. Available only for users who can create groups.' do
@@ -159,11 +177,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 'Transfer a project to the group namespace. Available only for admin.' do

lib/api/projects.rb

@@ -79,11 +79,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(
@@ -95,7 +95,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

@@ -314,7 +314,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
 
@@ -2458,7 +2457,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

@@ -463,6 +463,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/services/projects/count_service_spec.rb

@@ -4,9 +4,17 @@ 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