Automatic merge of gitlab-org/gitlab master

app/models/concerns/issuable.rb

@@ -65,7 +65,7 @@ module Issuable
 
     has_many :label_links, as: :target, dependent: :destroy, inverse_of: :target # rubocop:disable Cop/ActiveRecordDependent
     has_many :labels, through: :label_links
-    has_many :todos, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+    has_many :todos, as: :target
 
     has_one :metrics, inverse_of: model_name.singular.to_sym, autosave: true
 

app/models/user.rb

@@ -1668,8 +1668,7 @@ class User < ApplicationRecord
   def invalidate_cache_counts
     invalidate_issue_cache_counts
     invalidate_merge_request_cache_counts
-    invalidate_todos_done_count
-    invalidate_todos_pending_count
+    invalidate_todos_cache_counts
     invalidate_personal_projects_count
   end
 
@@ -1682,11 +1681,8 @@ class User < ApplicationRecord
     Rails.cache.delete(['users', id, 'review_requested_open_merge_requests_count'])
   end
 
-  def invalidate_todos_done_count
+  def invalidate_todos_cache_counts
     Rails.cache.delete(['users', id, 'todos_done_count'])
-  end
-
-  def invalidate_todos_pending_count
     Rails.cache.delete(['users', id, 'todos_pending_count'])
   end
 

app/services/issuable/destroy_service.rb

@@ -3,11 +3,25 @@
 module Issuable
   class DestroyService < IssuableBaseService
     def execute(issuable)
-      TodoService.new.destroy_target(issuable) do |issuable|
-        if issuable.destroy
-          issuable.update_project_counter_caches
-          issuable.assignees.each(&:invalidate_cache_counts)
-        end
+      if issuable.destroy
+        delete_todos(issuable)
+        issuable.update_project_counter_caches
+        issuable.assignees.each(&:invalidate_cache_counts)
+      end
+    end
+
+    private
+
+    def delete_todos(issuable)
+      actor = issuable.is_a?(Epic) ? issuable.resource_parent : issuable.resource_parent.group
+
+      if Feature.enabled?(:destroy_issuable_todos_async, actor, default_enabled: :yaml)
+        TodosDestroyer::DestroyedIssuableWorker
+          .perform_async(issuable.id, issuable.class.name)
+      else
+        TodosDestroyer::DestroyedIssuableWorker
+          .new
+          .perform(issuable.id, issuable.class.name)
       end
     end
   end

app/services/todos/destroy/destroyed_issuable_service.rb

