Commit 80545180 authored by Stan Hu's avatar Stan Hu

Fix N+1 queries with loading group issues with GraphQL

Previously when group issues were queried, we called a SELECT for each
project and its associated project features to check whether the user
had permission to read the issue. We now solve this by preloading those
items unconditionally.
parent e6407be9
......@@ -19,7 +19,7 @@ module Resolvers
milestone_due_asc milestone_due_desc].freeze
def continue_issue_resolve(parent, finder, **args)
issues = apply_lookahead(Gitlab::Graphql::Loaders::IssuableLoader.new(parent, finder).batching_find_all)
issues = Gitlab::Graphql::Loaders::IssuableLoader.new(parent, finder).batching_find_all { |q| apply_lookahead(q) }
if non_stable_cursor_sort?(args[:sort])
# Certain complex sorts are not supported by the stable cursor pagination yet.
......@@ -32,6 +32,14 @@ module Resolvers
private
def unconditional_includes
[
{
project: [:project_feature]
}
]
end
def preloads
{
alert_management_alert: [:alert_management_alert],
......
---
title: Fix N+1 queries with loading group issues with GraphQL
merge_request: 50328
author:
type: performance
......@@ -23,14 +23,19 @@ RSpec.describe GitlabSchema.types['Issue'] do
shared_examples 'avoids N+1 queries on blocked' do
specify do
# Warm up table schema and other data (e.g. SAML providers, license)
GitlabSchema.execute(query, context: { current_user: user })
control_count = ActiveRecord::QueryRecorder.new { GitlabSchema.execute(query, context: { current_user: user }) }.count
blocked_issue2 = create(:issue, project: project)
blocking_issue2 = create(:issue, project: project)
create :issue_link, source: blocked_issue2, target: blocking_issue2, link_type: IssueLink::TYPE_IS_BLOCKED_BY
# added the +1 due to an existing N+1 with issues
expect { GitlabSchema.execute(query, context: { current_user: user }) }.not_to exceed_query_limit(control_count + 1)
project2 = create(:project, :public, group: group)
create(:issue, project: project2)
expect { GitlabSchema.execute(query, context: { current_user: user }) }.not_to exceed_query_limit(control_count)
end
end
......
......@@ -264,13 +264,13 @@ RSpec.describe Resolvers::IssuesResolver do
end
it 'finds a specific issue with iid', :request_store do
result = batch_sync(max_queries: 2) { resolve_issues(iid: issue1.iid) }
result = batch_sync(max_queries: 4) { resolve_issues(iid: issue1.iid) }
expect(result).to contain_exactly(issue1)
end
it 'batches queries that only include IIDs', :request_store do
result = batch_sync(max_queries: 2) do
result = batch_sync(max_queries: 4) do
[issue1, issue2]
.map { |issue| resolve_issues(iid: issue.iid.to_s) }
.flat_map(&:to_a)
......@@ -280,7 +280,7 @@ RSpec.describe Resolvers::IssuesResolver do
end
it 'finds a specific issue with iids', :request_store do
result = batch_sync(max_queries: 2) do
result = batch_sync(max_queries: 4) do
resolve_issues(iids: [issue1.iid])
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