NOT NULL constraint to target_project_id column

This change adds and validates NOT NULL CHECK constraint on
merge_request_metrics.target_project_id column.

changelogs/unreleased/233507-set-not-null-on-mr-metrics-target-project-id.yml

+---
+title: Add NOT NULL constraint to merge_request_metrics.target_project_id
+merge_request: 40836
+author:
+type: other

db/post_migrate/20200831065320_add_not_valid_not_null_constraint_to_mr_metrics.rb

+# frozen_string_literal: true
+
+class AddNotValidNotNullConstraintToMrMetrics < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_not_null_constraint :merge_request_metrics, :target_project_id, validate: false
+  end
+
+  def down
+    remove_not_null_constraint :merge_request_metrics, :target_project_id
+  end
+end

db/post_migrate/20200831065322_add_tmp_index_to_target_project_id.rb

+# frozen_string_literal: true
+
+class AddTmpIndexToTargetProjectId < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  TMP_INDEX_NAME = 'tmp_index_on_mr_metrics_target_project_id_null'
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :merge_request_metrics, :id, where: 'target_project_id IS NULL', name: TMP_INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :merge_request_metrics, name: TMP_INDEX_NAME
+  end
+end

db/post_migrate/20200831065705_ensure_target_project_id_is_filled.rb

+# frozen_string_literal: true
+
+class EnsureTargetProjectIdIsFilled < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  BACKGROUND_MIGRATION_CLASS = 'CopyMergeRequestTargetProjectToMergeRequestMetrics'
+  BATCH_SIZE = 1_000
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class MergeRequest < ActiveRecord::Base
+    self.table_name = 'merge_requests'
+  end
+
+  class MergeRequestMetrics < ActiveRecord::Base
+    include EachBatch
+
+    belongs_to :merge_request
+
+    self.table_name = 'merge_request_metrics'
+  end
+
+  def up
+    Gitlab::BackgroundMigration.steal(BACKGROUND_MIGRATION_CLASS)
+
+    # Do a manual update in case we lost BG jobs. The expected record count should be 0 or very low.
+    MergeRequestMetrics.where(target_project_id: nil).each_batch do |scope|
+      query_for_cte = scope.joins(:merge_request).select(
+        MergeRequestMetrics.arel_table[:id].as('id'),
+        MergeRequest.arel_table[:target_project_id].as('target_project_id')
+      )
+
+      MergeRequestMetrics.connection.execute <<-SQL
+        WITH target_project_id_and_metrics_id as (
+          #{query_for_cte.to_sql}
+        )
+        UPDATE #{MergeRequestMetrics.connection.quote_table_name(MergeRequestMetrics.table_name)}
+        SET target_project_id = target_project_id_and_metrics_id.target_project_id
+        FROM target_project_id_and_metrics_id
+        WHERE merge_request_metrics.id = target_project_id_and_metrics_id.id
+      SQL
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

db/post_migrate/20200831074356_validate_not_null_constraint_on_mr_metrics.rb

+# frozen_string_literal: true
+
+class ValidateNotNullConstraintOnMrMetrics < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  TMP_INDEX_NAME = 'tmp_index_on_mr_metrics_target_project_id_null'
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    validate_not_null_constraint :merge_request_metrics, :target_project_id
+
+    remove_concurrent_index_by_name :merge_request_metrics, name: TMP_INDEX_NAME
+  end
+
+  def down
+    add_concurrent_index :merge_request_metrics, :id, where: 'target_project_id IS NULL', name: TMP_INDEX_NAME
+  end
+end

db/schema_migrations/20200831065320

+3e704cf329786e89f43fdefbc6a91272bebc6af5653dd83b5a81567937f75752
\ No newline at end of file

db/schema_migrations/20200831065322

+4cb28d6da005682bb077427f4f544996065e86f0e76d5de98fc1761555a0535b
\ No newline at end of file

db/schema_migrations/20200831065705

+c1457272bd8f0055992df5d4a8ba1a62cb74d3af3fff25447b3abd2eb090841e
\ No newline at end of file

db/schema_migrations/20200831074356

+8605268a026d4c66653008bb051c567c251044232b925b89f4407f9ad70f639c
\ No newline at end of file

db/structure.sql

@@ -13166,7 +13166,8 @@ CREATE TABLE public.merge_request_metrics (
     first_reassigned_at timestamp with time zone,
     added_lines integer,
     removed_lines integer,
-    target_project_id integer
+    target_project_id integer,
+    CONSTRAINT check_e03d0900bf CHECK ((target_project_id IS NOT NULL))
 );
 
 CREATE SEQUENCE public.merge_request_metrics_id_seq

spec/migrations/ensure_target_project_id_is_filled_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200831065705_ensure_target_project_id_is_filled.rb')
+
+RSpec.describe EnsureTargetProjectIdIsFilled, schema: 20200827085101 do
+  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_not_to_migrate) { merge_requests.create!(source_branch: 'e', target_branch: 'f', target_project_id: project_1.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!(:metrics_3) { metrics.create!(merge_request_id: merge_request_not_to_migrate.id, target_project_id: project_1.id) }
+
+  it 'migrates missing target_project_ids' do
+    migrate!
+
+    expect(metrics_1.reload.target_project_id).to eq(project_1.id)
+    expect(metrics_2.reload.target_project_id).to eq(project_2.id)
+    expect(metrics_3.reload.target_project_id).to eq(project_1.id)
+  end
+end

spec/models/merge_request/metrics_spec.rb

@@ -9,17 +9,6 @@ RSpec.describe MergeRequest::Metrics do
     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
-
   describe 'scopes' do
     let_it_be(:metrics_1) { create(:merge_request).metrics.tap { |m| m.update!(merged_at: 10.days.ago) } }
     let_it_be(:metrics_2) { create(:merge_request).metrics.tap { |m| m.update!(merged_at: 5.days.ago) } }

spec/models/merge_request_spec.rb

@@ -286,18 +286,6 @@ RSpec.describe MergeRequest, factory_default: :keep do
         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