Commit bc7b447c authored by Luke Duncalfe's avatar Luke Duncalfe

Fix timeouts on test webhooks

The Issues, Merge Requests and Notes test webhooks were susceptible to
timeouts due to the oldest records being selected to be in the test
webhook payload.

Indexes on these tables seem to favour sorting newest to oldest, the
difference between selecting the oldest Issue, Merge Request, or Note vs
the newest for `gitlab-org/gitlab` is ~ 25 seconds (or sometimes more)
on `#database-lab` (which is slower than production, but indicative) vs
~10 milliseconds when selecting the newest records.

Finders are now used for all scoping to make the scoping even safer.

https://gitlab.com/gitlab-org/gitlab/-/issues/290116
parent 9903c502
......@@ -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)
......
---
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
......@@ -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
context 'issue' do
let(:event) { 'issue' }
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
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) }
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.' })
context 'when the query optimization feature flag is disabled' do
before do
stub_feature_flags(integrations_test_webhook_optimizations: false)
end
it 'executes integration' do
allow(project).to receive(:issues).and_return([issue])
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
context 'issue' do
let(:event) { 'issue' }
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 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
......
......@@ -6,7 +6,7 @@ RSpec.describe TestHooks::ProjectService do
let(:current_user) { create(:user) }
describe '#execute' do
let(:project) { create(:project, :repository) }
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) }
......@@ -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
context 'issues_events' do
let(:trigger) { 'issues_events' }
let(:trigger_key) { :issue_hooks }
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
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
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.' })
context 'when the query optimization feature flag is disabled' do
before do
stub_feature_flags(integrations_test_webhook_optimizations: false)
end
it 'executes hook' do
allow(project).to receive(:issues).and_return([issue])
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 }
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 }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment