Merge branch 'jp-projetns-exclude-linear' into 'master'

Ignore ProjectNamespace objects in hierarchy queries

See merge request gitlab-org/gitlab!72119

app/models/namespaces/traversal/linear_scopes.rb

@@ -63,7 +63,11 @@ module Namespaces
         # Make sure we drop the STI `type = 'Group'` condition for better performance.
         # Logically equivalent so long as hierarchies remain homogeneous.
         def without_sti_condition
-          unscope(where: :type)
+          if Feature.enabled?(:include_sti_condition, default_enabled: :yaml)
+            all
+          else
+            unscope(where: :type)
+          end
         end
 
         def order_by_depth(hierarchy_order)

config/feature_flags/development/include_sti_condition.yml

+---
+name: include_sti_condition
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72119
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/343412
+milestone: '14.5'
+type: development
+group: group::workspace
+default_enabled: false

db/migrate/20211020095357_add_group_traversal_id_index.rb

+# frozen_string_literal: true
+
+class AddGroupTraversalIdIndex < Gitlab::Database::Migration[1.0]
+  INDEX_NAME = 'index_namespaces_on_traversal_ids_for_groups'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :namespaces, :traversal_ids, using: :gin, where: "type='Group'", name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :namespaces, INDEX_NAME
+  end
+end

db/schema_migrations/20211020095357

+a62ac8920223469c6e4c5a7f67ce9eec972189c98a8c542b377afe4ab28ee25a
\ No newline at end of file

db/structure.sql

@@ -25901,6 +25901,8 @@ CREATE INDEX index_namespaces_on_shared_and_extra_runners_minutes_limit ON names
 
 CREATE INDEX index_namespaces_on_traversal_ids ON namespaces USING gin (traversal_ids);
 
+CREATE INDEX index_namespaces_on_traversal_ids_for_groups ON namespaces USING gin (traversal_ids) WHERE ((type)::text = 'Group'::text);
+
 CREATE INDEX index_namespaces_on_type_and_id ON namespaces USING btree (type, id);
 
 CREATE INDEX index_namespaces_public_groups_name_id ON namespaces USING btree (name, id) WHERE (((type)::text = 'Group'::text) AND (visibility_level = 20));

spec/models/group_spec.rb

@@ -563,6 +563,25 @@ RSpec.describe Group do
           it { expect(group.ancestors.to_sql).not_to include 'traversal_ids <@' }
         end
       end
+
+      context 'when project namespace exists in the group' do
+        let!(:project) { create(:project, group: group) }
+        let!(:project_namespace) { create(:project_namespace, project: project) }
+
+        it 'filters out project namespace' do
+          expect(group.descendants.find_by_id(project_namespace.id)).to be_nil
+        end
+
+        context 'when include_sti_condition is disabled' do
+          before do
+            stub_feature_flags(include_sti_condition: false)
+          end
+
+          it 'raises an exception' do
+            expect { group.descendants.find_by_id(project_namespace.id)}.to raise_error(ActiveRecord::SubclassNotFound)
+          end
+        end
+      end
     end
   end
 

spec/requests/api/members_spec.rb

@@ -81,14 +81,22 @@ RSpec.describe API::Members do
         expect(json_response.map { |u| u['id'] }).to match_array [maintainer.id, developer.id]
       end
 
-      it 'finds members with query string' do
-        get api(members_url, developer), params: { query: maintainer.username }
+      context 'with cross db check disabled' do
+        around do |example|
+          allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/343305') do
+            example.run
+          end
+        end
 
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(response).to include_pagination_headers
-        expect(json_response).to be_an Array
-        expect(json_response.count).to eq(1)
-        expect(json_response.first['username']).to eq(maintainer.username)
+        it 'finds members with query string' do
+          get api(members_url, developer), params: { query: maintainer.username }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(response).to include_pagination_headers
+          expect(json_response).to be_an Array
+          expect(json_response.count).to eq(1)
+          expect(json_response.first['username']).to eq(maintainer.username)
+        end
       end
 
       it 'finds members with the given user_ids' do

spec/support/shared_examples/namespaces/traversal_examples.rb

