Merge branch '337974-fix-helm-metadata-endpoint' into 'master'

Refactor the helm index presenter

See merge request gitlab-org/gitlab!69223

app/finders/packages/helm/packages_finder.rb

+# frozen_string_literal: true
+
+module Packages
+  module Helm
+    class PackagesFinder
+      include ::Packages::FinderHelper
+
+      MAX_PACKAGES_COUNT = 300
+
+      def initialize(project, channel)
+        @project = project
+        @channel = channel
+      end
+
+      def execute
+        if @channel.blank? || @project.blank?
+          return ::Packages::Package.none
+        end
+
+        pkg_files = ::Packages::PackageFile.for_helm_with_channel(@project, @channel)
+
+        # we use a subquery to get unique packages and at the same time
+        # order + limit them.
+        ::Packages::Package
+          .limit_recent(MAX_PACKAGES_COUNT)
+          .id_in(pkg_files.select(:package_id))
+      end
+    end
+  end
+end

app/models/packages/package_file.rb

@@ -93,6 +93,24 @@ class Packages::PackageFile < ApplicationRecord
   skip_callback :commit, :after, :remove_previously_stored_file, if: :execute_move_in_object_storage?
   after_commit :move_in_object_storage, if: :execute_move_in_object_storage?
 
+  # Returns the most recent package files for *each* of the given packages.
+  # The order is not guaranteed.
+  def self.most_recent_for(packages, extra_join: nil, extra_where: nil)
+    cte_name = :packages_cte
+    cte = Gitlab::SQL::CTE.new(cte_name, packages.select(:id))
+
+    package_files = ::Packages::PackageFile.limit_recent(1)
+                      .where(arel_table[:package_id].eq(Arel.sql("#{cte_name}.id")))
+
+    package_files = package_files.joins(extra_join) if extra_join
+    package_files = package_files.where(extra_where) if extra_where
+
+    query = select('finder.*')
+              .from([Arel.sql(cte_name.to_s), package_files.arel.lateral.as('finder')])
+
+    query.with(cte.to_arel)
+  end
+
   def download_path
     Gitlab::Routing.url_helpers.download_project_package_file_path(project, self)
   end

app/presenters/packages/helm/index_presenter.rb

@@ -8,11 +8,12 @@ module Packages
       API_VERSION = 'v1'
       CHANNEL = 'channel'
       INDEX_YAML_SUFFIX = "/#{CHANNEL}/index.yaml"
+      EMPTY_HASH = {}.freeze
 
-      def initialize(project, project_id_param, package_files)
-        @project = project
+      def initialize(project_id_param, channel, packages)
         @project_id_param = project_id_param
-        @package_files = package_files
+        @channel = channel
+        @packages = packages
       end
 
       def api_version
@@ -20,10 +21,12 @@ module Packages
       end
 
       def entries
-        files = @package_files.preload_helm_file_metadata
+        return EMPTY_HASH unless @channel.present?
+
         result = Hash.new { |h, k| h[k] = [] }
 