+# frozen_string_literal: true
+
+module Todos
+  module Destroy
+    class DestroyedIssuableService
+      BATCH_SIZE = 100
+
+      def initialize(target_id, target_type)
+        @target_id = target_id
+        @target_type = target_type
+      end
+
+      def execute
+        inner_query = Todo.select(:id).for_target(target_id).for_type(target_type).limit(BATCH_SIZE)
+
+        delete_query = <<~SQL
+        DELETE FROM "#{Todo.table_name}"
+        WHERE id IN (#{inner_query.to_sql})
+        RETURNING user_id
+        SQL
+
+        loop do
+          result = ActiveRecord::Base.connection.execute(delete_query)
+
+          break if result.cmd_tuples == 0
+
+          user_ids = result.map { |row| row['user_id'] }.uniq
+
+          invalidate_todos_cache_counts(user_ids)
+        end
+      end
+
+      private
+
+      attr_reader :target_id, :target_type
+
+      def invalidate_todos_cache_counts(user_ids)
+        user_ids.each do |id|
+          # Only build a user instance since we only need its ID for
+          # `User#invalidate_todos_cache_counts` to work.
+          User.new(id: id).invalidate_todos_cache_counts
+        end
+      end
+    end
+  end
+end

app/workers/all_queues.yml

@@ -1411,6 +1411,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: todos_destroyer:todos_destroyer_destroyed_issuable
+  :feature_category: :issue_tracking
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: todos_destroyer:todos_destroyer_entity_leave
   :feature_category: :issue_tracking
   :has_external_dependencies:

app/workers/todos_destroyer/destroyed_issuable_worker.rb

+# frozen_string_literal: true
+
+module TodosDestroyer
+  class DestroyedIssuableWorker
+    include ApplicationWorker
+    include TodosDestroyerQueue
+
+    idempotent!
+
+    def perform(target_id, target_type)
+      ::Todos::Destroy::DestroyedIssuableService.new(target_id, target_type).execute
+    end
+  end
+end

changelogs/unreleased/325689-delete-issuable-todos-async.yml

+---
+title: Delete all issuable todos asynchronously when issuable is destroyed
+merge_request: 57830
+author:
+type: performance

config/feature_flags/development/destroy_issuable_todos_async.yml

+---
+name: destroy_issuable_todos_async
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57830
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/325689
+milestone: '13.11'
+type: development
+group: group::code review
+default_enabled: false

doc/install/installation.md

@@ -131,22 +131,45 @@ that:
 - Is always at the version required by GitLab.
 - May contain custom patches required for proper operation.
 
-```shell
-# Install dependencies
-sudo apt-get install -y libcurl4-openssl-dev libexpat1-dev gettext libz-dev libssl-dev libpcre2-dev build-essential
+1. Install the needed dependencies:
 
-# Clone the Gitaly repository
-git clone https://gitlab.com/gitlab-org/gitaly.git -b <X-Y-stable> /tmp/gitaly
+   ```shell
+   sudo apt-get install -y libcurl4-openssl-dev libexpat1-dev gettext libz-dev libssl-dev libpcre2-dev build-essential git-core
+   ```
 
-# Compile and install Git
-cd /tmp/gitaly
-sudo make git GIT_PREFIX=/usr/local
-```
+1. Clone the Gitaly repository and compile Git. Replace `<X-Y-stable>` with the
+   stable branch that matches the GitLab version you want to install. For example,
+   if you want to install GitLab 13.6, use the branch name `13-6-stable`:
+
+   ```shell
+   git clone https://gitlab.com/gitlab-org/gitaly.git -b <X-Y-stable> /tmp/gitaly
+   cd /tmp/gitaly
+   sudo make git GIT_PREFIX=/usr/local
+   ```
+
+1. Optionally, you can remove the system Git and its dependencies:
+
+   ```shell
+   sudo apt remove -y git-core
+   sudo apt autoremove
+   ```
+
+When [editing `config/gitlab.yml` later](#configure-it), remember to change
+the Git path:
+
+- From:
+
+  ```yaml
+  git:
+    bin_path: /usr/bin/git
+  ```
 
-Replace `<X-Y-stable>` with the stable branch that matches the GitLab version you want to
-install. For example, if you want to install GitLab 13.6, use the branch name `13-6-stable`.
+- To:
 
-When editing `config/gitlab.yml` later, change the `git -> bin_path` to `/usr/local/bin/git`.
+  ```yaml
+  git:
+    bin_path: /usr/local/bin/git
+  ```
 
 ### GraphicsMagick
 

doc/user/application_security/api_fuzzing/index.md

@@ -1145,6 +1145,35 @@ Profiles:
 
 ## Troubleshooting
 
+### Error, the OpenAPI document is not valid. Errors were found during validation of the document using the published OpenAPI schema
+
+At the start of an API Fuzzing job the OpenAPI specification is validated against the [published schema](https://github.com/OAI/OpenAPI-Specification/tree/master/schemas). This error is shown when the provided OpenAPI specification has validation errors. Errors can be introduced when creating an OpenAPI specification manually, and also when the schema is generated.
+
+For OpenAPI specifications that are generated automatically validation errors are often the result of missing code annotations.
+
+**Error message**
+
+- In [GitLab 13.11 and later](https://gitlab.com/gitlab-org/gitlab/-/issues/323939), `Error, the OpenAPI document is not valid. Errors were found during validation of the document using the published OpenAPI schema`
+  - `OpenAPI 2.0 schema validation error ...`
+  - `OpenAPI 3.0.x schema validation error ...`
+
+**Solution** 
+
+**For generated OpenAPI specifications**
+
+1. Identify the validation errors.
+    1. Use the [Swagger Editor](https://editor.swagger.io/) to identify validation problems in your specification. The visual nature of the Swagger Editor makes it easier to understand what needs to change.
+    1. Alternatively, you can check the log output and look for schema validation warnings. They are prefixed with messages such as `OpenAPI 2.0 schema validation error` or `OpenAPI 3.0.x schema validation error`. Each failed validation provides extra information about `location` and `description`. Note that JSON Schema validation messages might not be easy to understand. This is why we recommend the use of editors to validate schema documents.
+1. Review the documentation for the OpenAPI generation your framework/tech stack is using. Identify the changes needed to produce a correct OpenAPI document.
+1. Once the validation issues are resolved, re-run your pipeline.
+
+**For manually created OpenAPI Specifications**
+
+1. Identify the validation errors.
+   1. The simplest solution is to use a visual tool to edit and validate the OpenAPI document. For example the [Swagger Editor](https://editor.swagger.io/) will point out schema errors and possible solutions.
+   1. Alternatively, you can check the log output and look for schema validation warnings. They are prefixed with messages such as `OpenAPI 2.0 schema validation error` or `OpenAPI 3.0.x schema validation error`. Each failed validation provides extra information about `location` and `description`. Correct each of the validation failures and then resubmit the OpenAPI doc. Note that JSON Schema validation message might not be easy to understand. This is why we recommend the use of editors to validate document.
+1. Once the validation issues are resolved, re-run your pipeline.
+
 ### Failed to start scanner session (version header not found)
 
 The API Fuzzing engine outputs an error message when it cannot establish a connection with the scanner application component. The error message is shown in the job output window of the `apifuzzer_fuzz` job. A common cause of this issue is changing the `FUZZAPI_API` variable from its default.

spec/controllers/projects/issues_controller_spec.rb

@@ -1518,12 +1518,6 @@ RSpec.describe Projects::IssuesController do
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
         expect(json_response).to eq({ 'errors' => 'Destroy confirmation not provided for issue' })
       end
-
-      it 'delegates the update of the todos count cache to TodoService' do
-        expect_any_instance_of(TodoService).to receive(:destroy_target).with(issue).once
-
-        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, destroy_confirm: true }
-      end
     end
   end
 

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -728,12 +728,6 @@ RSpec.describe Projects::MergeRequestsController do
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
         expect(json_response).to eq({ 'errors' => 'Destroy confirmation not provided for merge request' })
       end
-
-      it 'delegates the update of the todos count cache to TodoService' do
-        expect_any_instance_of(TodoService).to receive(:destroy_target).with(merge_request).once
-
-        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid, destroy_confirm: true }
-      end
     end
   end
 

spec/models/concerns/issuable_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Issuable do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:author) }
     it { is_expected.to have_many(:notes).dependent(:destroy) }
-    it { is_expected.to have_many(:todos).dependent(:destroy) }
+    it { is_expected.to have_many(:todos) }
     it { is_expected.to have_many(:labels) }
     it { is_expected.to have_many(:note_authors).through(:notes) }
 

spec/services/issuable/destroy_service_spec.rb

@@ -9,6 +9,32 @@ RSpec.describe Issuable::DestroyService do
   subject(:service) { described_class.new(project, user) }
 
   describe '#execute' do
+    shared_examples_for 'service deleting todos' do
+      it 'destroys associated todos asynchronously' do
+        expect(TodosDestroyer::DestroyedIssuableWorker)
+          .to receive(:perform_async)
+          .with(issuable.id, issuable.class.name)
+
+        subject.execute(issuable)
+      end
+
+      context 'when destroy_issuable_todos_async feature is disabled' do
+        before do
+          stub_feature_flags(destroy_issuable_todos_async: false)
+        end
+
+        it 'destroy associated todos synchronously' do
+          expect_next_instance_of(TodosDestroyer::DestroyedIssuableWorker) do |worker|
+            expect(worker)
+              .to receive(:perform)
+              .with(issuable.id, issuable.class.name)
+          end
+
+          subject.execute(issuable)
+        end
+      end
+    end
+
     context 'when issuable is an issue' do
       let!(:issue) { create(:issue, project: project, author: user, assignees: [user]) }
 
@@ -22,17 +48,14 @@ RSpec.describe Issuable::DestroyService do
         service.execute(issue)
       end
 
-      it 'updates the todo caches for users with todos on the issue' do
-        create(:todo, target: issue, user: user, author: user, project: project)
-
-        expect { service.execute(issue) }
-          .to change { user.todos_pending_count }.from(1).to(0)
-      end
-
       it 'invalidates the issues count cache for the assignees' do
         expect_any_instance_of(User).to receive(:invalidate_cache_counts).once
         service.execute(issue)
       end
+
+      it_behaves_like 'service deleting todos' do
+        let(:issuable) { issue }
+      end
     end
 
     context 'when issuable is a merge request' do
@@ -53,11 +76,8 @@ RSpec.describe Issuable::DestroyService do
         service.execute(merge_request)
       end
 
-      it 'updates the todo caches for users with todos on the merge request' do
-        create(:todo, target: merge_request, user: user, author: user, project: project)
-
-        expect { service.execute(merge_request) }
-          .to change { user.todos_pending_count }.from(1).to(0)
+      it_behaves_like 'service deleting todos' do
+        let(:issuable) { merge_request }
       end
     end
   end

spec/services/todos/destroy/destroyed_issuable_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Todos::Destroy::DestroyedIssuableService do
+  describe '#execute' do
+    let_it_be(:target) { create(:merge_request) }
+    let_it_be(:pending_todo) { create(:todo, :pending, project: target.project, target: target, user: create(:user)) }
+    let_it_be(:done_todo) { create(:todo, :done, project: target.project, target: target, user: create(:user)) }
+
+    def execute
+      described_class.new(target.id, target.class.name).execute
+    end
+
+    it 'deletes todos for specified target ID and type' do
+      control_count = ActiveRecord::QueryRecorder.new { execute }.count
+
+      # Create more todos for the target
+      create(:todo, :pending, project: target.project, target: target, user: create(:user))
+      create(:todo, :pending, project: target.project, target: target, user: create(:user))
+      create(:todo, :done, project: target.project, target: target, user: create(:user))
+      create(:todo, :done, project: target.project, target: target, user: create(:user))
+
+      expect { execute }.not_to exceed_query_limit(control_count)
+      expect(target.reload.todos.count).to eq(0)
+    end
+
+    it 'invalidates todos cache counts of todo users', :use_clean_rails_redis_caching do
+      expect { execute }
+        .to change { pending_todo.user.todos_pending_count }.from(1).to(0)
+        .and change { done_todo.user.todos_done_count }.from(1).to(0)
+    end
+  end
+end

spec/workers/todos_destroyer/destroyed_issuable_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe TodosDestroyer::DestroyedIssuableWorker do
+  let(:job_args) { [1, 'MergeRequest'] }
+
+  it 'calls the Todos::Destroy::DestroyedIssuableService' do
+    expect_next_instance_of(::Todos::Destroy::DestroyedIssuableService, *job_args) do |service|
+      expect(service).to receive(:execute)
+    end
+
+    described_class.new.perform(*job_args)
+  end
+end