From 7856804b9541598b6bea910a2200622a7482b1cd Mon Sep 17 00:00:00 2001
From: Igor Drozdov <idrozdov@gitlab.com>
Date: Thu, 25 Mar 2021 14:29:39 +0300
Subject: [PATCH] Resolve N + 1 for JIRA pulls
When API request performed for multiple pulls
the number of SQL requests depends on the number of MRs
---
app/models/merge_request.rb | 1 +
.../unreleased/id-n-1-for-jira-pulls.yml | 5 ++
lib/api/v3/github.rb | 7 ++-
spec/requests/api/v3/github_spec.rb | 50 +++++++++++++++----
4 files changed, 50 insertions(+), 13 deletions(-)
create mode 100644 changelogs/unreleased/id-n-1-for-jira-pulls.yml
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 834d28f1e3c7d..133c6217bc089 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -316,6 +316,7 @@ def public_merge_status
}
scope :with_csv_entity_associations, -> { preload(:assignees, :approved_by_users, :author, :milestone, metrics: [:merged_by]) }
+ scope :with_jira_integration_associations, -> { preload(:metrics, :assignees, :author, :target_project, :source_project) }
scope :by_target_branch_wildcard, ->(wildcard_branch_name) do
where("target_branch LIKE ?", ApplicationRecord.sanitize_sql_like(wildcard_branch_name).tr('*', '%'))
diff --git a/changelogs/unreleased/id-n-1-for-jira-pulls.yml b/changelogs/unreleased/id-n-1-for-jira-pulls.yml
new file mode 100644
index 0000000000000..e6d37b54c49f2
--- /dev/null
+++ b/changelogs/unreleased/id-n-1-for-jira-pulls.yml
@@ -0,0 +1,5 @@
+---
+title: Resolve N + 1 for JIRA pulls
+merge_request: 57482
+author:
+type: performance
diff --git a/lib/api/v3/github.rb b/lib/api/v3/github.rb
index 3a0609651f4f0..7938339e1b15b 100644
--- a/lib/api/v3/github.rb
+++ b/lib/api/v3/github.rb
@@ -75,11 +75,14 @@ def find_merge_request_with_access(id, access_level = :read_merge_request)
# rubocop: enable CodeReuse/ActiveRecord
def authorized_merge_requests
- MergeRequestsFinder.new(current_user, authorized_only: !current_user.admin?).execute
+ MergeRequestsFinder.new(current_user, authorized_only: !current_user.admin?)
+ .execute.with_jira_integration_associations
end
def authorized_merge_requests_for_project(project)
- MergeRequestsFinder.new(current_user, authorized_only: !current_user.admin?, project_id: project.id).execute
+ MergeRequestsFinder
+ .new(current_user, authorized_only: !current_user.admin?, project_id: project.id)
+ .execute.with_jira_integration_associations
end
# rubocop: disable CodeReuse/ActiveRecord
diff --git a/spec/requests/api/v3/github_spec.rb b/spec/requests/api/v3/github_spec.rb
index 197c6cbb0ebe6..521187f0a8189 100644
--- a/spec/requests/api/v3/github_spec.rb
+++ b/spec/requests/api/v3/github_spec.rb
@@ -3,10 +3,10 @@
require 'spec_helper'
RSpec.describe API::V3::Github do
- let(:user) { create(:user) }
- let(:unauthorized_user) { create(:user) }
- let(:admin) { create(:user, :admin) }
- let(:project) { create(:project, :repository, creator: user) }
+ let_it_be(:user) { create(:user) }
+ let_it_be(:unauthorized_user) { create(:user) }
+ let_it_be(:admin) { create(:user, :admin) }
+ let_it_be(:project) { create(:project, :repository, creator: user) }
before do
project.add_maintainer(user)
@@ -210,14 +210,14 @@
end
describe 'repo pulls' do
- let(:project2) { create(:project, :repository, creator: user) }
- let(:assignee) { create(:user) }
- let(:assignee2) { create(:user) }
- let!(:merge_request) do
+ let_it_be(:project2) { create(:project, :repository, creator: user) }
+ let_it_be(:assignee) { create(:user) }
+ let_it_be(:assignee2) { create(:user) }
+ let_it_be(:merge_request) do
create(:merge_request, source_project: project, target_project: project, author: user, assignees: [assignee])
end
- let!(:merge_request_2) do
+ let_it_be(:merge_request_2) do
create(:merge_request, source_project: project2, target_project: project2, author: user, assignees: [assignee, assignee2])
end
@@ -225,26 +225,54 @@
project2.add_maintainer(user)
end
+ def perform_request
+ jira_get v3_api(route, user)
+ end
+
describe 'GET /-/jira/pulls' do
+ let(:route) { '/repos/-/jira/pulls' }
+
it 'returns an array of merge requests with github format' do
- jira_get v3_api('/repos/-/jira/pulls', user)
+ perform_request
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to be_an(Array)
expect(json_response.size).to eq(2)
expect(response).to match_response_schema('entities/github/pull_requests')
end
+
+ it 'returns multiple merge requests without N + 1' do
+ perform_request
+
+ control_count = ActiveRecord::QueryRecorder.new { perform_request }.count
+
+ create(:merge_request, source_project: project, source_branch: 'fix')
+
+ expect { perform_request }.not_to exceed_query_limit(control_count)
+ end
end
describe 'GET /repos/:namespace/:project/pulls' do
+ let(:route) { "/repos/#{project.namespace.path}/#{project.path}/pulls" }
+
it 'returns an array of merge requests for the proper project in github format' do
- jira_get v3_api("/repos/#{project.namespace.path}/#{project.path}/pulls", user)
+ perform_request
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to be_an(Array)
expect(json_response.size).to eq(1)
expect(response).to match_response_schema('entities/github/pull_requests')
end
+
+ it 'returns multiple merge requests without N + 1' do
+ perform_request
+
+ control_count = ActiveRecord::QueryRecorder.new { perform_request }.count
+
+ create(:merge_request, source_project: project, source_branch: 'fix')
+
+ expect { perform_request }.not_to exceed_query_limit(control_count)
+ end
end
describe 'GET /repos/:namespace/:project/pulls/:id' do
--
GitLab