Merge branch 'alexives/25230/add_foreign_key_to_chat_name_service_id' into 'master'

Add foreign key from chat_name to service

See merge request gitlab-org/gitlab!24341

app/models/project.rb

@@ -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
 
@@ -1908,6 +1909,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

changelogs/unreleased/alexives-25230-add_foreign_key_to_chat_name_service_id.yml

+---
+title: Fix 500 error on profile/chat_names for deleted projects
+merge_request: 24341
+author:
+type: fixed

db/migrate/20200313202430_add_index_chat_name_service_id.rb

+# 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

db/migrate/20200313203525_add_invalid_foreign_key_from_chat_name_to_service.rb

+# 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

db/post_migrate/20200313203550_remove_orphaned_chat_names.rb

+# 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

db/post_migrate/20200313204021_validate_foreign_key_from_chat_name_to_service.rb

+# 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

db/structure.sql

@@ -8627,6 +8627,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);
@@ -10333,6 +10335,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;
 
@@ -12879,6 +12884,10 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200312163407
 20200313101649
 20200313123934
+20200313202430
+20200313203525
+20200313203550
+20200313204021
 20200314060834
 20200316111759
 20200316162648

spec/db/schema_spec.rb

@@ -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],

spec/migrations/20200313203550_remove_orphaned_chat_names_spec.rb

+# 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

spec/models/chat_name_spec.rb

@@ -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

spec/models/project_spec.rb

@@ -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