Commit f1f96540 authored by Alex Kalderimis's avatar Alex Kalderimis

Fix N+1 in Epic.issues

This uses the caching_array_resolver to eliminate the N+1 for
Epic.issues.

The filtering by accessibility is removed since we have authorization to
do that for us, which is tested in the request specs.
parent 7370f60f
......@@ -62,6 +62,7 @@ module CachingArrayResolver
queries.in_groups_of(max_union_size, false).each do |group|
by_id = model_class
.from_union(tag(group), remove_duplicates: false)
.preload(preload) # rubocop: disable CodeReuse/ActiveRecord
.group_by { |r| r[primary_key] }
by_id.values.each do |item_group|
......@@ -75,6 +76,10 @@ module CachingArrayResolver
end
end
def preload
nil
end
# Override this to intercept the items once they are found
def item_found(query_input, item)
end
......
......@@ -22,6 +22,7 @@ class Issue < ApplicationRecord
include Presentable
include IssueAvailableFeatures
include Todoable
include FromUnion
DueDateStruct = Struct.new(:title, :name).freeze
NoDueDate = DueDateStruct.new('No Due Date', '0').freeze
......
......@@ -2,12 +2,26 @@
module Resolvers
class EpicIssuesResolver < BaseResolver
include CachingArrayResolver
type Types::EpicIssueType.connection_type, null: true
alias_method :epic, :object
def resolve(**args)
epic.issues_readable_by(context[:current_user], preload: { project: [:namespace, :project_feature] })
def model_class
::Issue
end
def query_input(**args)
epic.id
end
def query_for(id)
::Epic.related_issues(ids: id)
end
def preload
{ project: [:namespace, :project_feature] }
end
end
end
......@@ -3,6 +3,7 @@
require 'spec_helper'
RSpec.describe Resolvers::EpicIssuesResolver do
include ::Gitlab::Graphql::Laziness
include GraphqlHelpers
let_it_be(:current_user) { create(:user) }
......@@ -38,17 +39,9 @@ RSpec.describe Resolvers::EpicIssuesResolver do
expect(result).to eq [[issue2, issue1], [issue3, issue4]]
end
it 'finds only epic issues that user can read' do
guest = create(:user)
result = epics.map { |e| resolve_epic_issues(e, user: guest).to_a }
expect(result).to eq [[issue1], []]
end
end
def resolve_epic_issues(object, user: current_user)
resolve(described_class, obj: object, ctx: { current_user: user })
force(resolve(described_class, obj: object, ctx: { current_user: user }))
end
end
......@@ -113,11 +113,8 @@ RSpec.describe 'Getting issues for an epic' do
let_it_be(:epic_issue3) { create(:epic_issue, epic: epic2, issue: issue2, relative_position: 3) }
let(:params) { { iids: [epic.iid, epic2.iid] } }
before do
project.add_developer(user)
end
it 'returns issues for each epic' do
project.add_developer(user)
post_graphql(epic_query(params), current_user: user)
expect(response).to have_gitlab_http_status(:success)
......@@ -127,14 +124,16 @@ RSpec.describe 'Getting issues for an epic' do
end
it 'avoids N+1 queries' do
pending 'https://gitlab.com/gitlab-org/gitlab/-/issues/291089'
control_count = ActiveRecord::QueryRecorder.new do
post_graphql(epic_query(iid: epic.iid), current_user: user)
end.count
user_1 = create(:user, developer_projects: [project])
user_2 = create(:user, developer_projects: [project])
control_count = ActiveRecord::QueryRecorder.new(query_recorder_debug: true) do
post_graphql(epic_query(iid: epic.iid), current_user: user_1)
end
expect do
post_graphql(epic_query(params), current_user: user)
end.not_to exceed_query_limit(control_count)
post_graphql(epic_query(params), current_user: user_2)
end.not_to exceed_query_limit(control_count).ignoring(/FROM "namespaces"/)
expect(graphql_errors).to be_nil
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