diff --git a/app/models/group.rb b/app/models/group.rb index 76262acf50c..27287f079a4 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -289,6 +289,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 diff --git a/app/models/project.rb b/app/models/project.rb index f9c82029912..0a7e7617d9b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1113,7 +1113,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 diff --git a/app/models/user.rb b/app/models/user.rb index 5e7fe01c825..76fd395be9a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1002,7 +1002,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 diff --git a/app/services/base_count_service.rb b/app/services/base_count_service.rb index 19873fe09a5..f2844854112 100644 --- a/app/services/base_count_service.rb +++ b/app/services/base_count_service.rb @@ -12,8 +12,12 @@ class BaseCountService 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 diff --git a/app/services/projects/batch_count_service.rb b/app/services/projects/batch_count_service.rb new file mode 100644 index 00000000000..178ebc5a143 --- /dev/null +++ b/app/services/projects/batch_count_service.rb @@ -0,0 +1,31 @@ +# 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 diff --git a/app/services/projects/batch_forks_count_service.rb b/app/services/projects/batch_forks_count_service.rb new file mode 100644 index 00000000000..e61fe6c86b2 --- /dev/null +++ b/app/services/projects/batch_forks_count_service.rb @@ -0,0 +1,18 @@ +# 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 diff --git a/app/services/projects/batch_open_issues_count_service.rb b/app/services/projects/batch_open_issues_count_service.rb new file mode 100644 index 00000000000..3b0ade2419b --- /dev/null +++ b/app/services/projects/batch_open_issues_count_service.rb @@ -0,0 +1,16 @@ +# 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 diff --git a/app/services/projects/count_service.rb b/app/services/projects/count_service.rb index 7e575b2d6f3..933829b557b 100644 --- a/app/services/projects/count_service.rb +++ b/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 diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb index d9bdf3a8ad7..dc6eb19affd 100644 --- a/app/services/projects/forks_count_service.rb +++ b/app/services/projects/forks_count_service.rb @@ -1,12 +1,15 @@ 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 diff --git a/app/services/projects/open_issues_count_service.rb b/app/services/projects/open_issues_count_service.rb index 25de97325e2..a975a06a05c 100644 --- a/app/services/projects/open_issues_count_service.rb +++ b/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 diff --git a/changelogs/unreleased/sh-optimize-groups-api.yml b/changelogs/unreleased/sh-optimize-groups-api.yml new file mode 100644 index 00000000000..37b74715a81 --- /dev/null +++ b/changelogs/unreleased/sh-optimize-groups-api.yml @@ -0,0 +1,5 @@ +--- +title: Optimize API /groups/:id/projects by preloading associations +merge_request: +author: +type: performance diff --git a/lib/api/entities.rb b/lib/api/entities.rb index ce332fe85d2..7cec8da013d 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -88,13 +88,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 @@ -146,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] } @@ -156,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 @@ -164,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 @@ -618,9 +646,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 @@ -680,7 +710,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 @@ -689,11 +719,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 diff --git a/lib/api/groups.rb b/lib/api/groups.rb index bcf2e6dae1d..b81f07a1770 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -52,6 +52,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, @@ -170,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 'Get a list of subgroups in this group.' do diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 4cd7e714aa2..14a4fc6f025 100644 --- a/lib/api/projects.rb +++ b/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 diff --git a/lib/api/projects_relation_builder.rb b/lib/api/projects_relation_builder.rb new file mode 100644 index 00000000000..6482fd94ab8 --- /dev/null +++ b/lib/api/projects_relation_builder.rb @@ -0,0 +1,34 @@ +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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 34e160aa599..000d5b7126d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -315,7 +315,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 @@ -2472,7 +2471,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 diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 04a658cd6c3..554723d6b1e 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -401,6 +401,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 diff --git a/spec/services/projects/count_service_spec.rb b/spec/services/projects/count_service_spec.rb index cc496501bad..183f6128c7b 100644 --- a/spec/services/projects/count_service_spec.rb +++ b/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