Fix storing first_mentioned_in_commit_at attribute

This change fixes the way we calculate and store the
first_mentioned_in_commit_at attribute. Instead of looking at the most
recent commit date, the change will check the earliest commit date
looking back a maximum of 100 commits. The change also inclides a BG
migration that attempts to fix the existing records.

Changelog: fixed

app/services/issues/close_service.rb

@@ -82,11 +82,11 @@ module Issues
       end
     end
 
-    def store_first_mentioned_in_commit_at(issue, merge_request)
+    def store_first_mentioned_in_commit_at(issue, merge_request, max_commit_lookup: 100)
       metrics = issue.metrics
       return if metrics.nil? || metrics.first_mentioned_in_commit_at
 
-      first_commit_timestamp = merge_request.commits(limit: 1).first.try(:authored_date)
+      first_commit_timestamp = merge_request.commits(limit: max_commit_lookup).last.try(:authored_date)
       return unless first_commit_timestamp
 
       metrics.update!(first_mentioned_in_commit_at: first_commit_timestamp)

db/post_migrate/20211004110500_add_temporary_index_to_issue_metrics.rb

+# frozen_string_literal: true
+
+class AddTemporaryIndexToIssueMetrics < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_issue_metrics_first_mentioned_in_commit'
+
+  def up
+    add_concurrent_index :issue_metrics, :issue_id, where: 'EXTRACT(YEAR FROM first_mentioned_in_commit_at) > 2019', name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :issue_metrics, name: INDEX_NAME
+  end
+end

db/post_migrate/20211004110927_schedule_fix_first_mentioned_in_commit_at_job.rb

+# frozen_string_literal: true
+
+class ScheduleFixFirstMentionedInCommitAtJob < Gitlab::Database::Migration[1.0]
+  MIGRATION = 'FixFirstMentionedInCommitAt'
+  BATCH_SIZE = 10_000
+  INTERVAL = 2.minutes.to_i
+
+  disable_ddl_transaction!
+
+  def up
+    scope = define_batchable_model('issue_metrics')
+      .where('EXTRACT(YEAR FROM first_mentioned_in_commit_at) > 2019')
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      scope,
+      MIGRATION,
+      INTERVAL,
+      batch_size: BATCH_SIZE,
+      track_jobs: true,
+      primary_column_name: :issue_id
+    )
+  end
+
+  def down
+    # noop
+  end
+end

db/schema_migrations/20211004110500

+1b0b562aefb724afe24b8640a22013cea6fddd0e594d6723f6819f69804ba9f7
\ No newline at end of file

db/schema_migrations/20211004110927

+50c937f979c83f6937364d92bf65ed42ef963f2d241eadcee6355c1b256c3ec9
\ No newline at end of file

db/structure.sql

@@ -25359,6 +25359,8 @@ CREATE UNIQUE INDEX index_issue_links_on_source_id_and_target_id ON issue_links
 
 CREATE INDEX index_issue_links_on_target_id ON issue_links USING btree (target_id);
 
+CREATE INDEX index_issue_metrics_first_mentioned_in_commit ON issue_metrics USING btree (issue_id) WHERE (date_part('year'::text, first_mentioned_in_commit_at) > (2019)::double precision);
+
 CREATE INDEX index_issue_metrics_on_issue_id_and_timestamps ON issue_metrics USING btree (issue_id, first_mentioned_in_commit_at, first_associated_with_milestone_at, first_added_to_board_at);
 
 CREATE INDEX index_issue_on_project_id_state_id_and_blocking_issues_count ON issues USING btree (project_id, state_id, blocking_issues_count);

lib/gitlab/background_migration/fix_first_mentioned_in_commit_at.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # Class that fixes the incorrectly set authored_date within
+    # issue_metrics table
+    class FixFirstMentionedInCommitAt
+      SUB_BATCH_SIZE = 500
+
+      # rubocop: disable Style/Documentation
+      class TmpIssueMetrics < ActiveRecord::Base
+        include EachBatch
+
+        self.table_name = 'issue_metrics'
+
+        def self.from_2020
+          where('EXTRACT(YEAR FROM first_mentioned_in_commit_at) > 2019')
+        end
+      end
+      # rubocop: enable Style/Documentation
+
+      def perform(start_id, end_id)
+        scope(start_id, end_id).each_batch(of: SUB_BATCH_SIZE, column: :issue_id) do |sub_batch|
+          first, last = sub_batch.pluck(Arel.sql('min(issue_id), max(issue_id)')).first
+
+          # The query need to be reconstructed because .each_batch modifies the default scope
+          # See: https://gitlab.com/gitlab-org/gitlab/-/issues/330510
+          inner_query = TmpIssueMetrics
+            .unscoped
+            .merge(scope(first, last))
+            .from("issue_metrics, #{lateral_query}")
+            .select('issue_metrics.issue_id', 'first_authored_date.authored_date')
+            .where('issue_metrics.first_mentioned_in_commit_at > first_authored_date.authored_date')
+
+          TmpIssueMetrics.connection.execute <<~UPDATE_METRICS
+            WITH cte AS #{Gitlab::Database::AsWithMaterialized.materialized_if_supported} (
+              #{inner_query.to_sql}
+            )
+            UPDATE issue_metrics
+            SET
+              first_mentioned_in_commit_at = cte.authored_date
+            FROM
+              cte
+            WHERE
+              cte.issue_id = issue_metrics.issue_id
+          UPDATE_METRICS
+        end
+      end
+
+      private
+
+      def scope(start_id, end_id)
+        TmpIssueMetrics.from_2020.where(issue_id: start_id..end_id)
+      end
+
+      def lateral_query
+        <<~SQL
+        LATERAL (
+          SELECT MIN(first_authored_date.authored_date) as authored_date
+          FROM merge_requests_closing_issues,
+          LATERAL (
+            SELECT id
+            FROM merge_request_diffs
+            WHERE merge_request_id = merge_requests_closing_issues.merge_request_id
+            ORDER BY id DESC
+            LIMIT 1
+          ) last_diff_id,
+          LATERAL (
+            SELECT authored_date
+            FROM merge_request_diff_commits
+            WHERE
+            merge_request_diff_id = last_diff_id.id
+            ORDER BY relative_order DESC
+            LIMIT 1
+          ) first_authored_date
+          WHERE merge_requests_closing_issues.issue_id = issue_metrics.issue_id
+        ) first_authored_date
+        SQL
+      end
+    end
+  end
+end

lib/gitlab/database/migrations/background_migration_helpers.rb

@@ -106,7 +106,7 @@ module Gitlab
           final_delay = 0
           batch_counter = 0
 
-          model_class.each_batch(of: batch_size) do |relation, index|
+          model_class.each_batch(of: batch_size, column: primary_column_name) do |relation, index|
             max = relation.arel_table[primary_column_name].maximum
             min = relation.arel_table[primary_column_name].minimum
 

