Commit 6913ba46 authored by Igor Drozdov's avatar Igor Drozdov

Reduce SQL requests number for issue links

When API request performed for multiple issue links
the number of SQL requests depends on the number of issues.
parent e7b0bd33
...@@ -164,8 +164,8 @@ module LabelsHelper ...@@ -164,8 +164,8 @@ module LabelsHelper
end end
def label_subscription_status(label, project) def label_subscription_status(label, project)
return 'group-level' if label.lazy_subscribed?(current_user) return 'group-level' if label.subscribed?(current_user)
return 'project-level' if label.lazy_subscribed?(current_user, project) return 'project-level' if label.subscribed?(current_user, project)
'unsubscribed' 'unsubscribed'
end end
...@@ -181,7 +181,7 @@ module LabelsHelper ...@@ -181,7 +181,7 @@ module LabelsHelper
end end
def label_subscription_toggle_button_text(label, project = nil) def label_subscription_toggle_button_text(label, project = nil)
label.lazy_subscribed?(current_user, project) ? 'Unsubscribe' : 'Subscribe' label.subscribed?(current_user, project) ? 'Unsubscribe' : 'Subscribe'
end end
def create_label_title(subject) def create_label_title(subject)
......
...@@ -17,16 +17,6 @@ module Subscribable ...@@ -17,16 +17,6 @@ module Subscribable
def subscribed?(user, project = nil) def subscribed?(user, project = nil)
return false unless user return false unless user
if (subscription = subscriptions.find_by(user: user, project: project))
subscription.subscribed
else
subscribed_without_subscriptions?(user, project)
end
end
def lazy_subscribed?(user, project = nil)
return false unless user
if (subscription = lazy_subscription(user, project)&.itself) if (subscription = lazy_subscription(user, project)&.itself)
subscription.subscribed subscription.subscribed
else else
...@@ -75,8 +65,10 @@ module Subscribable ...@@ -75,8 +65,10 @@ module Subscribable
def toggle_subscription(user, project = nil) def toggle_subscription(user, project = nil)
unsubscribe_from_other_levels(user, project) unsubscribe_from_other_levels(user, project)
new_value = !subscribed?(user, project)
find_or_initialize_subscription(user, project) find_or_initialize_subscription(user, project)
.update(subscribed: !subscribed?(user, project)) .update(subscribed: new_value)
end end
def subscribe(user, project = nil) def subscribe(user, project = nil)
...@@ -117,6 +109,8 @@ module Subscribable ...@@ -117,6 +109,8 @@ module Subscribable
end end
def find_or_initialize_subscription(user, project) def find_or_initialize_subscription(user, project)
BatchLoader::Executor.clear_current
subscriptions subscriptions
.find_or_initialize_by(user_id: user.id, project_id: project.try(:id)) .find_or_initialize_by(user_id: user.id, project_id: project.try(:id))
end end
......
...@@ -115,6 +115,7 @@ class Issue < ApplicationRecord ...@@ -115,6 +115,7 @@ class Issue < ApplicationRecord
scope :preload_associated_models, -> { preload(:assignees, :labels, project: :namespace) } scope :preload_associated_models, -> { preload(:assignees, :labels, project: :namespace) }
scope :with_web_entity_associations, -> { preload(:author, project: [:project_feature, :route, namespace: :route]) } scope :with_web_entity_associations, -> { preload(:author, project: [:project_feature, :route, namespace: :route]) }
scope :preload_awardable, -> { preload(:award_emoji) }
scope :with_label_attributes, ->(label_attributes) { joins(:labels).where(labels: label_attributes) } scope :with_label_attributes, ->(label_attributes) { joins(:labels).where(labels: label_attributes) }
scope :with_alert_management_alerts, -> { joins(:alert_management_alert) } scope :with_alert_management_alerts, -> { joins(:alert_management_alert) }
scope :with_prometheus_alert_events, -> { joins(:issues_prometheus_alert_events) } scope :with_prometheus_alert_events, -> { joins(:issues_prometheus_alert_events) }
...@@ -343,6 +344,8 @@ class Issue < ApplicationRecord ...@@ -343,6 +344,8 @@ class Issue < ApplicationRecord
.preload(preload) .preload(preload)
.reorder('issue_link_id') .reorder('issue_link_id')
related_issues = yield related_issues if block_given?
cross_project_filter = -> (issues) { issues.where(project: project) } cross_project_filter = -> (issues) { issues.where(project: project) }
Ability.issues_readable_by_user(related_issues, Ability.issues_readable_by_user(related_issues,
current_user, current_user,
......
---
title: Reduce SQL requests number for issue links
merge_request: 57602
author:
type: performance
...@@ -18,7 +18,10 @@ module API ...@@ -18,7 +18,10 @@ module API
end end
get ':id/issues/:issue_iid/links' do get ':id/issues/:issue_iid/links' do
source_issue = find_project_issue(params[:issue_iid]) source_issue = find_project_issue(params[:issue_iid])
related_issues = source_issue.related_issues(current_user) related_issues = source_issue.related_issues(current_user) do |issues|
issues.with_api_entity_associations.preload_awardable
end
related_issues.each { |issue| issue.lazy_subscription(current_user, user_project) } # preload subscriptions
present related_issues, present related_issues,
with: Entities::RelatedIssue, with: Entities::RelatedIssue,
......
...@@ -194,10 +194,6 @@ RSpec.describe Subscribable, 'Subscribable' do ...@@ -194,10 +194,6 @@ RSpec.describe Subscribable, 'Subscribable' do
end end
end end
describe '#lazy_subscribed?' do
it_behaves_like 'returns expected values', :lazy_subscribed?
end
describe '#lazy_subscription' do describe '#lazy_subscription' do
let(:labels) { create_list(:group_label, 5) } let(:labels) { create_list(:group_label, 5) }
......
...@@ -12,26 +12,40 @@ RSpec.describe API::IssueLinks do ...@@ -12,26 +12,40 @@ RSpec.describe API::IssueLinks do
end end
describe 'GET /links' do describe 'GET /links' do
def perform_request(user = nil, params = {})
get api("/projects/#{project.id}/issues/#{issue.iid}/links", user), params: params
end
context 'when unauthenticated' do context 'when unauthenticated' do
it 'returns 401' do it 'returns 401' do
get api("/projects/#{project.id}/issues/#{issue.iid}/links") perform_request
expect(response).to have_gitlab_http_status(:unauthorized) expect(response).to have_gitlab_http_status(:unauthorized)
end end
end end
context 'when authenticated' do context 'when authenticated' do
it 'returns related issues' do let_it_be(:issue_link1) { create(:issue_link, source: issue, target: create(:issue, project: project)) }
target_issue = create(:issue, project: project) let_it_be(:issue_link2) { create(:issue_link, source: issue, target: create(:issue, project: project)) }
create(:issue_link, source: issue, target: target_issue)
get api("/projects/#{project.id}/issues/#{issue.iid}/links", user) it 'returns related issues' do
perform_request(user)
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.length).to eq(1) expect(json_response.length).to eq(2)
expect(response).to match_response_schema('public_api/v4/issue_links') expect(response).to match_response_schema('public_api/v4/issue_links')
end end
it 'returns multiple links without N + 1' do
perform_request(user)
control_count = ActiveRecord::QueryRecorder.new { perform_request(user) }.count
create(:issue_link, source: issue, target: create(:issue, project: project))
expect { perform_request(user) }.not_to exceed_query_limit(control_count)
end
end end
end end
......
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