From 43b27c04e39e4682565010d9d2fda52ac9ac91fb Mon Sep 17 00:00:00 2001 From: Radamanthus Batnag Date: Mon, 13 Feb 2023 15:47:10 +0800 Subject: [PATCH] Update the NPM instance endpoint to find the correct package When the same package name exists in multiple projects, the metadata instance endpoint returns only packages from one project. This commit implements the correct behavior, which is to return packages that match the name, from all visible projects. Changlog: fixed --- ...pm_allow_packages_in_multiple_projects.yml | 8 ++ lib/api/concerns/packages/npm_endpoints.rb | 9 +- lib/api/helpers/packages/npm.rb | 32 ++++- .../api/npm_instance_packages_spec.rb | 34 ++++- .../helpers/packages/npm_spec.rb | 133 ++++++++++++++++++ 5 files changed, 208 insertions(+), 8 deletions(-) create mode 100644 config/feature_flags/development/npm_allow_packages_in_multiple_projects.yml create mode 100644 spec/support_specs/helpers/packages/npm_spec.rb diff --git a/config/feature_flags/development/npm_allow_packages_in_multiple_projects.yml b/config/feature_flags/development/npm_allow_packages_in_multiple_projects.yml new file mode 100644 index 00000000000000..7541a0dd24e8d6 --- /dev/null +++ b/config/feature_flags/development/npm_allow_packages_in_multiple_projects.yml @@ -0,0 +1,8 @@ +--- +name: npm_allow_packages_in_multiple_projects +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111775 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/391692 +milestone: '15.10' +type: development +group: group::package registry +default_enabled: false diff --git a/lib/api/concerns/packages/npm_endpoints.rb b/lib/api/concerns/packages/npm_endpoints.rb index f26b3a1d8c2fc8..ef4aec44cb5219 100644 --- a/lib/api/concerns/packages/npm_endpoints.rb +++ b/lib/api/concerns/packages/npm_endpoints.rb @@ -163,8 +163,13 @@ def redirect_or_present_audit_report route_setting :authentication, job_token_allowed: true, deploy_token_allowed: true get '*package_name', format: false, requirements: ::API::Helpers::Packages::Npm::NPM_ENDPOINT_REQUIREMENTS do package_name = params[:package_name] - packages = ::Packages::Npm::PackageFinder.new(package_name, project: project_or_nil) - .execute + packages = + if Feature.enabled?(:npm_allow_packages_in_multiple_projects) + finder_for_endpoint_scope(package_name).execute + else + ::Packages::Npm::PackageFinder.new(package_name, project: project_or_nil) + .execute + end redirect_request = project_or_nil.blank? || packages.empty? diff --git a/lib/api/helpers/packages/npm.rb b/lib/api/helpers/packages/npm.rb index 352d77f472c86f..4eb6c39b7dce97 100644 --- a/lib/api/helpers/packages/npm.rb +++ b/lib/api/helpers/packages/npm.rb @@ -33,6 +33,15 @@ def project end end + def finder_for_endpoint_scope(package_name) + case endpoint_scope + when :project + ::Packages::Npm::PackageFinder.new(package_name, project: project_or_nil) + when :instance + ::Packages::Npm::PackageFinder.new(package_name, namespace: top_namespace_from(package_name)) + end + end + def project_or_nil # mainly used by the metadata endpoint where we need to get a project # and return nil if not found (no errors should be raised) @@ -50,11 +59,17 @@ def project_id_or_nil params[:id] when :instance package_name = params[:package_name] - namespace_path = ::Packages::Npm.scope_of(package_name) - next unless namespace_path - namespace = Namespace.top_most - .by_path(namespace_path) + namespace = + if Feature.enabled?(:npm_allow_packages_in_multiple_projects) + top_namespace_from(package_name) + else + namespace_path = ::Packages::Npm.scope_of(package_name) + next unless namespace_path + + Namespace.top_most.by_path(namespace_path) + end + next unless namespace finder = ::Packages::Npm::PackageFinder.new( @@ -67,6 +82,15 @@ def project_id_or_nil end end end + + private + + def top_namespace_from(package_name) + namespace_path = ::Packages::Npm.scope_of(package_name) + return unless namespace_path + + Namespace.top_most.by_path(namespace_path) + end end end end diff --git a/spec/requests/api/npm_instance_packages_spec.rb b/spec/requests/api/npm_instance_packages_spec.rb index dcd2e4ae67735b..591a8ee68dccfa 100644 --- a/spec/requests/api/npm_instance_packages_spec.rb +++ b/spec/requests/api/npm_instance_packages_spec.rb @@ -11,8 +11,38 @@ 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}") } + let(:url) { api("/packages/npm/#{package_name}") } + + it_behaves_like 'handling get metadata requests', scope: :instance + + context 'with a duplicate package name in another project' do + subject { get(url) } + + let_it_be(:project2) { create(:project, :public, namespace: namespace) } + let_it_be(:package2) do + create(:npm_package, + project: project2, + name: "@#{group.path}/scoped_package", + version: '1.2.0') + end + + it 'includes all matching package versions in the response' do + subject + + expect(json_response['versions'].keys).to match_array([package.version, package2.version]) + end + + context 'with the feature flag disabled' do + before do + stub_feature_flags(npm_allow_packages_in_multiple_projects: false) + end + + it 'returns matching package versions from only one project' do + subject + + expect(json_response['versions'].keys).to match_array([package2.version]) + end + end end end diff --git a/spec/support_specs/helpers/packages/npm_spec.rb b/spec/support_specs/helpers/packages/npm_spec.rb new file mode 100644 index 00000000000000..e1316a10fb1f5d --- /dev/null +++ b/spec/support_specs/helpers/packages/npm_spec.rb @@ -0,0 +1,133 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::API::Helpers::Packages::Npm, feature_category: :package_registry do # rubocop: disable RSpec/FilePath + let(:object) { klass.new(params) } + let(:klass) do + Struct.new(:params) do + include ::API::Helpers + include ::API::Helpers::Packages::Npm + end + end + + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:namespace) { group } + let_it_be(:project) { create(:project, :public, namespace: namespace) } + let_it_be(:package) { create(:npm_package, project: project) } + + describe '#endpoint_scope' do + subject { object.endpoint_scope } + + context 'when params includes an id' do + let(:params) { { id: 42, package_name: 'foo' } } + + it { is_expected.to eq(:project) } + end + + context 'when params does not include an id' do + let(:params) { { package_name: 'foo' } } + + it { is_expected.to eq(:instance) } + end + end + + describe '#finder_for_endpoint_scope' do + subject { object.finder_for_endpoint_scope(package_name) } + + let(:package_name) { package.name } + + context 'when called with project scope' do + let(:params) { { id: project.id } } + + it 'returns a PackageFinder for project scope' do + expect(::Packages::Npm::PackageFinder).to receive(:new).with(package_name, project: project) + + subject + end + end + + context 'when called with instance scope' do + let(:params) { { package_name: package_name } } + + it 'returns a PackageFinder for namespace scope' do + expect(::Packages::Npm::PackageFinder).to receive(:new).with(package_name, namespace: group) + + subject + end + end + end + + describe '#project_id_or_nil' do + subject { object.project_id_or_nil } + + context 'when called with project scope' do + let(:params) { { id: project.id } } + + it { is_expected.to eq(project.id) } + end + + context 'when called with namespace scope' do + context 'when given an unscoped name' do + let(:params) { { package_name: 'foo' } } + + it { is_expected.to eq(nil) } + end + + context 'when given a scope that does not match a group name' do + let(:params) { { package_name: '@nonexistent-group/foo' } } + + it { is_expected.to eq(nil) } + end + + context 'when given a scope that matches a group name' do + let(:params) { { package_name: package.name } } + + it { is_expected.to eq(project.id) } + + context 'with another package with the same name, in another project in the namespace' do + let_it_be(:project2) { create(:project, :public, namespace: namespace) } + let_it_be(:package2) { create(:npm_package, name: package.name, project: project2) } + + it 'returns the project id for the newest matching package within the scope' do + expect(subject).to eq(project2.id) + end + end + end + + context 'with npm_allow_packages_in_multiple_projects disabled' do + before do + stub_feature_flags(npm_allow_packages_in_multiple_projects: false) + end + + context 'when given an unscoped name' do + let(:params) { { package_name: 'foo' } } + + it { is_expected.to eq(nil) } + end + + context 'when given a scope that does not match a group name' do + let(:params) { { package_name: '@nonexistent-group/foo' } } + + it { is_expected.to eq(nil) } + end + + context 'when given a scope that matches a group name' do + let(:params) { { package_name: package.name } } + + it { is_expected.to eq(project.id) } + + context 'with another package with the same name, in another project in the namespace' do + let_it_be(:project2) { create(:project, :public, namespace: namespace) } + let_it_be(:package2) { create(:npm_package, name: package.name, project: project2) } + + it 'returns the project id for the newest matching package within the scope' do + expect(subject).to eq(project2.id) + end + end + end + end + end + end +end -- GitLab