Merge branch 'if-324749-linear_ancestors' into 'master'

Linear traversal query for Namespace#ancestors [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57137

app/helpers/groups_helper.rb

@@ -123,9 +123,7 @@ module GroupsHelper
     @has_group_title = true
     full_title = []
 
-    ancestors = group.ancestors.with_route
-
-    ancestors.reverse_each.with_index do |parent, index|
+    sorted_ancestors(group).with_route.reverse_each.with_index do |parent, index|
       if index > 0
         add_to_breadcrumb_dropdown(group_title_link(parent, hidable: false, show_avatar: true, for_dropdown: true), location: :before)
       else
@@ -294,11 +292,20 @@ module GroupsHelper
   end
 
   def oldest_consecutively_locked_ancestor(group)
-    group.ancestors.find do |group|
+    sorted_ancestors(group).find do |group|
       !group.has_parent? || !group.parent.share_with_group_lock?
     end
   end
 
+  # Ancestors sorted by hierarchy depth in bottom-top order.
+  def sorted_ancestors(group)
+    if group.root_ancestor.use_traversal_ids?
+      group.ancestors(hierarchy_order: :asc)
+    else
+      group.ancestors
+    end
+  end
+
   def default_help
     s_("GroupSettings|This setting will be applied to all subgroups unless overridden by a group owner. Groups that already have access to the project will continue to have access unless removed manually.")
   end

app/models/group.rb

@@ -839,7 +839,12 @@ class Group < Namespace
   end
 
   def uncached_ci_variables_for(ref, project, environment: nil)
-    list_of_ids = [self] + ancestors
+    list_of_ids = if root_ancestor.use_traversal_ids?
+                    [self] + ancestors(hierarchy_order: :asc)
+                  else
+                    [self] + ancestors
+                  end
+
     variables = Ci::GroupVariable.where(group: list_of_ids)
     variables = variables.unprotected unless project.protected_for?(ref)
 

app/models/namespaces/traversal/linear.rb

@@ -58,11 +58,18 @@ module Namespaces
       end
 
       def self_and_descendants
-        if use_traversal_ids?
-          lineage(self)
-        else
-          super
-        end
+        return super unless use_traversal_ids?
+
+        lineage(top: self)
+      end
+
+      def ancestors(hierarchy_order: nil)
+        return super() unless use_traversal_ids?
+        return super() unless Feature.enabled?(:use_traversal_ids_for_ancestors, root_ancestor, default_enabled: :yaml)
+
+        return self.class.none if parent_id.blank?
+
+        lineage(bottom: parent, hierarchy_order: hierarchy_order)
       end
 
       private
@@ -84,11 +91,29 @@ module Namespaces
       end
 
       # Search this namespace's lineage. Bound inclusively by top node.
-      def lineage(top)
-        raise UnboundedSearch, 'Must bound search by a top' unless top
+      def lineage(top: nil, bottom: nil, hierarchy_order: nil)
+        raise UnboundedSearch, 'Must bound search by either top or bottom' unless top || bottom
+
+        skope = without_sti_condition
+
+        if top
+          skope = skope.traversal_ids_contains("{#{top.id}}")
+        end
+
+        if bottom
+          skope = skope.where(id: bottom.traversal_ids[0..-1])
+        end
+
+        # The original `with_depth` attribute in ObjectHierarchy increments as you
+        # walk away from the "base" namespace. This direction changes depending on
+        # if you are walking up the ancestors or down the descendants.
+        if hierarchy_order
+          depth_sql = "ABS(#{traversal_ids.count} - array_length(traversal_ids, 1))"
+          skope = skope.select(skope.arel_table[Arel.star], "#{depth_sql} as depth")
+                       .order(depth: hierarchy_order)
+        end
 
-        without_sti_condition
-          .traversal_ids_contains("{#{top.id}}")
+        skope
       end
     end
   end

app/models/service.rb

@@ -268,7 +268,7 @@ class Service < ApplicationRecord
   private_class_method :instance_level_integration
 
   def self.create_from_active_default_integrations(scope, association, with_templates: false)
-    group_ids = scope.ancestors.select(:id)
+    group_ids = sorted_ancestors(scope).select(:id)
     array = group_ids.to_sql.present? ? "array(#{group_ids.to_sql})" : 'ARRAY[]'
 
     from_union([
@@ -459,6 +459,15 @@ class Service < ApplicationRecord
 
   private
 
+  # Ancestors sorted by hierarchy depth in bottom-top order.
+  def self.sorted_ancestors(scope)
+    if scope.root_ancestor.use_traversal_ids?
+      Namespace.from(scope.ancestors(hierarchy_order: :asc))
+    else
+      scope.ancestors
+    end
+  end
+
   def validate_is_instance_or_template
     errors.add(:template, 'The service should be a service template or instance-level integration') if template? && instance_level?
   end

changelogs/unreleased/324749-linear_ancestors.yml

+---
+title: Linear traversal query for Namespace#ancestors
+merge_request: 57137
+author:
+type: performance

config/feature_flags/development/use_traversal_ids_for_ancestors.yml

+---
+name: use_traversal_ids_for_ancestors
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57137
+rollout_issue_url:
+milestone: '13.12'
+type: development
+group: group::access
+default_enabled: false

ee/spec/presenters/group_member_presenter_spec.rb

@@ -93,7 +93,7 @@ RSpec.describe GroupMemberPresenter do
         let(:expected_roles) { { 'Developer' => 30, 'Maintainer' => 40, 'Owner' => 50, 'Reporter' => 20 } }
 
         before do
-          entity.parent = group
+          entity.update!(parent: group)
         end
       end
     end

spec/models/group_spec.rb

@@ -427,6 +427,50 @@ RSpec.describe Group do
     end
   end
 
+  context 'traversal queries' do
+    let_it_be(:group, reload: true) { create(:group, :nested) }
+
+    context 'recursive' do
+      before do
+        stub_feature_flags(use_traversal_ids: false)
+      end
+
+      it_behaves_like 'namespace traversal'
+
+      describe '#self_and_descendants' do
+        it { expect(group.self_and_descendants.to_sql).not_to include 'traversal_ids @>' }
+      end
+
+      describe '#ancestors' do
+        it { expect(group.ancestors.to_sql).not_to include 'traversal_ids <@' }
+      end
+    end
+
+    context 'linear' do
+      it_behaves_like 'namespace traversal'
+
+      describe '#self_and_descendants' do
+        it { expect(group.self_and_descendants.to_sql).to include 'traversal_ids @>' }
+      end
+
+      describe '#ancestors' do
+        it { expect(group.ancestors.to_sql).to include "\"namespaces\".\"id\" = #{group.parent_id}" }
+
+        it 'hierarchy order' do
+          expect(group.ancestors(hierarchy_order: :asc).to_sql).to include 'ORDER BY "depth" ASC'
+        end
+
+        context 'ancestor linear queries feature flag disabled' do
+          before do
+            stub_feature_flags(use_traversal_ids_for_ancestors: false)
+          end
+
+          it { expect(group.ancestors.to_sql).not_to include 'traversal_ids <@' }
+        end
+      end
+    end
+  end
+
   describe '.without_integration' do
     let(:another_group) { create(:group) }
     let(:instance_integration) { build(:jira_service, :instance) }
@@ -1798,13 +1842,35 @@ RSpec.describe Group do
         allow(project).to receive(:protected_for?).with('ref').and_return(true)
       end
 
-      it 'returns all variables belong to the group and parent groups' do
-        expected_array1 = [protected_variable, ci_variable]
-        expected_array2 = [variable_child, variable_child_2, variable_child_3]
-        got_array = group_child_3.ci_variables_for('ref', project).to_a
+      context 'traversal queries' do
+        shared_examples 'correct ancestor order' do
+          it 'returns all variables belong to the group and parent groups' do
+            expected_array1 = [protected_variable, ci_variable]
+            expected_array2 = [variable_child, variable_child_2, variable_child_3]
+            got_array = group_child_3.ci_variables_for('ref', project).to_a
+
+            expect(got_array.shift(2)).to contain_exactly(*expected_array1)
+            expect(got_array).to eq(expected_array2)
+          end
+        end
+
+        context 'recursive' do
+          before do
+            stub_feature_flags(use_traversal_ids: false)
+          end
 
-        expect(got_array.shift(2)).to contain_exactly(*expected_array1)
-        expect(got_array).to eq(expected_array2)
+          include_examples 'correct ancestor order'
+        end
+
+        context 'linear' do
+          before do
+            stub_feature_flags(use_traversal_ids: true)
+
+            group_child_3.reload # make sure traversal_ids are reloaded
+          end
+
+          include_examples 'correct ancestor order'
+        end
       end
     end
   end

spec/models/namespace_spec.rb

@@ -936,32 +936,6 @@ RSpec.describe Namespace do
     end
   end
 
-  context 'when use_traversal_ids feature flag is true' do
-    let_it_be(:namespace, reload: true) { create(:namespace) }
-
-    it_behaves_like 'namespace traversal'
-
-    describe '#self_and_descendants' do
-      subject { namespace.self_and_descendants }
-
-      it { expect(subject.to_sql).to include 'traversal_ids @>' }
-    end
-  end
-
-  context 'when use_traversal_ids feature flag is false' do
-    before do
-      stub_feature_flags(use_traversal_ids: false)
-    end
-
-    it_behaves_like 'namespace traversal'
-
-    describe '#self_and_descendants' do
-      subject { namespace.self_and_descendants }
-
-      it { expect(subject.to_sql).not_to include 'traversal_ids @>' }
-    end
-  end
-
   describe '#users_with_descendants' do
     let(:user_a) { create(:user) }
     let(:user_b) { create(:user) }

spec/models/service_spec.rb

@@ -597,23 +597,49 @@ RSpec.describe Service do
             context 'passing a group' do
               let!(:sub_subgroup) { create(:group, parent: subgroup) }
 
-              it 'creates a service from the subgroup-level integration' do
-                described_class.create_from_active_default_integrations(sub_subgroup, :group_id)
+              context 'traversal queries' do
+                shared_examples 'correct ancestor order' do
+                  it 'creates a service from the subgroup-level integration' do
+                    described_class.create_from_active_default_integrations(sub_subgroup, :group_id)
 
-                expect(sub_subgroup.reload.services.size).to eq(1)
-                expect(sub_subgroup.reload.services.first.api_url).to eq(subgroup_integration.api_url)
-                expect(sub_subgroup.reload.services.first.inherit_from_id).to eq(subgroup_integration.id)
-              end
+                    sub_subgroup.reload
+
+                    expect(sub_subgroup.services.size).to eq(1)
+                    expect(sub_subgroup.services.first.api_url).to eq(subgroup_integration.api_url)
+                    expect(sub_subgroup.services.first.inherit_from_id).to eq(subgroup_integration.id)
+                  end
+
+                  context 'having a service inheriting settings' do
+                    let!(:subgroup_integration) { create(:prometheus_service, group: subgroup, project: nil, inherit_from_id: group_integration.id, api_url: 'https://prometheus.subgroup.com/') }
+
+                    it 'creates a service from the group-level integration' do
+                      described_class.create_from_active_default_integrations(sub_subgroup, :group_id)
+
+                      sub_subgroup.reload
+
+                      expect(sub_subgroup.services.size).to eq(1)
+                      expect(sub_subgroup.services.first.api_url).to eq(group_integration.api_url)
+                      expect(sub_subgroup.services.first.inherit_from_id).to eq(group_integration.id)
+                    end
+                  end
+                end
+
+                context 'recursive' do
+                  before do
+                    stub_feature_flags(use_traversal_ids: false)
+                  end
+
+                  include_examples 'correct ancestor order'
+                end
 
-              context 'having a service inheriting settings' do
-                let!(:subgroup_integration) { create(:prometheus_service, group: subgroup, project: nil, inherit_from_id: group_integration.id, api_url: 'https://prometheus.subgroup.com/') }
+                context 'linear' do
+                  before do
+                    stub_feature_flags(use_traversal_ids: true)
 
-                it 'creates a service from the group-level integration' do
-                  described_class.create_from_active_default_integrations(sub_subgroup, :group_id)
+                    sub_subgroup.reload # make sure traversal_ids are reloaded
+                  end
 
-                  expect(sub_subgroup.reload.services.size).to eq(1)
-                  expect(sub_subgroup.reload.services.first.api_url).to eq(group_integration.api_url)
-                  expect(sub_subgroup.reload.services.first.inherit_from_id).to eq(group_integration.id)
+                  include_examples 'correct ancestor order'
                 end
               end
             end

spec/presenters/group_member_presenter_spec.rb

@@ -142,7 +142,7 @@ RSpec.describe GroupMemberPresenter do
     let(:expected_roles) { { 'Developer' => 30, 'Maintainer' => 40, 'Owner' => 50, 'Reporter' => 20 } }
 
     before do
-      entity.parent = group
+      entity.update!(parent: group)
     end
   end
 end

spec/services/groups/transfer_service_spec.rb

@@ -240,6 +240,7 @@ RSpec.describe Groups::TransferService do
       end
 
       context 'when the group is allowed to be transferred' do
+        let_it_be(:new_parent_group, reload: true) { create(:group, :public) }
         let_it_be(:new_parent_group_integration) { create(:slack_service, group: new_parent_group, project: nil, webhook: 'http://new-group.slack.com') }
 
         before do
@@ -273,11 +274,10 @@ RSpec.describe Groups::TransferService do
         end
 
         context 'with a group integration' do
-          let_it_be(:instance_integration) { create(:slack_service, :instance, webhook: 'http://project.slack.com') }
-
           let(:new_created_integration) { Service.find_by(group: group) }
 
           context 'with an inherited integration' do
+            let_it_be(:instance_integration) { create(:slack_service, :instance, webhook: 'http://project.slack.com') }
             let_it_be(:group_integration) { create(:slack_service, group: group, project: nil, webhook: 'http://group.slack.com', inherit_from_id: instance_integration.id) }
 
             it 'replaces inherited integrations', :aggregate_failures do
@@ -603,6 +603,7 @@ RSpec.describe Groups::TransferService do
           create(:group_member, :owner, group: new_parent_group, user: user)
           create(:group, :private, parent: group, require_two_factor_authentication: true)
           group.update!(require_two_factor_authentication: true)
+          new_parent_group.reload # make sure traversal_ids are reloaded
         end
 
         it 'does not update group two factor authentication setting' do

spec/support/shared_examples/namespaces/namespace_traversal_examples.rb → spec/support/shared_examples/namespaces/traversal_examples.rb

@@ -39,16 +39,17 @@ RSpec.shared_examples 'namespace traversal' do
   end
 
   describe '#ancestors' do
-    let(:group) { create(:group) }
-    let(:nested_group) { create(:group, parent: group) }
-    let(:deep_nested_group) { create(:group, parent: nested_group) }
-    let(:very_deep_nested_group) { create(:group, parent: deep_nested_group) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:nested_group) { create(:group, parent: group) }
+    let_it_be(:deep_nested_group) { create(:group, parent: nested_group) }
+    let_it_be(:very_deep_nested_group) { create(:group, parent: deep_nested_group) }
 
     it 'returns the correct ancestors' do
-      expect(very_deep_nested_group.ancestors).to include(group, nested_group, deep_nested_group)
-      expect(deep_nested_group.ancestors).to include(group, nested_group)
-      expect(nested_group.ancestors).to include(group)
-      expect(group.ancestors).to eq([])
+      # #reload is called to make sure traversal_ids are reloaded
+      expect(very_deep_nested_group.reload.ancestors).to contain_exactly(group, nested_group, deep_nested_group)
+      expect(deep_nested_group.reload.ancestors).to contain_exactly(group, nested_group)
+      expect(nested_group.reload.ancestors).to contain_exactly(group)
+      expect(group.reload.ancestors).to eq([])
     end
 
     describe '#recursive_ancestors' do