diff options
10 files changed, 137 insertions, 5 deletions
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index e35de9b97ee..afab72930c1 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -49,6 +49,7 @@ class MergeRequestDiff < ActiveRecord::Base ensure_commit_shas save_commits save_diffs + save keep_around_commits end @@ -56,7 +57,6 @@ class MergeRequestDiff < ActiveRecord::Base self.start_commit_sha ||= merge_request.target_branch_sha self.head_commit_sha ||= merge_request.source_branch_sha self.base_commit_sha ||= find_base_sha - save end # Override head_commit_sha to keep compatibility with merge request diff @@ -195,7 +195,7 @@ class MergeRequestDiff < ActiveRecord::Base end def commits_count - merge_request_diff_commits.size + super || merge_request_diff_commits.size end private @@ -264,13 +264,16 @@ class MergeRequestDiff < ActiveRecord::Base new_attributes[:state] = :overflow if diff_collection.overflow? end - update(new_attributes) + assign_attributes(new_attributes) end def save_commits MergeRequestDiffCommit.create_bulk(self.id, compare.commits.reverse) - merge_request_diff_commits.reload + # merge_request_diff_commits.reload is preferred way to reload associated + # objects but it returns cached result for some reason in this case + commits = merge_request_diff_commits(true) + self.commits_count = commits.size end def repository diff --git a/changelogs/unreleased/38068-commits-count.yml b/changelogs/unreleased/38068-commits-count.yml new file mode 100644 index 00000000000..3fbf554c98c --- /dev/null +++ b/changelogs/unreleased/38068-commits-count.yml @@ -0,0 +1,5 @@ +--- +title: Store number of commits in merge_request_diffs table. +merge_request: +author: +type: performance diff --git a/db/migrate/20180105212544_add_commits_count_to_merge_request_diff.rb b/db/migrate/20180105212544_add_commits_count_to_merge_request_diff.rb new file mode 100644 index 00000000000..f942b4c062e --- /dev/null +++ b/db/migrate/20180105212544_add_commits_count_to_merge_request_diff.rb @@ -0,0 +1,29 @@ +class AddCommitsCountToMergeRequestDiff < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + MIGRATION = 'AddMergeRequestDiffCommitsCount'.freeze + BATCH_SIZE = 5000 + DELAY_INTERVAL = 5.minutes.to_i + + class MergeRequestDiff < ActiveRecord::Base + self.table_name = 'merge_request_diffs' + + include ::EachBatch + end + + disable_ddl_transaction! + + def up + add_column :merge_request_diffs, :commits_count, :integer + + say 'Populating the MergeRequestDiff `commits_count`' + + queue_background_migration_jobs_by_range_at_intervals(MergeRequestDiff, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) + end + + def down + remove_column :merge_request_diffs, :commits_count + end +end diff --git a/db/schema.rb b/db/schema.rb index a16f756ccfb..07620349057 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20171230123729) do +ActiveRecord::Schema.define(version: 20180105212544) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1044,6 +1044,7 @@ ActiveRecord::Schema.define(version: 20171230123729) do t.string "real_size" t.string "head_commit_sha" t.string "start_commit_sha" + t.integer "commits_count" end add_index "merge_request_diffs", ["merge_request_id", "id"], name: "index_merge_request_diffs_on_merge_request_id_and_id", using: :btree diff --git a/lib/gitlab/background_migration/add_merge_request_diff_commits_count.rb b/lib/gitlab/background_migration/add_merge_request_diff_commits_count.rb new file mode 100644 index 00000000000..7bffffec94d --- /dev/null +++ b/lib/gitlab/background_migration/add_merge_request_diff_commits_count.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true +# rubocop:disable Style/Documentation +# rubocop:disable Metrics/LineLength + +module Gitlab + module BackgroundMigration + class AddMergeRequestDiffCommitsCount + class MergeRequestDiff < ActiveRecord::Base + self.table_name = 'merge_request_diffs' + end + + def perform(start_id, stop_id) + Rails.logger.info("Setting commits_count for merge request diffs: #{start_id} - #{stop_id}") + + update = ' + commits_count = ( + SELECT count(*) + FROM merge_request_diff_commits + WHERE merge_request_diffs.id = merge_request_diff_commits.merge_request_diff_id + )'.squish + + MergeRequestDiff.where(id: start_id..stop_id).update_all(update) + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/add_merge_request_diff_commits_count_spec.rb b/spec/lib/gitlab/background_migration/add_merge_request_diff_commits_count_spec.rb new file mode 100644 index 00000000000..21a791f5695 --- /dev/null +++ b/spec/lib/gitlab/background_migration/add_merge_request_diff_commits_count_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' + +describe Gitlab::BackgroundMigration::AddMergeRequestDiffCommitsCount, :migration, schema: 20180105212544 do + let(:projects_table) { table(:projects) } + let(:merge_requests_table) { table(:merge_requests) } + let(:merge_request_diffs_table) { table(:merge_request_diffs) } + let(:merge_request_diff_commits_table) { table(:merge_request_diff_commits) } + + let(:project) { projects_table.create!(name: 'gitlab', path: 'gitlab-org/gitlab-ce') } + let(:merge_request) do + merge_requests_table.create!(target_project_id: project.id, + target_branch: 'master', + source_project_id: project.id, + source_branch: 'mr name', + title: 'mr name') + end + + def create_diff!(name, commits: 0) + mr_diff = merge_request_diffs_table.create!( + merge_request_id: merge_request.id) + + commits.times do |i| + merge_request_diff_commits_table.create!( + merge_request_diff_id: mr_diff.id, + relative_order: i, sha: i) + end + + mr_diff + end + + describe '#perform' do + it 'migrates diffs that have no commits' do + diff = create_diff!('with_multiple_commits', commits: 0) + + subject.perform(diff.id, diff.id) + + expect(diff.reload.commits_count).to eq(0) + end + + it 'migrates multiple diffs to the correct values' do + diffs = Array.new(3).map.with_index { |_, i| create_diff!(i, commits: 3) } + + subject.perform(diffs.first.id, diffs.last.id) + + diffs.each do |diff| + expect(diff.reload.commits_count).to eq(3) + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb b/spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb index 84d9e635810..98730602863 100644 --- a/spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb +++ b/spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb @@ -10,6 +10,11 @@ describe Gitlab::BackgroundMigration::DeserializeMergeRequestDiffsAndCommits, :t let(:merge_request_diff) { MergeRequest.find(merge_request.id).create_merge_request_diff } let(:updated_merge_request_diff) { MergeRequestDiff.find(merge_request_diff.id) } + before do + allow_any_instance_of(MergeRequestDiff) + .to receive(:commits_count=).and_return(nil) + end + def diffs_to_hashes(diffs) diffs.as_json(only: Gitlab::Git::Diff::SERIALIZE_KEYS).map(&:with_indifferent_access) end diff --git a/spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb b/spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb index dfe3b31f1c0..e99257e3481 100644 --- a/spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb @@ -1,6 +1,12 @@ require 'rails_helper' describe Gitlab::BackgroundMigration::PopulateMergeRequestMetricsWithEventsData, :migration, schema: 20171128214150 do + # commits_count attribute is added in a next migration + before do + allow_any_instance_of(MergeRequestDiff) + .to receive(:commits_count=).and_return(nil) + end + describe '#perform' do let(:mr_with_event) { create(:merge_request) } let!(:merged_event) { create(:event, :merged, target: mr_with_event) } diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index ec577903eb5..c5bf23e65b3 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -180,6 +180,7 @@ MergeRequestDiff: - real_size - head_commit_sha - start_commit_sha +- commits_count MergeRequestDiffCommit: - merge_request_diff_id - relative_order diff --git a/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb b/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb index 2e6b2cff0ab..7494624066a 100644 --- a/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb +++ b/spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb @@ -2,6 +2,12 @@ require 'spec_helper' require Rails.root.join('db', 'post_migrate', '20171128214150_schedule_populate_merge_request_metrics_with_events_data.rb') describe SchedulePopulateMergeRequestMetricsWithEventsData, :migration, :sidekiq do + # commits_count attribute is added in a next migration + before do + allow_any_instance_of(MergeRequestDiff) + .to receive(:commits_count=).and_return(nil) + end + let!(:mrs) { create_list(:merge_request, 3) } it 'correctly schedules background migrations' do |