Merge branch '293755-generic-dupe-settings' into 'master'

Add setting for duplicate generic packages

See merge request gitlab-org/gitlab!60664

app/graphql/mutations/namespace/package_settings/update.rb

@@ -25,6 +25,16 @@ module Mutations
                 required: false,
                 description: copy_field_description(Types::Namespace::PackageSettingsType, :maven_duplicate_exception_regex)
 
+        argument :generic_duplicates_allowed,
+                GraphQL::BOOLEAN_TYPE,
+                required: false,
+                description: copy_field_description(Types::Namespace::PackageSettingsType, :generic_duplicates_allowed)
+
+        argument :generic_duplicate_exception_regex,
+                Types::UntrustedRegexp,
+                required: false,
+                description: copy_field_description(Types::Namespace::PackageSettingsType, :generic_duplicate_exception_regex)
+
         field :package_settings,
               Types::Namespace::PackageSettingsType,
               null: true,

app/graphql/types/namespace/package_settings_type.rb

@@ -10,5 +10,7 @@ module Types
 
     field :maven_duplicates_allowed, GraphQL::BOOLEAN_TYPE, null: false, description: 'Indicates whether duplicate Maven packages are allowed for this namespace.'
     field :maven_duplicate_exception_regex, Types::UntrustedRegexp, null: true, description: 'When maven_duplicates_allowed is false, you can publish duplicate packages with names that match this regex. Otherwise, this setting has no effect.'
+    field :generic_duplicates_allowed, GraphQL::BOOLEAN_TYPE, null: false, description: 'Indicates whether duplicate generic packages are allowed for this namespace.'
+    field :generic_duplicate_exception_regex, Types::UntrustedRegexp, null: true, description: 'When generic_duplicates_allowed is false, you can publish duplicate packages with names that match this regex. Otherwise, this setting has no effect.'
   end
 end

app/models/namespace/package_setting.rb

@@ -6,13 +6,15 @@ class Namespace::PackageSetting < ApplicationRecord
 
   PackageSettingNotImplemented = Class.new(StandardError)
 
-  PACKAGES_WITH_SETTINGS = %w[maven].freeze
+  PACKAGES_WITH_SETTINGS = %w[maven generic].freeze
 
   belongs_to :namespace, inverse_of: :package_setting_relation
 
   validates :namespace, presence: true
   validates :maven_duplicates_allowed, inclusion: { in: [true, false] }
   validates :maven_duplicate_exception_regex, untrusted_regexp: true, length: { maximum: 255 }
+  validates :generic_duplicates_allowed, inclusion: { in: [true, false] }
+  validates :generic_duplicate_exception_regex, untrusted_regexp: true, length: { maximum: 255 }
 
   class << self
     def duplicates_allowed?(package)

app/models/packages.rb

 # frozen_string_literal: true
 module Packages
+  DuplicatePackageError = Class.new(StandardError)
+
   def self.table_name_prefix
     'packages_'
   end

app/services/namespaces/package_settings/update_service.rb

@@ -5,7 +5,10 @@ module Namespaces
     class UpdateService < BaseContainerService
       include Gitlab::Utils::StrongMemoize
 
-      ALLOWED_ATTRIBUTES = %i[maven_duplicates_allowed maven_duplicate_exception_regex].freeze
+      ALLOWED_ATTRIBUTES = %i[maven_duplicates_allowed
+                              maven_duplicate_exception_regex
+                              generic_duplicates_allowed
+                              generic_duplicate_exception_regex].freeze
 
       def execute
         return ServiceResponse.error(message: 'Access Denied', http_status: 403) unless allowed?

app/services/packages/generic/create_package_file_service.rb

@@ -23,6 +23,10 @@ module Packages
           .new(project, current_user, package_params)
           .execute
 
+        unless Namespace::PackageSetting.duplicates_allowed?(package)
+          raise ::Packages::DuplicatePackageError if target_file_is_duplicate?(package)
+        end
+
         package.update_column(:status, params[:status]) if params[:status] && params[:status] != package.status
 
         package.build_infos.safe_find_or_create_by!(pipeline: params[:build].pipeline) if params[:build].present?
@@ -40,6 +44,10 @@ module Packages
 
         ::Packages::CreatePackageFileService.new(package, file_params).execute
       end
+
+      def target_file_is_duplicate?(package)
+        package.package_files.with_file_name(params[:file_name]).exists?
+      end
     end
   end
 end

changelogs/unreleased/293755-generic-dupe-settings.yml

+---
+title: Add setting to allow or disallow duplicates for generic packages
+merge_request: 60664
+author:
+type: added

db/migrate/20210429192653_add_generic_package_duplicate_settings_to_namespace_package_settings.rb

+# frozen_string_literal: true
+
+class AddGenericPackageDuplicateSettingsToNamespacePackageSettings < ActiveRecord::Migration[6.0]
+  # rubocop:disable Migration/AddLimitToTextColumns
+  # limit is added in 20210429193106_add_text_limit_to_namespace_package_settings_generic_duplicate_exception_regex
+  def change
+    add_column :namespace_package_settings, :generic_duplicates_allowed, :boolean, null: false, default: true
+    add_column :namespace_package_settings, :generic_duplicate_exception_regex, :text, null: false, default: ''
+  end
+  # rubocop:enable Migration/AddLimitToTextColumns
+end

db/migrate/20210429193106_add_text_limit_to_namespace_package_settings_generic_duplicate_exception_regex.rb

+# frozen_string_literal: true
+
+class AddTextLimitToNamespacePackageSettingsGenericDuplicateExceptionRegex < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    add_text_limit :namespace_package_settings, :generic_duplicate_exception_regex, 255
+  end
+
+  def down
+    remove_text_limit :namespace_package_settings, :generic_duplicate_exception_regex
+  end
+end

db/schema_migrations/20210429192653

+c2b5ad6786e1c71ccff391b03fcd0635dfd42d69484443291a692cef9f3ffda5
\ No newline at end of file

db/schema_migrations/20210429193106

+e0898e4e439cde4e3b84808e7505490fe956cf17922f5c779b3384997d36cafd
\ No newline at end of file

db/structure.sql

@@ -14798,6 +14798,9 @@ CREATE TABLE namespace_package_settings (
     namespace_id bigint NOT NULL,
     maven_duplicates_allowed boolean DEFAULT true NOT NULL,
     maven_duplicate_exception_regex text DEFAULT ''::text NOT NULL,
+    generic_duplicates_allowed boolean DEFAULT true NOT NULL,
+    generic_duplicate_exception_regex text DEFAULT ''::text NOT NULL,
+    CONSTRAINT check_31340211b1 CHECK ((char_length(generic_duplicate_exception_regex) <= 255)),
     CONSTRAINT check_d63274b2b6 CHECK ((char_length(maven_duplicate_exception_regex) <= 255))
 );
 

doc/api/graphql/reference/index.md

