From d137ad34db7afbbec069ae01b141594a7a178615 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Mon, 2 Aug 2021 13:16:04 +0200 Subject: [PATCH 01/18] Migrate project topics from `acts_as_taggable` gem to separate table Changelog: changed --- .../dashboard/projects_controller.rb | 2 +- app/finders/projects_finder.rb | 9 ++- app/graphql/types/project_type.rb | 4 ++ app/models/project.rb | 39 ++++++++++++- app/models/project_topic.rb | 6 ++ app/models/topic.rb | 8 +++ db/migrate/20210729081351_create_topics.rb | 16 ++++++ .../20210729081739_create_project_topics.rb | 15 +++++ ...foreign_key_to_project_on_project_topic.rb | 17 ++++++ ...d_foreign_key_to_topic_on_project_topic.rb | 17 ++++++ ...ract_project_topics_into_separate_table.rb | 30 ++++++++++ db/schema_migrations/20210729081351 | 1 + db/schema_migrations/20210729081739 | 1 + db/schema_migrations/20210729125641 | 1 + db/schema_migrations/20210729125659 | 1 + db/schema_migrations/20210730104800 | 1 + db/structure.sql | 56 +++++++++++++++++++ lib/api/entities/basic_project_details.rb | 2 +- lib/api/entities/project.rb | 2 +- ...ract_project_topics_into_separate_table.rb | 39 +++++++++++++ spec/factories/project_topics.rb | 8 +++ spec/factories/topics.rb | 7 +++ ...project_topics_into_separate_table_spec.rb | 43 ++++++++++++++ spec/lib/gitlab/import_export/all_models.yml | 2 + spec/models/project_spec.rb | 4 +- spec/models/project_topic_spec.rb | 16 ++++++ spec/models/topic_spec.rb | 21 +++++++ 27 files changed, 361 insertions(+), 7 deletions(-) create mode 100644 app/models/project_topic.rb create mode 100644 app/models/topic.rb create mode 100644 db/migrate/20210729081351_create_topics.rb create mode 100644 db/migrate/20210729081739_create_project_topics.rb create mode 100644 db/migrate/20210729125641_add_foreign_key_to_project_on_project_topic.rb create mode 100644 db/migrate/20210729125659_add_foreign_key_to_topic_on_project_topic.rb create mode 100644 db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb create mode 100644 db/schema_migrations/20210729081351 create mode 100644 db/schema_migrations/20210729081739 create mode 100644 db/schema_migrations/20210729125641 create mode 100644 db/schema_migrations/20210729125659 create mode 100644 db/schema_migrations/20210730104800 create mode 100644 lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb create mode 100644 spec/factories/project_topics.rb create mode 100644 spec/factories/topics.rb create mode 100644 spec/lib/gitlab/background_migration/extract_project_topics_into_separate_table_spec.rb create mode 100644 spec/models/project_topic_spec.rb create mode 100644 spec/models/topic_spec.rb diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index d861ef646f889f..74ad78ff4c1daa 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -36,7 +36,7 @@ def index # rubocop: disable CodeReuse/ActiveRecord def starred @projects = load_projects(params.merge(starred: true)) - .includes(:forked_from_project, :topics) + .includes(:forked_from_project, :topics, :topics_acts_as_taggable) @groups = [] diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index dca3d12f3c99d3..6bb1df9ee8e8bc 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -180,7 +180,14 @@ def by_visibility_level(items) # rubocop: enable CodeReuse/ActiveRecord def by_topics(items) - params[:topic].present? ? items.tagged_with(params[:topic]) : items + return items unless params[:topic].present? + + topics = params[:topic].instance_of?(String) ? params[:topic].strip.split(/\s*,\s*/) : params[:topic] + topics.each do |topic| + items = items.with_topic(topic) + end + + items end def by_search(items) diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index af1f1c54ec2d39..4530826dd859fb 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -406,6 +406,10 @@ def tag_list object.topic_list end + def topics + object.topic_list + end + private def project diff --git a/app/models/project.rb b/app/models/project.rb index 81b04e1316c7c4..db4dcd3d14f990 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -127,7 +127,24 @@ class Project < ApplicationRecord after_initialize :use_hashed_storage after_create :check_repository_absence! + # Required during the `ActsAsTaggableOn::Tag -> Topic` migration + # TODO: remove 'acts_as_ordered_taggable_on' and ':topics_acts_as_taggable' in the further process of the migration + # https://gitlab.com/gitlab-org/gitlab/-/issues/335946 acts_as_ordered_taggable_on :topics + has_many :topics_acts_as_taggable, -> { order("#{ActsAsTaggableOn::Tagging.table_name}.id") }, + class_name: 'ActsAsTaggableOn::Tag', + through: :topic_taggings, + source: :tag + + has_many :project_topics + has_many :topics, through: :project_topics + + # Required during the `ActsAsTaggableOn::Tag -> Topic` migration + # TODO: remove 'topics' in the further process of the migration + # https://gitlab.com/gitlab-org/gitlab/-/issues/335946 + def topics + self.topics_acts_as_taggable + super + end attr_accessor :old_path_with_namespace attr_accessor :template_name @@ -623,6 +640,11 @@ def self.integration_association_name(name) joins(:service_desk_setting).where('service_desk_settings.project_key' => key) end + scope :with_topic, ->(topic) do + topic = Topic.find_by_name(topic) + topic ? where(id: topic.project_topics.pluck(:project_id)) : none + end + enum auto_cancel_pending_pipelines: { disabled: 0, enabled: 1 } chronic_duration_attr :build_timeout_human_readable, :build_timeout, @@ -638,7 +660,7 @@ def self.integration_association_name(name) mount_uploader :bfg_object_map, AttachmentUploader def self.with_api_entity_associations - preload(:project_feature, :route, :topics, :group, :timelogs, namespace: [:route, :owner]) + preload(:project_feature, :route, :topics, :topics_acts_as_taggable, :group, :timelogs, namespace: [:route, :owner]) end def self.with_web_entity_associations @@ -2669,6 +2691,21 @@ def group_runners_enabled? ci_cd_settings.group_runners_enabled? end + def topic_list + self.topics.map(&:name) + end + + def topic_list=(value) + value = value.strip.split(/\s*,\s*/) if value.instance_of?(String) + self.topics = value.map { |topic| Topic.find_or_create_by(name: topic) } + + # Remove old topics (ActsAsTaggableOn::Tag) + # Required during the `ActsAsTaggableOn::Tag -> Topic` migration + # TODO: remove in the further process of the migration + # https://gitlab.com/gitlab-org/gitlab/-/issues/335946 + self.topic_taggings.clear + end + private def find_integration(integrations, name) diff --git a/app/models/project_topic.rb b/app/models/project_topic.rb new file mode 100644 index 00000000000000..acce057ed0cac8 --- /dev/null +++ b/app/models/project_topic.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +class ProjectTopic < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass + belongs_to :project + belongs_to :topic +end diff --git a/app/models/topic.rb b/app/models/topic.rb new file mode 100644 index 00000000000000..0b288691e8dffa --- /dev/null +++ b/app/models/topic.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class Topic < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass + validates :name, presence: true, uniqueness: true + + has_many :project_topics + has_many :projects, through: :project_topics +end diff --git a/db/migrate/20210729081351_create_topics.rb b/db/migrate/20210729081351_create_topics.rb new file mode 100644 index 00000000000000..c6fdc6bb98a8cf --- /dev/null +++ b/db/migrate/20210729081351_create_topics.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class CreateTopics < ActiveRecord::Migration[6.1] + include Gitlab::Database::MigrationHelpers + + def change + create_table_with_constraints :topics do |t| + t.text :name, null: false + t.text_limit :name, 255 + + t.index :name, unique: true + + t.timestamps_with_timezone + end + end +end diff --git a/db/migrate/20210729081739_create_project_topics.rb b/db/migrate/20210729081739_create_project_topics.rb new file mode 100644 index 00000000000000..b281ff744af422 --- /dev/null +++ b/db/migrate/20210729081739_create_project_topics.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class CreateProjectTopics < ActiveRecord::Migration[6.1] + def change + create_table :project_topics do |t| + t.bigint :project_id, null: false + t.bigint :topic_id, null: false + + t.index :project_id + t.index :topic_id + + t.timestamps_with_timezone + end + end +end diff --git a/db/migrate/20210729125641_add_foreign_key_to_project_on_project_topic.rb b/db/migrate/20210729125641_add_foreign_key_to_project_on_project_topic.rb new file mode 100644 index 00000000000000..27cf5c60cf0e73 --- /dev/null +++ b/db/migrate/20210729125641_add_foreign_key_to_project_on_project_topic.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddForeignKeyToProjectOnProjectTopic < ActiveRecord::Migration[6.1] + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def up + add_concurrent_foreign_key :project_topics, :projects, column: :project_id, on_delete: :cascade + end + + def down + with_lock_retries do + remove_foreign_key :project_topics, column: :project_id + end + end +end diff --git a/db/migrate/20210729125659_add_foreign_key_to_topic_on_project_topic.rb b/db/migrate/20210729125659_add_foreign_key_to_topic_on_project_topic.rb new file mode 100644 index 00000000000000..1ada08dca1a2d3 --- /dev/null +++ b/db/migrate/20210729125659_add_foreign_key_to_topic_on_project_topic.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddForeignKeyToTopicOnProjectTopic < ActiveRecord::Migration[6.1] + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def up + add_concurrent_foreign_key :project_topics, :topics, column: :topic_id, on_delete: :cascade + end + + def down + with_lock_retries do + remove_foreign_key :project_topics, column: :topic_id + end + end +end diff --git a/db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb b/db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb new file mode 100644 index 00000000000000..9c8db1475b66bb --- /dev/null +++ b/db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +class ScheduleExtractProjectTopicsIntoSeparateTable < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + BATCH_SIZE = 30_000 + DELAY_INTERVAL = 2.minutes + MIGRATION = 'ExtractProjectTopicsIntoSeparateTable' + + disable_ddl_transaction! + + class Tagging < ActiveRecord::Base + include ::EachBatch + + self.table_name = 'taggings' + end + + def up + queue_background_migration_jobs_by_range_at_intervals( + Tagging.where(taggable_type: 'Project'), + MIGRATION, + DELAY_INTERVAL, + batch_size: BATCH_SIZE + ) + end + + def down + end +end diff --git a/db/schema_migrations/20210729081351 b/db/schema_migrations/20210729081351 new file mode 100644 index 00000000000000..1f3974574438f3 --- /dev/null +++ b/db/schema_migrations/20210729081351 @@ -0,0 +1 @@ +17463867a8c14981386256dc90169fb879e1921d65eccca53eae576d49fba49d \ No newline at end of file diff --git a/db/schema_migrations/20210729081739 b/db/schema_migrations/20210729081739 new file mode 100644 index 00000000000000..2215c7d7e666ff --- /dev/null +++ b/db/schema_migrations/20210729081739 @@ -0,0 +1 @@ +af7963d27bda6ef85fb5b5a06ecf1de14f21829eecdaf13e763aa9a6ffc2e83c \ No newline at end of file diff --git a/db/schema_migrations/20210729125641 b/db/schema_migrations/20210729125641 new file mode 100644 index 00000000000000..e5ee4127656c45 --- /dev/null +++ b/db/schema_migrations/20210729125641 @@ -0,0 +1 @@ +b7bc495d010e0640b1145ca55f47696047fd4360d2dfc9a3da7941ab62840132 \ No newline at end of file diff --git a/db/schema_migrations/20210729125659 b/db/schema_migrations/20210729125659 new file mode 100644 index 00000000000000..64c8cd0aeef876 --- /dev/null +++ b/db/schema_migrations/20210729125659 @@ -0,0 +1 @@ +5826e87b2ce13d4951e9b8e774c87c29c6e0a0954a85d60ec68155f2c5cf3ccc \ No newline at end of file diff --git a/db/schema_migrations/20210730104800 b/db/schema_migrations/20210730104800 new file mode 100644 index 00000000000000..d8e2986e9465b0 --- /dev/null +++ b/db/schema_migrations/20210730104800 @@ -0,0 +1 @@ +7764c058665015707aff6e25ccbf60d4a329c67c16106b2ef523862ef82298b7 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index eee73636eb1565..e019f5d842519f 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -17333,6 +17333,23 @@ CREATE SEQUENCE project_statistics_id_seq ALTER SEQUENCE project_statistics_id_seq OWNED BY project_statistics.id; +CREATE TABLE project_topics ( + id bigint NOT NULL, + project_id bigint NOT NULL, + topic_id bigint NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL +); + +CREATE SEQUENCE project_topics_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE project_topics_id_seq OWNED BY project_topics.id; + CREATE TABLE project_tracing_settings ( id bigint NOT NULL, created_at timestamp with time zone NOT NULL, @@ -18789,6 +18806,23 @@ CREATE SEQUENCE token_with_ivs_id_seq ALTER SEQUENCE token_with_ivs_id_seq OWNED BY token_with_ivs.id; +CREATE TABLE topics ( + id bigint NOT NULL, + name text NOT NULL, + created_at timestamp with time zone NOT NULL, + updated_at timestamp with time zone NOT NULL, + CONSTRAINT check_7a90d4c757 CHECK ((char_length(name) <= 255)) +); + +CREATE SEQUENCE topics_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE topics_id_seq OWNED BY topics.id; + CREATE TABLE trending_projects ( id integer NOT NULL, project_id integer NOT NULL @@ -20727,6 +20761,8 @@ ALTER TABLE ONLY project_security_settings ALTER COLUMN project_id SET DEFAULT n ALTER TABLE ONLY project_statistics ALTER COLUMN id SET DEFAULT nextval('project_statistics_id_seq'::regclass); +ALTER TABLE ONLY project_topics ALTER COLUMN id SET DEFAULT nextval('project_topics_id_seq'::regclass); + ALTER TABLE ONLY project_tracing_settings ALTER COLUMN id SET DEFAULT nextval('project_tracing_settings_id_seq'::regclass); ALTER TABLE ONLY projects ALTER COLUMN id SET DEFAULT nextval('projects_id_seq'::regclass); @@ -20855,6 +20891,8 @@ ALTER TABLE ONLY todos ALTER COLUMN id SET DEFAULT nextval('todos_id_seq'::regcl ALTER TABLE ONLY token_with_ivs ALTER COLUMN id SET DEFAULT nextval('token_with_ivs_id_seq'::regclass); +ALTER TABLE ONLY topics ALTER COLUMN id SET DEFAULT nextval('topics_id_seq'::regclass); + ALTER TABLE ONLY trending_projects ALTER COLUMN id SET DEFAULT nextval('trending_projects_id_seq'::regclass); ALTER TABLE ONLY u2f_registrations ALTER COLUMN id SET DEFAULT nextval('u2f_registrations_id_seq'::regclass); @@ -22356,6 +22394,9 @@ ALTER TABLE ONLY project_settings ALTER TABLE ONLY project_statistics ADD CONSTRAINT project_statistics_pkey PRIMARY KEY (id); +ALTER TABLE ONLY project_topics + ADD CONSTRAINT project_topics_pkey PRIMARY KEY (id); + ALTER TABLE ONLY project_tracing_settings ADD CONSTRAINT project_tracing_settings_pkey PRIMARY KEY (id); @@ -22569,6 +22610,9 @@ ALTER TABLE ONLY todos ALTER TABLE ONLY token_with_ivs ADD CONSTRAINT token_with_ivs_pkey PRIMARY KEY (id); +ALTER TABLE ONLY topics + ADD CONSTRAINT topics_pkey PRIMARY KEY (id); + ALTER TABLE ONLY trending_projects ADD CONSTRAINT trending_projects_pkey PRIMARY KEY (id); @@ -24955,6 +24999,10 @@ CREATE INDEX index_project_statistics_on_storage_size_and_project_id ON project_ CREATE INDEX index_project_statistics_on_wiki_size_and_project_id ON project_statistics USING btree (wiki_size, project_id); +CREATE INDEX index_project_topics_on_project_id ON project_topics USING btree (project_id); + +CREATE INDEX index_project_topics_on_topic_id ON project_topics USING btree (topic_id); + CREATE UNIQUE INDEX index_project_tracing_settings_on_project_id ON project_tracing_settings USING btree (project_id); CREATE INDEX index_projects_aimed_for_deletion ON projects USING btree (marked_for_deletion_at) WHERE ((marked_for_deletion_at IS NOT NULL) AND (pending_delete = false)); @@ -25459,6 +25507,8 @@ CREATE UNIQUE INDEX index_token_with_ivs_on_hashed_plaintext_token ON token_with CREATE UNIQUE INDEX index_token_with_ivs_on_hashed_token ON token_with_ivs USING btree (hashed_token); +CREATE UNIQUE INDEX index_topics_on_name ON topics USING btree (name); + CREATE UNIQUE INDEX index_trending_projects_on_project_id ON trending_projects USING btree (project_id); CREATE INDEX index_u2f_registrations_on_key_handle ON u2f_registrations USING btree (key_handle); @@ -26281,6 +26331,9 @@ ALTER TABLE ONLY ci_group_variables ALTER TABLE ONLY namespaces ADD CONSTRAINT fk_3448c97865 FOREIGN KEY (push_rule_id) REFERENCES push_rules(id) ON DELETE SET NULL; +ALTER TABLE ONLY project_topics + ADD CONSTRAINT fk_34af9ab07a FOREIGN KEY (topic_id) REFERENCES topics(id) ON DELETE CASCADE; + ALTER TABLE ONLY in_product_marketing_emails ADD CONSTRAINT fk_35c9101b63 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE; @@ -26800,6 +26853,9 @@ ALTER TABLE ONLY project_group_links ALTER TABLE ONLY security_scans ADD CONSTRAINT fk_dbc89265b9 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY project_topics + ADD CONSTRAINT fk_db13576296 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY epics ADD CONSTRAINT fk_dccd3f98fc FOREIGN KEY (assignee_id) REFERENCES users(id) ON DELETE SET NULL; diff --git a/lib/api/entities/basic_project_details.rb b/lib/api/entities/basic_project_details.rb index 0b231906ccd8d7..49d20a309fc2f7 100644 --- a/lib/api/entities/basic_project_details.rb +++ b/lib/api/entities/basic_project_details.rb @@ -43,7 +43,7 @@ def self.preload_relation(projects_relation, options = {}) # N+1 is solved then by using `subject.topics.map(&:name)` # MR describing the solution: https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/20555 projects_relation.preload(:project_feature, :route) - .preload(:import_state, :topics) + .preload(:import_state, :topics, :topics_acts_as_taggable) .preload(:auto_devops) .preload(namespace: [:route, :owner]) end diff --git a/lib/api/entities/project.rb b/lib/api/entities/project.rb index 890b42ed8c8cc8..156870f0e1a435 100644 --- a/lib/api/entities/project.rb +++ b/lib/api/entities/project.rb @@ -140,7 +140,7 @@ def self.preload_relation(projects_relation, options = {}) .preload(project_group_links: { group: :route }, fork_network: :root_project, fork_network_member: :forked_from_project, - forked_from_project: [:route, :topics, :group, :project_feature, namespace: [:route, :owner]]) + forked_from_project: [:route, :topics, :topics_acts_as_taggable, :group, :project_feature, namespace: [:route, :owner]]) end # rubocop: enable CodeReuse/ActiveRecord diff --git a/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb b/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb new file mode 100644 index 00000000000000..808a9663f0ff3e --- /dev/null +++ b/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # The class to extract the project topics into a separate `topics` table + class ExtractProjectTopicsIntoSeparateTable + # Temporary AR table for tags + class Tag < ActiveRecord::Base + self.table_name = 'tags' + end + + # Temporary AR table for taggings + class Tagging < ActiveRecord::Base + self.table_name = 'taggings' + belongs_to :tag + end + + # Temporary AR table for topics + class Topic < ActiveRecord::Base + self.table_name = 'topics' + end + + # Temporary AR table for project topics + class ProjectTopic < ActiveRecord::Base + self.table_name = 'project_topics' + belongs_to :topic + end + + def perform(start_id, stop_id) + Tagging.where(taggable_type: 'Project', id: start_id..stop_id).each do |tagging| + topic = Topic.find_or_create_by(name: tagging.tag.name) + ProjectTopic.create(project_id: tagging.taggable_id, topic: topic) + + tagging.delete + end + end + end + end +end diff --git a/spec/factories/project_topics.rb b/spec/factories/project_topics.rb new file mode 100644 index 00000000000000..1dbaa2dd8bdbc2 --- /dev/null +++ b/spec/factories/project_topics.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :project_topic do + association :project, factory: :project + association :topic, factory: :topic + end +end diff --git a/spec/factories/topics.rb b/spec/factories/topics.rb new file mode 100644 index 00000000000000..72de345b53c19a --- /dev/null +++ b/spec/factories/topics.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :topic do + name { generate(:name) } + end +end diff --git a/spec/lib/gitlab/background_migration/extract_project_topics_into_separate_table_spec.rb b/spec/lib/gitlab/background_migration/extract_project_topics_into_separate_table_spec.rb new file mode 100644 index 00000000000000..8bab234757de94 --- /dev/null +++ b/spec/lib/gitlab/background_migration/extract_project_topics_into_separate_table_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BackgroundMigration::ExtractProjectTopicsIntoSeparateTable, schema: 20210730104800 do + it 'correctly extracts project topics into separate table' do + namespaces = table(:namespaces) + projects = table(:projects) + taggings = table(:taggings) + tags = table(:tags) + project_topics = table(:project_topics) + topics = table(:topics) + + namespace = namespaces.create!(name: 'foo', path: 'foo') + project = projects.create!(namespace_id: namespace.id) + tag_1 = tags.create!(name: 'Topic1') + tag_2 = tags.create!(name: 'Topic2') + tag_3 = tags.create!(name: 'Topic3') + topic_3 = topics.create!(name: 'Topic3') + tagging_1 = taggings.create!(taggable_type: 'Project', taggable_id: project.id, context: 'topics', tag_id: tag_1.id) + tagging_2a = taggings.create!(taggable_type: 'Project', taggable_id: project.id, context: 'topics', tag_id: tag_2.id) + tagging_2b = taggings.create!(taggable_type: 'Project', taggable_id: project.id, context: 'topics', tag_id: tag_2.id) + other_tagging = taggings.create!(taggable_type: 'Other', taggable_id: project.id, context: 'topics', tag_id: tag_1.id) + tagging_3 = taggings.create!(taggable_type: 'Project', taggable_id: project.id, context: 'topics', tag_id: tag_3.id) + + subject.perform(tagging_1.id, tagging_3.id) + + # Tagging records + expect { tagging_1.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { tagging_2a.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { tagging_2b.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { other_tagging.reload }.not_to raise_error(ActiveRecord::RecordNotFound) + expect { tagging_3.reload }.to raise_error(ActiveRecord::RecordNotFound) + + # Topic records + topic_1 = topics.find_by(name: 'Topic1') + topic_2 = topics.find_by(name: 'Topic2') + expect(topics.all).to contain_exactly(topic_1, topic_2, topic_3) + + # ProjectTopic records + expect(project_topics.all.map(&:topic_id)).to contain_exactly(topic_1.id, topic_2.id, topic_2.id, topic_3.id) + end +end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 2b7138a7a10dcd..d83a00589b125c 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -354,6 +354,8 @@ project: - taggings - base_tags - topic_taggings +- topics_acts_as_taggable +- project_topics - topics - chat_services - cluster diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index d8f3a63d221137..441afd29c12bbc 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -7137,8 +7137,8 @@ def subject expect(project.topic_list).to match_array(%w[topic1 topic2 topic3]) end - it 'topics returns correct tag records' do - expect(project.topics.first.class.name).to eq('ActsAsTaggableOn::Tag') + it 'topics returns correct topic records' do + expect(project.topics.first.class.name).to eq('Topic') expect(project.topics.map(&:name)).to match_array(%w[topic1 topic2 topic3]) end end diff --git a/spec/models/project_topic_spec.rb b/spec/models/project_topic_spec.rb new file mode 100644 index 00000000000000..4f3147a31100d0 --- /dev/null +++ b/spec/models/project_topic_spec.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ProjectTopic do + let_it_be(:project_topic, reload: true) { create(:project_topic) } + + subject { project_topic } + + it { expect(subject).to be_valid } + + describe 'associations' do + it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:topic) } + end +end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb new file mode 100644 index 00000000000000..e42377b3e77aa8 --- /dev/null +++ b/spec/models/topic_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Topic do + let_it_be(:topic, reload: true) { create(:topic) } + + subject { topic } + + it { expect(subject).to be_valid } + + describe 'associations' do + it { is_expected.to have_many(:project_topics) } + it { is_expected.to have_many(:projects) } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:name) } + it { is_expected.to validate_uniqueness_of(:name) } + end +end -- GitLab From 11169dfda83c8ec28688d43fc039becbf06de831 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Thu, 12 Aug 2021 14:18:59 +0200 Subject: [PATCH 02/18] Fix :with_topic to consider old topics as well --- app/models/project.rb | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index db4dcd3d14f990..79f059f2fcc929 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -640,9 +640,17 @@ def self.integration_association_name(name) joins(:service_desk_setting).where('service_desk_settings.project_key' => key) end - scope :with_topic, ->(topic) do - topic = Topic.find_by_name(topic) - topic ? where(id: topic.project_topics.pluck(:project_id)) : none + scope :with_topic, ->(topic_name) do + topic = Topic.find_by_name(topic_name) + acts_as_taggable_on_topic = ActsAsTaggableOn::Tag.find_by_name(topic_name) + + return self.where(id: topic.project_topics.select(:project_id)).or(self.where(id: acts_as_taggable_on_topic.taggings.select(:taggable_id))) if topic && acts_as_taggable_on_topic + + return self.where(id: topic.project_topics.select(:project_id)) if topic + + return self.where(id: acts_as_taggable_on_topic.taggings.select(:taggable_id)) if acts_as_taggable_on_topic + + none end enum auto_cancel_pending_pipelines: { disabled: 0, enabled: 1 } -- GitLab From e77eb2c34d47ca8223443ac471a93e88aa5f2b82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Thu, 12 Aug 2021 14:40:35 +0200 Subject: [PATCH 03/18] Refactor and test topic_list= --- app/models/project.rb | 17 +++++--- ...ract_project_topics_into_separate_table.rb | 2 +- spec/models/project_spec.rb | 41 +++++++++++++++++++ 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 79f059f2fcc929..7d1182a7f1a3cb 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -103,6 +103,8 @@ class Project < ApplicationRecord after_save :create_import_state, if: ->(project) { project.import? && project.import_state.nil? } + after_save :save_topics + after_create -> { create_or_load_association(:project_feature) } after_create -> { create_or_load_association(:ci_cd_settings) } @@ -150,6 +152,7 @@ def topics attr_accessor :template_name attr_writer :pipeline_status attr_accessor :skip_disk_validation + attr_writer :topic_list alias_attribute :title, :name @@ -2703,9 +2706,15 @@ def topic_list self.topics.map(&:name) end - def topic_list=(value) - value = value.strip.split(/\s*,\s*/) if value.instance_of?(String) - self.topics = value.map { |topic| Topic.find_or_create_by(name: topic) } + private + + def save_topics + return if @topic_list.nil? + + @topic_list = @topic_list.split(',') if @topic_list.instance_of?(String) + @topic_list = @topic_list.map(&:strip).uniq + self.topics = @topic_list.map { |topic| Topic.safe_find_or_create_by(name: topic) } + @topic_list = nil # Remove old topics (ActsAsTaggableOn::Tag) # Required during the `ActsAsTaggableOn::Tag -> Topic` migration @@ -2714,8 +2723,6 @@ def topic_list=(value) self.topic_taggings.clear end - private - def find_integration(integrations, name) integrations.find { _1.to_param == name } end diff --git a/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb b/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb index 808a9663f0ff3e..1ee1b4cee14e21 100644 --- a/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb +++ b/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb @@ -27,7 +27,7 @@ class ProjectTopic < ActiveRecord::Base end def perform(start_id, stop_id) - Tagging.where(taggable_type: 'Project', id: start_id..stop_id).each do |tagging| + Tagging.includes(:tag).where(taggable_type: 'Project', id: start_id..stop_id).each do |tagging| topic = Topic.find_or_create_by(name: tagging.tag.name) ProjectTopic.create(project_id: tagging.taggable_id, topic: topic) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 441afd29c12bbc..cfee8db246a235 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -7141,6 +7141,47 @@ def subject expect(project.topics.first.class.name).to eq('Topic') expect(project.topics.map(&:name)).to match_array(%w[topic1 topic2 topic3]) end + + context 'topic_list=' do + using RSpec::Parameterized::TableSyntax + + where(:topic_list, :expected_result) do + ['topicA', 'topicB'] | %w[topicA topicB] # rubocop:disable Style/WordArray, Lint/BinaryOperatorWithIdenticalOperands + ['topicB', 'topicA'] | %w[topicB topicA] # rubocop:disable Style/WordArray, Lint/BinaryOperatorWithIdenticalOperands + [' topicC ', ' topicD '] | %w[topicC topicD] + ['topicE', 'topicF', 'topicE'] | %w[topicE topicF] # rubocop:disable Style/WordArray + ['topicE ', 'topicF', ' topicE'] | %w[topicE topicF] + 'topicA, topicB' | %w[topicA topicB] + 'topicB, topicA' | %w[topicB topicA] + ' topicC , topicD ' | %w[topicC topicD] + 'topicE, topicF, topicE' | %w[topicE topicF] + 'topicE , topicF, topicE' | %w[topicE topicF] + end + + with_them do + it 'set topics' do + project.topic_list = topic_list + project.save! + + expect(project.topics.map(&:name)).to match_array(expected_result) + end + end + + it 'does not persist topics before project is saved' do + project.topic_list = 'topicA, topicB' + + expect(project.topics.map(&:name)).to match_array(%w[topic1 topic2 topic3]) + end + + it 'does not update topics if project is not valid' do + project.name = nil + project.topic_list = 'topicA, topicB' + + expect(project.save).to be_falsy + expect(project.topics.map(&:name)).to match_array(%w[topic1 topic2 topic3]) + project.reload + end + end end shared_examples 'all_runners' do -- GitLab From dadf18b2d51c8dca6e0b088a284f06a6657a176b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Fri, 13 Aug 2021 10:39:39 +0200 Subject: [PATCH 04/18] Fix topic specs --- ee/lib/gitlab/elastic/search_results.rb | 2 +- spec/services/projects/create_service_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ee/lib/gitlab/elastic/search_results.rb b/ee/lib/gitlab/elastic/search_results.rb index 01d71dc506338b..eead80343330fa 100644 --- a/ee/lib/gitlab/elastic/search_results.rb +++ b/ee/lib/gitlab/elastic/search_results.rb @@ -28,7 +28,7 @@ def objects(scope, page: 1, per_page: DEFAULT_PER_PAGE, preload_method: nil) case scope when 'projects' - eager_load(projects, page, per_page, preload_method, [:route, :namespace, :topics]) + eager_load(projects, page, per_page, preload_method, [:route, :namespace, :topics, :topics_acts_as_taggable]) # HERE when 'issues' eager_load(issues, page, per_page, preload_method, project: [:route, :namespace], labels: [], timelogs: [], assignees: []) when 'merge_requests' diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index c3928563125107..cd8a56e59c4358 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -86,7 +86,7 @@ subject(:project) { create_project(user, opts) } context "with 'topics' parameter" do - let(:opts) { { topics: 'topics' } } + let(:opts) { { name: 'topic-project', topics: 'topics' } } it 'keeps them as specified' do expect(project.topic_list).to eq(%w[topics]) @@ -94,7 +94,7 @@ end context "with 'topic_list' parameter" do - let(:opts) { { topic_list: 'topic_list' } } + let(:opts) { { name: 'topic-project', topic_list: 'topic_list' } } it 'keeps them as specified' do expect(project.topic_list).to eq(%w[topic_list]) @@ -102,7 +102,7 @@ end context "with 'tag_list' parameter (deprecated)" do - let(:opts) { { tag_list: 'tag_list' } } + let(:opts) { { name: 'topic-project', tag_list: 'tag_list' } } it 'keeps them as specified' do expect(project.topic_list).to eq(%w[tag_list]) -- GitLab From 58abc9713e43fcf2cd7a24a912b71f09c9fa4443 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Fri, 13 Aug 2021 10:56:26 +0200 Subject: [PATCH 05/18] Add unique index on project_topics --- db/migrate/20210729081739_create_project_topics.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/db/migrate/20210729081739_create_project_topics.rb b/db/migrate/20210729081739_create_project_topics.rb index b281ff744af422..cbb8842f653632 100644 --- a/db/migrate/20210729081739_create_project_topics.rb +++ b/db/migrate/20210729081739_create_project_topics.rb @@ -8,6 +8,7 @@ def change t.index :project_id t.index :topic_id + t.index [:project_id, :topic_id], unique: true t.timestamps_with_timezone end -- GitLab From 9a0194f70031f51560bd23028ea620784008e98a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Fri, 13 Aug 2021 13:29:03 +0200 Subject: [PATCH 06/18] Optimize background migration of project topics --- ...ry_index_for_project_topics_on_taggings.rb | 19 +++++++++++++++++++ ...ry_index_for_project_topics_on_taggings.rb | 19 +++++++++++++++++++ ...ract_project_topics_into_separate_table.rb | 5 +++-- db/schema_migrations/20210730104759 | 1 + db/schema_migrations/20210730104801 | 1 + db/structure.sql | 8 +++++--- ...project_topics_into_separate_table_spec.rb | 8 +++----- 7 files changed, 51 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20210730104759_add_temporary_index_for_project_topics_on_taggings.rb create mode 100644 db/migrate/20210730104801_remove_temporary_index_for_project_topics_on_taggings.rb create mode 100644 db/schema_migrations/20210730104759 create mode 100644 db/schema_migrations/20210730104801 diff --git a/db/migrate/20210730104759_add_temporary_index_for_project_topics_on_taggings.rb b/db/migrate/20210730104759_add_temporary_index_for_project_topics_on_taggings.rb new file mode 100644 index 00000000000000..b786a39007bd29 --- /dev/null +++ b/db/migrate/20210730104759_add_temporary_index_for_project_topics_on_taggings.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddTemporaryIndexForProjectTopicsOnTaggings < ActiveRecord::Migration[6.1] + include Gitlab::Database::MigrationHelpers + + INDEX_NAME = 'tmp_index_taggings_on_id_where_taggable_type_project' + INDEX_CONDITION = "taggable_type = 'Project'" + + disable_ddl_transaction! + + def up + # this index is used in 20210730104800_schedule_extract_project_topics_into_separate_table + add_concurrent_index :taggings, :id, where: INDEX_CONDITION, name: INDEX_NAME + end + + def down + remove_concurrent_index_by_name :taggings, INDEX_NAME + end +end diff --git a/db/migrate/20210730104801_remove_temporary_index_for_project_topics_on_taggings.rb b/db/migrate/20210730104801_remove_temporary_index_for_project_topics_on_taggings.rb new file mode 100644 index 00000000000000..9442e05cfe2218 --- /dev/null +++ b/db/migrate/20210730104801_remove_temporary_index_for_project_topics_on_taggings.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class RemoveTemporaryIndexForProjectTopicsOnTaggings < ActiveRecord::Migration[6.1] + include Gitlab::Database::MigrationHelpers + + INDEX_NAME = 'tmp_index_taggings_on_id_where_taggable_type_project' + INDEX_CONDITION = "taggable_type = 'Project'" + + disable_ddl_transaction! + + def up + # this index was used in 20210730104800_schedule_extract_project_topics_into_separate_table + remove_concurrent_index_by_name :taggings, INDEX_NAME + end + + def down + add_concurrent_index :taggings, :id, where: INDEX_CONDITION, name: INDEX_NAME + end +end diff --git a/db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb b/db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb index 9c8db1475b66bb..8adda3b54e8983 100644 --- a/db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb +++ b/db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb @@ -4,7 +4,7 @@ class ScheduleExtractProjectTopicsIntoSeparateTable < ActiveRecord::Migration[6. include Gitlab::Database::MigrationHelpers DOWNTIME = false - BATCH_SIZE = 30_000 + BATCH_SIZE = 1_000 DELAY_INTERVAL = 2.minutes MIGRATION = 'ExtractProjectTopicsIntoSeparateTable' @@ -21,7 +21,8 @@ def up Tagging.where(taggable_type: 'Project'), MIGRATION, DELAY_INTERVAL, - batch_size: BATCH_SIZE + batch_size: BATCH_SIZE, + track_jobs: true ) end diff --git a/db/schema_migrations/20210730104759 b/db/schema_migrations/20210730104759 new file mode 100644 index 00000000000000..c33c70182a9a12 --- /dev/null +++ b/db/schema_migrations/20210730104759 @@ -0,0 +1 @@ +270e4fd75e41d0b0fa68f0e667a96011f54fbb42fb248b9ee68dc569e8822fbd \ No newline at end of file diff --git a/db/schema_migrations/20210730104801 b/db/schema_migrations/20210730104801 new file mode 100644 index 00000000000000..8bd743a59da597 --- /dev/null +++ b/db/schema_migrations/20210730104801 @@ -0,0 +1 @@ +e95d930ad7f5072b91b67c3f9696053e5405354d5aa8f1fc06988dd8a93f035f \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index e019f5d842519f..fe27cd3f2fca64 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -25001,6 +25001,8 @@ CREATE INDEX index_project_statistics_on_wiki_size_and_project_id ON project_sta CREATE INDEX index_project_topics_on_project_id ON project_topics USING btree (project_id); +CREATE UNIQUE INDEX index_project_topics_on_project_id_and_topic_id ON project_topics USING btree (project_id, topic_id); + CREATE INDEX index_project_topics_on_topic_id ON project_topics USING btree (topic_id); CREATE UNIQUE INDEX index_project_tracing_settings_on_project_id ON project_tracing_settings USING btree (project_id); @@ -26850,12 +26852,12 @@ ALTER TABLE ONLY label_links ALTER TABLE ONLY project_group_links ADD CONSTRAINT fk_daa8cee94c FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; -ALTER TABLE ONLY security_scans - ADD CONSTRAINT fk_dbc89265b9 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; - ALTER TABLE ONLY project_topics ADD CONSTRAINT fk_db13576296 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY security_scans + ADD CONSTRAINT fk_dbc89265b9 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; + ALTER TABLE ONLY epics ADD CONSTRAINT fk_dccd3f98fc FOREIGN KEY (assignee_id) REFERENCES users(id) ON DELETE SET NULL; diff --git a/spec/lib/gitlab/background_migration/extract_project_topics_into_separate_table_spec.rb b/spec/lib/gitlab/background_migration/extract_project_topics_into_separate_table_spec.rb index 8bab234757de94..d8f8fda3e579ec 100644 --- a/spec/lib/gitlab/background_migration/extract_project_topics_into_separate_table_spec.rb +++ b/spec/lib/gitlab/background_migration/extract_project_topics_into_separate_table_spec.rb @@ -18,8 +18,7 @@ tag_3 = tags.create!(name: 'Topic3') topic_3 = topics.create!(name: 'Topic3') tagging_1 = taggings.create!(taggable_type: 'Project', taggable_id: project.id, context: 'topics', tag_id: tag_1.id) - tagging_2a = taggings.create!(taggable_type: 'Project', taggable_id: project.id, context: 'topics', tag_id: tag_2.id) - tagging_2b = taggings.create!(taggable_type: 'Project', taggable_id: project.id, context: 'topics', tag_id: tag_2.id) + tagging_2 = taggings.create!(taggable_type: 'Project', taggable_id: project.id, context: 'topics', tag_id: tag_2.id) other_tagging = taggings.create!(taggable_type: 'Other', taggable_id: project.id, context: 'topics', tag_id: tag_1.id) tagging_3 = taggings.create!(taggable_type: 'Project', taggable_id: project.id, context: 'topics', tag_id: tag_3.id) @@ -27,8 +26,7 @@ # Tagging records expect { tagging_1.reload }.to raise_error(ActiveRecord::RecordNotFound) - expect { tagging_2a.reload }.to raise_error(ActiveRecord::RecordNotFound) - expect { tagging_2b.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect { tagging_2.reload }.to raise_error(ActiveRecord::RecordNotFound) expect { other_tagging.reload }.not_to raise_error(ActiveRecord::RecordNotFound) expect { tagging_3.reload }.to raise_error(ActiveRecord::RecordNotFound) @@ -38,6 +36,6 @@ expect(topics.all).to contain_exactly(topic_1, topic_2, topic_3) # ProjectTopic records - expect(project_topics.all.map(&:topic_id)).to contain_exactly(topic_1.id, topic_2.id, topic_2.id, topic_3.id) + expect(project_topics.all.map(&:topic_id)).to contain_exactly(topic_1.id, topic_2.id, topic_3.id) end end -- GitLab From c737ff95cb5ed357245c687e8ebc5551d42d0ec2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Mon, 16 Aug 2021 10:03:58 +0200 Subject: [PATCH 07/18] Remove migration to remove temp index again --- ...ry_index_for_project_topics_on_taggings.rb | 19 ------------------- db/schema_migrations/20210730104801 | 1 - 2 files changed, 20 deletions(-) delete mode 100644 db/migrate/20210730104801_remove_temporary_index_for_project_topics_on_taggings.rb delete mode 100644 db/schema_migrations/20210730104801 diff --git a/db/migrate/20210730104801_remove_temporary_index_for_project_topics_on_taggings.rb b/db/migrate/20210730104801_remove_temporary_index_for_project_topics_on_taggings.rb deleted file mode 100644 index 9442e05cfe2218..00000000000000 --- a/db/migrate/20210730104801_remove_temporary_index_for_project_topics_on_taggings.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -class RemoveTemporaryIndexForProjectTopicsOnTaggings < ActiveRecord::Migration[6.1] - include Gitlab::Database::MigrationHelpers - - INDEX_NAME = 'tmp_index_taggings_on_id_where_taggable_type_project' - INDEX_CONDITION = "taggable_type = 'Project'" - - disable_ddl_transaction! - - def up - # this index was used in 20210730104800_schedule_extract_project_topics_into_separate_table - remove_concurrent_index_by_name :taggings, INDEX_NAME - end - - def down - add_concurrent_index :taggings, :id, where: INDEX_CONDITION, name: INDEX_NAME - end -end diff --git a/db/schema_migrations/20210730104801 b/db/schema_migrations/20210730104801 deleted file mode 100644 index 8bd743a59da597..00000000000000 --- a/db/schema_migrations/20210730104801 +++ /dev/null @@ -1 +0,0 @@ -e95d930ad7f5072b91b67c3f9696053e5405354d5aa8f1fc06988dd8a93f035f \ No newline at end of file -- GitLab From c3a5d76150b266e0dd18a3e6b1f635c8e4338e0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Mon, 16 Aug 2021 10:04:58 +0200 Subject: [PATCH 08/18] Add length validation for topic name --- app/models/topic.rb | 2 +- spec/models/topic_spec.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/topic.rb b/app/models/topic.rb index 0b288691e8dffa..5cf29e395303d6 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class Topic < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass - validates :name, presence: true, uniqueness: true + validates :name, presence: true, uniqueness: true, length: { maximum: 255 } has_many :project_topics has_many :projects, through: :project_topics diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index e42377b3e77aa8..08adb9f3ed25b5 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -17,5 +17,6 @@ describe 'validations' do it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_uniqueness_of(:name) } + it { is_expected.to validate_length_of(:name).is_at_most(255) } end end -- GitLab From 193ac3cc783b9e54480ab3c315693692627b6e42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Mon, 16 Aug 2021 10:08:28 +0200 Subject: [PATCH 09/18] Optimize background migration of project topics (2) --- .../extract_project_topics_into_separate_table.rb | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb b/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb index 1ee1b4cee14e21..6ef6f94a22bb1a 100644 --- a/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb +++ b/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb @@ -29,10 +29,21 @@ class ProjectTopic < ActiveRecord::Base def perform(start_id, stop_id) Tagging.includes(:tag).where(taggable_type: 'Project', id: start_id..stop_id).each do |tagging| topic = Topic.find_or_create_by(name: tagging.tag.name) - ProjectTopic.create(project_id: tagging.taggable_id, topic: topic) + ProjectTopic.find_or_create_by(project_id: tagging.taggable_id, topic: topic) tagging.delete end + + mark_job_as_succeeded(start_id, stop_id) + end + + private + + def mark_job_as_succeeded(*arguments) + Gitlab::Database::BackgroundMigrationJob.mark_all_as_succeeded( + self.class.name.demodulize, + arguments + ) end end end -- GitLab From c8a989890452d2d43fccd11ea75bb6944381e984 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Wed, 18 Aug 2021 10:03:47 +0200 Subject: [PATCH 10/18] Refactor :with_topic scope on project model --- app/models/project.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 7d1182a7f1a3cb..dd41d87d56356b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -647,13 +647,13 @@ def self.integration_association_name(name) topic = Topic.find_by_name(topic_name) acts_as_taggable_on_topic = ActsAsTaggableOn::Tag.find_by_name(topic_name) - return self.where(id: topic.project_topics.select(:project_id)).or(self.where(id: acts_as_taggable_on_topic.taggings.select(:taggable_id))) if topic && acts_as_taggable_on_topic + return none unless topic || acts_as_taggable_on_topic - return self.where(id: topic.project_topics.select(:project_id)) if topic + relations = [] + relations << where(id: topic.project_topics.select(:project_id)) if topic + relations << where(id: acts_as_taggable_on_topic.taggings.select(:taggable_id)) if acts_as_taggable_on_topic - return self.where(id: acts_as_taggable_on_topic.taggings.select(:taggable_id)) if acts_as_taggable_on_topic - - none + Project.from_union(relations) end enum auto_cancel_pending_pipelines: { disabled: 0, enabled: 1 } -- GitLab From d143ec5c257ec8b4d3304ba405f5874caf182609 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Wed, 18 Aug 2021 11:07:36 +0200 Subject: [PATCH 11/18] Move 'Topic' and 'ProjectTopic' into 'Projects' module --- app/models/project.rb | 8 ++++---- app/models/project_topic.rb | 6 ------ app/models/projects/project_topic.rb | 8 ++++++++ app/models/projects/topic.rb | 10 ++++++++++ app/models/topic.rb | 8 -------- spec/factories/project_topics.rb | 2 +- spec/factories/topics.rb | 2 +- spec/models/project_spec.rb | 2 +- spec/models/{ => projects}/project_topic_spec.rb | 2 +- spec/models/{ => projects}/topic_spec.rb | 2 +- 10 files changed, 27 insertions(+), 23 deletions(-) delete mode 100644 app/models/project_topic.rb create mode 100644 app/models/projects/project_topic.rb create mode 100644 app/models/projects/topic.rb delete mode 100644 app/models/topic.rb rename spec/models/{ => projects}/project_topic_spec.rb (88%) rename spec/models/{ => projects}/topic_spec.rb (93%) diff --git a/app/models/project.rb b/app/models/project.rb index dd41d87d56356b..49036fb2caa4ee 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -138,8 +138,8 @@ class Project < ApplicationRecord through: :topic_taggings, source: :tag - has_many :project_topics - has_many :topics, through: :project_topics + has_many :project_topics, class_name: 'Projects::ProjectTopic' + has_many :topics, through: :project_topics, class_name: 'Projects::Topic' # Required during the `ActsAsTaggableOn::Tag -> Topic` migration # TODO: remove 'topics' in the further process of the migration @@ -644,7 +644,7 @@ def self.integration_association_name(name) end scope :with_topic, ->(topic_name) do - topic = Topic.find_by_name(topic_name) + topic = Projects::Topic.find_by_name(topic_name) acts_as_taggable_on_topic = ActsAsTaggableOn::Tag.find_by_name(topic_name) return none unless topic || acts_as_taggable_on_topic @@ -2713,7 +2713,7 @@ def save_topics @topic_list = @topic_list.split(',') if @topic_list.instance_of?(String) @topic_list = @topic_list.map(&:strip).uniq - self.topics = @topic_list.map { |topic| Topic.safe_find_or_create_by(name: topic) } + self.topics = @topic_list.map { |topic| Projects::Topic.safe_find_or_create_by(name: topic) } @topic_list = nil # Remove old topics (ActsAsTaggableOn::Tag) diff --git a/app/models/project_topic.rb b/app/models/project_topic.rb deleted file mode 100644 index acce057ed0cac8..00000000000000 --- a/app/models/project_topic.rb +++ /dev/null @@ -1,6 +0,0 @@ -# frozen_string_literal: true - -class ProjectTopic < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass - belongs_to :project - belongs_to :topic -end diff --git a/app/models/projects/project_topic.rb b/app/models/projects/project_topic.rb new file mode 100644 index 00000000000000..d4b456ef48285a --- /dev/null +++ b/app/models/projects/project_topic.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +module Projects + class ProjectTopic < ApplicationRecord + belongs_to :project + belongs_to :topic + end +end diff --git a/app/models/projects/topic.rb b/app/models/projects/topic.rb new file mode 100644 index 00000000000000..a17aa550edb763 --- /dev/null +++ b/app/models/projects/topic.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +module Projects + class Topic < ApplicationRecord + validates :name, presence: true, uniqueness: true, length: { maximum: 255 } + + has_many :project_topics, class_name: 'Projects::ProjectTopic' + has_many :projects, through: :project_topics + end +end diff --git a/app/models/topic.rb b/app/models/topic.rb deleted file mode 100644 index 5cf29e395303d6..00000000000000 --- a/app/models/topic.rb +++ /dev/null @@ -1,8 +0,0 @@ -# frozen_string_literal: true - -class Topic < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass - validates :name, presence: true, uniqueness: true, length: { maximum: 255 } - - has_many :project_topics - has_many :projects, through: :project_topics -end diff --git a/spec/factories/project_topics.rb b/spec/factories/project_topics.rb index 1dbaa2dd8bdbc2..60f5357d129e6c 100644 --- a/spec/factories/project_topics.rb +++ b/spec/factories/project_topics.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true FactoryBot.define do - factory :project_topic do + factory :project_topic, class: 'Projects::ProjectTopic' do association :project, factory: :project association :topic, factory: :topic end diff --git a/spec/factories/topics.rb b/spec/factories/topics.rb index 72de345b53c19a..e77441d9eae786 100644 --- a/spec/factories/topics.rb +++ b/spec/factories/topics.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true FactoryBot.define do - factory :topic do + factory :topic, class: 'Projects::Topic' do name { generate(:name) } end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index cfee8db246a235..9eda95f5d8c573 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -7138,7 +7138,7 @@ def subject end it 'topics returns correct topic records' do - expect(project.topics.first.class.name).to eq('Topic') + expect(project.topics.first.class.name).to eq('Projects::Topic') expect(project.topics.map(&:name)).to match_array(%w[topic1 topic2 topic3]) end diff --git a/spec/models/project_topic_spec.rb b/spec/models/projects/project_topic_spec.rb similarity index 88% rename from spec/models/project_topic_spec.rb rename to spec/models/projects/project_topic_spec.rb index 4f3147a31100d0..c7a989040c705d 100644 --- a/spec/models/project_topic_spec.rb +++ b/spec/models/projects/project_topic_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ProjectTopic do +RSpec.describe Projects::ProjectTopic do let_it_be(:project_topic, reload: true) { create(:project_topic) } subject { project_topic } diff --git a/spec/models/topic_spec.rb b/spec/models/projects/topic_spec.rb similarity index 93% rename from spec/models/topic_spec.rb rename to spec/models/projects/topic_spec.rb index 08adb9f3ed25b5..409dc9327090a7 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/projects/topic_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Topic do +RSpec.describe Projects::Topic do let_it_be(:topic, reload: true) { create(:topic) } subject { topic } -- GitLab From 3c7a84a0296a71efe338a5df67cf23828e9db53a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Thu, 19 Aug 2021 09:38:17 +0200 Subject: [PATCH 12/18] Preserve order of project topics --- app/models/project.rb | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 49036fb2caa4ee..d2c31a5003dcc1 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -144,8 +144,9 @@ class Project < ApplicationRecord # Required during the `ActsAsTaggableOn::Tag -> Topic` migration # TODO: remove 'topics' in the further process of the migration # https://gitlab.com/gitlab-org/gitlab/-/issues/335946 + alias_method :topics_new, :topics def topics - self.topics_acts_as_taggable + super + self.topics_acts_as_taggable + self.topics_new end attr_accessor :old_path_with_namespace @@ -2712,15 +2713,20 @@ def save_topics return if @topic_list.nil? @topic_list = @topic_list.split(',') if @topic_list.instance_of?(String) - @topic_list = @topic_list.map(&:strip).uniq - self.topics = @topic_list.map { |topic| Projects::Topic.safe_find_or_create_by(name: topic) } - @topic_list = nil + @topic_list = @topic_list.map(&:strip).uniq.reject(&:empty?) + + if @topic_list != self.topic_list || self.topics_acts_as_taggable.any? + self.topics_new.destroy_all # rubocop: disable Cop/DestroyAll + self.topics = @topic_list.map { |topic| Projects::Topic.safe_find_or_create_by(name: topic) } - # Remove old topics (ActsAsTaggableOn::Tag) - # Required during the `ActsAsTaggableOn::Tag -> Topic` migration - # TODO: remove in the further process of the migration - # https://gitlab.com/gitlab-org/gitlab/-/issues/335946 - self.topic_taggings.clear + # Remove old topics (ActsAsTaggableOn::Tag) + # Required during the `ActsAsTaggableOn::Tag -> Topic` migration + # TODO: remove in the further process of the migration + # https://gitlab.com/gitlab-org/gitlab/-/issues/335946 + self.topic_taggings.clear + end + + @topic_list = nil end def find_integration(integrations, name) -- GitLab From 69a56057efae68dd7dba5fab9b562bf5035512c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Thu, 19 Aug 2021 12:48:56 +0200 Subject: [PATCH 13/18] Fix projects finder specs --- spec/finders/projects_finder_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index 21b5b2f6130c3d..1fdc05ca2d2473 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -135,6 +135,7 @@ describe 'filter by tags (deprecated)' do before do + public_project.reload public_project.topic_list = 'foo' public_project.save! end @@ -146,6 +147,7 @@ describe 'filter by topics' do before do + public_project.reload public_project.topic_list = 'foo, bar' public_project.save! end -- GitLab From 1b4c1ac7e14de8bb69664552dc770b0de9160e72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Fri, 20 Aug 2021 08:53:31 +0200 Subject: [PATCH 14/18] Integrate temporary index creation in post-migration --- ...ry_index_for_project_topics_on_taggings.rb | 19 ------------------- ...ract_project_topics_into_separate_table.rb | 6 ++++++ db/schema_migrations/20210730104759 | 1 - db/structure.sql | 2 ++ 4 files changed, 8 insertions(+), 20 deletions(-) delete mode 100644 db/migrate/20210730104759_add_temporary_index_for_project_topics_on_taggings.rb delete mode 100644 db/schema_migrations/20210730104759 diff --git a/db/migrate/20210730104759_add_temporary_index_for_project_topics_on_taggings.rb b/db/migrate/20210730104759_add_temporary_index_for_project_topics_on_taggings.rb deleted file mode 100644 index b786a39007bd29..00000000000000 --- a/db/migrate/20210730104759_add_temporary_index_for_project_topics_on_taggings.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -class AddTemporaryIndexForProjectTopicsOnTaggings < ActiveRecord::Migration[6.1] - include Gitlab::Database::MigrationHelpers - - INDEX_NAME = 'tmp_index_taggings_on_id_where_taggable_type_project' - INDEX_CONDITION = "taggable_type = 'Project'" - - disable_ddl_transaction! - - def up - # this index is used in 20210730104800_schedule_extract_project_topics_into_separate_table - add_concurrent_index :taggings, :id, where: INDEX_CONDITION, name: INDEX_NAME - end - - def down - remove_concurrent_index_by_name :taggings, INDEX_NAME - end -end diff --git a/db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb b/db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb index 8adda3b54e8983..bb498f89015b3e 100644 --- a/db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb +++ b/db/post_migrate/20210730104800_schedule_extract_project_topics_into_separate_table.rb @@ -7,6 +7,8 @@ class ScheduleExtractProjectTopicsIntoSeparateTable < ActiveRecord::Migration[6. BATCH_SIZE = 1_000 DELAY_INTERVAL = 2.minutes MIGRATION = 'ExtractProjectTopicsIntoSeparateTable' + INDEX_NAME = 'tmp_index_taggings_on_id_where_taggable_type_project' + INDEX_CONDITION = "taggable_type = 'Project'" disable_ddl_transaction! @@ -17,6 +19,9 @@ class Tagging < ActiveRecord::Base end def up + # this index is used in 20210730104800_schedule_extract_project_topics_into_separate_table + add_concurrent_index :taggings, :id, where: INDEX_CONDITION, name: INDEX_NAME + queue_background_migration_jobs_by_range_at_intervals( Tagging.where(taggable_type: 'Project'), MIGRATION, @@ -27,5 +32,6 @@ def up end def down + remove_concurrent_index_by_name :taggings, INDEX_NAME end end diff --git a/db/schema_migrations/20210730104759 b/db/schema_migrations/20210730104759 deleted file mode 100644 index c33c70182a9a12..00000000000000 --- a/db/schema_migrations/20210730104759 +++ /dev/null @@ -1 +0,0 @@ -270e4fd75e41d0b0fa68f0e667a96011f54fbb42fb248b9ee68dc569e8822fbd \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index fe27cd3f2fca64..c73b585442a8c8 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -25845,6 +25845,8 @@ CREATE INDEX tmp_index_namespaces_empty_traversal_ids_with_root_namespaces ON na CREATE INDEX tmp_index_on_vulnerabilities_non_dismissed ON vulnerabilities USING btree (id) WHERE (state <> 2); +CREATE INDEX tmp_index_taggings_on_id_where_taggable_type_project ON taggings USING btree (id) WHERE ((taggable_type)::text = 'Project'::text); + CREATE UNIQUE INDEX uniq_pkgs_deb_grp_architectures_on_distribution_id_and_name ON packages_debian_group_architectures USING btree (distribution_id, name); CREATE UNIQUE INDEX uniq_pkgs_deb_grp_components_on_distribution_id_and_name ON packages_debian_group_components USING btree (distribution_id, name); -- GitLab From bfe67e02d7989f2a08937bb564a5aec954749667 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Mon, 23 Aug 2021 06:32:30 +0000 Subject: [PATCH 15/18] Apply 3 suggestion(s) to 2 file(s) --- app/models/project.rb | 4 ++-- ee/lib/gitlab/elastic/search_results.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index d2c31a5003dcc1..8b2b3d0924716e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2716,8 +2716,8 @@ def save_topics @topic_list = @topic_list.map(&:strip).uniq.reject(&:empty?) if @topic_list != self.topic_list || self.topics_acts_as_taggable.any? - self.topics_new.destroy_all # rubocop: disable Cop/DestroyAll - self.topics = @topic_list.map { |topic| Projects::Topic.safe_find_or_create_by(name: topic) } + self.topics_new.delete_all + self.topics = @topic_list.map { |topic| Projects::Topic.find_or_create_by(name: topic) } # Remove old topics (ActsAsTaggableOn::Tag) # Required during the `ActsAsTaggableOn::Tag -> Topic` migration diff --git a/ee/lib/gitlab/elastic/search_results.rb b/ee/lib/gitlab/elastic/search_results.rb index eead80343330fa..d1409d9e839365 100644 --- a/ee/lib/gitlab/elastic/search_results.rb +++ b/ee/lib/gitlab/elastic/search_results.rb @@ -28,7 +28,7 @@ def objects(scope, page: 1, per_page: DEFAULT_PER_PAGE, preload_method: nil) case scope when 'projects' - eager_load(projects, page, per_page, preload_method, [:route, :namespace, :topics, :topics_acts_as_taggable]) # HERE + eager_load(projects, page, per_page, preload_method, [:route, :namespace, :topics, :topics_acts_as_taggable]) when 'issues' eager_load(issues, page, per_page, preload_method, project: [:route, :namespace], labels: [], timelogs: [], assignees: []) when 'merge_requests' -- GitLab From 2b7d5d60733be8687a3caa4fa59d4a321ea956a4 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Mon, 23 Aug 2021 07:17:58 +0000 Subject: [PATCH 16/18] Remove trailing whitespace --- ee/lib/gitlab/elastic/search_results.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/lib/gitlab/elastic/search_results.rb b/ee/lib/gitlab/elastic/search_results.rb index d1409d9e839365..847a45090c854b 100644 --- a/ee/lib/gitlab/elastic/search_results.rb +++ b/ee/lib/gitlab/elastic/search_results.rb @@ -28,7 +28,7 @@ def objects(scope, page: 1, per_page: DEFAULT_PER_PAGE, preload_method: nil) case scope when 'projects' - eager_load(projects, page, per_page, preload_method, [:route, :namespace, :topics, :topics_acts_as_taggable]) + eager_load(projects, page, per_page, preload_method, [:route, :namespace, :topics, :topics_acts_as_taggable]) when 'issues' eager_load(issues, page, per_page, preload_method, project: [:route, :namespace], labels: [], timelogs: [], assignees: []) when 'merge_requests' -- GitLab From 3f07589d4aa90f14aca2a7bc37199a27fb3465fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Mon, 23 Aug 2021 13:57:48 +0200 Subject: [PATCH 17/18] Fix and test project topics order --- app/models/project.rb | 2 +- spec/models/project_spec.rb | 25 ++++++++++++++++++------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 8b2b3d0924716e..6fc0cb5017b8fc 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -138,7 +138,7 @@ class Project < ApplicationRecord through: :topic_taggings, source: :tag - has_many :project_topics, class_name: 'Projects::ProjectTopic' + has_many :project_topics, -> { order(:id) }, class_name: 'Projects::ProjectTopic' has_many :topics, through: :project_topics, class_name: 'Projects::Topic' # Required during the `ActsAsTaggableOn::Tag -> Topic` migration diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 9eda95f5d8c573..6f4b171adeb51c 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -7131,15 +7131,15 @@ def subject end describe 'topics' do - let_it_be(:project) { create(:project, topic_list: 'topic1, topic2, topic3') } + let_it_be(:project) { create(:project, name: 'topic-project', topic_list: 'topic1, topic2, topic3') } it 'topic_list returns correct string array' do - expect(project.topic_list).to match_array(%w[topic1 topic2 topic3]) + expect(project.topic_list).to eq(%w[topic1 topic2 topic3]) end it 'topics returns correct topic records' do expect(project.topics.first.class.name).to eq('Projects::Topic') - expect(project.topics.map(&:name)).to match_array(%w[topic1 topic2 topic3]) + expect(project.topics.map(&:name)).to eq(%w[topic1 topic2 topic3]) end context 'topic_list=' do @@ -7163,14 +7163,26 @@ def subject project.topic_list = topic_list project.save! - expect(project.topics.map(&:name)).to match_array(expected_result) + expect(project.topics.map(&:name)).to eq(expected_result) end end + it 'set topics if only the order is changed' do + project.topic_list = 'topicA, topicB' + project.save! + + expect(project.reload.topics.map(&:name)).to eq(%w[topicA topicB]) + + project.topic_list = 'topicB, topicA' + project.save! + + expect(project.reload.topics.map(&:name)).to eq(%w[topicB topicA]) + end + it 'does not persist topics before project is saved' do project.topic_list = 'topicA, topicB' - expect(project.topics.map(&:name)).to match_array(%w[topic1 topic2 topic3]) + expect(project.reload.topics.map(&:name)).to eq(%w[topic1 topic2 topic3]) end it 'does not update topics if project is not valid' do @@ -7178,8 +7190,7 @@ def subject project.topic_list = 'topicA, topicB' expect(project.save).to be_falsy - expect(project.topics.map(&:name)).to match_array(%w[topic1 topic2 topic3]) - project.reload + expect(project.reload.topics.map(&:name)).to eq(%w[topic1 topic2 topic3]) end end end -- GitLab From 4709538529505cff08db4474867296d9179bca13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Tue, 24 Aug 2021 09:12:56 +0200 Subject: [PATCH 18/18] Optimize and test topics migration --- ...ract_project_topics_into_separate_table.rb | 4 +-- spec/models/project_spec.rb | 29 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb b/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb index 6ef6f94a22bb1a..15ce038321cefd 100644 --- a/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb +++ b/lib/gitlab/background_migration/extract_project_topics_into_separate_table.rb @@ -29,9 +29,9 @@ class ProjectTopic < ActiveRecord::Base def perform(start_id, stop_id) Tagging.includes(:tag).where(taggable_type: 'Project', id: start_id..stop_id).each do |tagging| topic = Topic.find_or_create_by(name: tagging.tag.name) - ProjectTopic.find_or_create_by(project_id: tagging.taggable_id, topic: topic) + project_topic = ProjectTopic.find_or_create_by(project_id: tagging.taggable_id, topic: topic) - tagging.delete + tagging.delete if project_topic.persisted? end mark_job_as_succeeded(start_id, stop_id) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 6f4b171adeb51c..71756cb25521ec 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -7193,6 +7193,35 @@ def subject expect(project.reload.topics.map(&:name)).to eq(%w[topic1 topic2 topic3]) end end + + context 'during ExtractProjectTopicsIntoSeparateTable migration' do + before do + topic_a = ActsAsTaggableOn::Tag.find_or_create_by!(name: 'topicA') + topic_b = ActsAsTaggableOn::Tag.find_or_create_by!(name: 'topicB') + + project.reload.topics_acts_as_taggable = [topic_a, topic_b] + project.save! + project.reload + end + + it 'topic_list returns correct string array' do + expect(project.topic_list).to eq(%w[topicA topicB topic1 topic2 topic3]) + end + + it 'topics returns correct topic records' do + expect(project.topics.map(&:class)).to eq([ActsAsTaggableOn::Tag, ActsAsTaggableOn::Tag, Projects::Topic, Projects::Topic, Projects::Topic]) + expect(project.topics.map(&:name)).to eq(%w[topicA topicB topic1 topic2 topic3]) + end + + it 'topic_list= sets new topics and removes old topics' do + project.topic_list = 'new-topic1, new-topic2' + project.save! + project.reload + + expect(project.topics.map(&:class)).to eq([Projects::Topic, Projects::Topic]) + expect(project.topics.map(&:name)).to eq(%w[new-topic1 new-topic2]) + end + end end shared_examples 'all_runners' do -- GitLab