Merge branch...

Merge branch '21140-api-post-api-projects-id-issues-executes-more-than-100-sql-queries' into 'master'

Improve issue creation response time [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!58588

app/controllers/projects/issues_controller.rb

@@ -18,7 +18,7 @@ class Projects::IssuesController < Projects::ApplicationController
   prepend_before_action :authenticate_user!, only: [:new, :export_csv]
   prepend_before_action :store_uri, only: [:new, :show, :designs]
 
-  before_action :disable_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update]
+  before_action :disable_query_limiting, only: [:create_merge_request, :move, :bulk_update]
   before_action :check_issues_available!
   before_action :issue, unless: ->(c) { ISSUES_EXCEPT_ACTIONS.include?(c.action_name.to_sym) }
   after_action :log_issue_show, unless: ->(c) { ISSUES_EXCEPT_ACTIONS.include?(c.action_name.to_sym) }

app/services/issuable_base_service.rb

@@ -483,6 +483,8 @@ class IssuableBaseService < BaseService
   # we need to check this because milestone from milestone_id param is displayed on "new" page
   # where private project milestone could leak without this check
   def ensure_milestone_available(issuable)
+    return unless issuable.supports_milestone? && issuable.milestone_id.present?
+
     issuable.milestone_id = nil unless issuable.milestone_available?
   end
 

app/services/issues/after_create_service.rb

+# frozen_string_literal: true
+
+module Issues
+  class AfterCreateService < Issues::BaseService
+    def execute(issue)
+      todo_service.new_issue(issue, current_user)
+      delete_milestone_total_issue_counter_cache(issue.milestone)
+      track_incident_action(current_user, issue, :incident_created)
+    end
+  end
+end
+
+Issues::AfterCreateService.prepend_ee_mod

app/services/issues/base_service.rb

@@ -52,7 +52,7 @@ module Issues
     end
 
     def execute_hooks(issue, action = 'open', old_associations: {})
-      issue_data  = hook_data(issue, action, old_associations: old_associations)
+      issue_data  = Gitlab::Lazy.new { hook_data(issue, action, old_associations: old_associations) }
       hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks
       issue.project.execute_hooks(issue_data, hooks_scope)
       issue.project.execute_services(issue_data, hooks_scope)

app/services/issues/create_service.rb

@@ -32,13 +32,17 @@ module Issues
       end
     end
 
+    # Add new items to Issues::AfterCreateService if they can be performed in Sidekiq
     def after_create(issue)
       add_incident_label(issue)
-      todo_service.new_issue(issue, current_user)
       user_agent_detail_service.create
       resolve_discussions_with_issue(issue)
-      delete_milestone_total_issue_counter_cache(issue.milestone)
-      track_incident_action(current_user, issue, :incident_created)
+
+      if Feature.disabled?(:issue_perform_after_creation_tasks_async, issue.project)
+        Issues::AfterCreateService
+          .new(issue.project, current_user)
+          .execute(issue)
+      end
 
       super
     end
@@ -77,4 +81,4 @@ module Issues
   end
 end
 
-Issues::CreateService.prepend_if_ee('EE::Issues::CreateService')
+Issues::CreateService.prepend_ee_mod

app/workers/new_issue_worker.rb

@@ -14,7 +14,14 @@ class NewIssueWorker # rubocop:disable Scalability/IdempotentWorker
 
     ::EventCreateService.new.open_issue(issuable, user)
     ::NotificationService.new.new_issue(issuable, user)
+
     issuable.create_cross_references!(user)
+
+    if Feature.enabled?(:issue_perform_after_creation_tasks_async, issuable.project)
+      Issues::AfterCreateService
+        .new(issuable.project, user)
+        .execute(issuable)
+    end
   end
 
   def issuable_class

config/feature_flags/development/issue_perform_after_creation_tasks_async.yml

+---
+name: issue_perform_after_creation_tasks_async
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/58588
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/21140
+milestone: '13.11'
+type: development
+group: group::geo
+default_enabled: false

ee/app/services/ee/issues/after_create_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Issues
+    module AfterCreateService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute(issue)
+        super
+
+        add_issue_sla(issue)
+      end
+
+      private
+
+      def add_issue_sla(issue)
+        return unless issue.sla_available?
+
+        ::IncidentManagement::Incidents::CreateSlaService.new(issue, current_user).execute
+      end
+    end
+  end
+end

ee/app/services/ee/issues/create_service.rb

@@ -31,6 +31,7 @@ module EE
       private
 
       def add_issue_sla(issue)
+        return if ::Feature.enabled?(:issue_perform_after_creation_tasks_async, issue.project)
         return unless issue.sla_available?
 
         ::IncidentManagement::Incidents::CreateSlaService.new(issue, current_user).execute

ee/spec/services/ee/issues/after_create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issues::AfterCreateService do
+  include AfterNextHelpers
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:issue) { create(:issue, project: project, author: current_user) }
+
+  subject(:after_create_service) { described_class.new(project, current_user) }
+
+  describe '#execute' do
+    context 'when issue sla is available' do
+      it 'calls IncidentManagement::Incidents::CreateSlaService' do
+        allow(issue).to receive(:sla_available?).and_return(true)
+
+        expect_next(::IncidentManagement::Incidents::CreateSlaService, issue, current_user)
+            .to receive(:execute)
+
+        after_create_service.execute(issue)
+      end
+    end
+
+    context 'when issue sla is not available' do
+      it 'does not call IncidentManagement::Incidents::CreateSlaService' do
+        allow(issue).to receive(:sla_available?).and_return(false)
+
+        expect(::IncidentManagement::Incidents::CreateSlaService)
+            .not_to receive(:new)
+
+        after_create_service.execute(issue)
+      end
+    end
+  end
+end

lib/api/issues.rb

@@ -242,7 +242,7 @@ module API
         use :issue_params
       end
       post ':id/issues' do
-        Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/20773')
+        Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/21140')
 
         check_rate_limit! :issues_create, [current_user]
 

spec/services/issues/after_create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issues::AfterCreateService do
+  include AfterNextHelpers
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:assignee) { create(:user) }
+  let_it_be(:milestone) { create(:milestone, project: project) }
+  let_it_be(:issue) { create(:issue, project: project, author: current_user, milestone: milestone, assignee_ids: [assignee.id]) }
+
+  subject(:after_create_service) { described_class.new(project, current_user) }
+
+  describe '#execute' do
+    it 'creates a pending todo for new assignee' do
+      attributes = {
+        project: project,
+        author: current_user,
+        user: assignee,
+        target_id: issue.id,
+        target_type: issue.class.name,
+        action: Todo::ASSIGNED,
+        state: :pending
+      }
+
+      expect { after_create_service.execute(issue) }.to change { Todo.where(attributes).count }.by(1)
+    end
+
+    it 'deletes milestone issues count cache' do
+      expect_next(Milestones::IssuesCountService, milestone)
+        .to receive(:delete_cache).and_call_original
+
+      after_create_service.execute(issue)
+    end
+
+    context 'with a regular issue' do
+      it_behaves_like 'does not track incident management event', :incident_management_incident_created do
+        subject { after_create_service.execute(issue) }
+      end
+    end
+
+    context 'with an incident issue' do
+      let(:issue) { create(:issue, :incident, project: project, author: current_user) }
+
+      it_behaves_like 'an incident management tracked event', :incident_management_incident_created do
+        subject { after_create_service.execute(issue) }
+      end
+    end
+  end
+end

spec/services/issues/create_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Issues::CreateService do
+  include AfterNextHelpers
+
   let_it_be_with_reload(:project) { create(:project) }
   let_it_be(:user) { create(:user) }
 
@@ -64,7 +66,6 @@ RSpec.describe Issues::CreateService do
 
         it_behaves_like 'incident issue'
         it_behaves_like 'has incident label'
-        it_behaves_like 'an incident management tracked event', :incident_management_incident_created
 
         it 'does create an incident label' do
           expect { subject }
@@ -112,26 +113,36 @@ RSpec.describe Issues::CreateService do
         end
       end
 
-      it 'creates a pending todo for new assignee' do
-        attributes = {
-          project: project,
-          author: user,
-          user: assignee,
-          target_id: issue.id,
-          target_type: issue.class.name,
-          action: Todo::ASSIGNED,
-          state: :pending
-        }
-
-        expect(Todo.where(attributes).count).to eq 1
-      end
-
       it 'moves the issue to the end, in an asynchronous worker' do
         expect(IssuePlacementWorker).to receive(:perform_async).with(be_nil, Integer)
 
         described_class.new(project, user, opts).execute
       end
 
+      context 'with issue_perform_after_creation_tasks_async feature disabled' do
+        before do
+          stub_feature_flags(issue_perform_after_creation_tasks_async: false)
+        end
+
+        it 'calls Issues::AfterCreateService' do
+          expect_next(::Issues::AfterCreateService, project, user).to receive(:execute)
+
+          described_class.new(project, user, opts).execute
+        end
+      end
+
+      context 'with issue_perform_after_creation_tasks_async feature enabled' do
+        before do
+          stub_feature_flags(issue_perform_after_creation_tasks_async: true)
+        end
+
+        it 'does not call Issues::AfterCreateService' do
+          expect(::Issues::AfterCreateService).not_to receive(:new)
+
+          described_class.new(project, user, opts).execute
+        end
+      end
+
       context 'when label belongs to project group' do
         let(:group) { create(:group) }
         let(:group_labels) { create_pair(:group_label, group: group) }
@@ -279,14 +290,6 @@ RSpec.describe Issues::CreateService do
         end
       end
 
-      it 'deletes milestone issues count cache' do
-        expect_next_instance_of(Milestones::IssuesCountService, milestone) do |service|
-          expect(service).to receive(:delete_cache).and_call_original
-        end
-
-        issue
-      end
-
       it 'schedules a namespace onboarding create action worker' do
         expect(Namespaces::OnboardingIssueCreatedWorker).to receive(:perform_async).with(project.namespace.id)
 

spec/workers/new_issue_worker_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe NewIssueWorker do
+  include AfterNextHelpers
+
   describe '#perform' do
     let(:worker) { described_class.new }
 
@@ -80,6 +82,31 @@ RSpec.describe NewIssueWorker do
 
           worker.perform(issue.id, user.id)
         end
+
+        context 'with issue_perform_after_creation_tasks_async feature disabled' do
+          before do
+            stub_feature_flags(issue_perform_after_creation_tasks_async: false)
+          end
+
+          it 'does not call Issues::AfterCreateService' do
+            expect(::Issues::AfterCreateService).not_to receive(:execute)
+
+            worker.perform(issue.id, user.id)
+          end
+        end
+
+        context 'with issue_perform_after_creation_tasks_async feature enabled' do
+          before do
+            stub_feature_flags(issue_perform_after_creation_tasks_async: true)
+          end
+
+          it 'calls Issues::AfterCreateService' do
+            expect_next(::Issues::AfterCreateService)
+                .to receive(:execute)
+
+            worker.perform(issue.id, user.id)
+          end
+        end
       end
     end
   end