Refactor the helm presenter

It will now loop on packages instead of package files.
The number of packages are now limited to 300.
The presenter gets all the most recent package files in a single SQL
query.

Changelog: fixed

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