Commit 169f45f4 authored by Kerri Miller's avatar Kerri Miller

Merge branch '330475-fix-npm-push_package-event-created-on-api-error' into 'master'

Prevent creation of push_package event on NPM packages API error

See merge request gitlab-org/gitlab!68054
parents e1076b8f caee0a10
...@@ -48,14 +48,13 @@ module API ...@@ -48,14 +48,13 @@ module API
put ':package_name', requirements: ::API::Helpers::Packages::Npm::NPM_ENDPOINT_REQUIREMENTS do put ':package_name', requirements: ::API::Helpers::Packages::Npm::NPM_ENDPOINT_REQUIREMENTS do
authorize_create_package!(project) authorize_create_package!(project)
track_package_event('push_package', :npm, category: 'API::NpmPackages', project: project, user: current_user, namespace: project.namespace)
created_package = ::Packages::Npm::CreatePackageService created_package = ::Packages::Npm::CreatePackageService
.new(project, current_user, params.merge(build: current_authenticated_job)).execute .new(project, current_user, params.merge(build: current_authenticated_job)).execute
if created_package[:status] == :error if created_package[:status] == :error
render_api_error!(created_package[:message], created_package[:http_status]) render_api_error!(created_package[:message], created_package[:http_status])
else else
track_package_event('push_package', :npm, category: 'API::NpmPackages', project: project, user: current_user, namespace: project.namespace)
created_package created_package
end end
end end
......
...@@ -120,9 +120,11 @@ RSpec.describe API::NpmProjectPackages do ...@@ -120,9 +120,11 @@ RSpec.describe API::NpmProjectPackages do
project.add_developer(user) project.add_developer(user)
end end
subject(:upload_package_with_token) { upload_with_token(package_name, params) }
shared_examples 'handling invalid record with 400 error' do shared_examples 'handling invalid record with 400 error' do
it 'handles an ActiveRecord::RecordInvalid exception with 400 error' do it 'handles an ActiveRecord::RecordInvalid exception with 400 error' do
expect { upload_package_with_token(package_name, params) } expect { upload_package_with_token }
.not_to change { project.packages.count } .not_to change { project.packages.count }
expect(response).to have_gitlab_http_status(:bad_request) expect(response).to have_gitlab_http_status(:bad_request)
...@@ -136,6 +138,7 @@ RSpec.describe API::NpmProjectPackages do ...@@ -136,6 +138,7 @@ RSpec.describe API::NpmProjectPackages do
let(:params) { upload_params(package_name: package_name) } let(:params) { upload_params(package_name: package_name) }
it_behaves_like 'handling invalid record with 400 error' it_behaves_like 'handling invalid record with 400 error'
it_behaves_like 'not a package tracking event'
end end
context 'invalid package version' do context 'invalid package version' do
...@@ -157,6 +160,7 @@ RSpec.describe API::NpmProjectPackages do ...@@ -157,6 +160,7 @@ RSpec.describe API::NpmProjectPackages do
let(:params) { upload_params(package_name: package_name, package_version: version) } let(:params) { upload_params(package_name: package_name, package_version: version) }
it_behaves_like 'handling invalid record with 400 error' it_behaves_like 'handling invalid record with 400 error'
it_behaves_like 'not a package tracking event'
end end
end end
end end
...@@ -169,8 +173,6 @@ RSpec.describe API::NpmProjectPackages do ...@@ -169,8 +173,6 @@ RSpec.describe API::NpmProjectPackages do
shared_examples 'handling upload with different authentications' do shared_examples 'handling upload with different authentications' do
context 'with access token' do context 'with access token' do
subject { upload_package_with_token(package_name, params) }
it_behaves_like 'a package tracking event', 'API::NpmPackages', 'push_package' it_behaves_like 'a package tracking event', 'API::NpmPackages', 'push_package'
it 'creates npm package with file' do it 'creates npm package with file' do
...@@ -184,7 +186,7 @@ RSpec.describe API::NpmProjectPackages do ...@@ -184,7 +186,7 @@ RSpec.describe API::NpmProjectPackages do
end end
it 'creates npm package with file with job token' do it 'creates npm package with file with job token' do
expect { upload_package_with_job_token(package_name, params) } expect { upload_with_job_token(package_name, params) }
.to change { project.packages.count }.by(1) .to change { project.packages.count }.by(1)
.and change { Packages::PackageFile.count }.by(1) .and change { Packages::PackageFile.count }.by(1)
...@@ -205,7 +207,7 @@ RSpec.describe API::NpmProjectPackages do ...@@ -205,7 +207,7 @@ RSpec.describe API::NpmProjectPackages do
end end
it 'creates the package metadata' do it 'creates the package metadata' do
upload_package_with_token(package_name, params) upload_package_with_token
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
expect(project.reload.packages.find(json_response['id']).original_build_info.pipeline).to eq job.pipeline expect(project.reload.packages.find(json_response['id']).original_build_info.pipeline).to eq job.pipeline
...@@ -215,7 +217,7 @@ RSpec.describe API::NpmProjectPackages do ...@@ -215,7 +217,7 @@ RSpec.describe API::NpmProjectPackages do
shared_examples 'uploading the package' do shared_examples 'uploading the package' do
it 'uploads the package' do it 'uploads the package' do
expect { upload_package_with_token(package_name, params) } expect { upload_package_with_token }
.to change { project.packages.count }.by(1) .to change { project.packages.count }.by(1)
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
...@@ -249,6 +251,7 @@ RSpec.describe API::NpmProjectPackages do ...@@ -249,6 +251,7 @@ RSpec.describe API::NpmProjectPackages do
let(:package_name) { "@#{group.path}/test" } let(:package_name) { "@#{group.path}/test" }
it_behaves_like 'handling invalid record with 400 error' it_behaves_like 'handling invalid record with 400 error'
it_behaves_like 'not a package tracking event'
context 'with a new version' do context 'with a new version' do
let_it_be(:version) { '4.5.6' } let_it_be(:version) { '4.5.6' }
...@@ -271,9 +274,14 @@ RSpec.describe API::NpmProjectPackages do ...@@ -271,9 +274,14 @@ RSpec.describe API::NpmProjectPackages do
let(:package_name) { "@#{group.path}/my_package_name" } let(:package_name) { "@#{group.path}/my_package_name" }
let(:params) { upload_params(package_name: package_name) } let(:params) { upload_params(package_name: package_name) }
it 'returns an error if the package already exists' do before do
create(:npm_package, project: project, version: '1.0.1', name: "@#{group.path}/my_package_name") create(:npm_package, project: project, version: '1.0.1', name: "@#{group.path}/my_package_name")
expect { upload_package_with_token(package_name, params) } end
it_behaves_like 'not a package tracking event'
it 'returns an error if the package already exists' do
expect { upload_package_with_token }
.not_to change { project.packages.count } .not_to change { project.packages.count }
expect(response).to have_gitlab_http_status(:forbidden) expect(response).to have_gitlab_http_status(:forbidden)
...@@ -285,7 +293,7 @@ RSpec.describe API::NpmProjectPackages do ...@@ -285,7 +293,7 @@ RSpec.describe API::NpmProjectPackages do
let(:params) { upload_params(package_name: package_name, file: 'npm/payload_with_duplicated_packages.json') } let(:params) { upload_params(package_name: package_name, file: 'npm/payload_with_duplicated_packages.json') }
it 'creates npm package with file and dependencies' do it 'creates npm package with file and dependencies' do
expect { upload_package_with_token(package_name, params) } expect { upload_package_with_token }
.to change { project.packages.count }.by(1) .to change { project.packages.count }.by(1)
.and change { Packages::PackageFile.count }.by(1) .and change { Packages::PackageFile.count }.by(1)
.and change { Packages::Dependency.count}.by(4) .and change { Packages::Dependency.count}.by(4)
...@@ -297,11 +305,11 @@ RSpec.describe API::NpmProjectPackages do ...@@ -297,11 +305,11 @@ RSpec.describe API::NpmProjectPackages do
context 'with existing dependencies' do context 'with existing dependencies' do
before do before do
name = "@#{group.path}/existing_package" name = "@#{group.path}/existing_package"
upload_package_with_token(name, upload_params(package_name: name, file: 'npm/payload_with_duplicated_packages.json')) upload_with_token(name, upload_params(package_name: name, file: 'npm/payload_with_duplicated_packages.json'))
end end
it 'reuses them' do it 'reuses them' do
expect { upload_package_with_token(package_name, params) } expect { upload_package_with_token }
.to change { project.packages.count }.by(1) .to change { project.packages.count }.by(1)
.and change { Packages::PackageFile.count }.by(1) .and change { Packages::PackageFile.count }.by(1)
.and not_change { Packages::Dependency.count} .and not_change { Packages::Dependency.count}
...@@ -317,11 +325,11 @@ RSpec.describe API::NpmProjectPackages do ...@@ -317,11 +325,11 @@ RSpec.describe API::NpmProjectPackages do
put api("/projects/#{project.id}/packages/npm/#{package_name.sub('/', '%2f')}"), params: params, headers: headers put api("/projects/#{project.id}/packages/npm/#{package_name.sub('/', '%2f')}"), params: params, headers: headers
end end
def upload_package_with_token(package_name, params = {}) def upload_with_token(package_name, params = {})
upload_package(package_name, params.merge(access_token: token.token)) upload_package(package_name, params.merge(access_token: token.token))
end end
def upload_package_with_job_token(package_name, params = {}) def upload_with_job_token(package_name, params = {})
upload_package(package_name, params.merge(job_token: job.token)) upload_package(package_name, params.merge(job_token: job.token))
end end
......
...@@ -153,3 +153,15 @@ RSpec.shared_examples 'a package tracking event' do |category, action| ...@@ -153,3 +153,15 @@ RSpec.shared_examples 'a package tracking event' do |category, action|
expect_snowplow_event(category: category, action: action, **snowplow_gitlab_standard_context) expect_snowplow_event(category: category, action: action, **snowplow_gitlab_standard_context)
end end
end end
RSpec.shared_examples 'not a package tracking event' do
before do
stub_feature_flags(collect_package_events: true)
end
it 'does not create a gitlab tracking event', :snowplow, :aggregate_failures do
expect { subject }.not_to change { Packages::Event.count }
expect_no_snowplow_event
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