Commit 1486afa3 authored by Alex Ives's avatar Alex Ives Committed by Mayra Cabrera

Add foreign key from chat_name to service

- Fix orphaned data causing 500 in profile/chat_names when projects
  are removed
- Add foreign key to prevent future orphaned rows
- Add before destroy hook to project cleaning up any chat names

Relates to https://gitlab.com/gitlab-org/gitlab/issues/25230
parent 0ec581bc
......@@ -107,6 +107,7 @@ class Project < ApplicationRecord
after_update :update_forks_visibility_level
before_destroy :remove_private_deploy_keys
before_destroy :cleanup_chat_names
use_fast_destroy :build_trace_chunks
......@@ -1916,6 +1917,17 @@ class Project < ApplicationRecord
import_export_upload&.export_file
end
# Before 12.9 we did not correctly clean up chat names and this causes issues.
# In 12.9, we add a foreign key relationship, but this code is used ensure the chat names are cleaned up while a post
# migration enables the foreign key relationship.
#
# This should be removed in 13.0.
#
# https://gitlab.com/gitlab-org/gitlab/issues/204787
def cleanup_chat_names
ChatName.where(service: services.select(:id)).delete_all
end
def full_path_slug
Gitlab::Utils.slugify(full_path.to_s)
end
......
---
title: Fix 500 error on profile/chat_names for deleted projects
merge_request: 24341
author:
type: fixed
# frozen_string_literal: true
class AddIndexChatNameServiceId < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_concurrent_index :chat_names, :service_id
end
def down
remove_concurrent_index :chat_names, :service_id
end
end
# frozen_string_literal: true
class AddInvalidForeignKeyFromChatNameToService < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_concurrent_foreign_key :chat_names, :services, column: :service_id, on_delete: :cascade, validate: false
end
def down
remove_foreign_key_if_exists :chat_names, column: :service_id
end
end
# frozen_string_literal: true
class RemoveOrphanedChatNames < ActiveRecord::Migration[6.0]
DOWNTIME = false
def up
execute("DELETE FROM chat_names WHERE service_id NOT IN(SELECT id FROM services WHERE services.type = 'chat')")
end
def down
say 'Orphaned user chat names were removed as a part of this migration and are non-recoverable'
end
end
# frozen_string_literal: true
class ValidateForeignKeyFromChatNameToService < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def up
validate_foreign_key :chat_names, :service_id
end
def down
# no-op
end
end
......@@ -8626,6 +8626,8 @@ CREATE INDEX index_boards_on_project_id ON public.boards USING btree (project_id
CREATE INDEX index_broadcast_message_on_ends_at_and_broadcast_type_and_id ON public.broadcast_messages USING btree (ends_at, broadcast_type, id);
CREATE INDEX index_chat_names_on_service_id ON public.chat_names USING btree (service_id);
CREATE UNIQUE INDEX index_chat_names_on_service_id_and_team_id_and_chat_id ON public.chat_names USING btree (service_id, team_id, chat_id);
CREATE UNIQUE INDEX index_chat_names_on_user_id_and_service_id ON public.chat_names USING btree (user_id, service_id);
......@@ -10330,6 +10332,9 @@ CREATE UNIQUE INDEX vulnerability_feedback_unique_idx ON public.vulnerability_fe
CREATE UNIQUE INDEX vulnerability_occurrence_pipelines_on_unique_keys ON public.vulnerability_occurrence_pipelines USING btree (occurrence_id, pipeline_id);
ALTER TABLE ONLY public.chat_names
ADD CONSTRAINT fk_00797a2bf9 FOREIGN KEY (service_id) REFERENCES public.services(id) ON DELETE CASCADE;
ALTER TABLE ONLY public.epics
ADD CONSTRAINT fk_013c9f36ca FOREIGN KEY (due_date_sourcing_epic_id) REFERENCES public.epics(id) ON DELETE SET NULL;
......@@ -12874,6 +12879,10 @@ COPY "schema_migrations" (version) FROM STDIN;
20200312163407
20200313101649
20200313123934
20200313202430
20200313203525
20200313203550
20200313204021
20200314060834
20200316111759
20200316162648
......
......@@ -21,7 +21,7 @@ describe 'Database schema' do
award_emoji: %w[awardable_id user_id],
aws_roles: %w[role_external_id],
boards: %w[milestone_id],
chat_names: %w[chat_id service_id team_id user_id],
chat_names: %w[chat_id team_id user_id],
chat_teams: %w[team_id],
ci_builds: %w[erased_by_id runner_id trigger_request_id user_id],
ci_pipelines: %w[user_id],
......
# frozen_string_literal: true
require 'spec_helper'
require Rails.root.join('db', 'post_migrate', '20200313203550_remove_orphaned_chat_names.rb')
describe RemoveOrphanedChatNames, schema: 20200313202430 do
let(:projects) { table(:projects) }
let(:namespaces) { table(:namespaces) }
let(:services) { table(:services) }
let(:chat_names) { table(:chat_names) }
let(:namespace) { namespaces.create(name: 'foo', path: 'foo') }
let(:project) { projects.create(namespace_id: namespace.id) }
let(:service) { services.create(project_id: project.id, type: 'chat') }
let(:chat_name) { chat_names.create!(service_id: service.id, team_id: 'TEAM', user_id: 12345, chat_id: 12345) }
let(:orphaned_chat_name) { chat_names.create!(team_id: 'TEAM', service_id: 0, user_id: 12345, chat_id: 12345) }
it 'removes the orphaned chat_name' do
expect(chat_name).to be_present
expect(orphaned_chat_name).to be_present
migrate!
expect(chat_names.where(id: orphaned_chat_name.id)).to be_empty
expect(chat_name.reload).to be_present
end
end
......@@ -17,6 +17,12 @@ describe ChatName do
it { is_expected.to validate_uniqueness_of(:user_id).scoped_to(:service_id) }
it { is_expected.to validate_uniqueness_of(:chat_id).scoped_to(:service_id, :team_id) }
it 'is removed when the project is deleted' do
expect { subject.reload.service.project.delete }.to change { ChatName.count }.by(-1)
expect(ChatName.where(id: subject.id)).not_to exist
end
describe '#update_last_used_at', :clean_gitlab_redis_shared_state do
it 'updates the last_used_at timestamp' do
expect(subject.last_used_at).to be_nil
......
......@@ -5729,6 +5729,20 @@ describe Project do
end
end
describe 'with services and chat names' do
subject { create(:project) }
let(:service) { create(:service, project: subject) }
before do
create_list(:chat_name, 5, service: service)
end
it 'removes chat names on removal' do
expect { subject.destroy }.to change { ChatName.count }.by(-5)
end
end
describe 'with_issues_or_mrs_available_for_user' do
before do
Project.delete_all
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment