Merge branch '339835-tweak-npm-package-finder' into 'master'

NPM package registry: fine tune a query for the instance level

See merge request gitlab-org/gitlab!69572

app/finders/packages/npm/package_finder.rb

@@ -5,17 +5,23 @@ module Packages
       delegate :find_by_version, to: :execute
       delegate :last, to: :execute
 
-      def initialize(package_name, project: nil, namespace: nil)
+      # /!\ CAUTION: don't use last_of_each_version: false with find_by_version. Ordering is not
+      # guaranteed!
+      def initialize(package_name, project: nil, namespace: nil, last_of_each_version: true)
         @package_name = package_name
         @project = project
         @namespace = namespace
+        @last_of_each_version = last_of_each_version
       end
 
       def execute
-        base.npm
-            .with_name(@package_name)
-            .installable
-            .last_of_each_version
+        result = base.npm
+                     .with_name(@package_name)
+                     .installable
+
+        return result unless @last_of_each_version
+
+        result.last_of_each_version
       end
 
       private

config/feature_flags/development/npm_finder_query_avoid_duplicated_conditions.yml

+---
+name: npm_finder_query_avoid_duplicated_conditions
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69572
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/340099
+milestone: '14.3'
+type: development
+group: group::package
+default_enabled: false

lib/api/helpers/packages/npm.rb

@@ -57,7 +57,11 @@ module API
                                    .by_path(namespace_path)
               next unless namespace
 
-              finder = ::Packages::Npm::PackageFinder.new(package_name, namespace: namespace)
+              finder = ::Packages::Npm::PackageFinder.new(
+                package_name,
+                namespace: namespace,
+                last_of_each_version: Feature.disabled?(:npm_finder_query_avoid_duplicated_conditions)
+              )
 
               finder.last&.project_id
             end

spec/finders/packages/npm/package_finder_spec.rb

@@ -7,6 +7,7 @@ RSpec.describe ::Packages::Npm::PackageFinder do
 
   let(:project) { package.project }
   let(:package_name) { package.name }
+  let(:last_of_each_version) { true }
 
   shared_examples 'accepting a namespace for' do |example_name|
     before do
@@ -38,6 +39,8 @@ RSpec.describe ::Packages::Npm::PackageFinder do
   end
 
   describe '#execute' do
+    subject { finder.execute }
+
     shared_examples 'finding packages by name' do
       it { is_expected.to eq([package]) }
 
@@ -56,13 +59,27 @@ RSpec.describe ::Packages::Npm::PackageFinder do
       end
     end
 
-    subject { finder.execute }
+    shared_examples 'handling last_of_each_version' do
+      include_context 'last_of_each_version setup context'
+
+      context 'disabled' do
+        let(:last_of_each_version) { false }
+
+        it { is_expected.to contain_exactly(package1, package2) }
+      end
+
+      context 'enabled' do
+        it { is_expected.to contain_exactly(package2) }
+      end
+    end
 
     context 'with a project' do
-      let(:finder) { described_class.new(package_name, project: project) }
+      let(:finder) { described_class.new(package_name, project: project, last_of_each_version: last_of_each_version) }
 
       it_behaves_like 'finding packages by name'
 
+      it_behaves_like 'handling last_of_each_version'
+
       context 'set to nil' do
         let(:project) { nil }
 
@@ -71,10 +88,12 @@ RSpec.describe ::Packages::Npm::PackageFinder do
     end
 
     context 'with a namespace' do
-      let(:finder) { described_class.new(package_name, namespace: namespace) }
+      let(:finder) { described_class.new(package_name, namespace: namespace, last_of_each_version: last_of_each_version) }
 
       it_behaves_like 'accepting a namespace for', 'finding packages by name'
 
+      it_behaves_like 'accepting a namespace for', 'handling last_of_each_version'
+
       context 'set to nil' do
         let_it_be(:namespace) { nil }
 
@@ -98,16 +117,28 @@ RSpec.describe ::Packages::Npm::PackageFinder do
       end
     end
 
