Merge branch '325869-improve-maven-packages-group-level-api' into 'master'

Improvements to the Maven API files endpoints [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57600

app/finders/packages/maven/package_finder.rb

@@ -3,13 +3,15 @@
 module Packages
   module Maven
     class PackageFinder
-      attr_reader :path, :current_user, :project, :group
+      include ::Packages::FinderHelper
+      include Gitlab::Utils::StrongMemoize
 
-      def initialize(path, current_user, project: nil, group: nil)
+      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
 
       def execute
@@ -23,9 +25,9 @@ module Packages
       private
 
       def base
-        if project
+        if @project
           packages_for_a_single_project
-        elsif group
+        elsif @group
           packages_for_multiple_projects
         else
           ::Packages::Package.none
@@ -33,8 +35,13 @@ module Packages
       end
 
       def packages_with_path
-        matching_packages = base.only_maven_packages_with_path(path, use_cte: group.present?)
+        matching_packages = base.only_maven_packages_with_path(@path, use_cte: @group.present?)
+
+        if group_level_improvements?
+          matching_packages = matching_packages.order_by_package_file if @order_by_package_file
+        else
           matching_packages = matching_packages.order_by_package_file if versionless_package?(matching_packages)
+        end
 
         matching_packages
       end
@@ -48,19 +55,29 @@ module Packages
 
       # Produces a query that retrieves packages from a single project.
       def packages_for_a_single_project
-        project.packages
+        @project.packages
       end
 
       # Produces a query that retrieves packages from multiple projects that
       # the current user can view within a group.
       def packages_for_multiple_projects
+        if group_level_improvements?
+          packages_visible_to_user(@current_user, within_group: @group)
+        else
           ::Packages::Package.for_projects(projects_visible_to_current_user)
         end
+      end
 
       # Returns the projects that the current user can view within a group.
       def projects_visible_to_current_user
-        group.all_projects
-             .public_or_visible_to_user(current_user)
+        @group.all_projects
+              .public_or_visible_to_user(@current_user)
+      end
+
+      def group_level_improvements?
+        strong_memoize(:group_level_improvements) do
+          Feature.enabled?(:maven_packages_group_level_improvements)
+        end
       end
     end
   end

app/models/packages/package.rb

@@ -136,7 +136,9 @@ class Packages::Package < ApplicationRecord
   after_commit :update_composer_cache, on: :destroy, if: -> { composer? }
 
   def self.for_projects(projects)
+    unless Feature.enabled?(:maven_packages_group_level_improvements)
       return none unless projects.any?
+    end
 
     where(project_id: projects)
   end

config/feature_flags/development/maven_packages_group_level_improvements.yml

+---
+name: maven_packages_group_level_improvements
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57600
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/326099
+milestone: '13.11'
+type: development
+group: group::package
+default_enabled: false

lib/api/maven_packages.rb

@@ -77,6 +77,22 @@ module API
           request.head? &&
           file.fog_credentials[:provider] == 'AWS'
       end
+
+      def fetch_package(file_name:, project: nil, group: nil)
+        order_by_package_file = false
+        if Feature.enabled?(:maven_packages_group_level_improvements)
+          order_by_package_file = file_name.include?(::Packages::Maven::Metadata.filename) &&
+                                    !params[:path].include?(::Packages::Maven::FindOrCreatePackageService::SNAPSHOT_TERM)
+        end
+
+        ::Packages::Maven::PackageFinder.new(
+          params[:path],
+          current_user,
+          project: project,
+          group: group,
+          order_by_package_file: order_by_package_file
+        ).execute!
+      end
     end
 
     desc 'Download the maven package file at instance level' do
@@ -97,8 +113,7 @@ module API
 
       authorize_read_package!(project)
 
-      package = ::Packages::Maven::PackageFinder
-        .new(params[:path], current_user, project: project).execute!
+      package = fetch_package(file_name: file_name, project: project)
 
       package_file = ::Packages::PackageFileFinder
         .new(package, file_name).execute!
@@ -133,8 +148,7 @@ module API
 
         not_found!('Group') unless can?(current_user, :read_group, group)
 
-        package = ::Packages::Maven::PackageFinder
-          .new(params[:path], current_user, group: group).execute!
+        package = fetch_package(file_name: file_name, group: group)
 
         authorize_read_package!(package.project)
 
@@ -171,8 +185,7 @@ module API
 
         file_name, format = extract_format(params[:file_name])
 
-        package = ::Packages::Maven::PackageFinder
-          .new(params[:path], current_user, project: user_project).execute!
+        package = fetch_package(file_name: file_name, project: user_project)
 
         package_file = ::Packages::PackageFileFinder
           .new(package, file_name).execute!

spec/finders/packages/maven/package_finder_spec.rb

@@ -11,7 +11,8 @@ RSpec.describe ::Packages::Maven::PackageFinder do
   let(:param_path) { nil }
   let(:param_project) { nil }
   let(:param_group) { nil }
-  let(:finder) { described_class.new(param_path, user, project: param_project, group: param_group) }
+  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) }
 
   before do
     group.add_developer(user)
@@ -46,9 +47,25 @@ RSpec.describe ::Packages::Maven::PackageFinder do
       context 'within a group' do
         let(:param_group) { group }
 
+        context 'with maven_packages_group_level_improvements enabled' do
+          before do
+            stub_feature_flags(maven_packages_group_level_improvements: true)
+            expect(finder).to receive(:packages_visible_to_user).with(user, within_group: group).and_call_original
+          end
+
           it_behaves_like 'handling valid and invalid paths'
         end
 
+        context 'with maven_packages_group_level_improvements disabled' do
+          before do
+            stub_feature_flags(maven_packages_group_level_improvements: false)
+            expect(finder).not_to receive(:packages_visible_to_user)
+          end
+
+          it_behaves_like 'handling valid and invalid paths'
+        end
+      end
+
       context 'across all projects' do
         it 'raises an error' do
           expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
@@ -76,8 +93,40 @@ RSpec.describe ::Packages::Maven::PackageFinder do
           create(:package_file, :xml, package: package2)
         end
 
+        context 'with maven_packages_group_level_improvements enabled' do
+          before do
+            stub_feature_flags(maven_packages_group_level_improvements: true)
+            expect(finder).not_to receive(:versionless_package?)
+          end
+
+          context 'without order by package file' do
+            it { is_expected.to eq(package3) }
+          end
+
+          context 'with order by package file' do
+            let(:param_order_by_package_file) { true }
+
+            it { is_expected.to eq(package2) }
+          end
+        end
+
+        context 'with maven_packages_group_level_improvements disabled' do
+          before do
+            stub_feature_flags(maven_packages_group_level_improvements: false)
+            expect(finder).to receive(:versionless_package?).and_call_original
+          end
+
+          context 'without order by package file' do
             it { is_expected.to eq(package2) }
           end
+
+          context 'with order by package file' do
+            let(:param_order_by_package_file) { true }
+
+            it { is_expected.to eq(package2) }
+          end
+        end
+      end
     end
   end
 

spec/models/packages/package_spec.rb

@@ -99,6 +99,34 @@ RSpec.describe Packages::Package, type: :model do
     end
   end
 
+  describe '.for_projects' do
+    let_it_be(:package1) { create(:maven_package) }
+    let_it_be(:package2) { create(:maven_package) }
+    let_it_be(:package3) { create(:maven_package) }
+
+    let(:projects) { ::Project.id_in([package1.project_id, package2.project_id]) }
+
+    subject { described_class.for_projects(projects.select(:id)) }
+
+    it 'returns package1 and package2' do
+      expect(projects).not_to receive(:any?)
+
+      expect(subject).to match_array([package1, package2])
+    end
+
+    context 'with maven_packages_group_level_improvements disabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: false)
+      end
+
+      it 'returns package1 and package2' do
+        expect(projects).to receive(:any?).and_call_original
+
+        expect(subject).to match_array([package1, package2])
+      end
+    end
+  end
+
   describe 'validations' do
     subject { build(:package) }
 

