Update the npm package finder

To let clients choose if they want to load the last package of each
version or not. Depending on what they do with the result of the finder,
the last_of_each_version scope can trigger queries with duplicated SQL
conditions which run poorly.

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
+        result = base.npm
                      .with_name(@package_name)
                      .installable
-            .last_of_each_version
+
+        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,11 +149,26 @@ RSpec.describe ::Packages::Npm::PackageFinder do
       it { is_expected.to eq(package) }
     end
 
-    shared_examples 'handling project or namespace parameter' do
+    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
+
+      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 package by last'
+
+      it_behaves_like 'handling last_of_each_version'
     end
 
     context 'with a namespace' do
@@ -130,6 +176,8 @@ RSpec.describe ::Packages::Npm::PackageFinder do
 
       it_behaves_like 'accepting a namespace for', 'finding package by last'
 
+      it_behaves_like 'accepting a namespace for', 'handling last_of_each_version'
+
       context 'with duplicate packages' do
         let_it_be(:namespace) { create(:group) }
         let_it_be(:subgroup1) { create(:group, parent: namespace) }
@@ -146,7 +194,4 @@ RSpec.describe ::Packages::Npm::PackageFinder do
       end
     end
   end
-
-    it_behaves_like 'handling project or namespace parameter'
-  end
 end

spec/requests/api/npm_instance_packages_spec.rb

@@ -10,6 +10,7 @@ RSpec.describe API::NpmInstancePackages do
 
   include_context 'npm api setup'
 
+  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}") }
@@ -33,4 +34,15 @@ RSpec.describe API::NpmInstancePackages do
         let(:url) { api("/packages/npm/-/package/#{package_name}/dist-tags/#{tag_name}") }
       end
     end
+  end
+
+  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