Commit 3182a946 authored by Drew Blessing's avatar Drew Blessing

Prevent Group API N+1 loading group plans

When querying the Groups Project API and including subgroups,
N+1 queries can take place when loading plans for all the groups
in the hierarchy. Plans are only applied at the top-level group
so it is unnecessarily inefficient to load plans for each project
and associated group in the hierarchy. This change loads the plans
once and memoizes them for all project groups.

Changelog: fixed
EE: true
parent 52ce12b6
# frozen_string_literal: true
module Preloaders
class SingleHierarchyProjectGroupPlansPreloader
attr_reader :projects
def initialize(projects_relation)
@projects = projects_relation
end
def execute
# no-op in FOSS
end
end
end
Preloaders::SingleHierarchyProjectGroupPlansPreloader.prepend_mod_with('Preloaders::SingleHierarchyProjectGroupPlansPreloader')
---
name: group_project_api_preload_plans
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77538
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/350176
milestone: '14.7'
type: development
group: group::authentication and authorization
default_enabled: false
# frozen_string_literal: true
module EE
module Preloaders
module SingleHierarchyProjectGroupPlansPreloader
def execute
return unless ::Feature.enabled?(:group_project_api_preload_plans, default_enabled: :yaml)
return unless ::Gitlab::CurrentSettings.should_check_namespace_plan?
return unless project = projects.take
plans = project.namespace.root_ancestor.plans
preload_plans(plans)
end
def preload_plans(plans)
return unless plans.any?
projects.each do |project|
project.namespace.memoized_plans = plans
end
end
end
end
end
...@@ -632,6 +632,54 @@ RSpec.describe API::Groups do ...@@ -632,6 +632,54 @@ RSpec.describe API::Groups do
end end
end end
end end
context 'when namespace license checks are enabled', :saas do
before do
enable_namespace_license_check!
end
context 'when there are plans and projects' do
let(:group) { create(:group_with_plan, plan: :ultimate_plan) }
before do
subgroup = create(:group, parent: group)
create(:project, group: group)
create(:project, group: subgroup)
end
it 'only loads plans once' do
expect(Plan).to receive(:hosted_plans_for_namespaces).once.and_call_original
get api("/groups/#{group.id}/projects", user), params: { include_subgroups: true }
expect(response).to have_gitlab_http_status(:ok)
end
context 'when the feature flag is disabled' do
before do
stub_feature_flags(group_project_api_preload_plans: false)
end
it 'does not preload plans' do
expect(Plan).to receive(:hosted_plans_for_namespaces).at_least(:twice).and_call_original
get api("/groups/#{group.id}/projects", user), params: { include_subgroups: true }
expect(response).to have_gitlab_http_status(:ok)
end
end
end
context 'when there are no projects' do
let(:group) { create(:group) }
it 'completes the request without error' do
get api("/groups/#{group.id}/projects", user), params: { include_subgroups: true }
expect(response).to have_gitlab_http_status(:ok)
end
end
end
end end
describe 'GET group/:id/audit_events' do describe 'GET group/:id/audit_events' do
......
...@@ -84,10 +84,11 @@ module API ...@@ -84,10 +84,11 @@ module API
paginate(projects) paginate(projects)
end end
def present_projects(params, projects) def present_projects(params, projects, single_hierarchy: false)
options = { options = {
with: params[:simple] ? Entities::BasicProjectDetails : Entities::Project, with: params[:simple] ? Entities::BasicProjectDetails : Entities::Project,
current_user: current_user current_user: current_user,
single_hierarchy: single_hierarchy
} }
projects, options = with_custom_attributes(projects, options) projects, options = with_custom_attributes(projects, options)
...@@ -306,7 +307,7 @@ module API ...@@ -306,7 +307,7 @@ module API
projects = find_group_projects(params, finder_options) projects = find_group_projects(params, finder_options)
present_projects(params, projects) present_projects(params, projects, single_hierarchy: true)
end end
desc 'Get a list of shared projects in this group' do desc 'Get a list of shared projects in this group' do
......
...@@ -13,6 +13,7 @@ module API ...@@ -13,6 +13,7 @@ module API
preload_repository_cache(projects_relation) preload_repository_cache(projects_relation)
Preloaders::UserMaxAccessLevelInProjectsPreloader.new(projects_relation, options[:current_user]).execute if options[:current_user] Preloaders::UserMaxAccessLevelInProjectsPreloader.new(projects_relation, options[:current_user]).execute if options[:current_user]
Preloaders::SingleHierarchyProjectGroupPlansPreloader.new(projects_relation).execute if options[:single_hierarchy]
projects_relation projects_relation
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