spec/lib/gitlab/background_migration/fix_first_mentioned_in_commit_at_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::FixFirstMentionedInCommitAt, :migration, schema: 20211004110500 do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:users) { table(:users) }
+  let(:merge_requests) { table(:merge_requests) }
+  let(:issues) { table(:issues) }
+  let(:issue_metrics) { table(:issue_metrics) }
+  let(:merge_requests_closing_issues) { table(:merge_requests_closing_issues) }
+  let(:diffs) { table(:merge_request_diffs) }
+  let(:ten_days_ago) { 10.days.ago }
+  let(:commits) do
+    table(:merge_request_diff_commits).tap do |t|
+      t.extend(SuppressCompositePrimaryKeyWarning)
+    end
+  end
+
+  let(:namespace) { namespaces.create!(name: 'ns', path: 'ns') }
+  let(:project) { projects.create!(namespace_id: namespace.id) }
+
+  let!(:issue1) do
+    issues.create!(
+      title: 'issue',
+      description: 'description',
+      project_id: project.id
+    )
+  end
+
+  let!(:issue2) do
+    issues.create!(
+      title: 'issue',
+      description: 'description',
+      project_id: project.id
+    )
+  end
+
+  let!(:merge_request1) do
+    merge_requests.create!(
+      source_branch: 'a',
+      target_branch: 'master',
+      target_project_id: project.id
+    )
+  end
+
+  let!(:merge_request2) do
+    merge_requests.create!(
+      source_branch: 'b',
+      target_branch: 'master',
+      target_project_id: project.id
+    )
+  end
+
+  let!(:merge_request_closing_issue1) do
+    merge_requests_closing_issues.create!(issue_id: issue1.id, merge_request_id: merge_request1.id)
+  end
+
+  let!(:merge_request_closing_issue2) do
+    merge_requests_closing_issues.create!(issue_id: issue2.id, merge_request_id: merge_request2.id)
+  end
+
+  let!(:diff1) { diffs.create!(merge_request_id: merge_request1.id) }
+  let!(:diff2) { diffs.create!(merge_request_id: merge_request1.id) }
+
+  let!(:other_diff) { diffs.create!(merge_request_id: merge_request2.id) }
+
+  let!(:commit1) do
+    commits.create!(
+      merge_request_diff_id: diff2.id,
+      relative_order: 0,
+      sha: Gitlab::Database::ShaAttribute.serialize('aaa'),
+      authored_date: 5.days.ago
+    )
+  end
+
+  let!(:commit2) do
+    commits.create!(
+      merge_request_diff_id: diff2.id,
+      relative_order: 1,
+      sha: Gitlab::Database::ShaAttribute.serialize('aaa'),
+      authored_date: 10.days.ago
+    )
+  end
+
+  let!(:commit3) do
+    commits.create!(
+      merge_request_diff_id: other_diff.id,
+      relative_order: 1,
+      sha: Gitlab::Database::ShaAttribute.serialize('aaa'),
+      authored_date: 5.days.ago
+    )
+  end
+
+  def run_migration
+    described_class
+      .new
+      .perform(issue_metrics.minimum(:issue_id), issue_metrics.maximum(:issue_id))
+  end
+
+  context 'when the persisted first_mentioned_in_commit_at is later than the first commit authored_date' do
+    it 'updates the issue_metrics record' do
+      record1 = issue_metrics.create!(issue_id: issue1.id, first_mentioned_in_commit_at: Time.current)
+      record2 = issue_metrics.create!(issue_id: issue2.id, first_mentioned_in_commit_at: Time.current)
+
+      run_migration
+      record1.reload
+      record2.reload
+
+      expect(record1.first_mentioned_in_commit_at).to be_within(2.seconds).of(commit2.authored_date)
+      expect(record2.first_mentioned_in_commit_at).to be_within(2.seconds).of(commit3.authored_date)
+    end
+  end
+
+  context 'when the persisted first_mentioned_in_commit_at is earlier than the first commit authored_date' do
+    it 'does not update the issue_metrics record' do
+      record = issue_metrics.create!(issue_id: issue1.id, first_mentioned_in_commit_at: 20.days.ago)
+
+      expect { run_migration }.not_to change { record.reload.first_mentioned_in_commit_at }
+    end
+  end
+
+  context 'when the first_mentioned_in_commit_at is null' do
+    it 'does nothing' do
+      record = issue_metrics.create!(issue_id: issue1.id, first_mentioned_in_commit_at: nil)
+
+      expect { run_migration }.not_to change { record.reload.first_mentioned_in_commit_at }
+    end
+  end
+end

spec/services/issues/close_service_spec.rb

@@ -156,7 +156,7 @@ RSpec.describe Issues::CloseService do
       context 'updating `metrics.first_mentioned_in_commit_at`' do
         context 'when `metrics.first_mentioned_in_commit_at` is not set' do
           it 'uses the first commit authored timestamp' do
-            expected = closing_merge_request.commits.first.authored_date
+            expected = closing_merge_request.commits.take(100).last.authored_date
 
             close_issue