Merge branch...

Merge branch '505645-package-registry-optimize-query-for-selecting-projects-within-a-namespace' into 'master' 

Package registry: Optimize query for selecting projects within a namespace

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/175358



Merged-by: Dzmitry (Dima) Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Approved-by: Dzmitry (Dima) Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Approved-by: Adam Hegyi <ahegyi@gitlab.com>
Reviewed-by: Dzmitry (Dima) Meshcharakou <12459192-dmeshcharakou@users.noreply.gitlab.com>
Reviewed-by: Radamanthus Batnag <rbatnag@gitlab.com>
Reviewed-by: Moaz Khalifa <mkhalifa@gitlab.com>
Co-authored-by: Radamanthus Batnag <rbatnag@gitlab.com>

app/finders/packages/npm/packages_for_user_finder.rb

@@ -20,6 +20,20 @@ def group_packages
         packages_visible_to_user(@current_user, within_group: @project_or_group, with_package_registry_enabled: true)
       end
 
+      # This overrides projects_visible_to_reporters in app/finders/concerns/packages/finder_helper.rb
+      # to implement npm-specific optimizations
+      def projects_visible_to_reporters(user, within_group:, _within_public_package_registry: false)
+        return user.accessible_projects if user.is_a?(DeployToken)
+
+        access = if Feature.enabled?(:allow_guest_plus_roles_to_pull_packages, within_group.root_ancestor)
+                   ::Gitlab::Access::GUEST
+                 else
+                   ::Gitlab::Access::REPORTER
+                 end
+
+        ::Project.public_or_visible_to_user(user, access).by_any_overlap_with_traversal_ids(within_group.id)
+      end
+
       override :packages_class
       def packages_class
         ::Packages::Npm::Package

spec/finders/concerns/packages/finder_helper_spec.rb

@@ -323,6 +323,20 @@ def packages_class
       it { is_expected.to be_empty }
     end
 
+    shared_examples 'handles a group deploy token' do
+      let_it_be(:user) { create(:deploy_token, :group, read_package_registry: true) }
+      let_it_be(:group_deploy_token) { create(:group_deploy_token, deploy_token: user, group: group) }
+
+      before do
+        project2.update!(visibility_level: Gitlab::VisibilityLevel.const_get('PRIVATE', false))
+        subgroup.update!(visibility_level: Gitlab::VisibilityLevel.const_get('PRIVATE', false))
+        project1.update!(visibility_level: Gitlab::VisibilityLevel.const_get('PRIVATE', false))
+        group.update!(visibility_level: Gitlab::VisibilityLevel.const_get('PRIVATE', false))
+      end
+
+      it_behaves_like 'returning both projects'
+    end
+
     describe '#projects_visible_to_user' do
       subject { finder.projects_visible_to_user(user, within_group: group) }
 
@@ -365,28 +379,7 @@ def packages_class
         end
       end
 
-      context 'with a group deploy token' do
-        let_it_be(:user) { create(:deploy_token, :group, read_package_registry: true) }
-        let_it_be(:group_deploy_token) { create(:group_deploy_token, deploy_token: user, group: group) }
-
-        where(:group_visibility, :subgroup_visibility, :project2_visibility, :shared_example_name) do
-          'PUBLIC'  | 'PUBLIC'  | 'PUBLIC'  | 'returning both projects'
-          'PUBLIC'  | 'PUBLIC'  | 'PRIVATE' | 'returning both projects'
-          'PUBLIC'  | 'PRIVATE' | 'PRIVATE' | 'returning both projects'
-          'PRIVATE' | 'PRIVATE' | 'PRIVATE' | 'returning both projects'
-        end
-
-        with_them do
-          before do
-            project2.update!(visibility_level: Gitlab::VisibilityLevel.const_get(project2_visibility, false))
-            subgroup.update!(visibility_level: Gitlab::VisibilityLevel.const_get(subgroup_visibility, false))
-            project1.update!(visibility_level: Gitlab::VisibilityLevel.const_get(group_visibility, false))
-            group.update!(visibility_level: Gitlab::VisibilityLevel.const_get(group_visibility, false))
-          end
-
-          it_behaves_like params[:shared_example_name]
-        end
-      end
+      it_behaves_like 'handles a group deploy token'
     end
 
     describe '#projects_visible_to_user_including_public_registries' do
@@ -415,5 +408,37 @@ def packages_class
         it_behaves_like params[:shared_example_name]
       end
     end
+
+    describe '#projects_visible_to_reporters' do
+      before_all do
+        project1.add_reporter(user)
+      end
+
+      let(:within_public_package_registry) { false }
+
+      subject do
+        finder.projects_visible_to_reporters(user,
+          within_group: group,
+          within_public_package_registry: within_public_package_registry)
+      end
+
+      it_behaves_like 'handles a group deploy token'
+
+      it_behaves_like 'returning project1'
+
+      context 'when within_public_package_registry set to true' do
+        let_it_be(:project_with_public_package_registry) { create(:project, group: group) }
+
+        let(:within_public_package_registry) { true }
+
+        before do
+          project_with_public_package_registry
+            .project_feature
+            .update_attribute(:package_registry_access_level, ::ProjectFeature::PUBLIC)
+        end
+
+        it { is_expected.to match_array [project1, project_with_public_package_registry] }
+      end
+    end
   end
 end

spec/finders/packages/npm/packages_for_user_finder_spec.rb

@@ -33,15 +33,32 @@
       let(:project_or_group) { group }
 
       before_all do
-        project.add_reporter(user)
+        project.add_guest(user)
       end
 
       it_behaves_like 'searches for packages'
       it_behaves_like 'avoids N+1 database queries in the package registry'
 
-      context 'when an user is a reporter of both projects' do
+      context 'with a project inaccessible to user, but with a public registry' do
+        let_it_be(:project_with_public_package_registry) { create(:project, group: group) }
+        let_it_be(:other_package) do
+          create(:npm_package, project: project_with_public_package_registry, name: package.name)
+        end
+
+        before do
+          project_with_public_package_registry
+            .project_feature
+            .update_attribute(:package_registry_access_level, ::ProjectFeature::PUBLIC)
+        end
+
+        it 'excludes the packages from the public registry' do
+          is_expected.to contain_exactly(package)
+        end
+      end
+
+      context 'when an user is a guest of both projects' do
         before_all do
-          project2.add_reporter(user)
+          project2.add_guest(user)
         end
 
         it { is_expected.to contain_exactly(package, package_with_diff_project) }
@@ -56,6 +73,33 @@
           it_behaves_like 'searches for packages'
         end
       end
+
+      context 'when allow_guest_plus_roles_to_pull_packages is disabled' do
+        before_all do
+          stub_feature_flags(allow_guest_plus_roles_to_pull_packages: false)
+          project.add_reporter(user)
+        end
+
+        it_behaves_like 'searches for packages'
+
+        context 'when an user is a reporter of both projects' do
+          before_all do
+            project2.add_reporter(user)
+          end
+
+          it { is_expected.to contain_exactly(package, package_with_diff_project) }
+
+          context 'when the second project has the package registry disabled' do
+            before_all do
+              project.reload.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+              project2.reload.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC,
+                package_registry_access_level: 'disabled', packages_enabled: false)
+            end
+
+            it_behaves_like 'searches for packages'
+          end
+        end
+      end
     end
   end
 end