summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStan Hu <stanhu@gmail.com>2017-09-16 23:25:34 -0700
committerStan Hu <stanhu@gmail.com>2017-09-18 06:53:33 -0700
commit8690ca5c2857da39c8f5839b9383931ee026f559 (patch)
tree287b8d1dab66e00c0094027510bbbae4f10423ae
parenta70c76df8fd746e5a83b305acbbc1c260955e332 (diff)
downloadgitlab-ce-8690ca5c2857da39c8f5839b9383931ee026f559.tar.gz
Eliminate N+1 queries in loading discussions.json endpointsh-optimize-discussion-json
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
-rw-r--r--app/controllers/projects/issues_controller.rb4
-rw-r--r--app/models/project_team.rb2
-rw-r--r--changelogs/unreleased/sh-optimize-discussion-json.yml5
-rw-r--r--spec/controllers/projects/issues_controller_spec.rb32
4 files changed, 40 insertions, 3 deletions
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 8990c919ca0..42bfa4b9d4f 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -87,9 +87,9 @@ class Projects::IssuesController < Projects::ApplicationController
.inc_relations_for_view
.includes(:noteable)
.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)
diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index 09049824ff7..1d35426050e 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -146,7 +146,7 @@ class ProjectTeam
def member?(user, min_access_level = Gitlab::Access::GUEST)
return false unless user
- user.authorized_project?(project, min_access_level)
+ max_member_access(user.id) >= min_access_level
end
def human_max_access(user_id)
diff --git a/changelogs/unreleased/sh-optimize-discussion-json.yml b/changelogs/unreleased/sh-optimize-discussion-json.yml
new file mode 100644
index 00000000000..4be1bc89a91
--- /dev/null
+++ b/changelogs/unreleased/sh-optimize-discussion-json.yml
@@ -0,0 +1,5 @@
+---
+title: Eliminate N+1 queries in loading discussions.json endpoint
+merge_request:
+author:
+type: fixed
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 5d9403c23ac..b4a22a46b51 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -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])
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