From a2d0cf679243340eb8afcda80fca118e9e3b3b70 Mon Sep 17 00:00:00 2001
From: Siddharth Dungarwal <sdungarwal@gitlab.com>
Date: Tue, 12 Dec 2023 17:34:53 +0000
Subject: [PATCH] Add method for removing duplicate epics
Changelog: fixed
EE: true
---
ee/app/services/ee/groups/transfer_service.rb | 20 +++++++++++++
...astic_group_association_deletion_worker.rb | 28 +++++++++++++------
.../elastic_integration/epic_index_spec.rb | 3 ++
.../services/groups/transfer_service_spec.rb | 12 +++++++-
..._group_association_deletion_worker_spec.rb | 21 ++++++++++++++
5 files changed, 74 insertions(+), 10 deletions(-)
diff --git a/ee/app/services/ee/groups/transfer_service.rb b/ee/app/services/ee/groups/transfer_service.rb
index 96cfa093760c..5c3252f6cfba 100644
--- a/ee/app/services/ee/groups/transfer_service.rb
+++ b/ee/app/services/ee/groups/transfer_service.rb
@@ -60,6 +60,8 @@ def post_update_hooks(updated_project_ids, old_root_ancestor_id)
end
process_wikis(group)
+
+ process_epics(old_root_ancestor_id, group)
end
def update_project_settings(updated_project_ids)
@@ -85,6 +87,24 @@ def process_elasticsearch_project(project, elasticsearch_limit_indexing_enabled)
::Elastic::ProcessInitialBookkeepingService.backfill_projects!(project) if project.maintaining_elasticsearch?
end
+ def process_epics(old_root_ancestor_id, group)
+ return unless group.use_elasticsearch? && ::Epic.elasticsearch_available?
+
+ epics_found = false
+ group.self_and_descendants.each_batch do |group_batch|
+ ::Epic.in_selected_groups(group_batch).each_batch do |epics|
+ epics_found ||= true
+ ::Elastic::ProcessInitialBookkeepingService.track!(*epics)
+ end
+ end
+
+ return unless epics_found
+ return if old_root_ancestor_id == group.root_ancestor.id && group.licensed_feature_available?(:epics)
+
+ ::Search::ElasticGroupAssociationDeletionWorker.perform_async(group.id, old_root_ancestor_id,
+ { include_descendants: true })
+ end
+
def process_wikis(group)
return unless ::Wiki.use_separate_indices?
diff --git a/ee/app/workers/search/elastic_group_association_deletion_worker.rb b/ee/app/workers/search/elastic_group_association_deletion_worker.rb
index ffebc1962027..489a0d8ac7bb 100644
--- a/ee/app/workers/search/elastic_group_association_deletion_worker.rb
+++ b/ee/app/workers/search/elastic_group_association_deletion_worker.rb
@@ -13,14 +13,28 @@ class ElasticGroupAssociationDeletionWorker
urgency :throttled
idempotent!
- def perform(group_id, ancestor_id)
- remove_epics(group_id, ancestor_id) if Epic.elasticsearch_available?
+ def perform(group_id, ancestor_id, options = {})
+ return unless ::Epic.elasticsearch_available?
+
+ return remove_epics(group_id, ancestor_id) unless options[:include_descendants]
+
+ # We have the return condition here because we still want to remove the deleted group epics in the above call
+ group = Group.find_by_id(group_id)
+ return if group.nil?
+
+ # rubocop: disable CodeReuse/ActiveRecord -- We need only the ids of self_and_descendants groups
+ group.self_and_descendants.each_batch { |groups| remove_epics(groups.pluck(:id), ancestor_id) }
+ # rubocop: enable CodeReuse/ActiveRecord
end
private
- def remove_epics(group_id, ancestor_id)
- Gitlab::Search::Client.new.delete_by_query(
+ def client
+ @client ||= ::Gitlab::Search::Client.new
+ end
+
+ def remove_epics(group_ids, ancestor_id)
+ client.delete_by_query(
{
index: ::Elastic::Latest::EpicConfig.index_name,
routing: "group_#{ancestor_id}",
@@ -29,11 +43,7 @@ def remove_epics(group_id, ancestor_id)
body: {
query: {
bool: {
- filter: {
- term: {
- group_id: group_id
- }
- }
+ filter: { terms: { group_id: Array.wrap(group_ids) } }
}
}
}
diff --git a/ee/spec/elastic_integration/epic_index_spec.rb b/ee/spec/elastic_integration/epic_index_spec.rb
index 11dbe3c3ed6b..4ccd94e426dc 100644
--- a/ee/spec/elastic_integration/epic_index_spec.rb
+++ b/ee/spec/elastic_integration/epic_index_spec.rb
@@ -173,11 +173,14 @@
context 'when the group is transferred', :sidekiq_inline do
it 'tracks the epic via Elastic::NamespaceUpdateWorker' do
expect(Elastic::NamespaceUpdateWorker).to receive(:perform_async).with(group.id).and_call_original
+ expect(Search::ElasticGroupAssociationDeletionWorker).to receive(:perform_async)
+ .with(group.id, parent_group.id, { include_descendants: true }).once
expect(ElasticAssociationIndexerWorker).to receive(:perform_async)
.with(anything, group.id, [:epics])
.and_call_original
expect(::Elastic::ProcessBookkeepingService).to receive(:track!).with(epic).once
+ expect(::Elastic::ProcessInitialBookkeepingService).to receive(:track!).with(epic).once
Groups::TransferService.new(group, user).execute(another_group)
end
end
diff --git a/ee/spec/services/groups/transfer_service_spec.rb b/ee/spec/services/groups/transfer_service_spec.rb
index d9507e10dd89..d46ef1b21398 100644
--- a/ee/spec/services/groups/transfer_service_spec.rb
+++ b/ee/spec/services/groups/transfer_service_spec.rb
@@ -3,6 +3,7 @@
require 'spec_helper'
RSpec.describe Groups::TransferService, '#execute', feature_category: :groups_and_projects do
+ include ElasticsearchHelpers
let_it_be(:user) { create(:user) }
let(:group) { create(:group, :public) }
@@ -286,6 +287,8 @@
context 'with epics' do
context 'when epics feature is disabled' do
it 'transfers a group successfully' do
+ expect(::Search::ElasticGroupAssociationDeletionWorker).not_to receive(:perform_async)
+ expect(::Elastic::ProcessInitialBookkeepingService).not_to receive(:track!)
transfer_service.execute(new_group)
expect(group.parent).to eq(new_group)
@@ -308,12 +311,15 @@
before do
root_group.add_owner(user)
-
+ set_elasticsearch_migration_to :create_epic_index, including: true
+ stub_ee_application_setting(elasticsearch_indexing: true)
stub_licensed_features(epics: true)
end
context 'when group is moved completely out of the main group' do
it 'keeps relations between all epics' do
+ expect(::Search::ElasticGroupAssociationDeletionWorker).to receive(:perform_async).with(subgroup_group_level_1.id, root_group.id, { include_descendants: true }).once
+ expect(::Elastic::ProcessInitialBookkeepingService).to receive(:track!).once
described_class.new(subgroup_group_level_1, user).execute(new_group)
expect(level_1_epic_2.reload.parent).to eq(level_1_epic_1)
@@ -327,6 +333,10 @@
context 'when group is moved some levels up' do
it 'keeps relations between all epics' do
+ expect(::Search::ElasticGroupAssociationDeletionWorker).not_to receive(:perform_async)
+ expect(::Elastic::ProcessInitialBookkeepingService).to receive(:track!) do |*args|
+ expect(args).to match_array(subgroup_group_level_2.self_and_descendants.flat_map(&:epics))
+ end
described_class.new(subgroup_group_level_2, user).execute(root_group)
expect(level_1_epic_1.reload.parent).to eq(root_epic)
diff --git a/ee/spec/workers/search/elastic_group_association_deletion_worker_spec.rb b/ee/spec/workers/search/elastic_group_association_deletion_worker_spec.rb
index befd5b4468ff..a83dd4078bd6 100644
--- a/ee/spec/workers/search/elastic_group_association_deletion_worker_spec.rb
+++ b/ee/spec/workers/search/elastic_group_association_deletion_worker_spec.rb
@@ -8,6 +8,7 @@
let_it_be(:parent_group) { create(:group) }
let_it_be(:group) { create(:group, parent: parent_group) }
+ let_it_be(:other_group) { create(:group, parent: group) }
let(:epic_index) { Epic.__elasticsearch__.index_name }
let(:helper) { Gitlab::Elastic::Helper.default }
let(:client) { helper.client }
@@ -35,6 +36,26 @@
allow(Epic).to receive(:elasticsearch_available?).and_return(true)
end
+ it 'deletes epics belonging to the group and its escendants' do
+ group_epic = create(:epic, group: group)
+ other_group_epic = create(:epic, group: other_group)
+ create(:epic)
+
+ ensure_elasticsearch_index!
+
+ expect(epics_in_index.count).to eq(3)
+ expect(epics_in_index).to include(group_epic.id)
+ expect(epics_in_index).to include(other_group_epic.id)
+
+ described_class.new.perform(group.id, parent_group.id, { include_descendants: true })
+
+ helper.refresh_index(index_name: epic_index)
+
+ expect(epics_in_index.count).to eq(1)
+ expect(epics_in_index).not_to include(group_epic.id)
+ expect(epics_in_index).not_to include(other_group_epic.id)
+ end
+
it 'deletes epics belonging to the group' do
group_epic = create(:epic, group: group)
create(:epic)
--
GitLab