-        files.find_each do |package_file|
+        # this .each is safe as we have max 300 objects
+        most_recent_package_files.each do |package_file|
           name = package_file.helm_metadata['name']
           result[name] << package_file.helm_metadata.merge({
             'created' => package_file.created_at.utc.strftime('%Y-%m-%dT%H:%M:%S.%NZ'),
@@ -48,6 +51,16 @@ module Packages
           'contextPath' => path.delete_suffix(INDEX_YAML_SUFFIX)
         }
       end
+
+      private
+
+      def most_recent_package_files
+        ::Packages::PackageFile.most_recent_for(
+          @packages,
+          extra_join: :helm_file_metadatum,
+          extra_where: { packages_helm_file_metadata: { channel: @channel } }
+        ).preload_helm_file_metadata
+      end
     end
   end
 end

db/migrate/20210831134840_add_package_file_id_channel_idx_to_packages_helm_file_metadata.rb

+# frozen_string_literal: true
+
+class AddPackageFileIdChannelIdxToPackagesHelmFileMetadata < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_packages_helm_file_metadata_on_pf_id_and_channel'
+
+  def up
+    add_concurrent_index :packages_helm_file_metadata, [:package_file_id, :channel], name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index :packages_helm_file_metadata, [:package_file_id, :channel], name: INDEX_NAME
+  end
+end

db/migrate/20210831135249_add_installable_helm_pkgs_idx_to_packages.rb

+# frozen_string_literal: true
+
+class AddInstallableHelmPkgsIdxToPackages < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'idx_installable_helm_pkgs_on_project_id_id'
+
+  def up
+    add_concurrent_index :packages_packages, [:project_id, :id], name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index :packages_packages, [:project_id, :id], name: INDEX_NAME
+  end
+end

db/migrate/20210909184349_add_index_package_id_id_on_package_files.rb

+# frozen_string_literal: true
+
+class AddIndexPackageIdIdOnPackageFiles < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_packages_package_files_on_package_id_id'
+
+  def up
+    disable_statement_timeout do
+      execute "CREATE INDEX CONCURRENTLY #{INDEX_NAME} ON packages_package_files (package_id, id)" unless index_exists_by_name?(:package_package_files, INDEX_NAME)
+    end
+  end
+
+  def down
+    remove_concurrent_index_by_name :packages_package_files, INDEX_NAME
+  end
+end

db/schema_migrations/20210831134840

+6cad93bd4c086a60164c3cb5c42737194c4b7b20c1ea9f4c6d998b7c3a8918b5
\ No newline at end of file

db/schema_migrations/20210831135249

+78b14e92c91e7ccb11b7b37e89e8e55749cf109b0fa5ce06e4e396a2ac743f1c
\ No newline at end of file

db/schema_migrations/20210909184349

+6be3a6f8f748d8f02e2d42cd1a5103ec8bd5c17097d9e440b152685fc8d6ff83
\ No newline at end of file

db/structure.sql

@@ -24007,6 +24007,8 @@ CREATE UNIQUE INDEX idx_environment_merge_requests_unique_index ON deployment_me
 
 CREATE INDEX idx_geo_con_rep_updated_events_on_container_repository_id ON geo_container_repository_updated_events USING btree (container_repository_id);
 
+CREATE INDEX idx_installable_helm_pkgs_on_project_id_id ON packages_packages USING btree (project_id, id);
+
 CREATE INDEX idx_installable_npm_pkgs_on_project_id_name_version_id ON packages_packages USING btree (project_id, name, version, id) WHERE ((package_type = 2) AND (status = 0));
 
 CREATE INDEX idx_issues_on_health_status_not_null ON issues USING btree (health_status) WHERE (health_status IS NOT NULL);
@@ -25817,6 +25819,8 @@ CREATE INDEX index_packages_events_on_package_id ON packages_events USING btree
 
 CREATE INDEX index_packages_helm_file_metadata_on_channel ON packages_helm_file_metadata USING btree (channel);
 
+CREATE INDEX index_packages_helm_file_metadata_on_pf_id_and_channel ON packages_helm_file_metadata USING btree (package_file_id, channel);
+
 CREATE INDEX index_packages_maven_metadata_on_package_id_and_path ON packages_maven_metadata USING btree (package_id, path);
 
 CREATE INDEX index_packages_maven_metadata_on_path ON packages_maven_metadata USING btree (path);
@@ -25837,6 +25841,8 @@ CREATE INDEX index_packages_package_files_on_file_store ON packages_package_file
 
 CREATE INDEX index_packages_package_files_on_package_id_and_file_name ON packages_package_files USING btree (package_id, file_name);
 
+CREATE INDEX index_packages_package_files_on_package_id_id ON packages_package_files USING btree (package_id, id);
+
 CREATE INDEX index_packages_package_files_on_verification_state ON packages_package_files USING btree (verification_state);
 
 CREATE INDEX index_packages_packages_on_creator_id ON packages_packages USING btree (creator_id);

doc/user/packages/helm_repository/index.md

@@ -89,6 +89,10 @@ upload:
 
 ## Install a package
 
+NOTE:
+When requesting a package, GitLab considers only the 300 most recent packages created.
+For each package, only the most recent package file is returned.
+
 To install the latest version of a chart, use the following command:
 
 ```shell

lib/api/helm_packages.rb

@@ -44,15 +44,10 @@ module API
         get ":channel/index.yaml" do
           authorize_read_package!(authorized_user_project)
 
-          package_files = Packages::Helm::PackageFilesFinder.new(
-            authorized_user_project,
-            params[:channel],
-            order_by: 'created_at',
-            sort: 'desc'
-          ).execute
+          packages = Packages::Helm::PackagesFinder.new(authorized_user_project, params[:channel]).execute
 
           env['api.format'] = :yaml
-          present ::Packages::Helm::IndexPresenter.new(authorized_user_project, params[:id], package_files),
+          present ::Packages::Helm::IndexPresenter.new(params[:id], params[:channel], packages),
                       with: ::API::Entities::Helm::Index
         end
 

spec/factories/packages/helm/file_metadatum.rb

@@ -2,8 +2,16 @@
 
 FactoryBot.define do
   factory :helm_file_metadatum, class: 'Packages::Helm::FileMetadatum' do
+    transient do
+      description { nil }
+    end
+
     package_file { association(:helm_package_file, without_loaded_metadatum: true) }
     sequence(:channel) { |n| "#{FFaker::Lorem.word}-#{n}" }
-    metadata { { 'name': package_file.package.name, 'version': package_file.package.version, 'apiVersion': 'v2' } }
+    metadata do
+      { 'name': package_file.package.name, 'version': package_file.package.version, 'apiVersion': 'v2' }.tap do |defaults|
+        defaults['description'] = description if description
+      end
+    end
   end
 end

spec/factories/packages/package_file.rb

@@ -212,11 +212,12 @@ FactoryBot.define do
         package_name { package&.name || 'foo' }
         sequence(:package_version) { |n| package&.version || "v#{n}" }
         channel { 'stable' }
+        description { nil }
       end
 
       after :create do |package_file, evaluator|
         unless evaluator.without_loaded_metadatum
-          create :helm_file_metadatum, package_file: package_file, channel: evaluator.channel
+          create :helm_file_metadatum, package_file: package_file, channel: evaluator.channel, description: evaluator.description
         end
       end
     end

spec/finders/packages/helm/packages_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Packages::Helm::PackagesFinder do
+  let_it_be(:project1) { create(:project) }
+  let_it_be(:project2) { create(:project) }
+  let_it_be(:helm_package) { create(:helm_package, project: project1) }
+  let_it_be(:npm_package) { create(:npm_package, project: project1) }
+  let_it_be(:npm_package) { create(:npm_package, project: project2) }
+
+  let(:project) { project1 }
+  let(:channel) { 'stable' }
+  let(:finder) { described_class.new(project, channel) }
+
+  describe '#execute' do
+    subject { finder.execute }
+
+    context 'with project' do
+      context 'with channel' do
+        it { is_expected.to eq([helm_package]) }
+
+        context 'ignores duplicate package files' do
+          let_it_be(:package_file1) { create(:helm_package_file, package: helm_package) }
+          let_it_be(:package_file2) { create(:helm_package_file, package: helm_package) }
+
+          it { is_expected.to eq([helm_package]) }
+
+          context 'let clients use select id' do
+            subject { finder.execute.pluck_primary_key }
+
+            it { is_expected.to eq([helm_package.id]) }
+          end
+        end
+      end
+
+      context 'with not existing channel' do
+        let(:channel) { 'alpha' }
+
+        it { is_expected.to be_empty }
+      end
+
+      context 'with no channel' do
+        let(:channel) { nil }
+
+        it { is_expected.to be_empty }
+      end
+
+      context 'with no helm packages' do
+        let(:project) { project2 }
+
+        it { is_expected.to be_empty }
+      end
+    end
+
+    context 'with no project' do
+      let(:project) { nil }
+
+      it { is_expected.to be_empty }
+    end
+
+    context 'when the limit is hit' do
+      let_it_be(:helm_package2) { create(:helm_package, project: project1) }
+      let_it_be(:helm_package3) { create(:helm_package, project: project1) }
+      let_it_be(:helm_package4) { create(:helm_package, project: project1) }
+
+      before do
+        stub_const("#{described_class}::MAX_PACKAGES_COUNT", 2)
+      end
+
+      it { is_expected.to eq([helm_package4, helm_package3]) }
+    end
+  end
+end

spec/models/packages/package_file_spec.rb

@@ -2,6 +2,8 @@
 require 'spec_helper'
 
 RSpec.describe Packages::PackageFile, type: :model do
+  using RSpec::Parameterized::TableSyntax
+
   let_it_be(:project) { create(:project) }
   let_it_be(:package_file1) { create(:package_file, :xml, file_name: 'FooBar') }
   let_it_be(:package_file2) { create(:package_file, :xml, file_name: 'ThisIsATest') }
@@ -143,6 +145,67 @@ RSpec.describe Packages::PackageFile, type: :model do
     it { expect(described_class.most_recent!).to eq(debian_package.package_files.last) }
   end
 
+  describe '.most_recent_for' do
+    let_it_be(:package1) { create(:npm_package) }
+    let_it_be(:package2) { create(:npm_package) }
+    let_it_be(:package3) { create(:npm_package) }
+    let_it_be(:package4) { create(:npm_package) }
+
+    let_it_be(:package_file2_2) { create(:package_file, :npm, package: package2) }
+
+    let_it_be(:package_file3_2) { create(:package_file, :npm, package: package3) }
+    let_it_be(:package_file3_3) { create(:package_file, :npm, package: package3) }
+
+    let_it_be(:package_file4_2) { create(:package_file, :npm, package: package2) }
+    let_it_be(:package_file4_3) { create(:package_file, :npm, package: package2) }
+    let_it_be(:package_file4_4) { create(:package_file, :npm, package: package2) }
+
+    let(:most_recent_package_file1) { package1.package_files.recent.first }
+    let(:most_recent_package_file2) { package2.package_files.recent.first }
+    let(:most_recent_package_file3) { package3.package_files.recent.first }
+    let(:most_recent_package_file4) { package4.package_files.recent.first }
+
+    subject { described_class.most_recent_for(packages) }
+
+    where(
+      package_input1: [1, nil],
+      package_input2: [2, nil],
+      package_input3: [3, nil],
+      package_input4: [4, nil]
+    )
+
+    with_them do
+      let(:compact_inputs) { [package_input1, package_input2, package_input3, package_input4].compact }
+      let(:packages) do
+        ::Packages::Package.id_in(
+          compact_inputs.map { |pkg_number| public_send("package#{pkg_number}") }
+            .map(&:id)
+        )
+      end
+
+      let(:expected_package_files) { compact_inputs.map { |pkg_number| public_send("most_recent_package_file#{pkg_number}") } }
+
+      it { is_expected.to contain_exactly(*expected_package_files) }
+    end
+
+    context 'extra join and extra where' do
+      let_it_be(:helm_package) { create(:helm_package, without_package_files: true) }
+      let_it_be(:helm_package_file1) { create(:helm_package_file, channel: 'alpha') }
+      let_it_be(:helm_package_file2) { create(:helm_package_file, channel: 'alpha', package: helm_package) }
+      let_it_be(:helm_package_file3) { create(:helm_package_file, channel: 'beta', package: helm_package) }
+      let_it_be(:helm_package_file4) { create(:helm_package_file, channel: 'beta', package: helm_package) }
+
+      let(:extra_join) { :helm_file_metadatum }
+      let(:extra_where) { { packages_helm_file_metadata: { channel: 'alpha' } } }
+
+      subject { described_class.most_recent_for(Packages::Package.id_in(helm_package.id), extra_join: extra_join, extra_where: extra_where) }
+
+      it 'returns the most recent package for the selected channel' do
+        expect(subject).to contain_exactly(helm_package_file2)
+      end
+    end
+  end
+
   describe '#update_file_store callback' do
     let_it_be(:package_file) { build(:package_file, :nuget, size: nil) }
 

spec/presenters/packages/helm/index_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Packages::Helm::IndexPresenter do
+  include_context 'with expected presenters dependency groups'
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:packages) { create_list(:helm_package, 5, project: project) }
+  let_it_be(:package_files3_1) { create(:helm_package_file, package: packages[2], file_sha256: '3_1', file_name: 'file3_1') }
+  let_it_be(:package_files3_2) { create(:helm_package_file, package: packages[2], file_sha256: '3_2', file_name: 'file3_2') }
+  let_it_be(:package_files4_1) { create(:helm_package_file, package: packages[3], file_sha256: '4_1', file_name: 'file4_1') }
+  let_it_be(:package_files4_2) { create(:helm_package_file, package: packages[3], file_sha256: '4_2', file_name: 'file4_2') }
+  let_it_be(:package_files4_3) { create(:helm_package_file, package: packages[3], file_sha256: '4_3', file_name: 'file4_3') }
+
+  let(:project_id_param) { project.id }
+  let(:channel) { 'stable' }
+  let(:presenter) { described_class.new(project_id_param, channel, ::Packages::Package.id_in(packages.map(&:id))) }
+
+  describe('#entries') do
+    subject { presenter.entries }
+
+    it 'returns the correct hash' do
+      expect(subject.size).to eq(5)
+      expect(subject.keys).to eq(packages.map(&:name))
+      subject.values.zip(packages) do |raws, pkg|
+        expect(raws.size).to eq(1)
+
+        file = pkg.package_files.recent.first
+        raw = raws.first
+        expect(raw['name']).to eq(pkg.name)
+        expect(raw['version']).to eq(pkg.version)
+        expect(raw['apiVersion']).to eq("v2")
+        expect(raw['created']).to eq(file.created_at.utc.strftime('%Y-%m-%dT%H:%M:%S.%NZ'))
+        expect(raw['digest']).to eq(file.file_sha256)
+        expect(raw['urls']).to eq(["charts/#{file.file_name}"])
+      end
+    end
+
+    context 'with an unknown channel' do
+      let(:channel) { 'unknown' }
+
+      it { is_expected.to be_empty }
+    end
+
+    context 'with a nil channel' do
+      let(:channel) { nil }
+
+      it { is_expected.to be_empty }
+    end
+  end
+
+  describe('#api_version') do
+    subject { presenter.api_version }
+
+    it { is_expected.to eq(described_class::API_VERSION) }
+  end
+
+  describe('#generated') do
+    subject { presenter.generated }
+
+    it 'returns the expected format' do
+      freeze_time do
+        expect(subject).to eq(Time.zone.now.utc.strftime('%Y-%m-%dT%H:%M:%S.%NZ'))
+      end
+    end
+  end
+
+  describe('#server_info') do
+    subject { presenter.server_info }
+
+    it { is_expected.to eq({ 'contextPath' => "/api/v4/projects/#{project.id}/packages/helm" }) }
+
+    context 'with url encoded project id param' do
+      let_it_be(:project_id_param) { 'foo/bar' }
+
+      it { is_expected.to eq({ 'contextPath' => '/api/v4/projects/foo%2Fbar/packages/helm' }) }
+    end
+  end
+end

spec/requests/api/helm_packages_spec.rb

@@ -12,10 +12,24 @@ RSpec.describe API::HelmPackages do
   let_it_be(:package) { create(:helm_package, project: project, without_package_files: true) }
   let_it_be(:package_file1) { create(:helm_package_file, package: package) }
   let_it_be(:package_file2) { create(:helm_package_file, package: package) }
+  let_it_be(:package2) { create(:helm_package, project: project, without_package_files: true) }
+  let_it_be(:package_file2_1) { create(:helm_package_file, package: package2, file_sha256: 'file2', file_name: 'filename2.tgz', description: 'hello from stable channel') }
+  let_it_be(:package_file2_2) { create(:helm_package_file, package: package2, file_sha256: 'file2', file_name: 'filename2.tgz', channel: 'test', description: 'hello from test channel') }
+  let_it_be(:other_package) { create(:npm_package, project: project) }
 
   describe 'GET /api/v4/projects/:id/packages/helm/:channel/index.yaml' do
-    it_behaves_like 'handling helm chart index requests' do
-      let(:url) { "/projects/#{project.id}/packages/helm/stable/index.yaml" }
+    let(:url) { "/projects/#{project_id}/packages/helm/stable/index.yaml" }
+
+    context 'with a project id' do
+      let(:project_id) { project.id }
+
+      it_behaves_like 'handling helm chart index requests'
+    end
+
+    context 'with an url encoded project id' do
+      let(:project_id) { ERB::Util.url_encode(project.full_path) }
+
+      it_behaves_like 'handling helm chart index requests'
     end
   end
 

spec/support/shared_examples/requests/api/helm_packages_shared_examples.rb

@@ -36,15 +36,23 @@ RSpec.shared_examples 'process helm service index request' do |user_type, status
 
       expect(yaml_response.keys).to contain_exactly('apiVersion', 'entries', 'generated', 'serverInfo')
       expect(yaml_response['entries']).to be_a(Hash)
-      expect(yaml_response['entries'].keys).to contain_exactly(package.name)
-      expect(yaml_response['serverInfo']).to eq({ 'contextPath' => "/api/v4/projects/#{project.id}/packages/helm" })
+      expect(yaml_response['entries'].keys).to contain_exactly(package.name, package2.name)
+      expect(yaml_response['serverInfo']).to eq({ 'contextPath' => "/api/v4/projects/#{project_id}/packages/helm" })
 
       package_entry = yaml_response['entries'][package.name]
 
-      expect(package_entry.length).to eq(2)
+      expect(package_entry.length).to eq(1)
       expect(package_entry.first.keys).to contain_exactly('name', 'version', 'apiVersion', 'created', 'digest', 'urls')
       expect(package_entry.first['digest']).to eq('fd2b2fa0329e80a2a602c2bb3b40608bcd6ee5cf96cf46fd0d2800a4c129c9db')
       expect(package_entry.first['urls']).to eq(["charts/#{package.name}-#{package.version}.tgz"])
+
+      package_entry = yaml_response['entries'][package2.name]
+
+      expect(package_entry.length).to eq(1)
+      expect(package_entry.first.keys).to contain_exactly('name', 'version', 'apiVersion', 'created', 'digest', 'urls', 'description')
+      expect(package_entry.first['digest']).to eq('file2')
+      expect(package_entry.first['description']).to eq('hello from stable channel')
+      expect(package_entry.first['urls']).to eq(['charts/filename2.tgz'])
     end
   end
 end
@@ -196,7 +204,7 @@ end
 
 RSpec.shared_examples 'rejects helm access with unknown project id' do
   context 'with an unknown project' do
-    let(:project) { OpenStruct.new(id: 1234567890) }
+    let(:project_id) { 1234567890 }
 
     context 'as anonymous' do
       it_behaves_like 'rejects helm packages access', :anonymous, :unauthorized