spec/requests/api/maven_packages_spec.rb

@@ -147,6 +147,7 @@ RSpec.describe API::MavenPackages do
   end
 
   describe 'GET /api/v4/packages/maven/*path/:file_name' do
+    shared_examples 'handling all conditions' do
       context 'a public project' do
         subject { download_file(package_file.file_name) }
 
@@ -260,6 +261,23 @@ RSpec.describe API::MavenPackages do
           expect(response).to have_gitlab_http_status(:forbidden)
         end
       end
+    end
+
+    context 'with maven_packages_group_level_improvements enabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: true)
+      end
+
+      it_behaves_like 'handling all conditions'
+    end
+
+    context 'with maven_packages_group_level_improvements disabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: false)
+      end
+
+      it_behaves_like 'handling all conditions'
+    end
 
     def download_file(file_name, params = {}, request_headers = headers)
       get api("/packages/maven/#{maven_metadatum.path}/#{file_name}"), params: params, headers: request_headers
@@ -274,6 +292,22 @@ RSpec.describe API::MavenPackages do
     let(:url) { "/packages/maven/#{package.maven_metadatum.path}/#{package_file.file_name}" }
 
     it_behaves_like 'processing HEAD requests'
+
+    context 'with maven_packages_group_level_improvements enabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: true)
+      end
+
+      it_behaves_like 'processing HEAD requests'
+    end
+
+    context 'with maven_packages_group_level_improvements disabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: false)
+      end
+
+      it_behaves_like 'processing HEAD requests'
+    end
   end
 
   describe 'GET /api/v4/groups/:id/-/packages/maven/*path/:file_name' do
@@ -282,6 +316,7 @@ RSpec.describe API::MavenPackages do
       group.add_developer(user)
     end
 
+    shared_examples 'handling all conditions' do
       context 'a public project' do
         subject { download_file(package_file.file_name) }
 
@@ -352,7 +387,8 @@ RSpec.describe API::MavenPackages do
 
           subject
 
-        expect(response).to have_gitlab_http_status(:forbidden)
+          status = Feature.enabled?(:maven_packages_group_level_improvements) ? :not_found : :forbidden
+          expect(response).to have_gitlab_http_status(status)
         end
 
         it 'denies download when no private token' do
@@ -386,6 +422,76 @@ RSpec.describe API::MavenPackages do
         end
       end
 
+      context 'maven metadata file' do
+        let_it_be(:sub_group1) { create(:group, parent: group) }
+        let_it_be(:sub_group2)   { create(:group, parent: group) }
+        let_it_be(:project1) { create(:project, :private, group: sub_group1) }
+        let_it_be(:project2) { create(:project, :private, group: sub_group2) }
+        let_it_be(:project3) { create(:project, :private, group: sub_group1) }
+        let_it_be(:package_name) { 'foo' }
+        let_it_be(:package1) { create(:maven_package, project: project1, name: package_name, version: nil) }
+        let_it_be(:package_file1) { create(:package_file, :xml, package: package1, file_name: 'maven-metadata.xml') }
+        let_it_be(:package2) { create(:maven_package, project: project2, name: package_name, version: nil) }
+        let_it_be(:package_file2) { create(:package_file, :xml, package: package2, file_name: 'maven-metadata.xml') }
+        let_it_be(:package3) { create(:maven_package, project: project3, name: package_name, version: nil) }
+        let_it_be(:package_file3) { create(:package_file, :xml, package: package3, file_name: 'maven-metadata.xml') }
+
+        let(:maven_metadatum) { package3.maven_metadatum }
+
+        subject { download_file_with_token(package_file3.file_name) }
+
+        before do
+          sub_group1.add_developer(user)
+          sub_group2.add_developer(user)
+          # the package with the most recently published file should be returned
+          create(:package_file, :xml, package: package2)
+        end
+
+        context 'in multiple versionless packages' do
+          it 'downloads the file' do
+            expect(::Packages::PackageFileFinder)
+              .to receive(:new).with(package2, 'maven-metadata.xml').and_call_original
+
+            subject
+          end
+        end
+
+        context 'in multiple snapshot packages' do
+          before do
+            version = '1.0.0-SNAPSHOT'
+            [package1, package2, package3].each do |pkg|
+              pkg.update!(version: version)
+
+              pkg.maven_metadatum.update!(path: "#{pkg.name}/#{pkg.version}")
+            end
+          end
+
+          it 'downloads the file' do
+            expect(::Packages::PackageFileFinder)
+              .to receive(:new).with(package3, 'maven-metadata.xml').and_call_original
+
+            subject
+          end
+        end
+      end
+    end
+
+    context 'with maven_packages_group_level_improvements enabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: true)
+      end
+
+      it_behaves_like 'handling all conditions'
+    end
+
+    context 'with maven_packages_group_level_improvements disabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: false)
+      end
+
+      it_behaves_like 'handling all conditions'
+    end
+
     def download_file(file_name, params = {}, request_headers = headers)
       get api("/groups/#{group.id}/-/packages/maven/#{maven_metadatum.path}/#{file_name}"), params: params, headers: request_headers
     end
@@ -398,10 +504,25 @@ RSpec.describe API::MavenPackages do
   describe 'HEAD /api/v4/groups/:id/-/packages/maven/*path/:file_name' do
     let(:url) { "/groups/#{group.id}/-/packages/maven/#{package.maven_metadatum.path}/#{package_file.file_name}" }
 
+    context 'with maven_packages_group_level_improvements enabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: true)
+      end
+
+      it_behaves_like 'processing HEAD requests'
+    end
+
+    context 'with maven_packages_group_level_improvements disabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: false)
+      end
+
       it_behaves_like 'processing HEAD requests'
     end
+  end
 
   describe 'GET /api/v4/projects/:id/packages/maven/*path/:file_name' do
+    shared_examples 'handling all conditions' do
       context 'a public project' do
         subject { download_file(package_file.file_name) }
 
@@ -457,6 +578,23 @@ RSpec.describe API::MavenPackages do
 
         it_behaves_like 'downloads with a deploy token'
       end
+    end
+
+    context 'with maven_packages_group_level_improvements enabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: true)
+      end
+
+      it_behaves_like 'handling all conditions'
+    end
+
+    context 'with maven_packages_group_level_improvements disabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: false)
+      end
+
+      it_behaves_like 'handling all conditions'
+    end
 
     def download_file(file_name, params = {}, request_headers = headers)
       get api("/projects/#{project.id}/packages/maven/" \
@@ -471,9 +609,23 @@ RSpec.describe API::MavenPackages do
   describe 'HEAD /api/v4/projects/:id/packages/maven/*path/:file_name' do
     let(:url) { "/projects/#{project.id}/packages/maven/#{package.maven_metadatum.path}/#{package_file.file_name}" }
 
+    context 'with maven_packages_group_level_improvements enabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: true)
+      end
+
       it_behaves_like 'processing HEAD requests'
     end
 
+    context 'with maven_packages_group_level_improvements disabled' do
+      before do
+        stub_feature_flags(maven_packages_group_level_improvements: false)
+      end
+
+      it_behaves_like 'processing HEAD requests'
+    end
+  end
+
   describe 'PUT /api/v4/projects/:id/packages/maven/*path/:file_name/authorize' do
     it 'rejects a malicious request' do
       put api("/projects/#{project.id}/packages/maven/com/example/my-app/#{version}/%2e%2e%2F.ssh%2Fauthorized_keys/authorize"), params: {}, headers: headers_with_token