Merge branch 'add-target-project-id-to-merge-request-metrics' into 'master'

BG migration for MR metrics target_project_id

See merge request gitlab-org/gitlab!37713

app/models/concerns/issuable.rb

@@ -65,7 +65,7 @@ module Issuable
     has_many :labels, through: :label_links
     has_many :todos, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
-    has_one :metrics
+    has_one :metrics, inverse_of: model_name.singular.to_sym, autosave: true
 
     delegate :name,
              :email,

app/models/merge_request.rb

@@ -1607,7 +1607,12 @@ class MergeRequest < ApplicationRecord
 
   override :ensure_metrics
   def ensure_metrics
-    MergeRequest::Metrics.safe_find_or_create_by(merge_request_id: id).tap do |metrics_record|
+    # Backward compatibility: some merge request metrics records will not have target_project_id filled in.
+    # In that case the first `safe_find_or_create_by` will return false.
+    # The second finder call will be eliminated in https://gitlab.com/gitlab-org/gitlab/-/issues/233507
+    metrics_record = MergeRequest::Metrics.safe_find_or_create_by(merge_request_id: id, target_project_id: target_project_id) || MergeRequest::Metrics.safe_find_or_create_by(merge_request_id: id)
+
+    metrics_record.tap do |metrics_record|
       # Make sure we refresh the loaded association object with the newly created/loaded item.
       # This is needed in order to have the exact functionality than before.
       #
@@ -1617,6 +1622,8 @@ class MergeRequest < ApplicationRecord
       # merge_request.ensure_metrics
       # merge_request.metrics # should return the metrics record and not nil
       # merge_request.metrics.merge_request # should return the same MR record
+
+      metrics_record.target_project_id = target_project_id
       metrics_record.association(:merge_request).target = self
       association(:metrics).target = metrics_record
     end

app/models/merge_request/metrics.rb

 # frozen_string_literal: true
 
 class MergeRequest::Metrics < ApplicationRecord
-  belongs_to :merge_request
+  belongs_to :merge_request, inverse_of: :metrics
   belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :pipeline_id
   belongs_to :latest_closed_by, class_name: 'User'
   belongs_to :merged_by, class_name: 'User'
+
+  before_save :ensure_target_project_id
+
+  private
+
+  def ensure_target_project_id
+    self.target_project_id ||= merge_request.target_project_id
+  end
 end
 
 MergeRequest::Metrics.prepend_if_ee('EE::MergeRequest::Metrics')

changelogs/unreleased/add-target-project-id-to-merge-request-metrics.yml

+---
+title: Add target_project_id to merge_request_metrics table
+merge_request: 37713
+author:
+type: added

db/migrate/20200723125205_add_target_project_id_to_mr_metrics.rb

+# frozen_string_literal: true
+
+class AddTargetProjectIdToMrMetrics < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :merge_request_metrics, :target_project_id, :integer
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :merge_request_metrics, :target_project_id, :integer
+    end
+  end
+end

db/migrate/20200723128332_add_fk_to_metrics_target_project_id.rb

+# frozen_string_literal: true
+
+class AddFkToMetricsTargetProjectId < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:merge_request_metrics, :target_project_id)
+    add_concurrent_foreign_key(:merge_request_metrics, :projects, column: :target_project_id, on_delete: :cascade)
+  end
+
+  def down
+    remove_foreign_key(:merge_request_metrics, column: :target_project_id)
+    remove_concurrent_index(:merge_request_metrics, :target_project_id)
+  end
+end

db/post_migrate/20200723132258_schedule_copy_of_mr_target_project_id_to_mr_metrics.rb

+# frozen_string_literal: true
+
+class ScheduleCopyOfMrTargetProjectIdToMrMetrics < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INTERVAL = 2.minutes.to_i
+  BATCH_SIZE = 5_000
+  MIGRATION = 'CopyMergeRequestTargetProjectToMergeRequestMetrics'
+
+  disable_ddl_transaction!
+
+  class MergeRequest < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'merge_requests'
+  end
+
+  def up
+    MergeRequest.reset_column_information
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      MergeRequest,
+      MIGRATION,
+      INTERVAL,
+      batch_size: BATCH_SIZE
+    )
+  end
+
+  def down
+    # noop
+  end
+end

