Commit 508e2b0b authored by David Fernandez's avatar David Fernandez Committed by Stan Hu

Update the npm package validator

parent 2e34d546
......@@ -324,7 +324,7 @@ class Packages::Package < ApplicationRecord
return unless project
return unless follows_npm_naming_convention?
if project.package_already_taken?(name, package_type: :npm)
if project.package_already_taken?(name, version, package_type: :npm)
errors.add(:base, _('Package already exists'))
end
end
......
......@@ -2566,14 +2566,14 @@ class Project < ApplicationRecord
[project&.id, root_group&.id]
end
def package_already_taken?(package_name, package_type:)
namespace.root_ancestor.all_projects
.joins(:packages)
.where.not(id: id)
.merge(
Packages::Package.default_scoped
.with_name(package_name)
.with_package_type(package_type)
def package_already_taken?(package_name, package_version, package_type:)
Packages::Package.with_name(package_name)
.with_version(package_version)
.with_package_type(package_type)
.for_projects(
root_ancestor.all_projects
.id_not_in(id)
.select(:id)
).exists?
end
......
......@@ -353,6 +353,13 @@ In this configuration:
You cannot publish a package if a package of the same name and version already exists.
You must delete the existing package first.
This rule has a different impact depending on the package name:
- For packages following the [naming convention](#package-naming-convention), you can't publish a
package with a duplicate name and version to the root namespace.
- For packages not following the [naming convention](#package-naming-convention), you can't publish
a package with a duplicate name and version to the project you target with the upload.
This aligns with npmjs.org's behavior. However, npmjs.org does not ever let you publish
the same version more than once, even if it has been deleted.
......
......@@ -3,6 +3,7 @@ require 'spec_helper'
RSpec.describe Packages::Package, type: :model do
include SortingHelper
using RSpec::Parameterized::TableSyntax
it_behaves_like 'having unique enum values'
......@@ -435,33 +436,154 @@ RSpec.describe Packages::Package, type: :model do
let_it_be(:second_project) { create(:project, namespace: group)}
let(:package) { build(:npm_package, project: project, name: name) }
let(:second_package) { build(:npm_package, project: second_project, name: name, version: '5.0.0') }
context 'following the naming convention' do
let(:name) { "@#{group.path}/test" }
it 'will allow the first package' do
shared_examples 'validating the first package' do
it 'validates the first package' do
expect(package).to be_valid
end
end
it 'will not allow npm package with duplicate name' do
shared_examples 'validating the second package' do
it 'validates the second package' do
package.save!
expect(second_package).to be_valid
end
end
shared_examples 'not validating the second package' do |field_with_error:|
it 'does not validate the second package' do
package.save!
expect(second_package).not_to be_valid
case field_with_error
when :base
expect(second_package.errors.messages[:base]).to eq ['Package already exists']
when :name
expect(second_package.errors.messages[:name]).to eq ['has already been taken']
else
raise ArgumentError, "field #{field_with_error} not expected"
end
end
end
context 'following the naming convention' do
let(:name) { "@#{group.path}/test" }
context 'with the second package in the project of the first package' do
let(:second_package) { build(:npm_package, project: project, name: second_package_name, version: second_package_version) }
context 'with no duplicated name' do
let(:second_package_name) { "@#{group.path}/test2" }
let(:second_package_version) { '5.0.0' }
it_behaves_like 'validating the first package'
it_behaves_like 'validating the second package'
end
context 'with duplicated name' do
let(:second_package_name) { package.name }
let(:second_package_version) { '5.0.0' }
it_behaves_like 'validating the first package'
it_behaves_like 'validating the second package'
end
context 'with duplicate name and duplicated version' do
let(:second_package_name) { package.name }
let(:second_package_version) { package.version }
it_behaves_like 'validating the first package'
it_behaves_like 'not validating the second package', field_with_error: :name
end
end
context 'with the second package in a different project than the first package' do
let(:second_package) { build(:npm_package, project: second_project, name: second_package_name, version: second_package_version) }
context 'with no duplicated name' do
let(:second_package_name) { "@#{group.path}/test2" }
let(:second_package_version) { '5.0.0' }
it_behaves_like 'validating the first package'
it_behaves_like 'validating the second package'
end
context 'with duplicated name' do
let(:second_package_name) { package.name }
let(:second_package_version) { '5.0.0' }
it_behaves_like 'validating the first package'
it_behaves_like 'validating the second package'
end
context 'with duplicate name and duplicated version' do
let(:second_package_name) { package.name }
let(:second_package_version) { package.version }
it_behaves_like 'validating the first package'
it_behaves_like 'not validating the second package', field_with_error: :base
end
end
end
context 'not following the naming convention' do
let(:name) { '@foobar/test' }
it 'will allow the first package' do
expect(package).to be_valid
context 'with the second package in the project of the first package' do
let(:second_package) { build(:npm_package, project: project, name: second_package_name, version: second_package_version) }
context 'with no duplicated name' do
let(:second_package_name) { "@foobar/test2" }
let(:second_package_version) { '5.0.0' }
it_behaves_like 'validating the first package'
it_behaves_like 'validating the second package'
end
context 'with duplicated name' do
let(:second_package_name) { package.name }
let(:second_package_version) { '5.0.0' }
it_behaves_like 'validating the first package'
it_behaves_like 'validating the second package'
end
context 'with duplicate name and duplicated version' do
let(:second_package_name) { package.name }
let(:second_package_version) { package.version }
it_behaves_like 'validating the first package'
it_behaves_like 'not validating the second package', field_with_error: :name
end
end
it 'will allow npm package with duplicate name' do
package.save!
context 'with the second package in a different project than the first package' do
let(:second_package) { build(:npm_package, project: second_project, name: second_package_name, version: second_package_version) }
expect(second_package).to be_valid
context 'with no duplicated name' do
let(:second_package_name) { "@foobar/test2" }
let(:second_package_version) { '5.0.0' }
it_behaves_like 'validating the first package'
it_behaves_like 'validating the second package'
end
context 'with duplicated name' do
let(:second_package_name) { package.name }
let(:second_package_version) { '5.0.0' }
it_behaves_like 'validating the first package'
it_behaves_like 'validating the second package'
end
context 'with duplicate name and duplicated version' do
let(:second_package_name) { package.name }
let(:second_package_version) { package.version }
it_behaves_like 'validating the first package'
it_behaves_like 'validating the second package'
end
end
end
end
......
......@@ -826,8 +826,6 @@ RSpec.describe Project, factory_default: :keep do
end
describe '#merge_method' do
using RSpec::Parameterized::TableSyntax
where(:ff, :rebase, :method) do
true | true | :ff
true | false | :ff
......@@ -1951,8 +1949,6 @@ RSpec.describe Project, factory_default: :keep do
end
context 'when set to INTERNAL in application settings' do
using RSpec::Parameterized::TableSyntax
before do
stub_application_setting(default_project_visibility: Gitlab::VisibilityLevel::INTERNAL)
end
......@@ -2013,8 +2009,6 @@ RSpec.describe Project, factory_default: :keep do
end
describe '#default_branch_protected?' do
using RSpec::Parameterized::TableSyntax
let_it_be(:namespace) { create(:namespace) }
let_it_be(:project) { create(:project, namespace: namespace) }
......@@ -6839,33 +6833,44 @@ RSpec.describe Project, factory_default: :keep do
end
describe '#package_already_taken?' do
let_it_be(:namespace) { create(:namespace) }
let_it_be(:namespace) { create(:namespace, path: 'test') }
let_it_be(:project) { create(:project, :public, namespace: namespace) }
let_it_be(:package) { create(:npm_package, project: project, name: "@#{namespace.path}/foo") }
let_it_be(:package) { create(:npm_package, project: project, name: "@#{namespace.path}/foo", version: '1.2.3') }
context 'no package exists with the same name' do
it 'returns false' do
result = project.package_already_taken?("@#{namespace.path}/bar", package_type: :npm)
expect(result).to be false
subject { project.package_already_taken?(package_name, package_version, package_type: :npm) }
context 'within the package project' do
where(:package_name, :package_version, :expected_result) do
'@test/bar' | '1.2.3' | false
'@test/bar' | '5.5.5' | false
'@test/foo' | '1.2.3' | false
'@test/foo' | '5.5.5' | false
end
it 'returns false if it is the project that the package belongs to' do
result = project.package_already_taken?("@#{namespace.path}/foo", package_type: :npm)
expect(result).to be false
with_them do
it { is_expected.to eq expected_result}
end
end
context 'a package already exists with the same name' do
context 'within a different project' do
let_it_be(:alt_project) { create(:project, :public, namespace: namespace) }
it 'returns true' do
result = alt_project.package_already_taken?(package.name, package_type: :npm)
expect(result).to be true
subject { alt_project.package_already_taken?(package_name, package_version, package_type: :npm) }
where(:package_name, :package_version, :expected_result) do
'@test/bar' | '1.2.3' | false
'@test/bar' | '5.5.5' | false
'@test/foo' | '1.2.3' | true
'@test/foo' | '5.5.5' | false
end
with_them do
it { is_expected.to eq expected_result}
end
context 'for a different package type' do
it 'returns false' do
result = alt_project.package_already_taken?(package.name, package_type: :nuget)
result = alt_project.package_already_taken?(package.name, package.version, package_type: :nuget)
expect(result).to be false
end
end
......
......@@ -161,8 +161,10 @@ RSpec.describe API::NpmProjectPackages do
end
end
context 'valid package record' do
let(:params) { upload_params(package_name: package_name) }
context 'valid package params' do
let_it_be(:version) { '1.2.3' }
let(:params) { upload_params(package_name: package_name, package_version: version) }
let(:snowplow_gitlab_standard_context) { { project: project, namespace: project.namespace, user: user } }
shared_examples 'handling upload with different authentications' do
......@@ -211,6 +213,15 @@ RSpec.describe API::NpmProjectPackages do
end
end
shared_examples 'uploading the package' do
it 'uploads the package' do
expect { upload_package_with_token(package_name, params) }
.to change { project.packages.count }.by(1)
expect(response).to have_gitlab_http_status(:ok)
end
end
context 'with a scoped name' do
let(:package_name) { "@#{group.path}/my_package_name" }
......@@ -233,24 +244,25 @@ RSpec.describe API::NpmProjectPackages do
let_it_be(:second_project) { create(:project, namespace: namespace) }
context 'following the naming convention' do
let_it_be(:second_package) { create(:npm_package, project: second_project, name: "@#{group.path}/test") }
let_it_be(:second_package) { create(:npm_package, project: second_project, name: "@#{group.path}/test", version: version) }
let(:package_name) { "@#{group.path}/test" }
it_behaves_like 'handling invalid record with 400 error'
context 'with a new version' do
let_it_be(:version) { '4.5.6' }
it_behaves_like 'uploading the package'
end
end
context 'not following the naming convention' do
let_it_be(:second_package) { create(:npm_package, project: second_project, name: "@any_scope/test") }
let_it_be(:second_package) { create(:npm_package, project: second_project, name: "@any_scope/test", version: version) }
let(:package_name) { "@any_scope/test" }
it "uploads the package" do
expect { upload_package_with_token(package_name, params) }
.to change { project.packages.count }.by(1)
expect(response).to have_gitlab_http_status(:ok)
end
it_behaves_like 'uploading the package'
end
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment