diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-03-07 18:08:21 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-03-07 18:08:21 +0000 |
commit | 996d54a81d799e6a69098b1e397a4ee7ea6d200c (patch) | |
tree | fffa63f837935d38b070b84eb6753f2cf60533de | |
parent | 73f25276606bed7d822153814de9467900aac244 (diff) | |
download | gitlab-ce-996d54a81d799e6a69098b1e397a4ee7ea6d200c.tar.gz |
Add latest changes from gitlab-org/gitlab@master
-rw-r--r-- | app/models/concerns/bulk_insertable_associations.rb | 112 | ||||
-rw-r--r-- | app/models/merge_request_diff.rb | 1 | ||||
-rw-r--r-- | lib/gitlab/import_export/relation_tree_restorer.rb | 16 | ||||
-rw-r--r-- | spec/models/concerns/bulk_insertable_associations_spec.rb | 237 |
4 files changed, 360 insertions, 6 deletions
diff --git a/app/models/concerns/bulk_insertable_associations.rb b/app/models/concerns/bulk_insertable_associations.rb new file mode 100644 index 00000000000..35bdabbcefd --- /dev/null +++ b/app/models/concerns/bulk_insertable_associations.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +## +# ActiveRecord model classes can mix in this concern if they own associations +# who declare themselves to be eligible for bulk-insertion via [BulkInsertSafe]. +# This allows the caller to write items from [has_many] associations en-bloc +# when the owner is first created. +# +# This implementation currently has a few limitations: +# - only works for [has_many] relations +# - does not support the [:through] option +# - it cannot bulk-insert items that had previously been saved, nor can the +# owner of the association have previously been saved; if you attempt to +# so, an error will be raised +# +# @example +# +# class MergeRequestDiff < ApplicationRecord +# include BulkInsertableAssociations +# +# # target association class must `include BulkInsertSafe` +# has_many :merge_request_diff_commits +# end +# +# diff = MergeRequestDiff.new(...) +# diff.diff_commits << MergeRequestDiffCommit.build(...) +# BulkInsertableAssociations.with_bulk_insert do +# diff.save! # this will also write all `diff_commits` in bulk +# end +# +# Note that just like [BulkInsertSafe.bulk_insert!], validations will run for +# all items that are scheduled for bulk-insertions. +# +module BulkInsertableAssociations + extend ActiveSupport::Concern + + class << self + def bulk_inserts_enabled? + Thread.current['bulk_inserts_enabled'] + end + + # All associations that are [BulkInsertSafe] and that as a result of calls to + # [save] or [save!] would be written to the database, will be inserted using + # [bulk_insert!] instead. + # + # Note that this will only work for entities that have not been persisted yet. + # + # @param [Boolean] enabled When [true], bulk-inserts will be attempted within + # the given block. If [false], bulk-inserts will be + # disabled. This behavior can be nested. + def with_bulk_insert(enabled: true) + previous = bulk_inserts_enabled? + Thread.current['bulk_inserts_enabled'] = enabled + yield + ensure + Thread.current['bulk_inserts_enabled'] = previous + end + end + + def bulk_insert_associations! + self.class.reflections.each do |_, reflection| + _bulk_insert_association!(reflection) + end + end + + private + + def _bulk_insert_association!(reflection) + return unless _association_supports_bulk_inserts?(reflection) + + association = self.association(reflection.name) + association_items = association.target + return if association_items.empty? + + if association_items.any?(&:persisted?) + raise 'Bulk-insertion of already persisted association items is not currently supported' + end + + _bulk_insert_configure_foreign_key(reflection, association_items) + association.klass.bulk_insert!(association_items, validate: false) + + # reset relation: + # 1. we successfully inserted all items + # 2. when accessed we force to reload the relation + association.reset + end + + def _association_supports_bulk_inserts?(reflection) + reflection.macro == :has_many && + reflection.klass < BulkInsertSafe && + !reflection.through_reflection? && + association_cached?(reflection.name) + end + + def _bulk_insert_configure_foreign_key(reflection, items) + primary_key = self[reflection.active_record_primary_key] + raise "Classes including `BulkInsertableAssociations` must define a `primary_key`" unless primary_key + + items.each do |item| + item[reflection.foreign_key] = primary_key + + if reflection.type + item[reflection.type] = self.class.polymorphic_name + end + end + end + + included do + delegate :bulk_inserts_enabled?, to: BulkInsertableAssociations + after_create :bulk_insert_associations!, if: :bulk_inserts_enabled?, prepend: true + end +end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index ffe95e8f034..fe769573e29 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -7,6 +7,7 @@ class MergeRequestDiff < ApplicationRecord include EachBatch include Gitlab::Utils::StrongMemoize include ObjectStorage::BackgroundMove + include BulkInsertableAssociations # Don't display more than 100 commits at once COMMITS_SAFE_SIZE = 100 diff --git a/lib/gitlab/import_export/relation_tree_restorer.rb b/lib/gitlab/import_export/relation_tree_restorer.rb index e0d6ade51c2..8359eefc846 100644 --- a/lib/gitlab/import_export/relation_tree_restorer.rb +++ b/lib/gitlab/import_export/relation_tree_restorer.rb @@ -26,8 +26,13 @@ module Gitlab ActiveRecord::Base.uncached do ActiveRecord::Base.no_touching do update_params! - update_relation_hashes! - create_relations! + + bulk_inserts_enabled = @importable.class == ::Project && + Feature.enabled?(:import_bulk_inserts, @importable.group) + BulkInsertableAssociations.with_bulk_insert(enabled: bulk_inserts_enabled) do + update_relation_hashes! + create_relations! + end end end @@ -170,7 +175,7 @@ module Gitlab # if object is a hash we can create simple object # as it means that this is 1-to-1 vs 1-to-many - sub_data_hash = + current_item = if sub_data_hash.is_a?(Array) build_relations( sub_relation_key, @@ -183,9 +188,8 @@ module Gitlab sub_data_hash) end - # persist object(s) or delete from relation - if sub_data_hash - data_hash[sub_relation_key] = sub_data_hash + if current_item + data_hash[sub_relation_key] = current_item else data_hash.delete(sub_relation_key) end diff --git a/spec/models/concerns/bulk_insertable_associations_spec.rb b/spec/models/concerns/bulk_insertable_associations_spec.rb new file mode 100644 index 00000000000..9e584417697 --- /dev/null +++ b/spec/models/concerns/bulk_insertable_associations_spec.rb @@ -0,0 +1,237 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe BulkInsertableAssociations do + class BulkFoo < ApplicationRecord + include BulkInsertSafe + + validates :name, presence: true + end + + class BulkBar < ApplicationRecord + include BulkInsertSafe + end + + SimpleBar = Class.new(ApplicationRecord) + + class BulkParent < ApplicationRecord + include BulkInsertableAssociations + + has_many :bulk_foos + has_many :bulk_hunks, class_name: 'BulkFoo' + has_many :bulk_bars + has_many :simple_bars # not `BulkInsertSafe` + has_one :bulk_foo # not supported + end + + before(:all) do + ActiveRecord::Schema.define do + create_table :bulk_parents, force: true do |t| + t.string :name, null: true + end + + create_table :bulk_foos, force: true do |t| + t.string :name, null: true + t.belongs_to :bulk_parent, null: false + end + + create_table :bulk_bars, force: true do |t| + t.string :name, null: true + t.belongs_to :bulk_parent, null: false + end + + create_table :simple_bars, force: true do |t| + t.string :name, null: true + t.belongs_to :bulk_parent, null: false + end + end + end + + after(:all) do + ActiveRecord::Schema.define do + drop_table :bulk_foos, force: true + drop_table :bulk_bars, force: true + drop_table :simple_bars, force: true + drop_table :bulk_parents, force: true + end + end + + before do + ActiveRecord::Base.connection.execute('TRUNCATE bulk_foos RESTART IDENTITY') + end + + context 'saving bulk insertable associations' do + let(:parent) { BulkParent.new(name: 'parent') } + + context 'when items already have IDs' do + it 'stores nothing and raises an error' do + build_items(parent: parent) { |n, item| item.id = 100 + n } + + expect { save_with_bulk_inserts(parent) }.to raise_error(BulkInsertSafe::PrimaryKeySetError) + expect(BulkFoo.count).to eq(0) + end + end + + context 'when items have no IDs set' do + it 'stores them all and updates items with IDs' do + items = build_items(parent: parent) + + expect(BulkFoo).to receive(:bulk_insert!).once.and_call_original + expect { save_with_bulk_inserts(parent) }.to change { BulkFoo.count }.from(0).to(items.size) + expect(parent.bulk_foos.pluck(:id)).to contain_exactly(*(1..10)) + end + end + + context 'when items are empty' do + it 'does nothing' do + expect(parent.bulk_foos).to be_empty + + expect { save_with_bulk_inserts(parent) }.not_to change { BulkFoo.count } + end + end + + context 'when relation name does not match class name' do + it 'stores them all' do + items = build_items(parent: parent, relation: :bulk_hunks) + + expect(BulkFoo).to receive(:bulk_insert!).once.and_call_original + + expect { save_with_bulk_inserts(parent) }.to( + change { BulkFoo.count }.from(0).to(items.size) + ) + end + end + + context 'with multiple threads' do + it 'isolates bulk insert behavior between threads' do + total_item_count = 10 + parent1 = BulkParent.new(name: 'parent1') + parent2 = BulkParent.new(name: 'parent2') + build_items(parent: parent1, count: total_item_count / 2) + build_items(parent: parent2, count: total_item_count / 2) + + expect(BulkFoo).to receive(:bulk_insert!).once.and_call_original + [ + Thread.new do + save_with_bulk_inserts(parent1) + end, + Thread.new do + parent2.save! + end + ].map(&:join) + + expect(BulkFoo.count).to eq(total_item_count) + end + end + + context 'with multiple associations' do + it 'isolates writes between associations' do + items1 = build_items(parent: parent, relation: :bulk_foos) + items2 = build_items(parent: parent, relation: :bulk_bars) + + expect(BulkFoo).to receive(:bulk_insert!).once.and_call_original + expect(BulkBar).to receive(:bulk_insert!).once.and_call_original + + expect { save_with_bulk_inserts(parent) }.to( + change { BulkFoo.count }.from(0).to(items1.size) + .and( + change { BulkBar.count }.from(0).to(items2.size) + )) + end + end + + context 'passing bulk insert arguments' do + it 'disables validations on target association' do + items = build_items(parent: parent) + + expect(BulkFoo).to receive(:bulk_insert!).with(items, validate: false).and_return true + + save_with_bulk_inserts(parent) + end + end + + it 'can disable bulk-inserts within a bulk-insert block' do + parent1 = BulkParent.new(name: 'parent1') + parent2 = BulkParent.new(name: 'parent2') + _items1 = build_items(parent: parent1) + items2 = build_items(parent: parent2) + + expect(BulkFoo).to receive(:bulk_insert!).once.with(items2, validate: false) + + BulkInsertableAssociations.with_bulk_insert(enabled: true) do + BulkInsertableAssociations.with_bulk_insert(enabled: false) do + parent1.save! + end + + parent2.save! + end + end + + context 'when association is not bulk-insert safe' do + it 'saves it normally' do + parent.simple_bars.build + + expect(SimpleBar).not_to receive(:bulk_insert!) + expect { save_with_bulk_inserts(parent) }.to change { SimpleBar.count }.from(0).to(1) + end + end + + context 'when association is not has_many' do + it 'saves it normally' do + parent.bulk_foo = BulkFoo.new(name: 'item') + + expect(BulkFoo).not_to receive(:bulk_insert!) + expect { save_with_bulk_inserts(parent) }.to change { BulkFoo.count }.from(0).to(1) + end + end + + context 'when an item is not valid' do + describe '.save' do + it 'invalidates the parent and returns false' do + build_invalid_items(parent: parent) + + expect(save_with_bulk_inserts(parent, bangify: false)).to be false + expect(parent.errors[:bulk_foos].size).to eq(1) + + expect(BulkFoo.count).to eq(0) + expect(BulkParent.count).to eq(0) + end + end + + describe '.save!' do + it 'invalidates the parent and raises error' do + build_invalid_items(parent: parent) + + expect { save_with_bulk_inserts(parent) }.to raise_error(ActiveRecord::RecordInvalid) + expect(parent.errors[:bulk_foos].size).to eq(1) + + expect(BulkFoo.count).to eq(0) + expect(BulkParent.count).to eq(0) + end + end + end + end + + private + + def save_with_bulk_inserts(entity, bangify: true) + BulkInsertableAssociations.with_bulk_insert { bangify ? entity.save! : entity.save } + end + + def build_items(parent:, relation: :bulk_foos, count: 10) + count.times do |n| + item = parent.send(relation).build(name: "item_#{n}", bulk_parent_id: parent.id) + yield(n, item) if block_given? + end + parent.send(relation) + end + + def build_invalid_items(parent:) + build_items(parent: parent).tap do |items| + invalid_item = items.first + invalid_item.name = nil + expect(invalid_item).not_to be_valid + end + end +end |