diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 238556f0cf03ddd972189a4dc8bf29afe77aca10..9020f90fd3c48e2b44b6e3174958b38b8cd52452 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -472,12 +472,8 @@ def multiple_issue_boards_available?
false
end
- def all_project_ids
- all_projects.pluck(:id)
- end
-
def all_project_ids_except(ids)
- all_projects.where.not(id: ids).pluck(:id)
+ all_project_ids.where.not(id: ids)
end
# Deprecated, use #licensed_feature_available? instead. Remove once Namespace#feature_available? isn't used anymore.
diff --git a/app/models/namespaces/traversal/cached.rb b/app/models/namespaces/traversal/cached.rb
index 55eaaa4667ef17261073d45fe9f67d18b11ee460..b962038d03933d2439cff54f62fc602917d60fc1 100644
--- a/app/models/namespaces/traversal/cached.rb
+++ b/app/models/namespaces/traversal/cached.rb
@@ -10,8 +10,62 @@ module Cached
after_destroy :invalidate_descendants_cache
end
+ override :self_and_descendant_ids
+ def self_and_descendant_ids
+ return super unless attempt_to_use_cached_data?
+
+ scope_with_cached_ids(
+ super,
+ self.class,
+ Namespaces::Descendants.arel_table[:self_and_descendant_group_ids]
+ )
+ end
+
+ override :all_project_ids
+ def all_project_ids
+ return super unless attempt_to_use_cached_data?
+
+ scope_with_cached_ids(
+ all_projects.select(:id),
+ Project,
+ Namespaces::Descendants.arel_table[:all_project_ids]
+ )
+ end
+
private
+ # This method implements an OR based cache lookup using COALESCE, similar what you would do in Ruby:
+ # return cheap_cached_data || expensive_uncached_data
+ def scope_with_cached_ids(consistent_ids_scope, model, cached_ids_column)
+ # Look up the cached ids and unnest them into rows if the cache is up to date.
+ cache_lookup_query = Namespaces::Descendants
+ .where(outdated_at: nil, namespace_id: id)
+ .select(cached_ids_column.as('ids'))
+
+ # Invoke the consistent lookup query and collect the ids as a single array value
+ consistent_descendant_ids_scope = model
+ .from(consistent_ids_scope.arel.as(model.table_name))
+ .reselect(Arel::Nodes::NamedFunction.new('ARRAY_AGG', [model.arel_table[:id]]).as('ids'))
+ .unscope(where: :type)
+
+ from = <<~SQL
+ UNNEST(
+ COALESCE(
+ (SELECT ids FROM (#{cache_lookup_query.to_sql}) cached_query),
+ (SELECT ids FROM (#{consistent_descendant_ids_scope.to_sql}) consistent_query))
+ ) AS #{model.table_name}(id)
+ SQL
+
+ model
+ .from(from)
+ .unscope(where: :type)
+ .select(:id)
+ end
+
+ def attempt_to_use_cached_data?
+ Feature.enabled?(:group_hierarchy_optimization, self, type: :beta)
+ end
+
override :sync_traversal_ids
def sync_traversal_ids
super
diff --git a/app/models/namespaces/traversal/linear.rb b/app/models/namespaces/traversal/linear.rb
index c3348c49ea1130c3aae55b18ffae763f9a95d1dc..5779b777fd7e28e193650598bab8eacbeaa7d826 100644
--- a/app/models/namespaces/traversal/linear.rb
+++ b/app/models/namespaces/traversal/linear.rb
@@ -106,6 +106,10 @@ def root_ancestor
end
end
+ def all_project_ids
+ all_projects.select(:id)
+ end
+
def self_and_descendants
return super unless use_traversal_ids?
diff --git a/app/models/namespaces/traversal/recursive.rb b/app/models/namespaces/traversal/recursive.rb
index 1c5d395cb3c117fcc3308477100e418126011425..3d551243cfb348e4bad05cf3149d3db51528fab8 100644
--- a/app/models/namespaces/traversal/recursive.rb
+++ b/app/models/namespaces/traversal/recursive.rb
@@ -19,6 +19,12 @@ def root_ancestor
end
alias_method :recursive_root_ancestor, :root_ancestor
+ def all_project_ids
+ namespace = user_namespace? ? self : recursive_self_and_descendant_ids
+ Project.where(namespace: namespace).select(:id)
+ end
+ alias_method :recursive_all_project_ids, :all_project_ids
+
# Returns all ancestors, self, and descendants of the current namespace.
def self_and_hierarchy
object_hierarchy(self.class.where(id: id))
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index a6ef8c8743b4e5c9fdc9f11a44837ceb8dc20a3b..bdf943091e9f711e16185d5b2bc965e1f9182c89 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -61,7 +61,8 @@ def handle_hierarchy_cache_update
params[:namespace_descendants_attributes] = {
traversal_ids: group.traversal_ids,
all_project_ids: [],
- self_and_descendant_group_ids: []
+ self_and_descendant_group_ids: [],
+ outdated_at: Time.current
}
else
return unless group.namespace_descendants
diff --git a/ee/app/services/security/orchestration/assign_service.rb b/ee/app/services/security/orchestration/assign_service.rb
index 083b77c88b3fc06929b09cd3f880c24b14de18f8..c5a9b1279c0059e3e8b93a9503b91592bc4a52a0 100644
--- a/ee/app/services/security/orchestration/assign_service.rb
+++ b/ee/app/services/security/orchestration/assign_service.rb
@@ -107,7 +107,7 @@ def create_security_policy_project_bot
if container.is_a?(Project)
Security::OrchestrationConfigurationCreateBotWorker.perform_async(container.id, current_user.id)
else
- container.all_project_ids.each do |project_id|
+ container.all_project_ids.pluck_primary_key.each do |project_id|
Security::OrchestrationConfigurationCreateBotWorker.perform_async(project_id, current_user.id)
end
end
diff --git a/ee/app/services/security/orchestration/unassign_service.rb b/ee/app/services/security/orchestration/unassign_service.rb
index 3ea4b2d5bd69a9ee9d91c6d4b9472e346e373a37..3a2466ef2637c5a635cf4d4466c8566b461ca179 100644
--- a/ee/app/services/security/orchestration/unassign_service.rb
+++ b/ee/app/services/security/orchestration/unassign_service.rb
@@ -42,7 +42,7 @@ def remove_bot
if container.is_a?(Project)
Security::OrchestrationConfigurationRemoveBotWorker.perform_async(container.id, current_user.id)
else
- container.all_project_ids.each do |project_id|
+ container.all_project_ids.pluck_primary_key.each do |project_id|
Security::OrchestrationConfigurationRemoveBotWorker.perform_async(project_id, current_user.id)
end
end
diff --git a/ee/lib/elastic/latest/application_class_proxy.rb b/ee/lib/elastic/latest/application_class_proxy.rb
index d8dd3434db6722f79f0e44aa20ffa43b1c43edb4..2c49a29c2eabcc345fb67dd8ec6a39bed5c83c4f 100644
--- a/ee/lib/elastic/latest/application_class_proxy.rb
+++ b/ee/lib/elastic/latest/application_class_proxy.rb
@@ -377,7 +377,7 @@ def rejected_project_filter(namespaces, options)
project_ids = filter_ids_by_feature(scoped_project_ids, current_user, options[:features])
rejected_ids = namespaces.flat_map do |namespace|
- namespace.all_project_ids_except(project_ids)
+ namespace.all_project_ids_except(project_ids).pluck_primary_key
end
{
diff --git a/spec/factories/namespaces/descendants.rb b/spec/factories/namespaces/descendants.rb
index 6325481294ae9c7988dca60b0a9c34a7e369171e..8251c396168ac1fe0255f4d3a88abece4622af77 100644
--- a/spec/factories/namespaces/descendants.rb
+++ b/spec/factories/namespaces/descendants.rb
@@ -8,5 +8,11 @@
traversal_ids { namespace.traversal_ids }
outdated_at { nil }
calculated_at { Time.current }
+
+ trait :up_to_date do
+ after(:create) do |record|
+ record.reload.update!(outdated_at: nil)
+ end
+ end
end
end
diff --git a/spec/models/namespaces/traversal/cached_spec.rb b/spec/models/namespaces/traversal/cached_spec.rb
index 8263e28bb985399e2955cc64e22a33cf493cc8a9..dd52f9c3d70542febb5bf97600b1cd918cc08e74 100644
--- a/spec/models/namespaces/traversal/cached_spec.rb
+++ b/spec/models/namespaces/traversal/cached_spec.rb
@@ -3,101 +3,175 @@
require 'spec_helper'
RSpec.describe Namespaces::Traversal::Cached, feature_category: :database do
- let_it_be_with_refind(:old_parent) { create(:group) }
- let_it_be_with_refind(:new_parent) { create(:group) }
- let_it_be_with_refind(:group) { create(:group, parent: old_parent) }
- let_it_be_with_refind(:subgroup) { create(:group, parent: group) }
+ describe 'callbacks' do
+ let_it_be_with_refind(:old_parent) { create(:group) }
+ let_it_be_with_refind(:new_parent) { create(:group) }
+ let_it_be_with_refind(:group) { create(:group, parent: old_parent) }
+ let_it_be_with_refind(:subgroup) { create(:group, parent: group) }
- context 'when the namespace_descendants_cache_expiration feature flag is off' do
- let!(:cache) { create(:namespace_descendants, namespace: group) }
+ context 'when the namespace_descendants_cache_expiration feature flag is off' do
+ let!(:cache) { create(:namespace_descendants, namespace: group) }
- before do
- stub_feature_flags(namespace_descendants_cache_expiration: false)
- end
+ before do
+ stub_feature_flags(namespace_descendants_cache_expiration: false)
+ end
- it 'does not invalidate the cache' do
- expect { group.update!(parent: new_parent) }.not_to change { cache.reload.outdated_at }
- end
+ it 'does not invalidate the cache' do
+ expect { group.update!(parent: new_parent) }.not_to change { cache.reload.outdated_at }
+ end
- context 'when the group is deleted' do
- it 'invalidates the cache' do
- expect { group.destroy! }.not_to change { cache.reload.outdated_at }
+ context 'when the group is deleted' do
+ it 'invalidates the cache' do
+ expect { group.destroy! }.not_to change { cache.reload.outdated_at }
+ end
end
end
- end
- context 'when no cached records are present' do
- it 'does nothing' do
- group.parent = new_parent
+ context 'when no cached records are present' do
+ it 'does nothing' do
+ group.parent = new_parent
- expect { group.save! }.not_to change { Namespaces::Descendants.all.to_a }
+ expect { group.save! }.not_to change { Namespaces::Descendants.all.to_a }
+ end
end
- end
- context 'when the namespace record is UserNamespace' do
- it 'does nothing' do
- # we won't use the optimization for UserNamespace
- namespace = create(:user_namespace)
- cache = create(:namespace_descendants, namespace: namespace)
+ context 'when the namespace record is UserNamespace' do
+ it 'does nothing' do
+ # we won't use the optimization for UserNamespace
+ namespace = create(:user_namespace)
+ cache = create(:namespace_descendants, namespace: namespace)
- expect { namespace.destroy! }.not_to change { cache.reload.outdated_at }
+ expect { namespace.destroy! }.not_to change { cache.reload.outdated_at }
+ end
end
- end
- context 'when cached record is present' do
- let!(:cache) { create(:namespace_descendants, namespace: group) }
+ context 'when cached record is present' do
+ let!(:cache) { create(:namespace_descendants, namespace: group) }
- it 'invalidates the cache' do
- expect { group.update!(parent: new_parent) }.to change { cache.reload.outdated_at }.from(nil)
- end
+ it 'invalidates the cache' do
+ expect { group.update!(parent: new_parent) }.to change { cache.reload.outdated_at }.from(nil)
+ end
- it 'does not invalidate the cache of subgroups' do
- subgroup_cache = create(:namespace_descendants, namespace: subgroup)
+ it 'does not invalidate the cache of subgroups' do
+ subgroup_cache = create(:namespace_descendants, namespace: subgroup)
- expect { group.update!(parent: new_parent) }.not_to change { subgroup_cache.reload.outdated_at }
+ expect { group.update!(parent: new_parent) }.not_to change { subgroup_cache.reload.outdated_at }
+ end
+
+ context 'when a new subgroup is added' do
+ it 'invalidates the cache' do
+ expect { create(:group, parent: group) }.to change { cache.reload.outdated_at }
+ end
+ end
+
+ context 'when a new project is added' do
+ it 'invalidates the cache' do
+ expect { create(:project, group: group) }.to change { cache.reload.outdated_at }
+ end
+ end
end
- context 'when a new subgroup is added' do
- it 'invalidates the cache' do
- expect { create(:group, parent: group) }.to change { cache.reload.outdated_at }
+ context 'when parent group has cached record' do
+ it 'invalidates the parent cache' do
+ old_parent_cache = create(:namespace_descendants, namespace: old_parent)
+ new_parent_cache = create(:namespace_descendants, namespace: new_parent)
+
+ group.update!(parent: new_parent)
+
+ expect(old_parent_cache.reload.outdated_at).not_to be_nil
+ expect(new_parent_cache.reload.outdated_at).not_to be_nil
end
end
- context 'when a new project is added' do
+ context 'when group is destroyed' do
it 'invalidates the cache' do
- expect { create(:project, group: group) }.to change { cache.reload.outdated_at }
+ cache = create(:namespace_descendants, namespace: group)
+
+ expect { group.destroy! }.to change { cache.reload.outdated_at }.from(nil)
end
- end
- end
- context 'when parent group has cached record' do
- it 'invalidates the parent cache' do
- old_parent_cache = create(:namespace_descendants, namespace: old_parent)
- new_parent_cache = create(:namespace_descendants, namespace: new_parent)
+ context 'when parent group has cached record' do
+ it 'invalidates the parent cache' do
+ old_parent_cache = create(:namespace_descendants, namespace: old_parent)
+ new_parent_cache = create(:namespace_descendants, namespace: new_parent)
- group.update!(parent: new_parent)
+ group.destroy!
- expect(old_parent_cache.reload.outdated_at).not_to be_nil
- expect(new_parent_cache.reload.outdated_at).not_to be_nil
+ expect(old_parent_cache.reload.outdated_at).not_to be_nil
+ expect(new_parent_cache.reload.outdated_at).to be_nil # no change
+ end
+ end
end
end
- context 'when group is destroyed' do
- it 'invalidates the cache' do
- cache = create(:namespace_descendants, namespace: group)
+ describe 'query methods' do
+ let_it_be(:group) { create(:group) }
+ let_it_be(:subgroup) { create(:group, parent: group) }
+ let_it_be(:subsubgroup) { create(:group, parent: subgroup) }
+
+ let_it_be(:project1) { create(:project, group: group) }
+ let_it_be(:project2) { create(:project, group: subsubgroup) }
+
+ # deliberately making self_and_descendant_group_ids different from the actual
+ # self_and_descendant_ids so we can verify that the cached query is running.
+ let_it_be_with_refind(:namespace_descendants) do
+ create(:namespace_descendants,
+ :up_to_date,
+ namespace: group,
+ self_and_descendant_group_ids: [group.id, subgroup.id],
+ all_project_ids: [project1.id]
+ )
+ end
+
+ describe '#self_and_descendant_ids' do
+ subject(:ids) { group.self_and_descendant_ids.pluck(:id) }
+
+ it 'returns the cached values' do
+ expect(ids).to eq(namespace_descendants.self_and_descendant_group_ids)
+ end
+
+ context 'when the cache is outdated' do
+ it 'returns the values from the uncached self_and_descendant_ids query' do
+ namespace_descendants.update!(outdated_at: Time.current)
+
+ expect(ids.sort).to eq([group.id, subgroup.id, subsubgroup.id])
+ end
+ end
+
+ context 'when the group_hierarchy_optimization feature flag is disabled' do
+ before do
+ stub_feature_flags(group_hierarchy_optimization: false)
+ end
- expect { group.destroy! }.to change { cache.reload.outdated_at }.from(nil)
+ it 'returns the values from the uncached self_and_descendant_ids query' do
+ expect(ids.sort).to eq([group.id, subgroup.id, subsubgroup.id])
+ end
+ end
end
- context 'when parent group has cached record' do
- it 'invalidates the parent cache' do
- old_parent_cache = create(:namespace_descendants, namespace: old_parent)
- new_parent_cache = create(:namespace_descendants, namespace: new_parent)
+ describe '#all_project_ids' do
+ subject(:ids) { group.all_project_ids.pluck(:id) }
- group.destroy!
+ it 'returns the cached values' do
+ expect(ids).to eq(namespace_descendants.all_project_ids)
+ end
- expect(old_parent_cache.reload.outdated_at).not_to be_nil
- expect(new_parent_cache.reload.outdated_at).to be_nil # no change
+ context 'when the cache is outdated' do
+ it 'returns the values from the uncached all_project_ids query' do
+ namespace_descendants.update!(outdated_at: Time.current)
+
+ expect(ids.sort).to eq([project1.id, project2.id])
+ end
+ end
+
+ context 'when the group_hierarchy_optimization feature flag is disabled' do
+ before do
+ stub_feature_flags(group_hierarchy_optimization: false)
+ end
+
+ it 'returns the values from the uncached all_project_ids query' do
+ expect(ids.sort).to eq([project1.id, project2.id])
+ end
end
end
end
diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb
index f50163041f8a79b6efcc47f4552f9403dd1c24b5..ceb0f5c45b4461b9f67446024141c351fde3f4b6 100644
--- a/spec/services/groups/update_service_spec.rb
+++ b/spec/services/groups/update_service_spec.rb
@@ -457,6 +457,8 @@
context 'when enabling the setting' do
it 'creates the initial Namespaces::Descendants record' do
expect { result }.to change { public_group.reload.namespace_descendants.present? }.from(false).to(true)
+
+ expect(public_group.namespace_descendants.outdated_at).to be_present
end
end
diff --git a/spec/support/shared_examples/namespaces/traversal_examples.rb b/spec/support/shared_examples/namespaces/traversal_examples.rb
index 960160395f858a64da7cb8e96283e589bac0c563..65f1abe0355e63c13f0e2c2d0512c26c179fbe3d 100644
--- a/spec/support/shared_examples/namespaces/traversal_examples.rb
+++ b/spec/support/shared_examples/namespaces/traversal_examples.rb
@@ -284,6 +284,14 @@
end
end
+ describe 'all_project_ids' do
+ it 'is a AR relation' do
+ expect(group.all_project_ids).to be_kind_of(ActiveRecord::Relation)
+ end
+
+ it_behaves_like 'recursive version', :all_project_ids
+ end
+
describe '#self_and_descendant_ids' do
subject { group.self_and_descendant_ids.pluck(:id) }