From ae537ddf9bda445f20e2e7607eed4a62fa0f4e69 Mon Sep 17 00:00:00 2001
From: Vasilii Iakliushin <viakliushin@gitlab.com>
Date: Tue, 22 Aug 2023 20:05:00 +0000
Subject: [PATCH] Optimize groups template page query
Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/381077
**Problem**
The query to fetch group templates is slow.
**Solution**
Optimize fetching logic to produce better execution plans.
EE: true
---
app/models/namespace.rb | 1 +
app/models/plan.rb | 2 +
.../optimize_group_template_query.yml | 8 ++
.../finders/groups_with_templates_finder.rb | 23 ++--
ee/app/models/ee/user.rb | 2 +-
ee/app/models/gitlab_subscription.rb | 4 +
.../groups_with_templates_finder_spec.rb | 115 +++++++++++-------
ee/spec/models/ee/user_spec.rb | 31 +++--
ee/spec/models/gitlab_subscription_spec.rb | 19 ++-
spec/models/namespace_spec.rb | 9 ++
spec/models/plan_spec.rb | 12 ++
11 files changed, 166 insertions(+), 60 deletions(-)
create mode 100644 config/feature_flags/development/optimize_group_template_query.yml
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index be39b894ef960..6ba2bf39bb6f3 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -166,6 +166,7 @@ class Namespace < ApplicationRecord
scope :sort_by_type, -> { order(arel_table[:type].asc.nulls_first) }
scope :include_route, -> { includes(:route) }
scope :by_parent, -> (parent) { where(parent_id: parent) }
+ scope :by_root_id, -> (root_id) { where('traversal_ids[1] IN (?)', root_id) }
scope :filter_by_path, -> (query) { where('lower(path) = :query', query: query.downcase) }
scope :in_organization, -> (organization) { where(organization: organization) }
diff --git a/app/models/plan.rb b/app/models/plan.rb
index 22c1201421c80..9ab22bc045a7d 100644
--- a/app/models/plan.rb
+++ b/app/models/plan.rb
@@ -5,6 +5,8 @@ class Plan < MainClusterwide::ApplicationRecord
has_one :limits, class_name: 'PlanLimits'
+ scope :by_name, ->(name) { where(name: name) }
+
ALL_PLANS = [DEFAULT].freeze
DEFAULT_PLANS = [DEFAULT].freeze
private_constant :ALL_PLANS, :DEFAULT_PLANS
diff --git a/config/feature_flags/development/optimize_group_template_query.yml b/config/feature_flags/development/optimize_group_template_query.yml
new file mode 100644
index 0000000000000..d5dc8b16476f3
--- /dev/null
+++ b/config/feature_flags/development/optimize_group_template_query.yml
@@ -0,0 +1,8 @@
+---
+name: optimize_group_template_query
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/129399
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/422390
+milestone: '16.4'
+type: development
+group: group::source code
+default_enabled: false
diff --git a/ee/app/finders/groups_with_templates_finder.rb b/ee/app/finders/groups_with_templates_finder.rb
index 6a11b6443acdc..64f6462a8ebe5 100644
--- a/ee/app/finders/groups_with_templates_finder.rb
+++ b/ee/app/finders/groups_with_templates_finder.rb
@@ -1,6 +1,7 @@
# frozen_string_literal: true
class GroupsWithTemplatesFinder
- def initialize(group_id = nil)
+ def initialize(user, group_id = nil)
+ @user = user
@group_id = group_id
end
@@ -15,14 +16,22 @@ def execute
private
- attr_reader :group_id
+ attr_reader :user, :group_id
def extended_group_search
- Group
- .with_project_templates
- .self_and_ancestors
- .with_feature_available_in_plan(:group_project_templates)
- .self_and_descendants
+ if Feature.enabled?(:optimize_group_template_query, user) && Feature.enabled?(:use_traversal_ids)
+ plan_ids = Plan
+ .by_name(GitlabSubscriptions::Features.saas_plans_with_feature(:group_project_templates))
+ .pluck(:id) # rubocop: disable CodeReuse/ActiveRecord
+
+ Group.by_root_id(GitlabSubscription.by_hosted_plan_ids(plan_ids).select(:namespace_id))
+ else
+ Group
+ .with_project_templates
+ .self_and_ancestors
+ .with_feature_available_in_plan(:group_project_templates)
+ .self_and_descendants
+ end
end
def simple_group_search(groups)
diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb
index 19cc1fef12650..88ff8cc9f566f 100644
--- a/ee/app/models/ee/user.rb
+++ b/ee/app/models/ee/user.rb
@@ -303,7 +303,7 @@ def search_membership_ancestry
end
def available_subgroups_with_custom_project_templates(group_id = nil)
- found_groups = GroupsWithTemplatesFinder.new(group_id).execute
+ found_groups = GroupsWithTemplatesFinder.new(self, group_id).execute
::GroupsFinder.new(self, min_access_level: ::Gitlab::Access::DEVELOPER)
.execute
diff --git a/ee/app/models/gitlab_subscription.rb b/ee/app/models/gitlab_subscription.rb
index cd487d2756499..0ae3b72c3649a 100644
--- a/ee/app/models/gitlab_subscription.rb
+++ b/ee/app/models/gitlab_subscription.rb
@@ -28,6 +28,10 @@ class GitlabSubscription < ApplicationRecord
delegate :name, :title, to: :hosted_plan, prefix: :plan, allow_nil: true
delegate :exclude_guests?, to: :namespace
+ scope :by_hosted_plan_ids, ->(plan_ids) do
+ where(hosted_plan_id: plan_ids)
+ end
+
scope :with_hosted_plan, -> (plan_name) do
joins(:hosted_plan).where(trial: false, 'plans.name' => plan_name)
.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/422013')
diff --git a/ee/spec/finders/groups_with_templates_finder_spec.rb b/ee/spec/finders/groups_with_templates_finder_spec.rb
index 74740cc9a6af5..96a424065008e 100644
--- a/ee/spec/finders/groups_with_templates_finder_spec.rb
+++ b/ee/spec/finders/groups_with_templates_finder_spec.rb
@@ -2,7 +2,10 @@
require 'spec_helper'
-RSpec.describe GroupsWithTemplatesFinder, :saas do
+RSpec.describe GroupsWithTemplatesFinder, :saas, feature_category: :source_code_management do
+ subject { described_class.new(user, group_id).execute }
+
+ let_it_be(:user) { create(:user) }
let_it_be(:group_1, reload: true) { create(:group, name: 'group-1') }
let_it_be(:group_2, reload: true) { create(:group, name: 'group-2') }
let_it_be(:group_3, reload: true) { create(:group, name: 'group-3') }
@@ -15,6 +18,8 @@
let_it_be(:subgroup_4, reload: true) { create(:group, parent: group_1, name: 'subgroup-4') }
let_it_be(:subgroup_5) { create(:group, parent: subgroup_4, name: 'subgroup-5') }
+ let(:group_id) { nil }
+
before do
group_1.update!(custom_project_templates_group_id: subgroup_1.id)
group_2.update!(custom_project_templates_group_id: subgroup_2.id)
@@ -26,56 +31,39 @@
create(:gitlab_subscription, :premium, namespace: group_2)
end
- describe 'without group id' do
- it 'returns all groups' do
- expect(described_class.new.execute).to contain_exactly(group_1, group_2, group_3)
- end
-
- context 'when namespace checked' do
- before do
- stub_ee_application_setting(should_check_namespace_plan: true)
+ shared_examples 'groups_with_templates' do
+ describe 'without group id' do
+ it 'returns all groups' do
+ is_expected.to contain_exactly(group_1, group_2, group_3)
end
- it 'returns groups on ultimate/premium plan' do
- expect(described_class.new.execute).to contain_exactly(group_1, group_2)
- end
-
- context 'with subgroup with template' do
+ context 'when namespace checked' do
before do
- subgroup_4.update!(custom_project_templates_group_id: subgroup_5.id)
- create(:project, namespace: subgroup_5)
+ stub_ee_application_setting(should_check_namespace_plan: true)
end
it 'returns groups on ultimate/premium plan' do
- expect(described_class.new.execute).to contain_exactly(group_1, group_2, subgroup_4)
+ is_expected.to contain_exactly(group_1, group_2)
end
- end
- end
- end
-
- describe 'with group id' do
- it 'returns given group with it descendants' do
- expect(described_class.new(group_1.id).execute).to contain_exactly(group_1)
- end
- context 'with subgroup with template' do
- before do
- subgroup_4.update!(custom_project_templates_group_id: subgroup_5.id)
- create(:project, namespace: subgroup_5)
- end
+ context 'with subgroup with template' do
+ before do
+ subgroup_4.update!(custom_project_templates_group_id: subgroup_5.id)
+ create(:project, namespace: subgroup_5)
+ end
- it 'returns only chosen group' do
- expect(described_class.new(group_1.id).execute).to contain_exactly(group_1)
+ it 'returns groups on ultimate/premium plan' do
+ is_expected.to contain_exactly(group_1, group_2, subgroup_4)
+ end
+ end
end
end
- context 'when namespace checked' do
- before do
- stub_ee_application_setting(should_check_namespace_plan: true)
- end
+ describe 'with group id' do
+ let(:group_id) { group_1.id }
- it 'does not return the group' do
- expect(described_class.new(group_3.id).execute).to be_empty
+ it 'returns given group with it descendants' do
+ is_expected.to contain_exactly(group_1)
end
context 'with subgroup with template' do
@@ -85,13 +73,58 @@
end
it 'returns only chosen group' do
- expect(described_class.new(group_1.id).execute).to contain_exactly(group_1)
+ is_expected.to contain_exactly(group_1)
end
+ end
+
+ context 'when namespace checked' do
+ before do
+ stub_ee_application_setting(should_check_namespace_plan: true)
+ end
+
+ context 'when group does not have a license' do
+ let(:group_id) { group_3.id }
- it 'returns only chosen subgroup' do
- expect(described_class.new(subgroup_4.id).execute).to contain_exactly(group_1, subgroup_4)
+ it 'does not return the group' do
+ is_expected.to be_empty
+ end
+ end
+
+ context 'with subgroup with template' do
+ before do
+ subgroup_4.update!(custom_project_templates_group_id: subgroup_5.id)
+ create(:project, namespace: subgroup_5)
+ end
+
+ context 'when group is provided' do
+ let(:group_id) { group_1.id }
+
+ it 'returns only chosen group' do
+ is_expected.to contain_exactly(group_1)
+ end
+ end
+
+ context 'when subgroup is provided' do
+ let(:group_id) { subgroup_4.id }
+
+ it 'returns only chosen subgroup' do
+ is_expected.to contain_exactly(group_1, subgroup_4)
+ end
+ end
end
end
end
end
+
+ context 'when feature flag "optimize_group_template_query" is enabled' do
+ it_behaves_like 'groups_with_templates'
+ end
+
+ context 'when feature flag "optimize_group_template_query" is disabled' do
+ before do
+ stub_feature_flags(optimize_group_template_query: false)
+ end
+
+ it_behaves_like 'groups_with_templates'
+ end
end
diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb
index e3e9e615e37ec..827665de55a1c 100644
--- a/ee/spec/models/ee/user_spec.rb
+++ b/ee/spec/models/ee/user_spec.rb
@@ -645,18 +645,29 @@
let!(:group_1) { create(:group, name: 'group-1') }
let!(:group_2) { create(:group, name: 'group-2') }
let!(:group_3) { create(:group, name: 'group-3') }
+ let!(:group_4) { create(:group, name: 'group-4') }
let!(:subgroup_1) { create(:group, parent: group_1, name: 'subgroup-1') }
let!(:subgroup_2) { create(:group, parent: group_2, name: 'subgroup-2') }
let!(:subgroup_3) { create(:group, parent: group_3, name: 'subgroup-3') }
+ let!(:subgroup_4) { create(:group, parent: group_4, name: 'subgroup-4') }
+
+ let!(:subsubgroup_1) { create(:group, parent: subgroup_1, name: 'sub-subgroup-1') }
+ let!(:subsubgroup_4) { create(:group, parent: subgroup_4, name: 'sub-subgroup-4') }
before do
group_1.update!(custom_project_templates_group_id: subgroup_1.id)
group_2.update!(custom_project_templates_group_id: subgroup_2.id)
group_3.update!(custom_project_templates_group_id: subgroup_3.id)
+ subgroup_1.update!(custom_project_templates_group_id: subsubgroup_1.id)
+ subgroup_4.update!(custom_project_templates_group_id: subsubgroup_4.id)
+
create(:project, namespace: subgroup_1)
create(:project, namespace: subgroup_2)
+
+ create(:project, namespace: subsubgroup_1)
+ create(:project, namespace: subsubgroup_4)
end
context 'when the access level of the user is below the required one' do
@@ -674,6 +685,7 @@
group_1.add_developer(user)
group_2.add_maintainer(user)
group_3.add_developer(user)
+ subgroup_4.add_developer(user)
end
context 'when a Group ID is passed' do
@@ -689,8 +701,8 @@
it 'returns all available Groups' do
groups = user.available_subgroups_with_custom_project_templates
- expect(groups.to_a.size).to eq(2)
- expect(groups.map(&:name)).to include('subgroup-1', 'subgroup-2')
+ expect(groups.to_a.size).to eq(4)
+ expect(groups.map(&:name)).to include('subgroup-1', 'subgroup-2', 'sub-subgroup-1', 'sub-subgroup-4')
end
it 'excludes Groups with the configured setting but without projects' do
@@ -701,17 +713,22 @@
end
context 'when namespace plan is checked', :saas do
+ let(:bronze_plan) { create(:bronze_plan) }
+ let(:ultimate_plan) { create(:ultimate_plan) }
+
before do
- create(:gitlab_subscription, namespace: group_1, hosted_plan: create(:bronze_plan))
- create(:gitlab_subscription, namespace: group_2, hosted_plan: create(:ultimate_plan))
- allow(Gitlab::CurrentSettings).to receive(:should_check_namespace_plan?) { true }
+ stub_ee_application_setting(should_check_namespace_plan: true)
+
+ create(:gitlab_subscription, namespace: group_1, hosted_plan: bronze_plan)
+ create(:gitlab_subscription, namespace: group_2, hosted_plan: ultimate_plan)
+ create(:gitlab_subscription, namespace: group_4, hosted_plan: ultimate_plan)
end
it 'returns groups on ultimate or premium plans' do
groups = user.available_subgroups_with_custom_project_templates
- expect(groups.to_a.size).to eq(1)
- expect(groups.map(&:name)).to include('subgroup-2')
+ expect(groups.to_a.size).to eq(2)
+ expect(groups.map(&:name)).to match_array(%w[subgroup-2 sub-subgroup-4])
end
end
end
diff --git a/ee/spec/models/gitlab_subscription_spec.rb b/ee/spec/models/gitlab_subscription_spec.rb
index d6269ebbc04a1..b852cfbd1366e 100644
--- a/ee/spec/models/gitlab_subscription_spec.rb
+++ b/ee/spec/models/gitlab_subscription_spec.rb
@@ -34,10 +34,9 @@
describe 'scopes' do
describe '.with_hosted_plan' do
- let!(:ultimate_subscription) { create(:gitlab_subscription, hosted_plan: ultimate_plan) }
- let!(:premium_subscription) { create(:gitlab_subscription, hosted_plan: premium_plan) }
-
- let!(:trial_subscription) { create(:gitlab_subscription, hosted_plan: ultimate_plan, trial: true) }
+ let_it_be(:ultimate_subscription) { create(:gitlab_subscription, hosted_plan: ultimate_plan) }
+ let_it_be(:premium_subscription) { create(:gitlab_subscription, hosted_plan: premium_plan) }
+ let_it_be(:trial_subscription) { create(:gitlab_subscription, hosted_plan: ultimate_plan, trial: true) }
it 'scopes to the plan' do
expect(described_class.with_hosted_plan('ultimate')).to contain_exactly(ultimate_subscription)
@@ -46,6 +45,18 @@
end
end
+ describe '.by_hosted_plan_ids' do
+ let_it_be(:ultimate_subscription) { create(:gitlab_subscription, hosted_plan: ultimate_plan) }
+ let_it_be(:premium_subscription) { create(:gitlab_subscription, hosted_plan: premium_plan) }
+ let_it_be(:trial_subscription) { create(:gitlab_subscription, hosted_plan: ultimate_plan, trial: true) }
+
+ it 'returns subscriptions by their plan ids' do
+ expect(described_class.by_hosted_plan_ids(ultimate_plan.id)).to match_array([ultimate_subscription, trial_subscription])
+ expect(described_class.by_hosted_plan_ids([ultimate_plan.id, premium_plan.id])).to match_array([ultimate_subscription, premium_subscription, trial_subscription])
+ expect(described_class.by_hosted_plan_ids(non_existing_record_id)).to be_empty
+ end
+ end
+
describe '.max_seats_used_changed_between', :timecop do
let(:from) { Time.current.beginning_of_day - 1.day }
let(:to) { Time.current.beginning_of_day }
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 623c9c7e07c18..a03dac83113a8 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -443,6 +443,15 @@
end
end
+ describe '.by_root_id' do
+ it 'returns correct namespaces' do
+ expect(described_class.by_root_id(namespace1.id)).to match_array([namespace1, namespace1sub])
+ expect(described_class.by_root_id(namespace2.id)).to match_array([namespace2, namespace2sub])
+ expect(described_class.by_root_id(namespace1sub.id)).to be_empty
+ expect(described_class.by_root_id(nil)).to be_empty
+ end
+ end
+
describe '.filter_by_path' do
it 'includes correct namespaces' do
expect(described_class.filter_by_path(namespace1.path)).to eq([namespace1])
diff --git a/spec/models/plan_spec.rb b/spec/models/plan_spec.rb
index 73e88a17e2421..fe3365ca78f1a 100644
--- a/spec/models/plan_spec.rb
+++ b/spec/models/plan_spec.rb
@@ -3,6 +3,18 @@
require 'spec_helper'
RSpec.describe Plan do
+ describe 'scopes', :aggregate_failures do
+ let_it_be(:default_plan) { create(:default_plan) }
+
+ describe '.by_name' do
+ it 'returns plans by their name' do
+ expect(described_class.by_name('default')).to match_array([default_plan])
+ expect(described_class.by_name(%w[default unknown])).to match_array([default_plan])
+ expect(described_class.by_name(nil)).to be_empty
+ end
+ end
+ end
+
describe '#default?' do
subject { plan.default? }
--
GitLab