Merge branch '436099-remove-NPM-distribution-tags-limit' into 'master'

Rewrite npm dist_tags retrieval query to load all tags

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



Merged-by: David Fernandez <dfernandez@gitlab.com>
Approved-by: Adie (she/her) <avpfestin@gitlab.com>
Approved-by: David Fernandez <dfernandez@gitlab.com>
Reviewed-by: David Fernandez <dfernandez@gitlab.com>
Reviewed-by: Moaz Khalifa <mkhalifa@gitlab.com>
Co-authored-by: moaz-khalifa <mkhalifa@gitlab.com>

app/models/packages/package.rb

@@ -190,6 +190,7 @@ class Packages::Package < ApplicationRecord
   scope :preload_files, -> { preload(:installable_package_files) }
   scope :preload_nuget_files, -> { preload(:installable_nuget_package_files) }
   scope :preload_pipelines, -> { preload(pipelines: :user) }
+  scope :preload_tags, -> { preload(:tags) }
   scope :limit_recent, ->(limit) { order_created_desc.limit(limit) }
   scope :select_distinct_name, -> { select(:name).distinct }
 

app/services/packages/npm/generate_metadata_service.rb

@@ -16,17 +16,29 @@ def initialize(name, packages)
         @packages = packages
         @dependencies = {}
         @dependency_ids = Hash.new { |h, key| h[key] = {} }
+        @tags = {}
+        @tags_updated_at = {}
+        @versions_hash = {}
+        @latest_version = nil
       end
 
       def execute(only_dist_tags: false)
-        ServiceResponse.success(payload: metadata(only_dist_tags))
+        payload = if Feature.enabled?(:package_registry_npm_fetch_all_tags, Feature.current_request,
+          type: :gitlab_com_derisk)
+                    metadata(only_dist_tags)
+                  else
+                    legacy_metadata(only_dist_tags)
+                  end
+
+        ServiceResponse.success(payload: payload)
       end
 
       private
 
-      attr_reader :name, :packages, :dependencies, :dependency_ids
+      attr_reader :name, :packages, :dependencies, :dependency_ids, :tags, :tags_updated_at, :versions_hash
+      attr_accessor :latest_version
 
-      def metadata(only_dist_tags)
+      def legacy_metadata(only_dist_tags)
         result = { dist_tags: dist_tags }
 
         unless only_dist_tags
@@ -37,6 +49,26 @@ def metadata(only_dist_tags)
         result
       end
 
+      def metadata(only_dist_tags)
+        packages.each_batch do |batch|
+          relation = preload_needed_relations(batch, only_dist_tags)
+
+          relation.each do |package|
+            build_tags(package)
+            store_latest_version(package.version)
+            next if only_dist_tags
+
+            build_versions(package)
+          end
+        end
+
+        {
+          name: only_dist_tags ? nil : name,
+          versions: versions_hash,
+          dist_tags: tags.tap { |t| t['latest'] ||= latest_version }
+        }.compact_blank
+      end
+
       def versions
         package_versions = {}
 
@@ -138,6 +170,40 @@ def load_dependency_ids(packages)
             end
           end
       end
+
+      def preload_needed_relations(batch, only_dist_tags)
+        relation = batch.preload_tags
+
+        unless only_dist_tags
+          load_dependencies(relation)
+          load_dependency_ids(relation)
+
+          relation = relation.preload_files.preload_npm_metadatum
+        end
+
+        relation
+      end
+
+      def build_tags(package)
+        package.tags.each do |tag|
+          next if tags.key?(tag.name) && tags_updated_at[tag.name] > tag.updated_at
+
+          tags[tag.name] = package.version
+          tags_updated_at[tag.name] = tag.updated_at
+        end
+      end
+
+      def store_latest_version(version)
+        self.latest_version = version if latest_version.blank? || VersionSorter.compare(version, latest_version) == 1
+      end
+
+      def build_versions(package)
+        package_file = package.installable_package_files.last
+
+        return unless package_file
+
+        versions_hash[package.version] = build_package_version(package, package_file)
+      end
     end
   end
 end

config/feature_flags/gitlab_com_derisk/package_registry_npm_fetch_all_tags.yml

+---
+name: package_registry_npm_fetch_all_tags
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/436099
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142573
+rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/17455
+milestone: '16.9'
+group: group::package registry
+type: gitlab_com_derisk
+default_enabled: false

spec/models/packages/package_spec.rb

@@ -1127,6 +1127,17 @@
     end
   end
 
+  describe '.preload_tags' do
+    let_it_be(:package) { create(:npm_package) }
+    let_it_be(:tags) { create_list(:packages_tag, 2, package: package) }
+
+    subject { described_class.preload_tags }
+
+    it 'preloads tags' do
+      expect(subject.first.association(:tags)).to be_loaded
+    end
+  end
+
   describe '#versions' do
     let_it_be(:project) { create(:project) }
     let_it_be(:package) { create(:maven_package, project: project) }

spec/services/packages/npm/generate_metadata_service_spec.rb

@@ -159,15 +159,54 @@
       end
 
       context 'with duplicate tags' do
-        let_it_be(:project2) { create(:project, namespace: group) }
-        let_it_be(:package2) { create(:npm_package, version: '3.0.0', project: project2, name: package_name) }
-        let_it_be(:package_tag1) { create(:packages_tag, package: package1, name: 'latest') }
-        let_it_be(:package_tag2) { create(:packages_tag, package: package2, name: 'latest') }
+        context 'when in different projects' do
+          let_it_be(:project2) { create(:project, namespace: group) }
+          let_it_be(:package2) { create(:npm_package, version: '3.0.0', project: project2, name: package_name) }
+          let_it_be(:package_tag1) { create(:packages_tag, package: package1, name: 'latest') }
+          let_it_be(:package_tag2) { create(:packages_tag, package: package2, name: 'latest') }
 
-        let(:packages) { ::Packages::Package.for_projects([project.id, project2.id]).with_name(package_name) }
+          let(:packages) { ::Packages::Package.for_projects([project.id, project2.id]).with_name(package_name) }
 
-        it "returns the tag of the latest package's version" do
-          expect(subject['latest']).to eq(package2.version)
+          it "returns the tag of the latest package's version" do
+            expect(subject['latest']).to eq(package2.version)
+          end
+        end
+
+        context 'when in the same project' do
+          let_it_be(:package_a) { create(:npm_package, version: '1.2.3', project: project, name: package_name) }
+          let_it_be(:package_b) { create(:npm_package, version: '1.1.1', project: project, name: package_name) }
+          let_it_be(:package_tag_a) { create(:packages_tag, package: package_a, name: 'tag', updated_at: 1.week.ago) }
+          let_it_be(:package_tag_b) { create(:packages_tag, package: package_b, name: 'tag') }
+
+          it "returns the most recent tagged package's version" do
+            expect(subject['tag']).to eq(package_b.version)
+          end
+        end
+      end
+
+      context 'when fetching all package tags' do
+        let_it_be(:tags_limit) { 3 }
+
+        before do
+          stub_const('Packages::Tag::FOR_PACKAGES_TAGS_LIMIT', tags_limit)
+        end
+
+        it 'returns all tags' do
+          expect(::Packages::Package).to receive(:preload_tags).and_call_original
+
+          expect(subject.size).to eq(Packages::Tag.count)
+        end
+
+        context 'with disabled package_registry_npm_fetch_all_tags feature flag' do
+          before do
+            stub_feature_flags(package_registry_npm_fetch_all_tags: false)
+          end
+
+          it 'adheres to the tags limit' do
+            expect(::Packages::Package).not_to receive(:preload_tags)
+
+            expect(subject.size).to eq(tags_limit)
+          end
         end
       end
     end