@@ -22,6 +22,8 @@ RSpec.shared_examples 'namespace traversal' do
   let_it_be(:deep_nested_group) { create(:group, parent: nested_group) }
   let_it_be(:very_deep_nested_group) { create(:group, parent: deep_nested_group) }
   let_it_be(:groups) { [group, nested_group, deep_nested_group, very_deep_nested_group] }
+  let_it_be(:project) { create(:project, group: nested_group) }
+  let_it_be(:project_namespace) { create(:project_namespace, project: project) }
 
   describe '#root_ancestor' do
     it 'returns the correct root ancestor' do
@@ -65,6 +67,7 @@ RSpec.shared_examples 'namespace traversal' do
       expect(deep_nested_group.ancestors).to contain_exactly(group, nested_group)
       expect(nested_group.ancestors).to contain_exactly(group)
       expect(group.ancestors).to eq([])
+      expect(project_namespace.ancestors).to be_empty
     end
 
     context 'with asc hierarchy_order' do
@@ -73,6 +76,7 @@ RSpec.shared_examples 'namespace traversal' do
         expect(deep_nested_group.ancestors(hierarchy_order: :asc)).to eq [nested_group, group]
         expect(nested_group.ancestors(hierarchy_order: :asc)).to eq [group]
         expect(group.ancestors(hierarchy_order: :asc)).to eq([])
+        expect(project_namespace.ancestors(hierarchy_order: :asc)).to be_empty
       end
     end
 
@@ -82,6 +86,7 @@ RSpec.shared_examples 'namespace traversal' do
         expect(deep_nested_group.ancestors(hierarchy_order: :desc)).to eq [group, nested_group]
         expect(nested_group.ancestors(hierarchy_order: :desc)).to eq [group]
         expect(group.ancestors(hierarchy_order: :desc)).to eq([])
+        expect(project_namespace.ancestors(hierarchy_order: :desc)).to be_empty
       end
     end
 
@@ -98,6 +103,7 @@ RSpec.shared_examples 'namespace traversal' do
       expect(deep_nested_group.ancestor_ids).to contain_exactly(group.id, nested_group.id)
       expect(nested_group.ancestor_ids).to contain_exactly(group.id)
       expect(group.ancestor_ids).to be_empty
+      expect(project_namespace.ancestor_ids).to be_empty
     end
 
     context 'with asc hierarchy_order' do
@@ -106,6 +112,7 @@ RSpec.shared_examples 'namespace traversal' do
         expect(deep_nested_group.ancestor_ids(hierarchy_order: :asc)).to eq [nested_group.id, group.id]
         expect(nested_group.ancestor_ids(hierarchy_order: :asc)).to eq [group.id]
         expect(group.ancestor_ids(hierarchy_order: :asc)).to eq([])
+        expect(project_namespace.ancestor_ids(hierarchy_order: :asc)).to eq([])
       end
     end
 
@@ -115,6 +122,7 @@ RSpec.shared_examples 'namespace traversal' do
         expect(deep_nested_group.ancestor_ids(hierarchy_order: :desc)).to eq [group.id, nested_group.id]
         expect(nested_group.ancestor_ids(hierarchy_order: :desc)).to eq [group.id]
         expect(group.ancestor_ids(hierarchy_order: :desc)).to eq([])
+        expect(project_namespace.ancestor_ids(hierarchy_order: :desc)).to eq([])
       end
     end
 
@@ -131,6 +139,7 @@ RSpec.shared_examples 'namespace traversal' do
       expect(deep_nested_group.self_and_ancestors).to contain_exactly(group, nested_group, deep_nested_group)
       expect(nested_group.self_and_ancestors).to contain_exactly(group, nested_group)
       expect(group.self_and_ancestors).to contain_exactly(group)
+      expect(project_namespace.self_and_ancestors).to contain_exactly(project_namespace)
     end
 
     context 'with asc hierarchy_order' do
@@ -139,6 +148,7 @@ RSpec.shared_examples 'namespace traversal' do
         expect(deep_nested_group.self_and_ancestors(hierarchy_order: :asc)).to eq [deep_nested_group, nested_group, group]
         expect(nested_group.self_and_ancestors(hierarchy_order: :asc)).to eq [nested_group, group]
         expect(group.self_and_ancestors(hierarchy_order: :asc)).to eq([group])
