Merge branch 'mc_rocha-update-bulk_create_policy_service-441078' into 'master'

Update bulk create service for custom software licenses

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/155588



Merged-by: Pavel Shutsin <pshutsin@gitlab.com>
Approved-by: Martin Čavoj <mcavoj@gitlab.com>
Approved-by: Tianwen Chen <tchen@gitlab.com>
Approved-by: Harsha Muralidhar <hmuralidhar@gitlab.com>
Approved-by: Pavel Shutsin <pshutsin@gitlab.com>
Reviewed-by: Sashi Kumar Kumaresan <skumar@gitlab.com>
Reviewed-by: Tianwen Chen <tchen@gitlab.com>
Reviewed-by: Martin Čavoj <mcavoj@gitlab.com>
Co-authored-by: mc_rocha <mrocha@gitlab.com>

ee/app/models/security/custom_software_license.rb

@@ -7,5 +7,8 @@ class CustomSoftwareLicense < ApplicationRecord
     belongs_to :project
 
     validates :name, presence: true, uniqueness: { scope: :project_id }, length: { maximum: 255 }
+
+    scope :by_name, ->(names) { where(name: names) }
+    scope :by_project, ->(project) { where(project: project) }
   end
 end

ee/app/services/software_license_policies/bulk_create_scan_result_policy_service.rb

@@ -2,6 +2,8 @@
 
 module SoftwareLicensePolicies
   class BulkCreateScanResultPolicyService < ::BaseService
+    include ::Gitlab::Utils::StrongMemoize
+
     BATCH_SIZE = 250
 
     def initialize(project, params)
@@ -9,46 +11,65 @@ def initialize(project, params)
     end
 
     def execute
-      create_unknown_licenses
+      existing_licenses = SoftwareLicense.by_name(license_names).limit(license_names.size).pluck(:name, :id).to_h # rubocop:disable CodeReuse/ActiveRecord, Database/AvoidUsingPluckWithoutLimit -- Array#pluck
+      missing_licenses_names = license_names - existing_licenses.keys
+
+      created_custom_licenses = create_unknown_custom_licenses(missing_licenses_names)
+
+      software_license_policies = create_software_license_policies(created_custom_licenses, existing_licenses)
 
-      licenses = SoftwareLicense.by_name(license_names).to_h do |license|
-        [license.name, license.id]
+      success(software_license_policy: software_license_policies)
+    end
+
+    private
+
+    def license_names
+      params.map { |license| license.with_indifferent_access[:name].strip }.uniq
+    end
+    strong_memoize_attr :license_names
+
+    def create_unknown_custom_licenses(missing_licenses_names)
+      created_custom_licenses = []
+      missing_licenses_names.each_slice(BATCH_SIZE) do |names|
+        created_custom_licenses << Security::CustomSoftwareLicense.upsert_all(names.map do |name|
+          { name: name, project_id: project.id }
+        end, unique_by: [:project_id, :name], returning: %w[name id])
       end
 
-      result = software_license_policies(licenses)
+      created_custom_licenses.flatten.pluck('name', 'id').to_h # rubocop:disable CodeReuse/ActiveRecord, Database/AvoidUsingPluckWithoutLimit -- Array#pluck
+    end
 
-      result.each_slice(BATCH_SIZE) do |batch|
+    def create_software_license_policies(created_custom_licenses, existing_licenses)
+      software_license_policies =
+        software_license_policies_to_insert(software_licenses: existing_licenses) +
+        software_license_policies_to_insert(custom_software_licenses: created_custom_licenses)
+
+      software_license_policies.each_slice(BATCH_SIZE) do |batch|
         SoftwareLicensePolicy.insert_all(batch)
       end
-
-      success(software_license_policy: result)
     end
 
-    private
+    def software_license_policies_to_insert(software_licenses: nil, custom_software_licenses: nil)
+      attributes_to_reject = %w[id created_at updated_at approval_policy_rule_id]
 
-    def software_license_policies(licenses)
       params.filter_map do |policy_params|
+        license_name = policy_params[:name].strip
+
         record = SoftwareLicensePolicy.new(
           project_id: project.id,
-          software_license_id: licenses[policy_params[:name].strip],
+          # software_license_id will be nil if software_licenses is nil
+          software_license_id: software_licenses && software_licenses[license_name],
+          # custom_software_license_id will be nil if custom_software_licenses is nil
+          custom_software_license_id: custom_software_licenses && custom_software_licenses[license_name],
           classification: policy_params[:approval_status],
           scan_result_policy_id: policy_params[:scan_result_policy_read]&.id
         )
 
         next if record.scan_result_policy_id.nil? || record.invalid?
 
-        record.attributes.compact
-      end
-    end
-
-    def create_unknown_licenses
-      license_names.each_slice(BATCH_SIZE) do |names|
-        SoftwareLicense.upsert_all(names.map { |l| { name: l } }, unique_by: :name, returning: %w[name id])
+        # Ensure the same keys for bulk insert
+        record.attributes.reject! { |k| attributes_to_reject.include?(k) }
       end
     end
