Merge branch '290116-test-webhook-optimisations' into 'master'

Fix timeout problems on test webhooks

See merge request gitlab-org/gitlab!52646

app/services/concerns/integrations/project_test_data.rb

@@ -8,22 +8,41 @@ module Integrations
       Gitlab::DataBuilder::Push.build_sample(project, current_user)
     end
 
+    def use_optimal_query?
+      Feature.enabled?(:integrations_test_webhook_optimizations, project)
+    end
+
     def note_events_data
-      note = project.notes.first
+      note = if use_optimal_query?
+               NotesFinder.new(current_user, project: project, target: project).execute.reorder(nil).last # rubocop: disable CodeReuse/ActiveRecord
+             else
+               project.notes.first
+             end
+
       return { error: s_('TestHooks|Ensure the project has notes.') } unless note.present?
 
       Gitlab::DataBuilder::Note.build(note, current_user)
     end
 
     def issues_events_data
-      issue = project.issues.first
+      issue = if use_optimal_query?
+                IssuesFinder.new(current_user, project_id: project.id, sort: 'created_desc').execute.first
+              else
+                project.issues.first
+              end
+
       return { error: s_('TestHooks|Ensure the project has issues.') } unless issue.present?
 
       issue.to_hook_data(current_user)
     end
 
     def merge_requests_events_data
-      merge_request = project.merge_requests.first
+      merge_request = if use_optimal_query?
+                        MergeRequestsFinder.new(current_user, project_id: project.id, sort: 'created_desc').execute.first
+                      else
+                        project.merge_requests.first
+                      end
+
       return { error: s_('TestHooks|Ensure the project has merge requests.') } unless merge_request.present?
 
       merge_request.to_hook_data(current_user)

config/feature_flags/development/integrations_test_webhook_optimizations.yml

+---
+name: integrations_test_webhook_optimizations
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52646
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/300105
+milestone: '13.9'
+type: development
+group: group::ecosystem
+default_enabled: false

spec/services/integrations/test/project_service_spec.rb

@@ -3,11 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe Integrations::Test::ProjectService do
-  let(:user) { double('user') }
-
   describe '#execute' do
-    let(:project) { create(:project) }
+    let_it_be(:project) { create(:project) }
     let(:integration) { create(:slack_service, project: project) }
+    let(:user) { project.owner }
     let(:event) { nil }
     let(:sample_data) { { data: 'sample' } }
     let(:success_result) { { success: true, result: {} } }
@@ -70,16 +69,34 @@ RSpec.describe Integrations::Test::ProjectService do
         end
 
         it 'executes integration' do
-          allow(project).to receive(:notes).and_return([Note.new])
+          create(:note, project: project)
+
           allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data)
+          allow_next_instance_of(NotesFinder) do |finder|
+            allow(finder).to receive(:execute).and_return(Note.all)
+          end
 
           expect(integration).to receive(:test).with(sample_data).and_return(success_result)
           expect(subject).to eq(success_result)
         end
+
+        context 'when the query optimization feature flag is disabled' do
+          before do
+            stub_feature_flags(integrations_test_webhook_optimizations: false)
+          end
+
+          it 'executes the old query' do
+            allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data)
+
+            expect(NotesFinder).not_to receive(:new)
+            expect(project).to receive(:notes).and_return([Note.new])
+            expect(integration).to receive(:test).with(sample_data).and_return(success_result)
+            expect(subject).to eq(success_result)
+          end
+        end
       end
 
-      context 'issue' do
-        let(:event) { 'issue' }
+      shared_examples_for 'a test of an integration that operates on issues' do
         let(:issue) { build(:issue) }
 
         it 'returns error message if not enough data' do
@@ -90,32 +107,45 @@ RSpec.describe Integrations::Test::ProjectService do
         it 'executes integration' do
           allow(project).to receive(:issues).and_return([issue])
           allow(issue).to receive(:to_hook_data).and_return(sample_data)
+          allow_next_instance_of(IssuesFinder) do |finder|
+            allow(finder).to receive(:execute).and_return([issue])
+          end
 
           expect(integration).to receive(:test).with(sample_data).and_return(success_result)
           expect(subject).to eq(success_result)
         end
-      end
 
-      context 'confidential_issue' do
-        let(:event) { 'confidential_issue' }
-        let(:issue) { build(:issue) }
+        context 'when the query optimization feature flag is disabled' do
+          before do
+            stub_feature_flags(integrations_test_webhook_optimizations: false)
+          end
 
-        it 'returns error message if not enough data' do
-          expect(integration).not_to receive(:test)
-          expect(subject).to include({ status: :error, message: 'Ensure the project has issues.' })
+          it 'executes the old query' do
+            allow(issue).to receive(:to_hook_data).and_return(sample_data)
+
+            expect(IssuesFinder).not_to receive(:new)
+            expect(project).to receive(:issues).and_return([issue])
+            expect(integration).to receive(:test).with(sample_data).and_return(success_result)
+            expect(subject).to eq(success_result)
+          end
         end
+      end
 
-        it 'executes integration' do
-          allow(project).to receive(:issues).and_return([issue])
-          allow(issue).to receive(:to_hook_data).and_return(sample_data)
+      context 'issue' do
+        let(:event) { 'issue' }
 
-          expect(integration).to receive(:test).with(sample_data).and_return(success_result)
-          expect(subject).to eq(success_result)
-        end
+        it_behaves_like 'a test of an integration that operates on issues'
+      end
+
+      context 'confidential_issue' do
+        let(:event) { 'confidential_issue' }
+
+        it_behaves_like 'a test of an integration that operates on issues'
       end
 
       context 'merge_request' do
         let(:event) { 'merge_request' }
+        let(:merge_request) { build(:merge_request) }
 
         it 'returns error message if not enough data' do
           expect(integration).not_to receive(:test)
@@ -123,16 +153,34 @@ RSpec.describe Integrations::Test::ProjectService do
         end
 
         it 'executes integration' do
-          create(:merge_request, source_project: project)
-          allow_any_instance_of(MergeRequest).to receive(:to_hook_data).and_return(sample_data)
+          allow(merge_request).to receive(:to_hook_data).and_return(sample_data)
+          allow_next_instance_of(MergeRequestsFinder) do |finder|
+            allow(finder).to receive(:execute).and_return([merge_request])
+          end
 
           expect(integration).to receive(:test).with(sample_data).and_return(success_result)
-          expect(subject).to eq(success_result)
+          expect(subject).to include(success_result)
+        end
+
+        context 'when the query optimization feature flag is disabled' do
+          before do
+            stub_feature_flags(integrations_test_webhook_optimizations: false)
+          end
+
+          it 'executes the old query' do
+            expect(MergeRequestsFinder).not_to receive(:new)
+            expect(project).to receive(:merge_requests).and_return([merge_request])
+
+            allow(merge_request).to receive(:to_hook_data).and_return(sample_data)
+
+            expect(integration).to receive(:test).with(sample_data).and_return(success_result)
+            expect(subject).to eq(success_result)
+          end
         end
       end
 
       context 'deployment' do
-        let(:project) { create(:project, :test_repo) }
+        let_it_be(:project) { create(:project, :test_repo) }
         let(:event) { 'deployment' }
 
         it 'returns error message if not enough data' do
@@ -167,7 +215,7 @@ RSpec.describe Integrations::Test::ProjectService do
       end
 
       context 'wiki_page' do
-        let(:project) { create(:project, :wiki_repo) }
+        let_it_be(:project) { create(:project, :wiki_repo) }
         let(:event) { 'wiki_page' }
 
         it 'returns error message if wiki disabled' do

spec/services/test_hooks/project_service_spec.rb

@@ -6,8 +6,8 @@ RSpec.describe TestHooks::ProjectService do
   let(:current_user) { create(:user) }
 
   describe '#execute' do
-    let(:project) { create(:project, :repository) }
-    let(:hook)    { create(:project_hook, project: project) }
+    let_it_be(:project) { create(:project, :repository) }
+    let(:hook) { create(:project_hook, project: project) }
     let(:trigger) { 'not_implemented_events' }
     let(:service) { described_class.new(hook, current_user, trigger) }
     let(:sample_data) { { data: 'sample' } }
@@ -61,17 +61,34 @@ RSpec.describe TestHooks::ProjectService do
       end
 
       it 'executes hook' do
-        allow(project).to receive(:notes).and_return([Note.new])
+        create(:note, project: project)
+
         allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data)
+        allow_next_instance_of(NotesFinder) do |finder|
+          allow(finder).to receive(:execute).and_return(Note.all)
+        end
 
         expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
+
+      context 'when the query optimization feature flag is disabled' do
+        before do
+          stub_feature_flags(integrations_test_webhook_optimizations: false)
+        end
+
+        it 'executes the old query' do
+          allow(Gitlab::DataBuilder::Note).to receive(:build).and_return(sample_data)
+
+          expect(NotesFinder).not_to receive(:new)
+          expect(project).to receive(:notes).and_return([Note.new])
+          expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+          expect(service.execute).to include(success_result)
+        end
+      end
     end
 
-    context 'issues_events' do
-      let(:trigger) { 'issues_events' }
-      let(:trigger_key) { :issue_hooks }
+    shared_examples_for 'a test webhook that operates on issues' do
       let(:issue) { build(:issue) }
 
       it 'returns error message if not enough data' do
@@ -80,36 +97,49 @@ RSpec.describe TestHooks::ProjectService do
       end
 
       it 'executes hook' do
-        allow(project).to receive(:issues).and_return([issue])
         allow(issue).to receive(:to_hook_data).and_return(sample_data)
+        allow_next_instance_of(IssuesFinder) do |finder|
+          allow(finder).to receive(:execute).and_return([issue])
+        end
 
         expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
+
+      context 'when the query optimization feature flag is disabled' do
+        before do
+          stub_feature_flags(integrations_test_webhook_optimizations: false)
+        end
+
+        it 'executes the old query' do
+          allow(issue).to receive(:to_hook_data).and_return(sample_data)
+
+          expect(IssuesFinder).not_to receive(:new)
+          expect(project).to receive(:issues).and_return([issue])
+          expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+          expect(service.execute).to include(success_result)
+        end
+      end
+    end
+
+    context 'issues_events' do
+      let(:trigger) { 'issues_events' }
+      let(:trigger_key) { :issue_hooks }
+
+      it_behaves_like 'a test webhook that operates on issues'
     end
 
     context 'confidential_issues_events' do
       let(:trigger) { 'confidential_issues_events' }
       let(:trigger_key) { :confidential_issue_hooks }
-      let(:issue) { build(:issue) }
 
-      it 'returns error message if not enough data' do
-        expect(hook).not_to receive(:execute)
-        expect(service.execute).to include({ status: :error, message: 'Ensure the project has issues.' })
-      end
-
-      it 'executes hook' do
-        allow(project).to receive(:issues).and_return([issue])
-        allow(issue).to receive(:to_hook_data).and_return(sample_data)
-
-        expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
-        expect(service.execute).to include(success_result)
-      end
+      it_behaves_like 'a test webhook that operates on issues'
     end
 
     context 'merge_requests_events' do
       let(:trigger) { 'merge_requests_events' }
       let(:trigger_key) { :merge_request_hooks }
+      let(:merge_request) { build(:merge_request) }
 
       it 'returns error message if not enough data' do
         expect(hook).not_to receive(:execute)
@@ -117,12 +147,29 @@ RSpec.describe TestHooks::ProjectService do
       end
 
       it 'executes hook' do
-        create(:merge_request, source_project: project)
-        allow_any_instance_of(MergeRequest).to receive(:to_hook_data).and_return(sample_data)
+        allow(merge_request).to receive(:to_hook_data).and_return(sample_data)
+        allow_next_instance_of(MergeRequestsFinder) do |finder|
+          allow(finder).to receive(:execute).and_return([merge_request])
+        end
 
         expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
         expect(service.execute).to include(success_result)
       end
+
+      context 'when the query optimization feature flag is disabled' do
+        before do
+          stub_feature_flags(integrations_test_webhook_optimizations: false)
+        end
+
+        it 'executes the old query' do
+          allow(merge_request).to receive(:to_hook_data).and_return(sample_data)
+
+          expect(MergeRequestsFinder).not_to receive(:new)
+          expect(project).to receive(:merge_requests).and_return([merge_request])
+          expect(hook).to receive(:execute).with(sample_data, trigger_key).and_return(success_result)
+          expect(service.execute).to include(success_result)
+        end
+      end
     end
 
     context 'job_events' do
@@ -162,7 +209,7 @@ RSpec.describe TestHooks::ProjectService do
     end
 
     context 'wiki_page_events' do
-      let(:project) { create(:project, :wiki_repo) }
+      let_it_be(:project) { create(:project, :wiki_repo) }
       let(:trigger) { 'wiki_page_events' }
       let(:trigger_key) { :wiki_page_hooks }