db/schema_migrations/20200723125205

+630029f7d90da29022404146ce8c488108a2232d2bfd0864a6f5d659f3999af8
\ No newline at end of file

db/schema_migrations/20200723128332

+7d6f3601187c98f091cb0c5449ff7c6ca53392f006435223dcc067e4a73dab11
\ No newline at end of file

db/schema_migrations/20200723132258

+b3fcb58bbeae8af800a32158a8d272ec524594391e96357fdad955f70864bc95
\ No newline at end of file

db/structure.sql

@@ -13038,7 +13038,8 @@ CREATE TABLE public.merge_request_metrics (
     first_approved_at timestamp with time zone,
     first_reassigned_at timestamp with time zone,
     added_lines integer,
-    removed_lines integer
+    removed_lines integer,
+    target_project_id integer
 );
 
 CREATE SEQUENCE public.merge_request_metrics_id_seq
@@ -19881,6 +19882,8 @@ CREATE INDEX index_merge_request_metrics_on_merged_by_id ON public.merge_request
 
 CREATE INDEX index_merge_request_metrics_on_pipeline_id ON public.merge_request_metrics USING btree (pipeline_id);
 
+CREATE INDEX index_merge_request_metrics_on_target_project_id ON public.merge_request_metrics USING btree (target_project_id);
+
 CREATE UNIQUE INDEX index_merge_request_user_mentions_on_note_id ON public.merge_request_user_mentions USING btree (note_id) WHERE (note_id IS NOT NULL);
 
 CREATE INDEX index_merge_requests_closing_issues_on_issue_id ON public.merge_requests_closing_issues USING btree (issue_id);
