Commit 10096256 authored by Stan Hu's avatar Stan Hu

Improve performance of filtering notes in NotesController

Reduces the number of queries needed to redact notes to which
the user does not have access. Also includes an N+1 query test
as a guard against future issues.

This is a follow-up from https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14327#note_40976854.
parent 4d5ea927
...@@ -15,9 +15,9 @@ module NotesActions ...@@ -15,9 +15,9 @@ module NotesActions
notes = notes_finder.execute notes = notes_finder.execute
.inc_relations_for_view .inc_relations_for_view
.reject { |n| n.cross_reference_not_visible_for?(current_user) }
notes = prepare_notes_for_rendering(notes) notes = prepare_notes_for_rendering(notes)
notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
notes_json[:notes] = notes_json[:notes] =
if noteable.discussions_rendered_on_frontend? if noteable.discussions_rendered_on_frontend?
......
...@@ -120,6 +120,40 @@ describe Projects::NotesController do ...@@ -120,6 +120,40 @@ describe Projects::NotesController do
expect(note_json[:diff_discussion_html]).to be_nil expect(note_json[:diff_discussion_html]).to be_nil
end end
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
note
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 :index, request_params
expect(parsed_response[:notes].count).to eq(1)
expect(note_json[:id]).to eq(note.id)
end
it 'does not result in N+1 queries' do
# Instantiate the controller variables to ensure QueryRecorder has an accurate base count
get :index, request_params
RequestStore.clear!
control_count = ActiveRecord::QueryRecorder.new do
get :index, request_params
end.count
RequestStore.clear!
create_list(:discussion_note_on_issue, 2, :system, noteable: issue, project: issue.project, note: cross_reference)
expect { get :index, request_params }.not_to exceed_query_limit(control_count)
end
end
end end
describe 'POST create' do describe 'POST create' do
......
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