diff --git a/app/models/concerns/bulk_insert_safe.rb b/app/models/concerns/bulk_insert_safe.rb new file mode 100644 index 0000000000000000000000000000000000000000..6d75906b21fc79244b75e91a6a1095f05933741c --- /dev/null +++ b/app/models/concerns/bulk_insert_safe.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module BulkInsertSafe + extend ActiveSupport::Concern + + # These are the callbacks we think safe when used on models that are + # written to the database in bulk + CALLBACK_NAME_WHITELIST = Set[ + :initialize, + :validate, + :validation, + :find, + :destroy + ].freeze + + MethodNotAllowedError = Class.new(StandardError) + + class_methods do + def set_callback(name, *args) + unless _bulk_insert_callback_allowed?(name, args) + raise MethodNotAllowedError.new( + "Not allowed to call `set_callback(#{name}, #{args})` when model extends `BulkInsertSafe`." \ + "Callbacks that fire per each record being inserted do not work with bulk-inserts.") + end + + super + end + + private + + def _bulk_insert_callback_allowed?(name, args) + _bulk_insert_whitelisted?(name) || _bulk_insert_saved_from_belongs_to?(name, args) + end + + # belongs_to associations will install a before_save hook during class loading + def _bulk_insert_saved_from_belongs_to?(name, args) + args.first == :before && args.second.to_s.start_with?('autosave_associated_records_for_') + end + + def _bulk_insert_whitelisted?(name) + CALLBACK_NAME_WHITELIST.include?(name) + end + end +end diff --git a/app/models/label_link.rb b/app/models/label_link.rb index ffc0afd8e8564eae11b7bbfea84a59eaaa9480ca..5ae1e88e14eec7412da9e54952cf0dc0a903f73f 100644 --- a/app/models/label_link.rb +++ b/app/models/label_link.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class LabelLink < ApplicationRecord + include BulkInsertSafe include Importable belongs_to :target, polymorphic: true, inverse_of: :label_links # rubocop:disable Cop/PolymorphicAssociations diff --git a/app/models/merge_request_diff_commit.rb b/app/models/merge_request_diff_commit.rb index aa41a68f18424657e1570575ee81690354586069..460b394f06749cfcbd5ea577454ecadeb6af2e62 100644 --- a/app/models/merge_request_diff_commit.rb +++ b/app/models/merge_request_diff_commit.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class MergeRequestDiffCommit < ApplicationRecord + include BulkInsertSafe include ShaAttribute include CachedCommit diff --git a/app/models/merge_request_diff_file.rb b/app/models/merge_request_diff_file.rb index 14c86ec69da560396fefc8bc55b09271b1e63ed3..23319445a38b2834a41a2f5cc4f57172720a7004 100644 --- a/app/models/merge_request_diff_file.rb +++ b/app/models/merge_request_diff_file.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class MergeRequestDiffFile < ApplicationRecord + include BulkInsertSafe include Gitlab::EncodingHelper include DiffFile diff --git a/spec/models/concerns/bulk_insert_safe_spec.rb b/spec/models/concerns/bulk_insert_safe_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..918846807385a8ea8db18bd08f87ae150373ee11 --- /dev/null +++ b/spec/models/concerns/bulk_insert_safe_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe BulkInsertSafe do + class BulkInsertItem < ApplicationRecord + include BulkInsertSafe + end + + module InheritedUnsafeMethods + extend ActiveSupport::Concern + + included do + after_save -> { "unsafe" } + end + end + + module InheritedSafeMethods + extend ActiveSupport::Concern + + included do + after_initialize -> { "safe" } + end + end + + it_behaves_like 'a BulkInsertSafe model', BulkInsertItem + + context 'when inheriting class methods' do + it 'raises an error when method is not bulk-insert safe' do + expect { BulkInsertItem.include(InheritedUnsafeMethods) }.to( + raise_error(subject::MethodNotAllowedError)) + end + + it 'does not raise an error when method is bulk-insert safe' do + expect { BulkInsertItem.include(InheritedSafeMethods) }.not_to raise_error + end + end +end diff --git a/spec/models/label_link_spec.rb b/spec/models/label_link_spec.rb index b160e72e75962598f700b8493f95124cbb67b118..0a5cb5374b045f707932c5ad95c7229f54571d2a 100644 --- a/spec/models/label_link_spec.rb +++ b/spec/models/label_link_spec.rb @@ -7,4 +7,6 @@ it { is_expected.to belong_to(:label) } it { is_expected.to belong_to(:target) } + + it_behaves_like 'a BulkInsertSafe model', LabelLink end diff --git a/spec/models/merge_request_diff_commit_spec.rb b/spec/models/merge_request_diff_commit_spec.rb index a74c0b2064220a819246d08c67bb030bca5b6aac..a296122ae09e2c3c635eb429849f3b3756c4b51b 100644 --- a/spec/models/merge_request_diff_commit_spec.rb +++ b/spec/models/merge_request_diff_commit_spec.rb @@ -6,6 +6,8 @@ let(:merge_request) { create(:merge_request) } let(:project) { merge_request.project } + it_behaves_like 'a BulkInsertSafe model', MergeRequestDiffCommit + describe '#to_hash' do subject { merge_request.commits.first } diff --git a/spec/models/merge_request_diff_file_spec.rb b/spec/models/merge_request_diff_file_spec.rb index 84f9c9d06ba4580ae5958583cc729ed032bd8e88..6ecbc5bf832146922b7ee3aaef02eb7b1b2c0ddf 100644 --- a/spec/models/merge_request_diff_file_spec.rb +++ b/spec/models/merge_request_diff_file_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe MergeRequestDiffFile do + it_behaves_like 'a BulkInsertSafe model', MergeRequestDiffFile + describe '#diff' do context 'when diff is not stored' do let(:unpacked) { 'unpacked' } diff --git a/spec/support/shared_examples/models/concerns/bulk_insert_safe_shared_examples.rb b/spec/support/shared_examples/models/concerns/bulk_insert_safe_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..0a2b6616bb4dd1a0ef58cf9c306beff499e47726 --- /dev/null +++ b/spec/support/shared_examples/models/concerns/bulk_insert_safe_shared_examples.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'a BulkInsertSafe model' do |target_class| + # We consider all callbacks unsafe for bulk insertions unless we have explicitly + # whitelisted them (esp. anything related to :save, :create, :commit etc.) + let(:callback_method_blacklist) do + ActiveRecord::Callbacks::CALLBACKS.reject do |callback| + cb_name = callback.to_s.gsub(/(before_|after_|around_)/, '').to_sym + BulkInsertSafe::CALLBACK_NAME_WHITELIST.include?(cb_name) + end.to_set + end + + context 'when calling class methods directly' do + it 'raises an error when method is not bulk-insert safe' do + callback_method_blacklist.each do |m| + expect { target_class.send(m, nil) }.to( + raise_error(BulkInsertSafe::MethodNotAllowedError), + "Expected call to #{m} to raise an error, but it didn't" + ) + end + end + + it 'does not raise an error when method is bulk-insert safe' do + BulkInsertSafe::CALLBACK_NAME_WHITELIST.each do |name| + expect { target_class.set_callback(name) {} }.not_to raise_error + end + end + + it 'does not raise an error when the call is triggered by belongs_to' do + expect { target_class.belongs_to(:other_record) }.not_to raise_error + end + end +end