Commit 65b7a7a0 authored by Douwe Maan's avatar Douwe Maan

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

Optimize API /groups/:id/projects by preloading assocations

Closes #40308

See merge request gitlab-org/gitlab-ce!15475
parents 6a27f9b1 264171f7
...@@ -289,6 +289,14 @@ class Group < Namespace ...@@ -289,6 +289,14 @@ class Group < Namespace
"#{parent.full_path}/#{path_was}" "#{parent.full_path}/#{path_was}"
end 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 private
def update_two_factor_requirement def update_two_factor_requirement
......
...@@ -1113,7 +1113,11 @@ class Project < ActiveRecord::Base ...@@ -1113,7 +1113,11 @@ class Project < ActiveRecord::Base
end end
def project_member(user) 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 end
def default_branch def default_branch
......
...@@ -1002,7 +1002,11 @@ class User < ActiveRecord::Base ...@@ -1002,7 +1002,11 @@ class User < ActiveRecord::Base
end end
def notification_settings_for(source) 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 end
# Lazy load global notification setting # Lazy load global notification setting
......
...@@ -12,8 +12,12 @@ class BaseCountService ...@@ -12,8 +12,12 @@ class BaseCountService
Rails.cache.fetch(cache_key, cache_options) { uncached_count }.to_i Rails.cache.fetch(cache_key, cache_options) { uncached_count }.to_i
end end
def refresh_cache def count_stored?
Rails.cache.write(cache_key, uncached_count, raw: raw?) Rails.cache.read(cache_key).present?
end
def refresh_cache(&block)
Rails.cache.write(cache_key, block_given? ? yield : uncached_count, raw: raw?)
end end
def uncached_count def uncached_count
......
# 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
# 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
# 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
...@@ -11,6 +11,10 @@ module Projects ...@@ -11,6 +11,10 @@ module Projects
@project = project @project = project
end end
def relation_for_count
self.class.query(@project.id)
end
def cache_key_name def cache_key_name
raise( raise(
NotImplementedError, NotImplementedError,
...@@ -21,5 +25,12 @@ module Projects ...@@ -21,5 +25,12 @@ module Projects
def cache_key def cache_key
['projects', 'count_service', VERSION, @project.id, cache_key_name] ['projects', 'count_service', VERSION, @project.id, cache_key_name]
end end
def self.query(project_ids)
raise(
NotImplementedError,
'"query" must be implemented and return an ActiveRecord::Relation'
)
end
end end
end end
module Projects module Projects
# Service class for getting and caching the number of forks of a project. # Service class for getting and caching the number of forks of a project.
class ForksCountService < Projects::CountService class ForksCountService < Projects::CountService
def relation_for_count
@project.forks
end
def cache_key_name def cache_key_name
'forks_count' 'forks_count'
end 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
end end
...@@ -2,14 +2,14 @@ module Projects ...@@ -2,14 +2,14 @@ module Projects
# Service class for counting and caching the number of open issues of a # Service class for counting and caching the number of open issues of a
# project. # project.
class OpenIssuesCountService < Projects::CountService 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 def cache_key_name
'open_issues_count' 'open_issues_count'
end 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
end end
---
title: Optimize API /groups/:id/projects by preloading associations
merge_request:
author:
type: performance
...@@ -88,13 +88,29 @@ module API ...@@ -88,13 +88,29 @@ module API
end end
class BasicProjectDetails < ProjectIdentity 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 :ssh_url_to_repo, :http_url_to_repo, :web_url
expose :avatar_url do |project, options| expose :avatar_url do |project, options|
project.avatar_url(only_path: false) project.avatar_url(only_path: false)
end end
expose :star_count, :forks_count expose :star_count, :forks_count
expose :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 end
class Project < BasicProjectDetails class Project < BasicProjectDetails
...@@ -146,7 +162,7 @@ module API ...@@ -146,7 +162,7 @@ module API
expose :shared_runners_enabled expose :shared_runners_enabled
expose :lfs_enabled?, as: :lfs_enabled expose :lfs_enabled?, as: :lfs_enabled
expose :creator_id 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 :forked_from_project, using: Entities::BasicProjectDetails, if: lambda { |project, options| project.forked? }
expose :import_status expose :import_status
expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] } expose :import_error, if: lambda { |_project, options| options[:user_can_admin_project] }
...@@ -156,7 +172,7 @@ module API ...@@ -156,7 +172,7 @@ module API
expose :public_builds, as: :public_jobs expose :public_builds, as: :public_jobs
expose :ci_config_path expose :ci_config_path
expose :shared_with_groups do |project, options| expose :shared_with_groups do |project, options|
SharedGroup.represent(project.project_group_links.all, options) SharedGroup.represent(project.project_group_links, options)
end end
expose :only_allow_merge_if_pipeline_succeeds expose :only_allow_merge_if_pipeline_succeeds
expose :request_access_enabled expose :request_access_enabled
...@@ -164,6 +180,18 @@ module API ...@@ -164,6 +180,18 @@ module API
expose :printing_merge_request_link_enabled expose :printing_merge_request_link_enabled
expose :statistics, using: 'API::Entities::ProjectStatistics', if: :statistics 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 end
class ProjectStatistics < Grape::Entity class ProjectStatistics < Grape::Entity
...@@ -618,9 +646,11 @@ module API ...@@ -618,9 +646,11 @@ module API
expose :created_at expose :created_at
end end
class Namespace < Grape::Entity class NamespaceBasic < Grape::Entity
expose :id, :name, :path, :kind, :full_path, :parent_id 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, _| expose :members_count_with_descendants, if: -> (namespace, opts) { expose_members_count_with_descendants?(namespace, opts) } do |namespace, _|
namespace.users_with_descendants.count namespace.users_with_descendants.count
end end
...@@ -680,7 +710,7 @@ module API ...@@ -680,7 +710,7 @@ module API
if options.key?(:project_members) if options.key?(:project_members)
(options[:project_members] || []).find { |member| member.source_id == project.id } (options[:project_members] || []).find { |member| member.source_id == project.id }
else else
project.project_members.find_by(user_id: options[:current_user].id) project.project_member(options[:current_user])
end end
end end
...@@ -689,11 +719,25 @@ module API ...@@ -689,11 +719,25 @@ module API
if options.key?(:group_members) if options.key?(:group_members)
(options[:group_members] || []).find { |member| member.source_id == project.namespace_id } (options[:group_members] || []).find { |member| member.source_id == project.namespace_id }
else else
project.group.group_members.find_by(user_id: options[:current_user].id) project.group.group_member(options[:current_user])
end end
end end
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 end
class LabelBasic < Grape::Entity class LabelBasic < Grape::Entity
......
...@@ -52,6 +52,13 @@ module API ...@@ -52,6 +52,13 @@ module API
groups groups
end 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) def present_groups(params, groups)
options = { options = {
with: Entities::Group, with: Entities::Group,
...@@ -170,11 +177,10 @@ module API ...@@ -170,11 +177,10 @@ module API
use :pagination use :pagination
end end
get ":id/projects" do get ":id/projects" do
group = find_group!(params[:id]) projects = find_group_projects(params)
projects = GroupProjectsFinder.new(group: group, current_user: current_user, params: project_finder_params).execute
projects = reorder_projects(projects)
entity = params[:simple] ? Entities::BasicProjectDetails : Entities::Project 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 end
desc 'Get a list of subgroups in this group.' do desc 'Get a list of subgroups in this group.' do
......
...@@ -79,11 +79,11 @@ module API ...@@ -79,11 +79,11 @@ module API
projects = projects.with_statistics if params[:statistics] projects = projects.with_statistics if params[:statistics]
projects = projects.with_issues_enabled if params[:with_issues_enabled] projects = projects.with_issues_enabled if params[:with_issues_enabled]
projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled] projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled]
projects = paginate(projects)
if current_user if current_user
projects = projects.includes(:route, :taggings, namespace: :route) project_members = current_user.project_members.preload(:source, user: [notification_settings: :source])
project_members = current_user.project_members group_members = current_user.group_members.preload(:source, user: [notification_settings: :source])
group_members = current_user.group_members
end end
options = options.reverse_merge( options = options.reverse_merge(
...@@ -95,7 +95,7 @@ module API ...@@ -95,7 +95,7 @@ module API
) )
options[:with] = Entities::BasicProjectDetails if params[:simple] options[:with] = Entities::BasicProjectDetails if params[:simple]
present paginate(projects), options present options[:with].prepare_relation(projects, options), options
end end
end end
......
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
...@@ -315,7 +315,6 @@ describe Project do ...@@ -315,7 +315,6 @@ describe Project do
it { is_expected.to delegate_method(:empty_repo?).to(:repository) } 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(: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) } it { is_expected.to delegate_method(:name).to(:owner).with_prefix(true).with_arguments(allow_nil: true) }
end end
...@@ -2472,7 +2471,7 @@ describe Project do ...@@ -2472,7 +2471,7 @@ describe Project do
it 'returns the number of forks' do it 'returns the number of forks' do
project = build(:project) 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) expect(project.forks_count).to eq(1)
end end
......
...@@ -401,6 +401,20 @@ describe API::Groups do ...@@ -401,6 +401,20 @@ describe API::Groups do
expect(response).to have_gitlab_http_status(404) expect(response).to have_gitlab_http_status(404)
end 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 end
context 'when using group path in URL' do context 'when using group path in URL' do
......
...@@ -4,9 +4,17 @@ describe Projects::CountService do ...@@ -4,9 +4,17 @@ describe Projects::CountService do
let(:project) { build(:project, id: 1) } let(:project) { build(:project, id: 1) }
let(:service) { described_class.new(project) } let(:service) { described_class.new(project) }
describe '#relation_for_count' do describe '.query' do
it 'raises NotImplementedError' 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
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment