From 743fa08860f5b643952a823b80b6699d61009253 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Tue, 24 Aug 2021 16:33:13 +0200 Subject: [PATCH 1/2] Eliminate `acts_as_taggable` gem for project topics Changelog: removed --- .../dashboard/projects_controller.rb | 2 +- app/finders/projects_finder.rb | 4 +- app/models/project.rb | 38 ++----------------- app/services/search/global_service.rb | 2 +- ...ry_index_for_project_topics_on_taggings.rb | 17 +++++++++ db/schema_migrations/20210917134321 | 1 + db/structure.sql | 2 - ee/lib/gitlab/elastic/search_results.rb | 2 +- lib/api/entities/basic_project_details.rb | 4 +- lib/api/entities/project.rb | 4 +- spec/lib/gitlab/import_export/all_models.yml | 3 -- spec/models/project_spec.rb | 29 -------------- 12 files changed, 31 insertions(+), 77 deletions(-) create mode 100644 db/migrate/20210917134321_remove_temporary_index_for_project_topics_on_taggings.rb create mode 100644 db/schema_migrations/20210917134321 diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index 74ad78ff4c1daa..d861ef646f889f 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, :topics_acts_as_taggable) + .includes(:forked_from_project, :topics) @groups = [] diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 5537058cc79226..7245bb36ac9d95 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -182,8 +182,8 @@ def by_visibility_level(items) def by_topics(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| + topics = params[:topic].instance_of?(String) ? params[:topic].split(',') : params[:topic] + topics.map(&:strip).uniq.reject(&:empty?).each do |topic| items = items.with_topic(topic) end diff --git a/app/models/project.rb b/app/models/project.rb index 74ffeef797e9ac..6297d06679fd84 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -128,26 +128,9 @@ 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, -> { order(:id) }, 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 - # https://gitlab.com/gitlab-org/gitlab/-/issues/335946 - alias_method :topics_new, :topics - def topics - self.topics_acts_as_taggable + self.topics_new - end - attr_accessor :old_path_with_namespace attr_accessor :template_name attr_writer :pipeline_status @@ -652,15 +635,8 @@ def self.integration_association_name(name) scope :with_topic, ->(topic_name) do 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 - - 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 - - Project.from_union(relations) + topic ? where(id: topic.project_topics.select(:project_id)) : none end enum auto_cancel_pending_pipelines: { disabled: 0, enabled: 1 } @@ -678,7 +654,7 @@ def self.integration_association_name(name) mount_uploader :bfg_object_map, AttachmentUploader def self.with_api_entity_associations - preload(:project_feature, :route, :topics, :topics_acts_as_taggable, :group, :timelogs, namespace: [:route, :owner]) + preload(:project_feature, :route, :topics, :group, :timelogs, namespace: [:route, :owner]) end def self.with_web_entity_associations @@ -2734,15 +2710,9 @@ def save_topics @topic_list = @topic_list.split(',') if @topic_list.instance_of?(String) @topic_list = @topic_list.map(&:strip).uniq.reject(&:empty?) - if @topic_list != self.topic_list || self.topics_acts_as_taggable.any? - self.topics_new.delete_all + if @topic_list != self.topic_list + self.topics.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 - # 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 diff --git a/app/services/search/global_service.rb b/app/services/search/global_service.rb index 33faf2d66987f3..cee59360b4ba61 100644 --- a/app/services/search/global_service.rb +++ b/app/services/search/global_service.rb @@ -24,7 +24,7 @@ def execute # rubocop: disable CodeReuse/ActiveRecord def projects - @projects ||= ProjectsFinder.new(params: { non_archived: true }, current_user: current_user).execute.preload(:topics, :taggings) + @projects ||= ProjectsFinder.new(params: { non_archived: true }, current_user: current_user).execute.preload(:topics, :project_topics) end def allowed_scopes diff --git a/db/migrate/20210917134321_remove_temporary_index_for_project_topics_on_taggings.rb b/db/migrate/20210917134321_remove_temporary_index_for_project_topics_on_taggings.rb new file mode 100644 index 00000000000000..7752d04cf731dc --- /dev/null +++ b/db/migrate/20210917134321_remove_temporary_index_for_project_topics_on_taggings.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class RemoveTemporaryIndexForProjectTopicsOnTaggings < Gitlab::Database::Migration[1.0] + 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 # rubocop:disable Migration/PreventIndexCreation + end +end diff --git a/db/schema_migrations/20210917134321 b/db/schema_migrations/20210917134321 new file mode 100644 index 00000000000000..3c5c397ecbf4ed --- /dev/null +++ b/db/schema_migrations/20210917134321 @@ -0,0 +1 @@ +a0ba9fb9e2f7f738926a2273f9ff644c43acb999f4d27adf192e5006582a2a0e \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index cb79fe4610c1ce..fc7d9b3a1c4fa7 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -26954,8 +26954,6 @@ 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); diff --git a/ee/lib/gitlab/elastic/search_results.rb b/ee/lib/gitlab/elastic/search_results.rb index ee70d57c833b72..a8be57b500b3eb 100644 --- a/ee/lib/gitlab/elastic/search_results.rb +++ b/ee/lib/gitlab/elastic/search_results.rb @@ -30,7 +30,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]) when 'issues' eager_load(issues, page, per_page, preload_method, project: [:route, :namespace], labels: [], timelogs: [], assignees: []) when 'merge_requests' diff --git a/lib/api/entities/basic_project_details.rb b/lib/api/entities/basic_project_details.rb index 5c33af86b843ca..e96504db53e67f 100644 --- a/lib/api/entities/basic_project_details.rb +++ b/lib/api/entities/basic_project_details.rb @@ -39,11 +39,11 @@ class BasicProjectDetails < Entities::ProjectIdentity # rubocop: disable CodeReuse/ActiveRecord def self.preload_relation(projects_relation, options = {}) # Preloading topics, should be done with using only `:topics`, - # as `:topics` are defined as: `has_many :topics, through: :taggings` + # as `:topics` are defined as: `has_many :topics, through: :project_topics` # 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, :topics_acts_as_taggable) + .preload(:import_state, :topics) .preload(:auto_devops) .preload(namespace: [:route, :owner]) end diff --git a/lib/api/entities/project.rb b/lib/api/entities/project.rb index b0e53ac3794ed4..df0c1d7a4c5c0d 100644 --- a/lib/api/entities/project.rb +++ b/lib/api/entities/project.rb @@ -132,7 +132,7 @@ def self.preload_resource(project) def self.preload_relation(projects_relation, options = {}) # Preloading topics, should be done with using only `:topics`, - # as `:topics` are defined as: `has_many :topics, through: :taggings` + # as `:topics` are defined as: `has_many :topics, through: :project_topics` # 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 super(projects_relation).preload(group: :namespace_settings) @@ -144,7 +144,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, :topics_acts_as_taggable, :group, :project_feature, namespace: [:route, :owner]]) + forked_from_project: [:route, :topics, :group, :project_feature, namespace: [:route, :owner]]) end # rubocop: enable CodeReuse/ActiveRecord diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 614aa55c3c548c..16334167ffe624 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -354,10 +354,7 @@ container_repositories: - name project: - external_status_checks -- taggings - base_tags -- topic_taggings -- topics_acts_as_taggable - project_topics - topics - chat_services diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 3989ddc31e84b1..a30fe1772cec5a 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -7239,35 +7239,6 @@ def has_external_wiki 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 From 72e33acbf60be5772a87d63cadc0cbc074508c2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20W=C3=A4lter?= Date: Mon, 20 Sep 2021 11:44:02 +0200 Subject: [PATCH 2/2] Finalize background migration first --- ...21_remove_temporary_index_for_project_topics_on_taggings.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/db/migrate/20210917134321_remove_temporary_index_for_project_topics_on_taggings.rb b/db/migrate/20210917134321_remove_temporary_index_for_project_topics_on_taggings.rb index 7752d04cf731dc..ac4821b8007393 100644 --- a/db/migrate/20210917134321_remove_temporary_index_for_project_topics_on_taggings.rb +++ b/db/migrate/20210917134321_remove_temporary_index_for_project_topics_on_taggings.rb @@ -1,12 +1,15 @@ # frozen_string_literal: true class RemoveTemporaryIndexForProjectTopicsOnTaggings < Gitlab::Database::Migration[1.0] + MIGRATION = 'ExtractProjectTopicsIntoSeparateTable' INDEX_NAME = 'tmp_index_taggings_on_id_where_taggable_type_project' INDEX_CONDITION = "taggable_type = 'Project'" disable_ddl_transaction! def up + # Ensure that no background jobs of 20210730104800_schedule_extract_project_topics_into_separate_table remain + finalize_background_migration MIGRATION # this index was used in 20210730104800_schedule_extract_project_topics_into_separate_table remove_concurrent_index_by_name :taggings, INDEX_NAME end -- GitLab