+        expect(project_namespace.self_and_ancestors(hierarchy_order: :asc)).to eq([project_namespace])
       end
     end
 
@@ -148,6 +158,7 @@ RSpec.shared_examples 'namespace traversal' do
         expect(deep_nested_group.self_and_ancestors(hierarchy_order: :desc)).to eq [group, nested_group, deep_nested_group]
         expect(nested_group.self_and_ancestors(hierarchy_order: :desc)).to eq [group, nested_group]
         expect(group.self_and_ancestors(hierarchy_order: :desc)).to eq([group])
+        expect(project_namespace.self_and_ancestors(hierarchy_order: :desc)).to eq([project_namespace])
       end
     end
 
@@ -164,6 +175,7 @@ RSpec.shared_examples 'namespace traversal' do
       expect(deep_nested_group.self_and_ancestor_ids).to contain_exactly(group.id, nested_group.id, deep_nested_group.id)
       expect(nested_group.self_and_ancestor_ids).to contain_exactly(group.id, nested_group.id)
       expect(group.self_and_ancestor_ids).to contain_exactly(group.id)
+      expect(project_namespace.self_and_ancestor_ids).to contain_exactly(project_namespace.id)
     end
 
     context 'with asc hierarchy_order' do
@@ -172,6 +184,7 @@ RSpec.shared_examples 'namespace traversal' do
         expect(deep_nested_group.self_and_ancestor_ids(hierarchy_order: :asc)).to eq [deep_nested_group.id, nested_group.id, group.id]
         expect(nested_group.self_and_ancestor_ids(hierarchy_order: :asc)).to eq [nested_group.id, group.id]
         expect(group.self_and_ancestor_ids(hierarchy_order: :asc)).to eq([group.id])
+        expect(project_namespace.self_and_ancestor_ids(hierarchy_order: :asc)).to eq([project_namespace.id])
       end
     end
 
@@ -181,6 +194,7 @@ RSpec.shared_examples 'namespace traversal' do
         expect(deep_nested_group.self_and_ancestor_ids(hierarchy_order: :desc)).to eq [group.id, nested_group.id, deep_nested_group.id]
         expect(nested_group.self_and_ancestor_ids(hierarchy_order: :desc)).to eq [group.id, nested_group.id]
         expect(group.self_and_ancestor_ids(hierarchy_order: :desc)).to eq([group.id])
+        expect(project_namespace.self_and_ancestor_ids(hierarchy_order: :desc)).to eq([project_namespace.id])
       end
     end
 
@@ -205,6 +219,10 @@ RSpec.shared_examples 'namespace traversal' do
     describe '#recursive_descendants' do
       it_behaves_like 'recursive version', :descendants
     end
+
+    it 'does not include project namespaces' do
+      expect(group.descendants.to_a).not_to include(project_namespace)
+    end
   end
 
   describe '#self_and_descendants' do
@@ -223,6 +241,10 @@ RSpec.shared_examples 'namespace traversal' do
 
       it_behaves_like 'recursive version', :self_and_descendants
     end
+
+    it 'does not include project namespaces' do
+      expect(group.self_and_descendants.to_a).not_to include(project_namespace)
+    end
   end
 
   describe '#self_and_descendant_ids' do

spec/support/shared_examples/namespaces/traversal_scope_examples.rb

@@ -26,9 +26,23 @@ RSpec.shared_examples 'namespace traversal scopes' do
   end
 
   describe '.without_sti_condition' do
-    subject { described_class.without_sti_condition }
+    subject { described_class.where(type: 'Group').without_sti_condition }
 
-    it { expect(subject.where_values_hash).not_to have_key(:type) }
+    context 'when include_sti_condition is enabled' do
+      before do
+        stub_feature_flags(include_sti_condition: true)
+      end
+
+      it { expect(subject.where_values_hash).to have_key('type') }
+    end
+
+    context 'when include_sti_condition is disabled' do
+      before do
+        stub_feature_flags(include_sti_condition: false)
+      end
+
+      it { expect(subject.where_values_hash).not_to have_key('type') }
+    end
   end
 
   describe '.order_by_depth' do