Linear traversal query for Namespace#ancestors

Linear queries have better performance than recursive CTEs.

app/helpers/groups_helper.rb

@@ -115,9 +115,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
@@ -286,11 +284,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

@@ -835,7 +835,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

@@ -45,6 +45,7 @@ module Namespaces
         after_update :sync_traversal_ids, if: -> { sync_traversal_ids? && saved_change_to_parent_id? }
 
         scope :traversal_ids_contains, ->(ids) { where("traversal_ids @> (?)", ids) }
+        scope :traversal_ids_contained_by, ->(ids) { where("traversal_ids <@ (?)", ids) }
       end
 
       def sync_traversal_ids?
@@ -58,11 +59,15 @@ 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?
+
+        lineage(bottom: latest_parent_id, hierarchy_order: hierarchy_order)
       end
 
       private
@@ -84,11 +89,51 @@ 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.traversal_ids_contained_by(latest_traversal_ids(bottom))
+        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(array_length((#{latest_traversal_ids.to_sql}), 1) - array_length(traversal_ids, 1))"
+          skope = skope.select(skope.arel_table[Arel.star], "#{depth_sql} as depth")
+                       .order(depth: hierarchy_order)
+        end
+
+        skope
+      end
+
+      # traversal_ids are a cached value.
+      #
+      # The traversal_ids value in a loaded object can become stale when compared
+      # to the database value. For example, if you load a hierarchy and then move
+      # a group, any previously loaded descendant objects will have out of date
+      # traversal_ids.
+      #
+      # To solve this problem, we never depend on the object's traversal_ids
+      # value. We always query the database first with a sub-select for the
+      # latest traversal_ids.
+      #
+      # Note that ActiveRecord will cache query results. You can avoid this by
+      # using `Model.uncached { ... }`
+      def latest_traversal_ids(namespace = self)
+        without_sti_condition.where('id = (?)', namespace)
+                .select('traversal_ids as latest_traversal_ids')
+      end
 
-        without_sti_condition
-          .traversal_ids_contains("{#{top.id}}")
+      def latest_parent_id(namespace = self)
+        without_sti_condition.where(id: namespace).select(:parent_id)
       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

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

@@ -1798,13 +1798,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
+
+          include_examples 'correct ancestor order'
+        end
+
+        context 'linear' do
+          before do
+            stub_feature_flags(use_traversal_ids: true)
 
-        expect(got_array.shift(2)).to contain_exactly(*expected_array1)
-        expect(got_array).to eq(expected_array2)
+            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,29 +936,39 @@ RSpec.describe Namespace do
     end
   end
 
-  context 'when use_traversal_ids feature flag is true' do
+  context 'traversal queries' do
     let_it_be(:namespace, reload: true) { create(:namespace) }
 
-    it_behaves_like 'namespace traversal'
+    context 'recursive' do
+      before do
+        stub_feature_flags(use_traversal_ids: false)
+      end
 
-    describe '#self_and_descendants' do
-      subject { namespace.self_and_descendants }
+      it_behaves_like 'namespace traversal'
 
-      it { expect(subject.to_sql).to include 'traversal_ids @>' }
-    end
-  end
+      describe '#self_and_descendants' do
+        it { expect(namespace.self_and_descendants.to_sql).not_to include 'traversal_ids @>' }
+      end
 
-  context 'when use_traversal_ids feature flag is false' do
-    before do
-      stub_feature_flags(use_traversal_ids: false)
+      describe '#ancestors' do
+        it { expect(namespace.ancestors.to_sql).not_to include 'traversal_ids <@' }
+      end
     end
 
-    it_behaves_like 'namespace traversal'
+    context 'linear' do
+      it_behaves_like 'namespace traversal'
+
+      describe '#self_and_descendants' do
+        it { expect(namespace.self_and_descendants.to_sql).to include 'traversal_ids @>' }
+      end
 
-    describe '#self_and_descendants' do
-      subject { namespace.self_and_descendants }
+      describe '#ancestors' do
+        it { expect(namespace.ancestors.to_sql).to include 'traversal_ids <@' }
 
-      it { expect(subject.to_sql).not_to include 'traversal_ids @>' }
+        it 'hierarchy order' do
+          expect(namespace.ancestors(hierarchy_order: :asc).to_sql).to include 'ORDER BY "depth" ASC'
+        end
+      end
     end
   end
 

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