diff options
-rw-r--r-- | app/models/project.rb | 12 | ||||
-rw-r--r-- | changelogs/unreleased/alexives-25230-add_foreign_key_to_chat_name_service_id.yml | 5 | ||||
-rw-r--r-- | db/migrate/20200313202430_add_index_chat_name_service_id.rb | 17 | ||||
-rw-r--r-- | db/migrate/20200313203525_add_invalid_foreign_key_from_chat_name_to_service.rb | 16 | ||||
-rw-r--r-- | db/post_migrate/20200313203550_remove_orphaned_chat_names.rb | 13 | ||||
-rw-r--r-- | db/post_migrate/20200313204021_validate_foreign_key_from_chat_name_to_service.rb | 14 | ||||
-rw-r--r-- | db/structure.sql | 9 | ||||
-rw-r--r-- | spec/db/schema_spec.rb | 2 | ||||
-rw-r--r-- | spec/migrations/20200313203550_remove_orphaned_chat_names_spec.rb | 27 | ||||
-rw-r--r-- | spec/models/chat_name_spec.rb | 6 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 14 |
11 files changed, 134 insertions, 1 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index 03b247f213b..93ac9b64a98 100644 --- a/app/models/project.rb +++ b/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 diff --git a/changelogs/unreleased/alexives-25230-add_foreign_key_to_chat_name_service_id.yml b/changelogs/unreleased/alexives-25230-add_foreign_key_to_chat_name_service_id.yml new file mode 100644 index 00000000000..6e2cec9588d --- /dev/null +++ b/changelogs/unreleased/alexives-25230-add_foreign_key_to_chat_name_service_id.yml @@ -0,0 +1,5 @@ +--- +title: Fix 500 error on profile/chat_names for deleted projects +merge_request: 24341 +author: +type: fixed diff --git a/db/migrate/20200313202430_add_index_chat_name_service_id.rb b/db/migrate/20200313202430_add_index_chat_name_service_id.rb new file mode 100644 index 00000000000..f4a88973751 --- /dev/null +++ b/db/migrate/20200313202430_add_index_chat_name_service_id.rb @@ -0,0 +1,17 @@ +# 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 diff --git a/db/migrate/20200313203525_add_invalid_foreign_key_from_chat_name_to_service.rb b/db/migrate/20200313203525_add_invalid_foreign_key_from_chat_name_to_service.rb new file mode 100644 index 00000000000..c6e340693f8 --- /dev/null +++ b/db/migrate/20200313203525_add_invalid_foreign_key_from_chat_name_to_service.rb @@ -0,0 +1,16 @@ +# 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 diff --git a/db/post_migrate/20200313203550_remove_orphaned_chat_names.rb b/db/post_migrate/20200313203550_remove_orphaned_chat_names.rb new file mode 100644 index 00000000000..59cd2b31772 --- /dev/null +++ b/db/post_migrate/20200313203550_remove_orphaned_chat_names.rb @@ -0,0 +1,13 @@ +# 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 diff --git a/db/post_migrate/20200313204021_validate_foreign_key_from_chat_name_to_service.rb b/db/post_migrate/20200313204021_validate_foreign_key_from_chat_name_to_service.rb new file mode 100644 index 00000000000..fd9feab17db --- /dev/null +++ b/db/post_migrate/20200313204021_validate_foreign_key_from_chat_name_to_service.rb @@ -0,0 +1,14 @@ +# 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 diff --git a/db/structure.sql b/db/structure.sql index d5bd08b41d0..86a931b6cb4 100644 --- a/db/structure.sql +++ b/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 diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index 08e4920b020..2e22e67f4e2 100644 --- a/spec/db/schema_spec.rb +++ b/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], diff --git a/spec/migrations/20200313203550_remove_orphaned_chat_names_spec.rb b/spec/migrations/20200313203550_remove_orphaned_chat_names_spec.rb new file mode 100644 index 00000000000..fd30ebaa66f --- /dev/null +++ b/spec/migrations/20200313203550_remove_orphaned_chat_names_spec.rb @@ -0,0 +1,27 @@ +# 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 diff --git a/spec/models/chat_name_spec.rb b/spec/models/chat_name_spec.rb index 863c28a86fb..02594b98665 100644 --- a/spec/models/chat_name_spec.rb +++ b/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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 508098bfc39..1b12550ebac 100644 --- a/spec/models/project_spec.rb +++ b/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 |