Merge branch '225545-refactor-pypi-finder' into 'master'

Refactor package finders and add PyPI finders

See merge request gitlab-org/gitlab!61534

app/finders/packages/group_or_project_package_finder.rb

+# frozen_string_literal: true
+
+module Packages
+  class GroupOrProjectPackageFinder
+    include ::Packages::FinderHelper
+
+    def initialize(current_user, project_or_group, params = {})
+      @current_user = current_user
+      @project_or_group = project_or_group
+      @params = params
+    end
+
+    def execute
+      raise NotImplementedError
+    end
+
+    def execute!
+      raise NotImplementedError
+    end
+
+    private
+
+    def packages
+      raise NotImplementedError
+    end
+
+    def base
+      if project?
+        packages_for_project(@project_or_group)
+      elsif group?
+        packages_visible_to_user(@current_user, within_group: @project_or_group)
+      else
+        ::Packages::Package.none
+      end
+    end
+
+    def project?
+      @project_or_group.is_a?(::Project)
+    end
+
+    def group?
+      @project_or_group.is_a?(::Group)
+    end
+  end
+end

app/finders/packages/maven/package_finder.rb

@@ -2,41 +2,20 @@
 
 module Packages
   module Maven
-    class PackageFinder
-      include ::Packages::FinderHelper
-      include Gitlab::Utils::StrongMemoize
-
-      def initialize(path, current_user, project: nil, group: nil, order_by_package_file: false)
-        @path = path
-        @current_user = current_user
-        @project = project
-        @group = group
-        @order_by_package_file = order_by_package_file
-      end
-
+    class PackageFinder < ::Packages::GroupOrProjectPackageFinder
       def execute
-        packages_with_path.last
+        packages.last
       end
 
       def execute!
-        packages_with_path.last!
+        packages.last!
       end
 
       private
 
-      def base
-        if @project
-          packages_for_project(@project)
-        elsif @group
-          packages_visible_to_user(@current_user, within_group: @group)
-        else
-          ::Packages::Package.none
-        end
-      end
-
-      def packages_with_path
-        matching_packages = base.only_maven_packages_with_path(@path, use_cte: @group.present?)
-        matching_packages = matching_packages.order_by_package_file if @order_by_package_file
+      def packages
+        matching_packages = base.only_maven_packages_with_path(@params[:path], use_cte: group?)
+        matching_packages = matching_packages.order_by_package_file if @params[:order_by_package_file]
 
         matching_packages
       end

app/finders/packages/nuget/package_finder.rb

@@ -2,51 +2,23 @@
 
 module Packages
   module Nuget
-    class PackageFinder
-      include ::Packages::FinderHelper
-
+    class PackageFinder < ::Packages::GroupOrProjectPackageFinder
       MAX_PACKAGES_COUNT = 300
 
-      def initialize(current_user, project_or_group, package_name:, package_version: nil, limit: MAX_PACKAGES_COUNT)
-        @current_user = current_user
-        @project_or_group = project_or_group
-        @package_name = package_name
-        @package_version = package_version
-        @limit = limit
-      end
-
       def execute
-        packages.limit_recent(@limit)
+        packages.limit_recent(@params[:limit] || MAX_PACKAGES_COUNT)
       end
 
       private
 
-      def base
-        if project?
-          packages_for_project(@project_or_group)
-        elsif group?
-          packages_visible_to_user(@current_user, within_group: @project_or_group)
-        else
-          ::Packages::Package.none
-        end
-      end
-
       def packages
         result = base.nuget
                      .has_version
                      .processed
-                     .with_name_like(@package_name)
-        result = result.with_version(@package_version) if @package_version.present?
+                     .with_name_like(@params[:package_name])
+        result = result.with_version(@params[:package_version]) if @params[:package_version].present?
         result
       end
-
-      def project?
-        @project_or_group.is_a?(::Project)
-      end
-
-      def group?
-        @project_or_group.is_a?(::Group)
-      end
     end
   end
 end

app/finders/packages/pypi/package_finder.rb

+# frozen_string_literal: true
+
+module Packages
+  module Pypi
+    class PackageFinder < ::Packages::GroupOrProjectPackageFinder
+      def execute
+        packages.by_file_name_and_sha256(@params[:filename], @params[:sha256])
+      end
+
+      private
+
+      def packages
+        base.pypi.has_version.processed
+      end
+    end
+  end
+end

app/finders/packages/pypi/packages_finder.rb

+# frozen_string_literal: true
+
+module Packages
+  module Pypi
+    class PackagesFinder < ::Packages::GroupOrProjectPackageFinder
+      def execute!
+        results = packages.with_normalized_pypi_name(@params[:package_name])
+        raise ActiveRecord::RecordNotFound if results.empty?
+
+        results
+      end
+
+      private
+
+      def packages
+        base.pypi.has_version.processed
+      end
+    end
+  end
+end

app/services/packages/maven/find_or_create_package_service.rb

@@ -6,7 +6,7 @@ module Packages
 
       def execute
         package =
-          ::Packages::Maven::PackageFinder.new(params[:path], current_user, project: project)
+          ::Packages::Maven::PackageFinder.new(current_user, project, path: params[:path])
                                           .execute
 
         unless Namespace::PackageSetting.duplicates_allowed?(package)

lib/api/maven_packages.rb

@@ -92,10 +92,9 @@ module API
                                   !params[:path].include?(::Packages::Maven::FindOrCreatePackageService::SNAPSHOT_TERM)
 
         ::Packages::Maven::PackageFinder.new(
-          params[:path],
           current_user,
-          project: project,
-          group: group,
+          project || group,
+          path: params[:path],
           order_by_package_file: order_by_package_file
         ).execute!
       end

lib/api/pypi_packages.rb

@@ -24,25 +24,6 @@ module API
       render_api_error!(e.message, 400)
     end
 
-    helpers do
-      def packages_finder(project = authorized_user_project)
-        project
-          .packages
-          .pypi
-          .has_version
-          .processed
-      end
-
-      def find_package_versions
-        packages = packages_finder
-          .with_normalized_pypi_name(params[:package_name])
-
-        not_found!('Package') if packages.empty?
-
-        packages
-      end
-    end
-
     before do
       require_packages_enabled!
     end
@@ -71,7 +52,7 @@ module API
           project = unauthorized_user_project!
 
           filename = "#{params[:file_identifier]}.#{params[:format]}"
-          package = packages_finder(project).by_file_name_and_sha256(filename, params[:sha256])
+          package = Packages::Pypi::PackageFinder.new(current_user, project, { filename: filename, sha256: params[:sha256] }).execute
           package_file = ::Packages::PackageFileFinder.new(package, filename, with_file_name_like: false).execute
 
           track_package_event('pull_package', :pypi)
@@ -95,7 +76,7 @@ module API
 
           track_package_event('list_package', :pypi)
 
-          packages = find_package_versions
+          packages = Packages::Pypi::PackagesFinder.new(current_user, authorized_user_project, { package_name: params[:package_name] }).execute!
           presenter = ::Packages::Pypi::PackagePresenter.new(packages, authorized_user_project)
 
           # Adjusts grape output format

spec/finders/packages/group_or_project_package_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Packages::GroupOrProjectPackageFinder do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project) }
+
+  let(:finder) { described_class.new(user, project) }
+
+  describe 'execute' do
+    subject(:run_finder) { finder.execute }
+
+    it { expect { run_finder }.to raise_error(NotImplementedError) }
+  end
+
+  describe 'execute!' do
+    subject(:run_finder) { finder.execute! }
+
+    it { expect { run_finder }.to raise_error(NotImplementedError) }
+  end
+end

spec/finders/packages/maven/package_finder_spec.rb

@@ -9,10 +9,9 @@ RSpec.describe ::Packages::Maven::PackageFinder do
   let_it_be_with_refind(:package) { create(:maven_package, project: project) }
 
   let(:param_path) { nil }
-  let(:param_project) { nil }
-  let(:param_group) { nil }
+  let(:project_or_group) { nil }
   let(:param_order_by_package_file) { false }
-  let(:finder) { described_class.new(param_path, user, project: param_project, group: param_group, order_by_package_file: param_order_by_package_file) }
+  let(:finder) { described_class.new(user, project_or_group, path: param_path, order_by_package_file: param_order_by_package_file) }
 
   before do
     group.add_developer(user)
@@ -49,13 +48,13 @@ RSpec.describe ::Packages::Maven::PackageFinder do
       end
 
       context 'within the project' do
-        let(:param_project) { project }
+        let(:project_or_group) { project }
 
         it_behaves_like 'handling valid and invalid paths'
       end
 
       context 'within a group' do
-        let(:param_group) { group }
+        let(:project_or_group) { group }
 
         it_behaves_like 'handling valid and invalid paths'
       end
@@ -77,7 +76,7 @@ RSpec.describe ::Packages::Maven::PackageFinder do
         let_it_be(:package2) { create(:maven_package, project: project2, name: package_name, version: nil) }
         let_it_be(:package3) { create(:maven_package, project: project3, name: package_name, version: nil) }
 
-        let(:param_group) { group }
+        let(:project_or_group) { group }
         let(:param_path) { package_name }
 
         before do
@@ -116,7 +115,7 @@ RSpec.describe ::Packages::Maven::PackageFinder do
     it_behaves_like 'Packages::Maven::PackageFinder examples'
 
     it 'uses CTE in the query' do
-      sql = described_class.new('some_path', user, group: group).send(:packages_with_path).to_sql
+      sql = described_class.new(user, group, path: package.maven_metadatum.path).send(:packages).to_sql
 
       expect(sql).to include('WITH "maven_metadata_by_path" AS')
     end

spec/finders/packages/pypi/package_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Packages::Pypi::PackageFinder do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, group: group) }
+  let_it_be(:project2) { create(:project, group: group) }
+  let_it_be(:package1) { create(:pypi_package, project: project) }
+  let_it_be(:package2) { create(:pypi_package, project: project) }
+  let_it_be(:package3) { create(:pypi_package, project: project2) }
+
+  let(:package_file) { package2.package_files.first }
+  let(:params) do
+    {
+      filename: package_file.file_name,
+      sha256: package_file.file_sha256
+    }
+  end
+
+  describe 'execute' do
+    subject { described_class.new(user, scope, params).execute }
+
+    context 'within a project' do
+      let(:scope) { project }
+
+      it { is_expected.to eq(package2) }
+    end
+
+    context 'within a group' do
+      let(:scope) { group }
+
+      it { expect { subject }.to raise_error(ActiveRecord::RecordNotFound) }
+
+      context 'user with access' do
+        before do
+          project.add_developer(user)
+        end
+
+        it { is_expected.to eq(package2) }
+      end
+    end
+  end
+end

spec/finders/packages/pypi/packages_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Packages::Pypi::PackagesFinder do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, group: group) }
+  let_it_be(:project2) { create(:project, group: group) }
+  let_it_be(:package1) { create(:pypi_package, project: project) }
+  let_it_be(:package2) { create(:pypi_package, project: project) }
+  let_it_be(:package3) { create(:pypi_package, name: package2.name, project: project) }
+  let_it_be(:package4) { create(:pypi_package, name: package2.name, project: project2) }
+
+  let(:package_name) { package2.name }
+
+  describe 'execute!' do
+    subject { described_class.new(user, scope, package_name: package_name).execute! }
+
+    shared_examples 'when no package is found' do
+      context 'non-existing package' do
+        let(:package_name) { 'none' }
+
+        it { expect { subject }.to raise_error(ActiveRecord::RecordNotFound) }
+      end
+    end
+
+    shared_examples 'when package_name param is a non-normalized name' do
+      context 'non-existing package' do
+        let(:package_name) { package2.name.upcase.tr('-', '.') }
+
+        it { expect { subject }.to raise_error(ActiveRecord::RecordNotFound) }
+      end
+    end
+
+    context 'within a project' do
+      let(:scope) { project }
+
+      it { is_expected.to contain_exactly(package2, package3) }
+
+      it_behaves_like 'when no package is found'
+      it_behaves_like 'when package_name param is a non-normalized name'
+    end
+
+    context 'within a group' do
+      let(:scope) { group }
+
+      it { expect { subject }.to raise_error(ActiveRecord::RecordNotFound) }
+
+      context 'user with access to only one project' do
+        before do
+          project2.add_developer(user)
+        end
+
+        it { is_expected.to contain_exactly(package4) }
+
+        it_behaves_like 'when no package is found'
+        it_behaves_like 'when package_name param is a non-normalized name'
+
+        context ' user with access to multiple projects' do
+          before do
+            project.add_developer(user)
+          end
+
+          it { is_expected.to contain_exactly(package2, package3, package4) }
+        end
+      end
+    end
+  end
+end