Eliminate N+1 queries in loading discussions.json endpoint

In #37955,we see that the profile had a number of N+1 queries from repeated
access to `cross_reference_not_visible_for?`. This was optimized in previous
versions of GitLab by rendering all notes at once, counting the number of
visible references, and then using that number to check whether a system note
should be fully redacted.

There was also another N+1 query calling `ProjectTeam#member?`, which did not
take advantage of an optimization in prepare_notes_for_rendering that would
preload the maximum access level per project.

Closes #37955
This commit is contained in:
Stan Hu 2017-09-16 23:25:34 -07:00
parent a70c76df8f
commit 8690ca5c28
4 changed files with 40 additions and 3 deletions

View file

@ -87,9 +87,9 @@ class Projects::IssuesController < Projects::ApplicationController
.inc_relations_for_view .inc_relations_for_view
.includes(:noteable) .includes(:noteable)
.fresh .fresh
.reject { |n| n.cross_reference_not_visible_for?(current_user) }
prepare_notes_for_rendering(notes) notes = prepare_notes_for_rendering(notes)
notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
discussions = Discussion.build_collection(notes, @issue) discussions = Discussion.build_collection(notes, @issue)

View file

@ -146,7 +146,7 @@ class ProjectTeam
def member?(user, min_access_level = Gitlab::Access::GUEST) def member?(user, min_access_level = Gitlab::Access::GUEST)
return false unless user return false unless user
user.authorized_project?(project, min_access_level) max_member_access(user.id) >= min_access_level
end end
def human_max_access(user_id) def human_max_access(user_id)

View file

@ -0,0 +1,5 @@
---
title: Eliminate N+1 queries in loading discussions.json endpoint
merge_request:
author:
type: fixed

View file

@ -900,5 +900,37 @@ describe Projects::IssuesController do
expect(JSON.parse(response.body).first.keys).to match_array(%w[id reply_id expanded notes individual_note]) expect(JSON.parse(response.body).first.keys).to match_array(%w[id reply_id expanded notes individual_note])
end end
context 'with cross-reference system note', :request_store do
let(:new_issue) { create(:issue) }
let(:cross_reference) { "mentioned in #{new_issue.to_reference(issue.project)}" }
before do
create(:discussion_note_on_issue, :system, noteable: issue, project: issue.project, note: cross_reference)
end
it 'filters notes that the user should not see' do
get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
expect(JSON.parse(response.body).count).to eq(1)
end
it 'does not result in N+1 queries' do
# Instantiate the controller variables to ensure QueryRecorder has an accurate base count
get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
RequestStore.clear!
control_count = ActiveRecord::QueryRecorder.new do
get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
end.count
RequestStore.clear!
create_list(:discussion_note_on_issue, 2, :system, noteable: issue, project: issue.project, note: cross_reference)
expect { get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid }.not_to exceed_query_limit(control_count)
end
end
end end
end end