From 271985998f0dfbe2ba1970e99033591878a94465 Mon Sep 17 00:00:00 2001 From: Adam Hegyi Date: Thu, 23 Jul 2020 16:54:20 +0200 Subject: [PATCH] Add target_project_id to merge_request_metrics This change adds target_project_id to merge_request_metrics table and keeps the column in sync with the merge_requests.target_project_id column. --- app/models/concerns/issuable.rb | 2 +- app/models/merge_request.rb | 9 ++++- app/models/merge_request/metrics.rb | 10 ++++- ...et-project-id-to-merge-request-metrics.yml | 5 +++ ...205_add_target_project_id_to_mr_metrics.rb | 19 +++++++++ ...332_add_fk_to_metrics_target_project_id.rb | 19 +++++++++ ...y_of_mr_target_project_id_to_mr_metrics.rb | 33 ++++++++++++++++ db/schema_migrations/20200723125205 | 1 + db/schema_migrations/20200723128332 | 1 + db/schema_migrations/20200723132258 | 1 + db/structure.sql | 8 +++- ...target_project_to_merge_request_metrics.rb | 25 ++++++++++++ ...t_project_to_merge_request_metrics_spec.rb | 39 +++++++++++++++++++ .../import_export/safe_model_attributes.yml | 1 + spec/models/merge_request/metrics_spec.rb | 11 ++++++ spec/models/merge_request_spec.rb | 29 +++++++++++--- 16 files changed, 203 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/add-target-project-id-to-merge-request-metrics.yml create mode 100644 db/migrate/20200723125205_add_target_project_id_to_mr_metrics.rb create mode 100644 db/migrate/20200723128332_add_fk_to_metrics_target_project_id.rb create mode 100644 db/post_migrate/20200723132258_schedule_copy_of_mr_target_project_id_to_mr_metrics.rb create mode 100644 db/schema_migrations/20200723125205 create mode 100644 db/schema_migrations/20200723128332 create mode 100644 db/schema_migrations/20200723132258 create mode 100644 lib/gitlab/background_migration/copy_merge_request_target_project_to_merge_request_metrics.rb create mode 100644 spec/lib/gitlab/background_migration/copy_merge_request_target_project_to_merge_request_metrics_spec.rb diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 715cbd15d93c20..fe51bdc2486121 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -65,7 +65,7 @@ def award_emojis_loaded? 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, diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index a3eb684698a672..927990ee4715fe 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1607,7 +1607,12 @@ def banzai_render_context(field) 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 @@ def ensure_metrics # 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 diff --git a/app/models/merge_request/metrics.rb b/app/models/merge_request/metrics.rb index ba363019c72973..f679beabf9489e 100644 --- a/app/models/merge_request/metrics.rb +++ b/app/models/merge_request/metrics.rb @@ -1,10 +1,18 @@ # 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') diff --git a/changelogs/unreleased/add-target-project-id-to-merge-request-metrics.yml b/changelogs/unreleased/add-target-project-id-to-merge-request-metrics.yml new file mode 100644 index 00000000000000..22f1bd84b71d8e --- /dev/null +++ b/changelogs/unreleased/add-target-project-id-to-merge-request-metrics.yml @@ -0,0 +1,5 @@ +--- +title: Add target_project_id to merge_request_metrics table +merge_request: 37713 +author: +type: added diff --git a/db/migrate/20200723125205_add_target_project_id_to_mr_metrics.rb b/db/migrate/20200723125205_add_target_project_id_to_mr_metrics.rb new file mode 100644 index 00000000000000..fb01b3fe0469ce --- /dev/null +++ b/db/migrate/20200723125205_add_target_project_id_to_mr_metrics.rb @@ -0,0 +1,19 @@ +# 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 diff --git a/db/migrate/20200723128332_add_fk_to_metrics_target_project_id.rb b/db/migrate/20200723128332_add_fk_to_metrics_target_project_id.rb new file mode 100644 index 00000000000000..6536b20b9c1a1d --- /dev/null +++ b/db/migrate/20200723128332_add_fk_to_metrics_target_project_id.rb @@ -0,0 +1,19 @@ +# 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 diff --git a/db/post_migrate/20200723132258_schedule_copy_of_mr_target_project_id_to_mr_metrics.rb b/db/post_migrate/20200723132258_schedule_copy_of_mr_target_project_id_to_mr_metrics.rb new file mode 100644 index 00000000000000..bca703121bc2dc --- /dev/null +++ b/db/post_migrate/20200723132258_schedule_copy_of_mr_target_project_id_to_mr_metrics.rb @@ -0,0 +1,33 @@ +# 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 diff --git a/db/schema_migrations/20200723125205 b/db/schema_migrations/20200723125205 new file mode 100644 index 00000000000000..ca25997865818c --- /dev/null +++ b/db/schema_migrations/20200723125205 @@ -0,0 +1 @@ +630029f7d90da29022404146ce8c488108a2232d2bfd0864a6f5d659f3999af8 \ No newline at end of file diff --git a/db/schema_migrations/20200723128332 b/db/schema_migrations/20200723128332 new file mode 100644 index 00000000000000..2e180ef80f14bd --- /dev/null +++ b/db/schema_migrations/20200723128332 @@ -0,0 +1 @@ +7d6f3601187c98f091cb0c5449ff7c6ca53392f006435223dcc067e4a73dab11 \ No newline at end of file diff --git a/db/schema_migrations/20200723132258 b/db/schema_migrations/20200723132258 new file mode 100644 index 00000000000000..3a3c0341f4205f --- /dev/null +++ b/db/schema_migrations/20200723132258 @@ -0,0 +1 @@ +b3fcb58bbeae8af800a32158a8d272ec524594391e96357fdad955f70864bc95 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 4068d89205d22d..eaa095eeb655f4 100644 --- a/db/structure.sql +++ b/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 @@ -19870,6 +19871,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); @@ -21370,6 +21373,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; diff --git a/lib/gitlab/background_migration/copy_merge_request_target_project_to_merge_request_metrics.rb b/lib/gitlab/background_migration/copy_merge_request_target_project_to_merge_request_metrics.rb new file mode 100644 index 00000000000000..6014ccc12ebbff --- /dev/null +++ b/lib/gitlab/background_migration/copy_merge_request_target_project_to_merge_request_metrics.rb @@ -0,0 +1,25 @@ +# 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 diff --git a/spec/lib/gitlab/background_migration/copy_merge_request_target_project_to_merge_request_metrics_spec.rb b/spec/lib/gitlab/background_migration/copy_merge_request_target_project_to_merge_request_metrics_spec.rb new file mode 100644 index 00000000000000..71bb794d5392ab --- /dev/null +++ b/spec/lib/gitlab/background_migration/copy_merge_request_target_project_to_merge_request_metrics_spec.rb @@ -0,0 +1,39 @@ +# 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 diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 9528d8d17e7a0e..5d3d8d1d91efe5 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/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 diff --git a/spec/models/merge_request/metrics_spec.rb b/spec/models/merge_request/metrics_spec.rb index 4d9e768ecc6c00..60a8fd21c44ba2 100644 --- a/spec/models/merge_request/metrics_spec.rb +++ b/spec/models/merge_request/metrics_spec.rb @@ -8,4 +8,15 @@ 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 diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 06febddef0c40a..f123a8192246a0 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -247,24 +247,20 @@ 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 @@ 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 -- GitLab