From f1d3f26cff2e9679ef88b82411f647e27068dbea Mon Sep 17 00:00:00 2001 From: Matthias Kaeppler Date: Fri, 31 Jan 2020 14:19:19 +0100 Subject: [PATCH 1/3] BulkInsertSafe mixin guards against bulk-unsafe calls This mixin overrides certain active record calls that modify the callback chain in ways incompatible with bulk inserts --- app/models/concerns/bulk_insert_safe.rb | 50 +++++++++++++++++++ app/models/label_link.rb | 1 + app/models/merge_request_diff_commit.rb | 1 + app/models/merge_request_diff_file.rb | 1 + spec/models/concerns/bulk_insert_safe_spec.rb | 38 ++++++++++++++ spec/models/label_link_spec.rb | 2 + spec/models/merge_request_diff_commit_spec.rb | 2 + spec/models/merge_request_diff_file_spec.rb | 2 + .../bulk_insert_safe_shared_examples.rb | 31 ++++++++++++ 9 files changed, 128 insertions(+) create mode 100644 app/models/concerns/bulk_insert_safe.rb create mode 100644 spec/models/concerns/bulk_insert_safe_spec.rb create mode 100644 spec/support/shared_examples/models/concerns/bulk_insert_safe_shared_examples.rb diff --git a/app/models/concerns/bulk_insert_safe.rb b/app/models/concerns/bulk_insert_safe.rb new file mode 100644 index 00000000000000..f4cfa5fffc0a28 --- /dev/null +++ b/app/models/concerns/bulk_insert_safe.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module BulkInsertSafe + extend ActiveSupport::Concern + extend self + + # 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) + bis = BulkInsertSafe + + unless bis._bulk_insert_callback_allowed?(name, args) + bis._bulk_insert_raise_not_allowed("set_callback(#{name}, #{args})") + end + + super + end + end + + def _bulk_insert_callback_allowed?(name, args) + _bulk_insert_whitelisted?(name) || _bulk_insert_saved_from_belongs_to?(name, args) + end + + def _bulk_insert_raise_not_allowed(invocation) + raise MethodNotAllowedError.new( + "Not allowed to call `#{invocation}` when model extends `BulkInsertSafe`") + end + + private + + # 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 diff --git a/app/models/label_link.rb b/app/models/label_link.rb index ffc0afd8e8564e..5ae1e88e14eec7 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 aa41a68f184246..460b394f06749c 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 14c86ec69da560..23319445a38b28 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 00000000000000..918846807385a8 --- /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 b160e72e759625..0a5cb5374b045f 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 a74c0b2064220a..a296122ae09e2c 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 84f9c9d06ba458..6ecbc5bf832146 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 00000000000000..3f33670761f671 --- /dev/null +++ b/spec/support/shared_examples/models/concerns/bulk_insert_safe_shared_examples.rb @@ -0,0 +1,31 @@ +# 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.) + CALLBACK_METHOD_BLACKLIST = 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 + + 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 -- GitLab From 30c46c83fa5105468f8af42145046ac41758785c Mon Sep 17 00:00:00 2001 From: Matthias Kaeppler Date: Tue, 11 Feb 2020 16:50:23 +0100 Subject: [PATCH 2/3] Move mixin helpers back to class scope --- app/models/concerns/bulk_insert_safe.rb | 36 +++++++++++-------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/app/models/concerns/bulk_insert_safe.rb b/app/models/concerns/bulk_insert_safe.rb index f4cfa5fffc0a28..6d75906b21fc79 100644 --- a/app/models/concerns/bulk_insert_safe.rb +++ b/app/models/concerns/bulk_insert_safe.rb @@ -2,7 +2,6 @@ module BulkInsertSafe extend ActiveSupport::Concern - extend self # These are the callbacks we think safe when used on models that are # written to the database in bulk @@ -18,33 +17,28 @@ module BulkInsertSafe class_methods do def set_callback(name, *args) - bis = BulkInsertSafe - - unless bis._bulk_insert_callback_allowed?(name, args) - bis._bulk_insert_raise_not_allowed("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 - end - def _bulk_insert_callback_allowed?(name, args) - _bulk_insert_whitelisted?(name) || _bulk_insert_saved_from_belongs_to?(name, args) - end - - def _bulk_insert_raise_not_allowed(invocation) - raise MethodNotAllowedError.new( - "Not allowed to call `#{invocation}` when model extends `BulkInsertSafe`") - end + private - 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 + # 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) + def _bulk_insert_whitelisted?(name) + CALLBACK_NAME_WHITELIST.include?(name) + end end end -- GitLab From a9e669632a2200df99ff84b6da54d9623eb81e21 Mon Sep 17 00:00:00 2001 From: Matthias Kaeppler Date: Wed, 12 Feb 2020 17:04:27 +0100 Subject: [PATCH 3/3] Prefer let block over constant in spec --- .../concerns/bulk_insert_safe_shared_examples.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) 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 index 3f33670761f671..0a2b6616bb4dd1 100644 --- 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 @@ -3,14 +3,16 @@ 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.) - CALLBACK_METHOD_BLACKLIST = 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 + 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| + 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" -- GitLab