Merge branch '325133-todosdestroyer-generates-expensive-sql-queries' into 'master'

`TodosDestroyer::EntityLeaveWorker` generates expensive SQL queries

See merge request gitlab-org/gitlab!56995

app/services/todos/destroy/entity_leave_service.rb

@@ -65,8 +65,10 @@ def remove_project_todos
       end
       def remove_group_todos
+        return unless entity.is_a?(Namespace)
+
         Todo
-          .for_group(non_authorized_groups)
+          .for_group(non_authorized_non_public_groups)
           .for_user(user)
           .delete_all
       end
@@ -102,12 +104,19 @@ def authorized_reporter_groups
         GroupsFinder.new(user, min_access_level: Gitlab::Access::REPORTER).execute.select(:id)
       end
-      def non_authorized_groups
+      # since the entity is a private group, we can assume all subgroups are also
+      # private.  We can therefore limit GroupsFinder with `all_available: false`.
+      # Otherwise it tries to include all public groups. This generates an expensive
+      # SQL queries: https://gitlab.com/gitlab-org/gitlab/-/issues/325133
+      # rubocop: disable CodeReuse/ActiveRecord
+      def non_authorized_non_public_groups
         return [] unless entity.is_a?(Namespace)
+        return [] unless entity.private?
         entity.self_and_descendants.select(:id)
-          .id_not_in(GroupsFinder.new(user).execute.select(:id))
+          .id_not_in(GroupsFinder.new(user, all_available: false).execute.select(:id).reorder(nil))
       end
+      # rubocop: enable CodeReuse/ActiveRecord
       def non_authorized_reporter_groups
         entity.self_and_descendants.select(:id)

changelogs/unreleased/325133-todosdestroyer-generates-expensive-sql-queries.yml

+---
+title: Speed up destroying of group Todos when user leaves group
+merge_request: 56995
+author:
+type: performance

spec/services/todos/destroy/entity_leave_service_spec.rb

@@ -224,6 +224,8 @@ def set_access(object, user, access_name)
         end
         context 'with nested groups' do
+          let(:parent_group) { create(:group, :public) }
+          let(:parent_subgroup) { create(:group)}
           let(:subgroup) { create(:group, :private, parent: group) }
           let(:subgroup2) { create(:group, :private, parent: group) }
           let(:subproject) { create(:project, group: subgroup) }
@@ -235,12 +237,17 @@ def set_access(object, user, access_name)
           let!(:todo_subgroup2_user) { create(:todo, user: user, group: subgroup2) }
           let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) }
           let!(:todo_subpgroup_user2)  { create(:todo, user: user2, group: subgroup) }
+          let!(:todo_parent_group_user) { create(:todo, user: user, group: parent_group) }
+
+          before do
+            group.update!(parent: parent_group)
+          end
           context 'when the user is not a member of any groups/projects' do
             it 'removes todos for the user including subprojects todos' do
-              expect { subject }.to change { Todo.count }.from(12).to(4)
-              expect(user.todos).to be_empty
+              expect { subject }.to change { Todo.count }.from(13).to(5)
+              expect(user.todos).to eq([todo_parent_group_user])
               expect(user2.todos)
                 .to match_array(
                   [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
@@ -250,8 +257,6 @@ def set_access(object, user, access_name)
           context 'when the user is member of a parent group' do
             before do
-              parent_group = create(:group)
-              group.update!(parent: parent_group)
               parent_group.add_developer(user)
             end
@@ -264,9 +269,12 @@ def set_access(object, user, access_name)
             end
             it 'does not remove group and subproject todos' do
-              expect { subject }.to change { Todo.count }.from(12).to(7)
-              expect(user.todos).to match_array([todo_group_user, todo_subgroup_user, todo_subproject_user])
+              expect { subject }.to change { Todo.count }.from(13).to(8)
+              expect(user.todos)
+                .to match_array(
+                  [todo_group_user, todo_subgroup_user, todo_subproject_user, todo_parent_group_user]
+                )
               expect(user2.todos)
                 .to match_array(
                   [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
@@ -280,9 +288,12 @@ def set_access(object, user, access_name)
             end
             it 'does not remove subproject and group todos' do
-              expect { subject }.to change { Todo.count }.from(12).to(7)
-              expect(user.todos).to match_array([todo_subgroup_user, todo_group_user, todo_subproject_user])
+              expect { subject }.to change { Todo.count }.from(13).to(8)
+              expect(user.todos)
+                .to match_array(
+                  [todo_subgroup_user, todo_group_user, todo_subproject_user, todo_parent_group_user]
+                )
               expect(user2.todos)
                 .to match_array(
                   [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]