+    shared_examples 'handling last_of_each_version' do
+      include_context 'last_of_each_version setup context'
+
+      context 'enabled' do
+        it { is_expected.to eq(package2) }
+      end
+    end
+
     context 'with a project' do
-      let(:finder) { described_class.new(package_name, project: project) }
+      let(:finder) { described_class.new(package_name, project: project, last_of_each_version: last_of_each_version) }
 
       it_behaves_like 'finding packages by version'
+
+      it_behaves_like 'handling last_of_each_version'
     end
 
     context 'with a namespace' do
-      let(:finder) { described_class.new(package_name, namespace: namespace) }
+      let(:finder) { described_class.new(package_name, namespace: namespace, last_of_each_version: last_of_each_version) }
 
       it_behaves_like 'accepting a namespace for', 'finding packages by version'
+
+      it_behaves_like 'accepting a namespace for', 'handling last_of_each_version'
     end
   end
 
@@ -118,35 +149,49 @@ RSpec.describe ::Packages::Npm::PackageFinder do
       it { is_expected.to eq(package) }
     end
 
-    shared_examples 'handling project or namespace parameter' do
-      context 'with a project' do
-        let(:finder) { described_class.new(package_name, project: project) }
+    shared_examples 'handling last_of_each_version' do
+      include_context 'last_of_each_version setup context'
+
+      context 'disabled' do
+        let(:last_of_each_version) { false }
+
+        it { is_expected.to eq(package2) }
+      end
 
-        it_behaves_like 'finding package by last'
+      context 'enabled' do
+        it { is_expected.to eq(package2) }
       end
+    end
+
+    context 'with a project' do
+      let(:finder) { described_class.new(package_name, project: project, last_of_each_version: last_of_each_version) }
 
-      context 'with a namespace' do
-        let(:finder) { described_class.new(package_name, namespace: namespace) }
+      it_behaves_like 'finding package by last'
 
-        it_behaves_like 'accepting a namespace for', 'finding package by last'
+      it_behaves_like 'handling last_of_each_version'
+    end
+
+    context 'with a namespace' do
+      let(:finder) { described_class.new(package_name, namespace: namespace) }
 
-        context 'with duplicate packages' do
-          let_it_be(:namespace) { create(:group) }
-          let_it_be(:subgroup1) { create(:group, parent: namespace) }
-          let_it_be(:subgroup2) { create(:group, parent: namespace) }
-          let_it_be(:project2) { create(:project, namespace: subgroup2) }
-          let_it_be(:package2) { create(:npm_package, name: package.name, project: project2) }
+      it_behaves_like 'accepting a namespace for', 'finding package by last'
 
-          before do
-            project.update!(namespace: subgroup1)
-          end
+      it_behaves_like 'accepting a namespace for', 'handling last_of_each_version'
 
-          # the most recent one is returned
-          it { is_expected.to eq(package2) }
+      context 'with duplicate packages' do
+        let_it_be(:namespace) { create(:group) }
+        let_it_be(:subgroup1) { create(:group, parent: namespace) }
+        let_it_be(:subgroup2) { create(:group, parent: namespace) }
+        let_it_be(:project2) { create(:project, namespace: subgroup2) }
+        let_it_be(:package2) { create(:npm_package, name: package.name, project: project2) }
+
+        before do
+          project.update!(namespace: subgroup1)
         end
+
+        # the most recent one is returned
+        it { is_expected.to eq(package2) }
       end
     end
-
-    it_behaves_like 'handling project or namespace parameter'
   end
 end

spec/requests/api/npm_instance_packages_spec.rb

@@ -10,27 +10,39 @@ RSpec.describe API::NpmInstancePackages do
 
   include_context 'npm api setup'
 
-  describe 'GET /api/v4/packages/npm/*package_name' do
-    it_behaves_like 'handling get metadata requests', scope: :instance do
-      let(:url) { api("/packages/npm/#{package_name}") }
+  shared_examples 'handling all endpoints' do
+    describe 'GET /api/v4/packages/npm/*package_name' do
+      it_behaves_like 'handling get metadata requests', scope: :instance do
+        let(:url) { api("/packages/npm/#{package_name}") }
+      end
     end
-  end
 