-
-    def license_names
-      params.map { |license| license.with_indifferent_access[:name].strip }
-    end
   end
 end

ee/spec/models/security/custom_software_license_spec.rb

@@ -14,4 +14,17 @@
     it { is_expected.to validate_length_of(:name).is_at_most(255) }
     it { is_expected.to(validate_uniqueness_of(:name).scoped_to(%i[project_id])) }
   end
+
+  describe 'scopes' do
+    let_it_be(:custom_license) { create(:custom_software_license, name: 'CustomLicense') }
+    let_it_be(:other_custom_license) { create(:custom_software_license, name: 'OtherCustomLicense') }
+
+    describe '.by_name' do
+      it { expect(described_class.by_name(custom_license.name)).to contain_exactly(custom_license) }
+    end
+
+    describe '.by_project' do
+      it { expect(described_class.by_project(custom_license.project)).to contain_exactly(custom_license) }
+    end
+  end
 end

ee/spec/services/software_license_policies/bulk_create_scan_result_policy_service_spec.rb

@@ -17,11 +17,7 @@
 
   describe '#execute', :aggregate_failures do
     context 'when valid parameters are specified' do
-      it 'creates missing software licenses' do
-        expect { execute_service }.to change { SoftwareLicense.count }.by(3)
-      end
-
-      it 'creates one software license policy correctly' do
+      it 'creates software license policies correctly' do
         result = execute_service
         created_policy = SoftwareLicensePolicy.find_by(result[:software_license_policy].first)
 
@@ -38,20 +34,60 @@
         )
       end
 
-      it 'inserts software licenses and license policies in batches' do
+      it 'inserts custom software licenses and license policies in batches' do
         stub_const("#{described_class.name}::BATCH_SIZE", 2)
 
         query_recorder = ActiveRecord::QueryRecorder.new { execute_service }
 
-        license_queries = query_recorder.log.count { |q| q.include?('INSERT INTO "software_licenses"') }
+        license_queries = query_recorder.log.count { |q| q.include?('INSERT INTO "custom_software_licenses"') }
         policy_queries = query_recorder.log.count { |q| q.include?('INSERT INTO "software_license_policies"') }
 
         expect(license_queries).to eq(2)
         expect(policy_queries).to eq(2)
       end
 
-      context 'when name contains whitespaces' do
-        let(:params) { [{ name: '  MIT   ', approval_status: 'allowed', scan_result_policy_read: scan_result_policy }] }
+      it 'does not create software licenses' do
+        expect { execute_service }.not_to change { SoftwareLicense.count }
+      end
+
+      shared_examples 'creates missing licenses as custom software licenses' do |missing_licenses_count|
+        it 'creates missing licenses as custom software licenses' do
+          expect { execute_service }.to change { Security::CustomSoftwareLicense.count }.by(missing_licenses_count)
+        end
+      end
+
+      context 'when no software license matching the name exists' do
+        it_behaves_like 'creates missing licenses as custom software licenses', 3
+      end
+
+      context 'when a software license matching the name exists' do
+        before do
+          create(:software_license, name: 'MIT', spdx_identifier: 'MIT')
+        end
+
+        it_behaves_like 'creates missing licenses as custom software licenses', 2
+      end
+
+      context 'when a custom software license matching the name exists in the same project' do
+        before do
+          create(:custom_software_license, name: 'ExamplePL/2.1', project: project)
+        end
+
+        it_behaves_like 'creates missing licenses as custom software licenses', 2
+      end
+
+      context 'when a custom software license matching the name exists in the another project' do
+        before do
+          create(:custom_software_license, name: 'ExamplePL/2.1')
+        end
+
+        it_behaves_like 'creates missing licenses as custom software licenses', 3
+      end
+
+      context 'when the license name contains whitespaces' do
+        let(:params) do
+          [{ name: '  NOT MIT   ', approval_status: 'allowed', scan_result_policy_read: scan_result_policy }]
+        end
 
         it 'creates one software license policy with stripped name' do
           result = execute_service
@@ -59,7 +95,7 @@
 
           expect(project.software_license_policies.count).to be(1)
           expect(result[:status]).to be(:success)
-          expect(created_policy.software_license.name).to eq('MIT')
+          expect(created_policy.custom_software_license.name).to eq('NOT MIT')
         end
       end
     end

ee/spec/support/helpers/features/security_policy_helpers.rb

@@ -12,6 +12,8 @@ def create_security_policy
 
     def create_policy_setup
       stub_feature_flags(merge_when_checks_pass: false)
+      stub_feature_flags(bulk_create_scan_result_policies: false)
+      stub_feature_flags(custom_software_license: false)
       stub_licensed_features(security_dashboard: true,
         multiple_approval_rules: true,
         sast: true, report_approver_rules: true,