Merge branch '301142-replace-namespace-self_and_descendants-with-linear-version' into 'master'

Resolve "Replace Namespace#self_and_descendants with linear version" [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!56296

app/models/namespaces/traversal/linear.rb

@@ -38,15 +38,31 @@ module Namespaces
     module Linear
       extend ActiveSupport::Concern
 
+      UnboundedSearch = Class.new(StandardError)
+
       included do
         after_create :sync_traversal_ids, if: -> { sync_traversal_ids? }
         after_update :sync_traversal_ids, if: -> { sync_traversal_ids? && saved_change_to_parent_id? }
+
+        scope :traversal_ids_contains, ->(ids) { where("traversal_ids @> (?)", ids) }
       end
 
       def sync_traversal_ids?
         Feature.enabled?(:sync_traversal_ids, root_ancestor, default_enabled: :yaml)
       end
 
+      def use_traversal_ids?
+        Feature.enabled?(:use_traversal_ids, root_ancestor, default_enabled: :yaml)
+      end
+
+      def self_and_descendants
+        if use_traversal_ids?
+          lineage(self)
+        else
+          super
+        end
+      end
+
       private
 
       # Update the traversal_ids for the full hierarchy.
@@ -58,6 +74,38 @@ module Namespaces
 
         Namespace::TraversalHierarchy.for_namespace(root_ancestor).sync_traversal_ids!
       end
+
+      # Make sure we drop the STI `type = 'Group'` condition for better performance.
+      # Logically equivalent so long as hierarchies remain homogeneous.
+      def without_sti_condition
+        self.class.unscope(where: :type)
+      end
+
+      # Search this namespace's lineage. Bound inclusively by top node.
+      def lineage(top)
+        raise UnboundedSearch.new('Must bound search by a top') unless top
+
+        without_sti_condition
+          .traversal_ids_contains(latest_traversal_ids(top))
+      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
     end
   end
 end

changelogs/unreleased/301142-replace-namespace-self_and_descendants-with-linear-version.yml

+---
+title: Linear version of Namespace#self_and_descendants
+merge_request: 56296
+author:
+type: performance

config/feature_flags/development/use_traversal_ids.yml

+---
+name: use_traversal_ids
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56296
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/321948
+milestone: '13.11'
+type: development
+group: group::access
+default_enabled: false

spec/lib/gitlab/object_hierarchy_spec.rb

@@ -195,9 +195,24 @@ RSpec.describe Gitlab::ObjectHierarchy do
     it_behaves_like 'Gitlab::ObjectHierarchy test cases'
 
     it 'calls DISTINCT' do
-      expect(parent.self_and_descendants.to_sql).to include("DISTINCT")
       expect(child2.self_and_ancestors.to_sql).to include("DISTINCT")
     end
+
+    context 'when use_traversal_ids feature flag is enabled' do
+      it 'does not call DISTINCT' do
+        expect(parent.self_and_descendants.to_sql).not_to include("DISTINCT")
+      end
+    end
+
+    context 'when use_traversal_ids feature flag is disabled' do
+      before do
+        stub_feature_flags(use_traversal_ids: false)
+      end
+
+      it 'calls DISTINCT' do
+        expect(parent.self_and_descendants.to_sql).to include("DISTINCT")
+      end
+    end
   end
 
   context 'when the use_distinct_for_all_object_hierarchy feature flag is enabled' do
@@ -209,20 +224,35 @@ RSpec.describe Gitlab::ObjectHierarchy do
     it_behaves_like 'Gitlab::ObjectHierarchy test cases'
 
     it 'calls DISTINCT' do
-      expect(parent.self_and_descendants.to_sql).to include("DISTINCT")
       expect(child2.self_and_ancestors.to_sql).to include("DISTINCT")
     end
 
-    context 'when the skip_ordering option is set' do
-      let(:options) { { skip_ordering: true } }
+    context 'when use_traversal_ids feature flag is enabled' do
+      it 'does not call DISTINCT' do
+        expect(parent.self_and_descendants.to_sql).not_to include("DISTINCT")
+      end
+    end
+
+    context 'when use_traversal_ids feature flag is disabled' do
+      before do
+        stub_feature_flags(use_traversal_ids: false)
+      end
 
-      it_behaves_like 'Gitlab::ObjectHierarchy test cases'
+      it 'calls DISTINCT' do
+        expect(parent.self_and_descendants.to_sql).to include("DISTINCT")
+      end
+
+      context 'when the skip_ordering option is set' do
+        let(:options) { { skip_ordering: true } }
 
-      it 'does not include ROW_NUMBER()' do
-        query = described_class.new(Group.where(id: parent.id), options: options).base_and_descendants.to_sql
+        it_behaves_like 'Gitlab::ObjectHierarchy test cases'
 
-        expect(query).to include("DISTINCT")
-        expect(query).not_to include("ROW_NUMBER()")
+        it 'does not include ROW_NUMBER()' do
+          query = described_class.new(Group.where(id: parent.id), options: options).base_and_descendants.to_sql
+
+          expect(query).to include("DISTINCT")
+          expect(query).not_to include("ROW_NUMBER()")
+        end
       end
     end
   end

spec/models/namespace_spec.rb

@@ -873,7 +873,51 @@ RSpec.describe Namespace do
     end
   end
 
-  it_behaves_like 'recursive namespace traversal'
+  describe '#use_traversal_ids?' do
+    let_it_be(:namespace) { build(:namespace) }
+
+    subject { namespace.use_traversal_ids? }
+
+    context 'when use_traversal_ids feature flag is true' do
+      before do
+        stub_feature_flags(use_traversal_ids: true)
+      end
+
+      it { is_expected.to eq true }
+    end
+
+    context 'when use_traversal_ids feature flag is false' do
+      before do
+        stub_feature_flags(use_traversal_ids: false)
+      end
+
+      it { is_expected.to eq false }
+    end
+  end
+
+  context 'when use_traversal_ids feature flag is true' do
+    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) }

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

 # frozen_string_literal: true
 
-RSpec.shared_examples 'recursive namespace traversal' do
+RSpec.shared_examples 'namespace traversal' do
   describe '#self_and_hierarchy' do
     let!(:group) { create(:group, path: 'git_lab') }
     let!(:nested_group) { create(:group, parent: group) }