Merge branch '38068-commits-count' into 'master'

Resolve "Store the number of merge request commits in merge_request_diffs.commits_count"

Closes #38068

See merge request gitlab-org/gitlab-ce!16164

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

changelogs/unreleased/38068-commits-count.yml

+---
+title: Store number of commits in merge_request_diffs table.
+merge_request:
+author:
+type: performance

db/migrate/20180105212544_add_commits_count_to_merge_request_diff.rb

+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

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

lib/gitlab/background_migration/add_merge_request_diff_commits_count.rb

+# 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

spec/lib/gitlab/background_migration/add_merge_request_diff_commits_count_spec.rb

+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

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

spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb

 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) }

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

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