From 4cbd01b17a3d32bd3abb08c9e7e00c5be03313ea Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Tue, 12 Apr 2022 15:11:23 +0800
Subject: [PATCH] Nullify merge_request_metrics pipeline_id on pipeline
deletion
Before, when a pipeline is deleted, the associated
`merge_request_metrics` record will be deleted as well. This is a
destructive thing as `merge_request_metrics` include other data
not related to a pipeline.
To fix this issue, instead of deleting the association record, we
nullify the `pipeline_id` on `merge_request_metrics` instead. When
this happens, related build data (`latest_build_started_at` and
`latest_build_finished_at`) will be nullified as well. This is to
ensure that when those data are requested, it'll still behave the
same way as before.
Changelog: fixed
---
config/gitlab_loose_foreign_keys.yml | 2 +-
...d_data_trigger_on_merge_request_metrics.rb | 28 +++++++++++++
db/schema_migrations/20220412060931 | 1 +
db/structure.sql | 15 +++++++
spec/models/merge_request/metrics_spec.rb | 39 +++++++++++++++++++
5 files changed, 84 insertions(+), 1 deletion(-)
create mode 100644 db/migrate/20220412060931_add_nullify_build_data_trigger_on_merge_request_metrics.rb
create mode 100644 db/schema_migrations/20220412060931
diff --git a/config/gitlab_loose_foreign_keys.yml b/config/gitlab_loose_foreign_keys.yml
index 8f228120c8e5e021..ae6c2bde0819d4f8 100644
--- a/config/gitlab_loose_foreign_keys.yml
+++ b/config/gitlab_loose_foreign_keys.yml
@@ -186,7 +186,7 @@ external_pull_requests:
merge_request_metrics:
- table: ci_pipelines
column: pipeline_id
- on_delete: async_delete
+ on_delete: async_nullify
merge_requests:
- table: ci_pipelines
column: head_pipeline_id
diff --git a/db/migrate/20220412060931_add_nullify_build_data_trigger_on_merge_request_metrics.rb b/db/migrate/20220412060931_add_nullify_build_data_trigger_on_merge_request_metrics.rb
new file mode 100644
index 0000000000000000..96ec44c16c56b325
--- /dev/null
+++ b/db/migrate/20220412060931_add_nullify_build_data_trigger_on_merge_request_metrics.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+class AddNullifyBuildDataTriggerOnMergeRequestMetrics < Gitlab::Database::Migration[1.0]
+ include Gitlab::Database::SchemaHelpers
+
+ TABLE_NAME = 'merge_request_metrics'
+ FUNCTION_NAME = 'nullify_merge_request_metrics_build_data'
+ TRIGGER_NAME = 'nullify_merge_request_metrics_build_data_on_update'
+
+ def up
+ create_trigger_function(FUNCTION_NAME) do
+ <<~SQL
+ IF (OLD.pipeline_id IS NOT NULL) AND (NEW.pipeline_id IS NULL) THEN
+ NEW.latest_build_started_at = NULL;
+ NEW.latest_build_finished_at = NULL;
+ END IF;
+ RETURN NEW;
+ SQL
+ end
+
+ create_trigger(TABLE_NAME, TRIGGER_NAME, FUNCTION_NAME, fires: 'BEFORE UPDATE')
+ end
+
+ def down
+ drop_trigger(TABLE_NAME, TRIGGER_NAME)
+ drop_function(FUNCTION_NAME)
+ end
+end
diff --git a/db/schema_migrations/20220412060931 b/db/schema_migrations/20220412060931
new file mode 100644
index 0000000000000000..145d3aaf1019b986
--- /dev/null
+++ b/db/schema_migrations/20220412060931
@@ -0,0 +1 @@
+504e7df63be512fb4f6d3abbf9ebe381752f2c24b63b2d6a4d11c64894c1555b
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index a4c42e90791cb035..74ffa6035088703e 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -106,6 +106,19 @@ BEGIN
END;
$$;
+CREATE FUNCTION nullify_merge_request_metrics_build_data() RETURNS trigger
+ LANGUAGE plpgsql
+ AS $$
+BEGIN
+IF (OLD.pipeline_id IS NOT NULL) AND (NEW.pipeline_id IS NULL) THEN
+ NEW.latest_build_started_at = NULL;
+ NEW.latest_build_finished_at = NULL;
+END IF;
+RETURN NEW;
+
+END
+$$;
+
CREATE FUNCTION postgres_pg_stat_activity_autovacuum() RETURNS TABLE(query text, query_start timestamp with time zone)
LANGUAGE sql SECURITY DEFINER
SET search_path TO 'pg_catalog', 'pg_temp'
@@ -31061,6 +31074,8 @@ CREATE TRIGGER merge_requests_loose_fk_trigger AFTER DELETE ON merge_requests RE
CREATE TRIGGER namespaces_loose_fk_trigger AFTER DELETE ON namespaces REFERENCING OLD TABLE AS old_table FOR EACH STATEMENT EXECUTE FUNCTION insert_into_loose_foreign_keys_deleted_records();
+CREATE TRIGGER nullify_merge_request_metrics_build_data_on_update BEFORE UPDATE ON merge_request_metrics FOR EACH ROW EXECUTE FUNCTION nullify_merge_request_metrics_build_data();
+
CREATE TRIGGER projects_loose_fk_trigger AFTER DELETE ON projects REFERENCING OLD TABLE AS old_table FOR EACH STATEMENT EXECUTE FUNCTION insert_into_loose_foreign_keys_deleted_records();
CREATE TRIGGER trigger_delete_project_namespace_on_project_delete AFTER DELETE ON projects FOR EACH ROW WHEN ((old.project_namespace_id IS NOT NULL)) EXECUTE FUNCTION delete_associated_project_namespace();
diff --git a/spec/models/merge_request/metrics_spec.rb b/spec/models/merge_request/metrics_spec.rb
index a4bdac39074fb8bc..8d1d503b3238d7c1 100644
--- a/spec/models/merge_request/metrics_spec.rb
+++ b/spec/models/merge_request/metrics_spec.rb
@@ -54,4 +54,43 @@
let!(:parent) { create(:ci_pipeline, project: merge_request.target_project) }
let!(:model) { merge_request.metrics.tap { |metrics| metrics.update!(pipeline: parent) } }
end
+
+ describe 'update' do
+ let(:merge_request) { create(:merge_request) }
+ let(:metrics) { merge_request.metrics }
+
+ before do
+ metrics.update!(
+ pipeline_id: 1,
+ latest_build_started_at: Time.current,
+ latest_build_finished_at: Time.current
+ )
+ end
+
+ context 'when pipeline_id is nullified' do
+ before do
+ metrics.update!(pipeline_id: nil)
+ end
+
+ it 'nullifies build related columns via DB trigger' do
+ metrics.reload
+
+ expect(metrics.latest_build_started_at).to be_nil
+ expect(metrics.latest_build_finished_at).to be_nil
+ end
+ end
+
+ context 'when updated but pipeline_id is not nullified' do
+ before do
+ metrics.update!(latest_closed_at: Time.current)
+ end
+
+ it 'does not nullify build related columns' do
+ metrics.reload
+
+ expect(metrics.latest_build_started_at).not_to be_nil
+ expect(metrics.latest_build_finished_at).not_to be_nil
+ end
+ end
+ end
end
--
GitLab