-  describe 'GET /api/v4/packages/npm/-/package/*package_name/dist-tags' do
-    it_behaves_like 'handling get dist tags requests', scope: :instance do
-      let(:url) { api("/packages/npm/-/package/#{package_name}/dist-tags") }
+    describe 'GET /api/v4/packages/npm/-/package/*package_name/dist-tags' do
+      it_behaves_like 'handling get dist tags requests', scope: :instance do
+        let(:url) { api("/packages/npm/-/package/#{package_name}/dist-tags") }
+      end
+    end
+
+    describe 'PUT /api/v4/packages/npm/-/package/*package_name/dist-tags/:tag' do
+      it_behaves_like 'handling create dist tag requests', scope: :instance do
+        let(:url) { api("/packages/npm/-/package/#{package_name}/dist-tags/#{tag_name}") }
+      end
     end
-  end
 
-  describe 'PUT /api/v4/packages/npm/-/package/*package_name/dist-tags/:tag' do
-    it_behaves_like 'handling create dist tag requests', scope: :instance do
-      let(:url) { api("/packages/npm/-/package/#{package_name}/dist-tags/#{tag_name}") }
+    describe 'DELETE /api/v4/packages/npm/-/package/*package_name/dist-tags/:tag' do
+      it_behaves_like 'handling delete dist tag requests', scope: :instance do
+        let(:url) { api("/packages/npm/-/package/#{package_name}/dist-tags/#{tag_name}") }
+      end
     end
   end
 
-  describe 'DELETE /api/v4/packages/npm/-/package/*package_name/dist-tags/:tag' do
-    it_behaves_like 'handling delete dist tag requests', scope: :instance do
-      let(:url) { api("/packages/npm/-/package/#{package_name}/dist-tags/#{tag_name}") }
+  it_behaves_like 'handling all endpoints'
+
+  context 'with npm_finder_query_avoid_duplicated_conditions disabled' do
+    before do
+      stub_feature_flags(npm_finder_query_avoid_duplicated_conditions: false)
     end
+
+    it_behaves_like 'handling all endpoints'
   end
 end

spec/support/shared_contexts/finders/packages/npm/package_finder_shared_context.rb

+# frozen_string_literal: true
+
+RSpec.shared_context 'last_of_each_version setup context' do
+  let_it_be(:package1) { create(:npm_package, name: 'test', version: '1.2.3', project: project) }
+  let_it_be(:package2) { create(:npm_package, name: 'test2', version: '1.2.3', project: project) }
+
+  let(:package_name) { 'test' }
+  let(:version) { '1.2.3' }
+
+  before do
+    # create a duplicated package without triggering model validation errors
+    package2.update_column(:name, 'test')
+  end
+end

spec/support/shared_contexts/requests/api/npm_packages_shared_context.rb

@@ -8,7 +8,8 @@ RSpec.shared_context 'npm api setup' do
   let_it_be(:group) { create(:group, name: 'test-group') }
   let_it_be(:namespace) { group }
   let_it_be(:project, reload: true) { create(:project, :public, namespace: namespace) }
-  let_it_be(:package, reload: true) { create(:npm_package, project: project, name: "@#{group.path}/scoped_package") }
+  let_it_be(:package1, reload: true) { create(:npm_package, project: project, name: "@#{group.path}/scoped_package", version: '1.2.4') }
+  let_it_be(:package, reload: true) { create(:npm_package, project: project, name: "@#{group.path}/scoped_package", version: '1.2.3') }
   let_it_be(:token) { create(:oauth_access_token, scopes: 'api', resource_owner: user) }
   let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
   let_it_be(:job, reload: true) { create(:ci_build, user: user, status: :running, project: project) }
@@ -16,6 +17,11 @@ RSpec.shared_context 'npm api setup' do
   let_it_be(:project_deploy_token) { create(:project_deploy_token, deploy_token: deploy_token, project: project) }
 
   let(:package_name) { package.name }
+
+  before do
+    # create a duplicated package without triggering model validation errors
+    package1.update_column(:version, '1.2.3')
+  end
 end
 
 RSpec.shared_context 'set package name from package name type' do