@@ -3911,6 +3911,8 @@ Input type: `UpdateNamespacePackageSettingsInput`
 | Name | Type | Description |
 | ---- | ---- | ----------- |
 | <a id="mutationupdatenamespacepackagesettingsclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationupdatenamespacepackagesettingsgenericduplicateexceptionregex"></a>`genericDuplicateExceptionRegex` | [`UntrustedRegexp`](#untrustedregexp) | When generic_duplicates_allowed is false, you can publish duplicate packages with names that match this regex. Otherwise, this setting has no effect. |
+| <a id="mutationupdatenamespacepackagesettingsgenericduplicatesallowed"></a>`genericDuplicatesAllowed` | [`Boolean`](#boolean) | Indicates whether duplicate generic packages are allowed for this namespace. |
 | <a id="mutationupdatenamespacepackagesettingsmavenduplicateexceptionregex"></a>`mavenDuplicateExceptionRegex` | [`UntrustedRegexp`](#untrustedregexp) | When maven_duplicates_allowed is false, you can publish duplicate packages with names that match this regex. Otherwise, this setting has no effect. |
 | <a id="mutationupdatenamespacepackagesettingsmavenduplicatesallowed"></a>`mavenDuplicatesAllowed` | [`Boolean`](#boolean) | Indicates whether duplicate Maven packages are allowed for this namespace. |
 | <a id="mutationupdatenamespacepackagesettingsnamespacepath"></a>`namespacePath` | [`ID!`](#id) | The namespace path where the namespace package setting is located. |
@@ -10488,6 +10490,8 @@ Namespace-level Package Registry settings.
 
 | Name | Type | Description |
 | ---- | ---- | ----------- |
+| <a id="packagesettingsgenericduplicateexceptionregex"></a>`genericDuplicateExceptionRegex` | [`UntrustedRegexp`](#untrustedregexp) | When generic_duplicates_allowed is false, you can publish duplicate packages with names that match this regex. Otherwise, this setting has no effect. |
+| <a id="packagesettingsgenericduplicatesallowed"></a>`genericDuplicatesAllowed` | [`Boolean!`](#boolean) | Indicates whether duplicate generic packages are allowed for this namespace. |
 | <a id="packagesettingsmavenduplicateexceptionregex"></a>`mavenDuplicateExceptionRegex` | [`UntrustedRegexp`](#untrustedregexp) | When maven_duplicates_allowed is false, you can publish duplicate packages with names that match this regex. Otherwise, this setting has no effect. |
 | <a id="packagesettingsmavenduplicatesallowed"></a>`mavenDuplicatesAllowed` | [`Boolean!`](#boolean) | Indicates whether duplicate Maven packages are allowed for this namespace. |
 

lib/api/generic_packages.rb

@@ -74,6 +74,8 @@ module API
             Gitlab::ErrorTracking.track_exception(e, extra: { file_name: params[:file_name], project_id: project.id })
 
             forbidden!
+          rescue ::Packages::DuplicatePackageError
+            bad_request!('Duplicate package is not allowed')
           end
 
           desc 'Download package file' do

spec/factories/namespace_package_settings.rb

@@ -7,6 +7,9 @@ FactoryBot.define do
     maven_duplicates_allowed { true }
     maven_duplicate_exception_regex { 'SNAPSHOT' }
 
+    generic_duplicates_allowed { true }
+    generic_duplicate_exception_regex { 'foo' }
+
     trait :group do
       namespace { association(:group) }
     end

spec/graphql/mutations/namespace/package_settings/update_spec.rb

@@ -25,7 +25,9 @@ RSpec.describe Mutations::Namespace::PackageSettings::Update do
     end
 
     RSpec.shared_examples 'updating the namespace package setting' do
-      it_behaves_like 'updating the namespace package setting attributes', from: { maven_duplicates_allowed: true, maven_duplicate_exception_regex: 'SNAPSHOT' }, to: { maven_duplicates_allowed: false, maven_duplicate_exception_regex: 'RELEASE' }
+      it_behaves_like 'updating the namespace package setting attributes',
+        from: { maven_duplicates_allowed: true, maven_duplicate_exception_regex: 'SNAPSHOT', generic_duplicates_allowed: true, generic_duplicate_exception_regex: 'foo' },
+        to: { maven_duplicates_allowed: false, maven_duplicate_exception_regex: 'RELEASE', generic_duplicates_allowed: false, generic_duplicate_exception_regex: 'bar' }
 
       it_behaves_like 'returning a success'
 
@@ -56,7 +58,13 @@ RSpec.describe Mutations::Namespace::PackageSettings::Update do
 
     context 'with existing namespace package setting' do
       let_it_be(:package_settings) { create(:namespace_package_setting, namespace: namespace) }
-      let_it_be(:params) { { namespace_path: namespace.full_path, maven_duplicates_allowed: false, maven_duplicate_exception_regex: 'RELEASE' } }
+      let_it_be(:params) do
+        { namespace_path: namespace.full_path,
+          maven_duplicates_allowed: false,
+          maven_duplicate_exception_regex: 'RELEASE',
+          generic_duplicates_allowed: false,
+          generic_duplicate_exception_regex: 'bar' }
+      end
 
       where(:user_role, :shared_examples_name) do
         :maintainer | 'updating the namespace package setting'

spec/models/namespace/package_setting_spec.rb

@@ -14,9 +14,12 @@ RSpec.describe Namespace::PackageSetting do
       it { is_expected.to allow_value(true).for(:maven_duplicates_allowed) }
       it { is_expected.to allow_value(false).for(:maven_duplicates_allowed) }
       it { is_expected.not_to allow_value(nil).for(:maven_duplicates_allowed) }
+      it { is_expected.to allow_value(true).for(:generic_duplicates_allowed) }
+      it { is_expected.to allow_value(false).for(:generic_duplicates_allowed) }
+      it { is_expected.not_to allow_value(nil).for(:generic_duplicates_allowed) }
     end
 
-    describe '#maven_duplicate_exception_regex' do
+    describe 'regex values' do
       let_it_be(:package_settings) { create(:namespace_package_setting) }
 
       subject { package_settings }
@@ -24,12 +27,14 @@ RSpec.describe Namespace::PackageSetting do
       valid_regexps = %w[SNAPSHOT .* v.+ v10.1.* (?:v.+|SNAPSHOT|TEMP)]
       invalid_regexps = ['[', '(?:v.+|SNAPSHOT|TEMP']
 
-      valid_regexps.each do |valid_regexp|
-        it { is_expected.to allow_value(valid_regexp).for(:maven_duplicate_exception_regex) }
-      end
+      [:maven_duplicate_exception_regex, :generic_duplicate_exception_regex].each do |attribute|
+        valid_regexps.each do |valid_regexp|
+          it { is_expected.to allow_value(valid_regexp).for(attribute) }
+        end
 
-      invalid_regexps.each do |invalid_regexp|
-        it { is_expected.not_to allow_value(invalid_regexp).for(:maven_duplicate_exception_regex) }
+        invalid_regexps.each do |invalid_regexp|
+          it { is_expected.not_to allow_value(invalid_regexp).for(attribute) }
+        end
       end
     end
   end
@@ -41,7 +46,7 @@ RSpec.describe Namespace::PackageSetting do
 
     context 'package types with package_settings' do
       # As more package types gain settings they will be added to this list
-      [:maven_package].each do |format|
+      [:maven_package, :generic_package].each do |format|
         let_it_be(:package) { create(format, name: 'foo', version: 'beta') } # rubocop:disable Rails/SaveBang
         let_it_be(:package_type) { package.package_type }
         let_it_be(:package_setting) { package.project.namespace.package_settings }
@@ -70,7 +75,7 @@ RSpec.describe Namespace::PackageSetting do
     end
 
     context 'package types without package_settings' do
-      [:npm_package, :conan_package, :nuget_package, :pypi_package, :composer_package, :generic_package, :golang_package, :debian_package].each do |format|
+      [:npm_package, :conan_package, :nuget_package, :pypi_package, :composer_package, :golang_package, :debian_package].each do |format|
         let_it_be(:package) { create(format) } # rubocop:disable Rails/SaveBang
         let_it_be(:package_setting) { package.project.namespace.package_settings }
 

spec/requests/api/graphql/mutations/namespace/package_settings/update_spec.rb

@@ -12,7 +12,9 @@ RSpec.describe 'Updating the package settings' do
     {
       namespace_path: namespace.full_path,
       maven_duplicates_allowed: false,
-      maven_duplicate_exception_regex: 'foo-.*'
+      maven_duplicate_exception_regex: 'foo-.*',
+      generic_duplicates_allowed: false,
+      generic_duplicate_exception_regex: 'bar-.*'
     }
   end
 
@@ -22,6 +24,8 @@ RSpec.describe 'Updating the package settings' do
         packageSettings {
           mavenDuplicatesAllowed
           mavenDuplicateExceptionRegex
+          genericDuplicatesAllowed
+          genericDuplicateExceptionRegex
         }
         errors
       QL
@@ -40,6 +44,8 @@ RSpec.describe 'Updating the package settings' do
       expect(mutation_response['errors']).to be_empty
       expect(package_settings_response['mavenDuplicatesAllowed']).to eq(params[:maven_duplicates_allowed])
       expect(package_settings_response['mavenDuplicateExceptionRegex']).to eq(params[:maven_duplicate_exception_regex])
+      expect(package_settings_response['genericDuplicatesAllowed']).to eq(params[:generic_duplicates_allowed])
+      expect(package_settings_response['genericDuplicateExceptionRegex']).to eq(params[:generic_duplicate_exception_regex])
     end
   end
 
@@ -69,8 +75,8 @@ RSpec.describe 'Updating the package settings' do
 
   RSpec.shared_examples 'accepting the mutation request updating the package settings' do
     it_behaves_like 'updating the namespace package setting attributes',
-      from: { maven_duplicates_allowed: true, maven_duplicate_exception_regex: 'SNAPSHOT' },
-      to: { maven_duplicates_allowed: false, maven_duplicate_exception_regex: 'foo-.*' }
+      from: { maven_duplicates_allowed: true, maven_duplicate_exception_regex: 'SNAPSHOT', generic_duplicates_allowed: true, generic_duplicate_exception_regex: 'foo' },
+      to: { maven_duplicates_allowed: false, maven_duplicate_exception_regex: 'foo-.*', generic_duplicates_allowed: false, generic_duplicate_exception_regex: 'bar-.*' }
 
     it_behaves_like 'returning a success'
     it_behaves_like 'rejecting invalid regex'

spec/services/namespaces/package_settings/update_service_spec.rb

@@ -32,7 +32,9 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService do
     end
 
     shared_examples 'updating the namespace package setting' do
-      it_behaves_like 'updating the namespace package setting attributes', from: { maven_duplicates_allowed: true, maven_duplicate_exception_regex: 'SNAPSHOT' }, to: { maven_duplicates_allowed: false, maven_duplicate_exception_regex: 'RELEASE' }
+      it_behaves_like 'updating the namespace package setting attributes',
+        from: { maven_duplicates_allowed: true, maven_duplicate_exception_regex: 'SNAPSHOT', generic_duplicates_allowed: true, generic_duplicate_exception_regex: 'foo' },
+        to: { maven_duplicates_allowed: false, maven_duplicate_exception_regex: 'RELEASE', generic_duplicates_allowed: false, generic_duplicate_exception_regex: 'bar' }
 
       it_behaves_like 'returning a success'
 
@@ -60,7 +62,12 @@ RSpec.describe ::Namespaces::PackageSettings::UpdateService do
 
     context 'with existing namespace package setting' do
       let_it_be(:package_settings) { create(:namespace_package_setting, namespace: namespace) }
-      let_it_be(:params) { { maven_duplicates_allowed: false, maven_duplicate_exception_regex: 'RELEASE' } }
+      let_it_be(:params) do
+        { maven_duplicates_allowed: false,
+          maven_duplicate_exception_regex: 'RELEASE',
+          generic_duplicates_allowed: false,
+          generic_duplicate_exception_regex: 'bar' }
+      end
 
       where(:user_role, :shared_examples_name) do
         :maintainer | 'updating the namespace package setting'

spec/services/packages/generic/create_package_file_service_spec.rb

@@ -6,13 +6,16 @@ RSpec.describe Packages::Generic::CreatePackageFileService do
   let_it_be(:project) { create(:project) }
   let_it_be(:user) { create(:user) }
   let_it_be(:pipeline) { create(:ci_pipeline, user: user) }
+  let_it_be(:file_name) { 'myfile.tar.gz.1' }
+
   let(:build) { double('build', pipeline: pipeline) }
 
   describe '#execute' do
+    let_it_be(:package) { create(:generic_package, project: project) }
+
     let(:sha256) { '440e5e148a25331bbd7991575f7d54933c0ebf6cc735a18ee5066ac1381bb590' }
     let(:temp_file) { Tempfile.new("test") }
     let(:file) { UploadedFile.new(temp_file.path, sha256: sha256) }
-    let(:package) { create(:generic_package, project: project) }
     let(:package_service) { double }
 
     let(:params) do
@@ -20,7 +23,7 @@ RSpec.describe Packages::Generic::CreatePackageFileService do
         package_name: 'mypackage',
         package_version: '0.0.1',
         file: file,
-        file_name: 'myfile.tar.gz.1',
+        file_name: file_name,
         build: build
       }
     end
@@ -34,7 +37,7 @@ RSpec.describe Packages::Generic::CreatePackageFileService do
       }
     end
 
-    subject { described_class.new(project, user, params).execute }
+    subject(:execute_service) { described_class.new(project, user, params).execute }
 
     before do
       FileUtils.touch(temp_file)
@@ -47,14 +50,14 @@ RSpec.describe Packages::Generic::CreatePackageFileService do
     end
 
     it 'creates package file', :aggregate_failures do
-      expect { subject }.to change { package.package_files.count }.by(1)
+      expect { execute_service }.to change { package.package_files.count }.by(1)
         .and change { Packages::PackageFileBuildInfo.count }.by(1)
 
       package_file = package.package_files.last
       aggregate_failures do
         expect(package_file.package.status).to eq('default')
         expect(package_file.package).to eq(package)
-        expect(package_file.file_name).to eq('myfile.tar.gz.1')
+        expect(package_file.file_name).to eq(file_name)
         expect(package_file.size).to eq(file.size)
         expect(package_file.file_sha256).to eq(sha256)
       end
@@ -65,7 +68,7 @@ RSpec.describe Packages::Generic::CreatePackageFileService do
       let(:package_params) { super().merge(status: 'hidden') }
 
       it 'updates an existing packages status' do
-        expect { subject }.to change { package.package_files.count }.by(1)
+        expect { execute_service }.to change { package.package_files.count }.by(1)
           .and change { Packages::PackageFileBuildInfo.count }.by(1)
 
         package_file = package.package_files.last
@@ -76,5 +79,32 @@ RSpec.describe Packages::Generic::CreatePackageFileService do
     end
 
     it_behaves_like 'assigns build to package file'
+
+    context 'with existing package' do
+      before do
+        create(:package_file, package: package, file_name: file_name)
+      end
+
+      it { expect { execute_service }.to change { project.package_files.count }.by(1) }
+
+      context 'when duplicates are not allowed' do
+        before do
+          package.project.namespace.package_settings.update!(generic_duplicates_allowed: false)
+        end
+
+        it 'does not allow duplicates' do
+          expect { execute_service }.to raise_error(::Packages::DuplicatePackageError)
+            .and change { project.package_files.count }.by(0)
+        end
+
+        context 'when the package name matches the exception regex' do
+          before do
+            package.project.namespace.package_settings.update!(generic_duplicate_exception_regex: '.*')
+          end
+
+          it { expect { execute_service }.to change { project.package_files.count }.by(1) }
+        end
+      end
+    end
   end
 end

spec/support/shared_examples/services/namespace_package_settings_shared_examples.rb

@@ -7,6 +7,8 @@ RSpec.shared_examples 'updating the namespace package setting attributes' do |fr
     expect { subject }
       .to change { namespace.package_settings.reload.maven_duplicates_allowed }.from(from[:maven_duplicates_allowed]).to(to[:maven_duplicates_allowed])
       .and change { namespace.package_settings.reload.maven_duplicate_exception_regex }.from(from[:maven_duplicate_exception_regex]).to(to[:maven_duplicate_exception_regex])
+      .and change { namespace.package_settings.reload.generic_duplicates_allowed }.from(from[:generic_duplicates_allowed]).to(to[:generic_duplicates_allowed])
+      .and change { namespace.package_settings.reload.generic_duplicate_exception_regex }.from(from[:generic_duplicate_exception_regex]).to(to[:generic_duplicate_exception_regex])
   end
 end
 
@@ -26,6 +28,8 @@ RSpec.shared_examples 'creating the namespace package setting' do
 
     expect(namespace.package_setting_relation.maven_duplicates_allowed).to eq(package_settings[:maven_duplicates_allowed])
     expect(namespace.package_setting_relation.maven_duplicate_exception_regex).to eq(package_settings[:maven_duplicate_exception_regex])
+    expect(namespace.package_setting_relation.generic_duplicates_allowed).to eq(package_settings[:generic_duplicates_allowed])
+    expect(namespace.package_setting_relation.generic_duplicate_exception_regex).to eq(package_settings[:generic_duplicate_exception_regex])
   end
 
   it_behaves_like 'returning a success'