@@ -21385,6 +21388,9 @@ ALTER TABLE ONLY public.path_locks
 ALTER TABLE ONLY public.clusters_applications_prometheus
     ADD CONSTRAINT fk_557e773639 FOREIGN KEY (cluster_id) REFERENCES public.clusters(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY public.merge_request_metrics
+    ADD CONSTRAINT fk_56067dcb44 FOREIGN KEY (target_project_id) REFERENCES public.projects(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.vulnerability_feedback
     ADD CONSTRAINT fk_563ff1912e FOREIGN KEY (merge_request_id) REFERENCES public.merge_requests(id) ON DELETE SET NULL;
 

lib/gitlab/background_migration/copy_merge_request_target_project_to_merge_request_metrics.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class CopyMergeRequestTargetProjectToMergeRequestMetrics
+      extend ::Gitlab::Utils::Override
+
+      def perform(start_id, stop_id)
+        ActiveRecord::Base.connection.execute <<~SQL
+          WITH merge_requests_batch AS (
+            SELECT id, target_project_id
+            FROM merge_requests WHERE id BETWEEN #{Integer(start_id)} AND #{Integer(stop_id)}
+          )
+          UPDATE
+            merge_request_metrics
+          SET
+            target_project_id = merge_requests_batch.target_project_id
+          FROM merge_requests_batch
+          WHERE merge_request_metrics.merge_request_id=merge_requests_batch.id
+        SQL
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/copy_merge_request_target_project_to_merge_request_metrics_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::CopyMergeRequestTargetProjectToMergeRequestMetrics, :migration, schema: 20200723125205 do
+  let(:migration) { described_class.new }
+
+  let_it_be(:namespaces) { table(:namespaces) }
+  let_it_be(:projects) { table(:projects) }
+  let_it_be(:merge_requests) { table(:merge_requests) }
+  let_it_be(:metrics) { table(:merge_request_metrics) }
+
+  let!(:namespace) { namespaces.create!(name: 'namespace', path: 'namespace') }
+  let!(:project_1) { projects.create!(namespace_id: namespace.id) }
+  let!(:project_2) { projects.create!(namespace_id: namespace.id) }
+  let!(:merge_request_to_migrate_1) { merge_requests.create!(source_branch: 'a', target_branch: 'b', target_project_id: project_1.id) }
+  let!(:merge_request_to_migrate_2) { merge_requests.create!(source_branch: 'c', target_branch: 'd', target_project_id: project_2.id) }
+  let!(:merge_request_without_metrics) { merge_requests.create!(source_branch: 'e', target_branch: 'f', target_project_id: project_2.id) }
+
+  let!(:metrics_1) { metrics.create!(merge_request_id: merge_request_to_migrate_1.id) }
+  let!(:metrics_2) { metrics.create!(merge_request_id: merge_request_to_migrate_2.id) }
+
+  let(:merge_request_ids) { [merge_request_to_migrate_1.id, merge_request_to_migrate_2.id, merge_request_without_metrics.id] }
+
+  subject { migration.perform(merge_request_ids.min, merge_request_ids.max) }
+
+  it 'copies `target_project_id` to the associated `merge_request_metrics` record' do
+    subject
+
+    expect(metrics_1.reload.target_project_id).to eq(project_1.id)
+    expect(metrics_2.reload.target_project_id).to eq(project_2.id)
+  end
+
+  it 'does not create metrics record when it is missing' do
+    subject
+
+    expect(metrics.find_by_merge_request_id(merge_request_without_metrics.id)).to be_nil
+  end
+end

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -287,6 +287,7 @@ MergeRequest::Metrics:
 - first_approved_at
 - first_reassigned_at
 - added_lines
+- target_project_id
 - removed_lines
 Ci::Pipeline:
 - id

spec/models/merge_request/metrics_spec.rb

@@ -8,4 +8,15 @@ RSpec.describe MergeRequest::Metrics do
     it { is_expected.to belong_to(:latest_closed_by).class_name('User') }
     it { is_expected.to belong_to(:merged_by).class_name('User') }
   end
+
+  it 'sets `target_project_id` before save' do
+    merge_request = create(:merge_request)
+    metrics = merge_request.metrics
+
+    metrics.update_column(:target_project_id, nil)
+
+    metrics.save!
+
+    expect(metrics.target_project_id).to eq(merge_request.target_project_id)
+  end
 end

spec/models/merge_request_spec.rb

@@ -247,24 +247,20 @@ RSpec.describe MergeRequest do
 
   describe 'callbacks' do
     describe '#ensure_merge_request_metrics' do
-      it 'creates metrics after saving' do
-        merge_request = create(:merge_request)
+      let(:merge_request) { create(:merge_request) }
 
+      it 'creates metrics after saving' do
         expect(merge_request.metrics).to be_persisted
         expect(MergeRequest::Metrics.count).to eq(1)
       end
 
       it 'does not duplicate metrics for a merge request' do
-        merge_request = create(:merge_request)
-
         merge_request.mark_as_merged!
 
         expect(MergeRequest::Metrics.count).to eq(1)
       end
 
       it 'does not create duplicated metrics records when MR is concurrently updated' do
-        merge_request = create(:merge_request)
-
         merge_request.metrics.destroy
 
         instance1 = MergeRequest.find(merge_request.id)
@@ -276,6 +272,27 @@ RSpec.describe MergeRequest do
         metrics_records = MergeRequest::Metrics.where(merge_request_id: merge_request.id)
         expect(metrics_records.size).to eq(1)
       end
+
+      it 'syncs the `target_project_id` to the metrics record' do
+        project = create(:project)
+
+        merge_request.update!(target_project: project, state: :closed)
+
+        expect(merge_request.target_project_id).to eq(project.id)
+        expect(merge_request.target_project_id).to eq(merge_request.metrics.target_project_id)
+      end
+
+      context 'when metrics record already exists with NULL target_project_id' do
+        before do
+          merge_request.metrics.update_column(:target_project_id, nil)
+        end
+
+        it 'returns the metrics record' do
+          metrics_record = merge_request.ensure_metrics
+
+          expect(metrics_record).to be_persisted
+        end
+      end
     end
   end