From 3e1227e7f58b687bcfe192e3805626494343c667 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 14 Nov 2018 04:14:50 +0100 Subject: Eager-load relations for Dashboard::ProjectsController This removes remaining N+1 queries --- app/controllers/dashboard/projects_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index 57e612d89d3..f073b6de444 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -56,7 +56,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController projects = ProjectsFinder .new(params: finder_params, current_user: current_user) .execute - .includes(:route, :creator, namespace: [:route, :owner]) + .includes(:route, :creator, :group, namespace: [:route, :owner]) .page(finder_params[:page]) prepare_projects_for_rendering(projects) -- cgit v1.2.1 From d81c646014a112d15b0510a58ef74a28fd55f125 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 14 Nov 2018 04:15:17 +0100 Subject: Eager-load relations for Explore::ProjectsController This removes remaining N+1 queries --- app/controllers/explore/projects_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/explore/projects_controller.rb b/app/controllers/explore/projects_controller.rb index 7ecbc32cf4e..778fdda8dbd 100644 --- a/app/controllers/explore/projects_controller.rb +++ b/app/controllers/explore/projects_controller.rb @@ -57,7 +57,7 @@ class Explore::ProjectsController < Explore::ApplicationController def load_projects projects = ProjectsFinder.new(current_user: current_user, params: params) .execute - .includes(:route, namespace: :route) + .includes(:route, :creator, :group, namespace: [:route, :owner]) .page(params[:page]) .without_count -- cgit v1.2.1 From 1297a1bd7dcccaa4e233925c8467c2bbe41a4bf2 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 14 Nov 2018 04:17:16 +0100 Subject: Remove STI #becomes call from Dashboard index By defining the correct route helper path instead of relying on the polymorphic route, we don't invalidate the eager-loaded relations. This change removes remaining N+1 queries --- app/views/admin/dashboard/index.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/dashboard/index.html.haml b/app/views/admin/dashboard/index.html.haml index 7ac79cc77f5..6756299cf43 100644 --- a/app/views/admin/dashboard/index.html.haml +++ b/app/views/admin/dashboard/index.html.haml @@ -174,7 +174,7 @@ %h4 Latest projects - @projects.each do |project| %p - = link_to project.full_name, [:admin, project.namespace.becomes(Namespace), project], class: 'str-truncated-60' + = link_to project.full_name, admin_project_path(project), class: 'str-truncated-60' %span.light.float-right #{time_ago_with_tooltip(project.created_at)} .col-md-4 -- cgit v1.2.1 From 4f5abe43279e96efde5f8cac66cbff30d8a95f28 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 14 Nov 2018 06:07:35 +0100 Subject: Reduce N+1 from Activity Dashboard and Banzai There is a combination of few strategies implemented here: 1. Few relations were eager loaded 2. Changed few polymorphic routes to specific ones so we don't have to use `#becomes(Namespace)` which doesn't preserve association cache --- app/helpers/events_helper.rb | 4 +++ app/helpers/projects_helper.rb | 2 +- app/models/event.rb | 2 +- app/models/note.rb | 2 +- .../unreleased/fix-n-plus-1-queries-projects.yml | 6 ++++ spec/helpers/events_helper_spec.rb | 32 ++++++++++++++++++++++ spec/helpers/projects_helper_spec.rb | 12 ++++++++ 7 files changed, 57 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/fix-n-plus-1-queries-projects.yml diff --git a/app/helpers/events_helper.rb b/app/helpers/events_helper.rb index 3ce2398f1de..1371e9993b4 100644 --- a/app/helpers/events_helper.rb +++ b/app/helpers/events_helper.rb @@ -161,6 +161,10 @@ module EventsHelper project_commit_url(event.project, event.note_target, anchor: dom_id(event.target)) elsif event.project_snippet_note? project_snippet_url(event.project, event.note_target, anchor: dom_id(event.target)) + elsif event.issue_note? + project_issue_url(event.project, id: event.note_target, anchor: dom_id(event.target)) + elsif event.merge_request_note? + project_merge_request_url(event.project, id: event.note_target, anchor: dom_id(event.target)) else polymorphic_url([event.project.namespace.becomes(Namespace), event.project, event.note_target], diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 87aebe415c8..7c8557a1a8a 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -2,7 +2,7 @@ module ProjectsHelper def link_to_project(project) - link_to [project.namespace.becomes(Namespace), project], title: h(project.name) do + link_to namespace_project_path(namespace_id: project.namespace, id: project), title: h(project.name) do title = content_tag(:span, project.name, class: 'project-name') if project.namespace diff --git a/app/models/event.rb b/app/models/event.rb index 2e690f8c013..2ceef412af5 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -87,7 +87,7 @@ class Event < ActiveRecord::Base scope :with_associations, -> do # We're using preload for "push_event_payload" as otherwise the association # is not always available (depending on the query being built). - includes(:author, :project, project: :namespace) + includes(:author, :project, project: [:project_feature, :import_data, :namespace]) .preload(:target, :push_event_payload) end diff --git a/app/models/note.rb b/app/models/note.rb index a6ae4f58ac4..17c7d97fa0a 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -131,7 +131,7 @@ class Note < ActiveRecord::Base scope :with_associations, -> do # FYI noteable cannot be loaded for LegacyDiffNote for commits includes(:author, :noteable, :updated_by, - project: [:project_members, { group: [:group_members] }]) + project: [:project_members, :namespace, { group: [:group_members] }]) end scope :with_metadata, -> { includes(:system_note_metadata) } diff --git a/changelogs/unreleased/fix-n-plus-1-queries-projects.yml b/changelogs/unreleased/fix-n-plus-1-queries-projects.yml new file mode 100644 index 00000000000..cb625784267 --- /dev/null +++ b/changelogs/unreleased/fix-n-plus-1-queries-projects.yml @@ -0,0 +1,6 @@ +--- +title: Fix some N+1 queries related to Admin Dashboard, User Dashboards and Activity + Stream +merge_request: 23034 +author: +type: performance diff --git a/spec/helpers/events_helper_spec.rb b/spec/helpers/events_helper_spec.rb index 8d0679e5699..3d15306d4d2 100644 --- a/spec/helpers/events_helper_spec.rb +++ b/spec/helpers/events_helper_spec.rb @@ -84,4 +84,36 @@ describe EventsHelper do expect(helper.event_feed_url(event)).to eq(push_event_feed_url(event)) end end + + describe '#event_note_target_url' do + let(:project) { create(:project, :public, :repository) } + let(:event) { create(:event, project: project) } + let(:project_base_url) { namespace_project_url(namespace_id: project.namespace, id: project) } + + subject { helper.event_note_target_url(event) } + + it 'returns a commit note url' do + event.target = create(:note_on_commit, note: '+1 from me') + + expect(subject).to eq("#{project_base_url}/commit/#{event.target.commit_id}#note_#{event.target.id}") + end + + it 'returns a project snippet note url' do + event.target = create(:note, :on_snippet, note: 'keep going') + + expect(subject).to eq("#{project_base_url}/snippets/#{event.note_target.id}#note_#{event.target.id}") + end + + it 'returns a project issue url' do + event.target = create(:note_on_issue, note: 'nice work') + + expect(subject).to eq("#{project_base_url}/issues/#{event.note_target.iid}#note_#{event.target.id}") + end + + it 'returns a merge request url' do + event.target = create(:note_on_merge_request, note: 'LGTM!') + + expect(subject).to eq("#{project_base_url}/merge_requests/#{event.note_target.iid}#note_#{event.target.id}") + end + end end diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index a857b7646b2..486416c3370 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -229,6 +229,18 @@ describe ProjectsHelper do end end + describe '#link_to_project' do + let(:group) { create(:group, name: 'group name with space') } + let(:project) { create(:project, group: group, name: 'project name with space') } + subject { link_to_project(project) } + + it 'returns an HTML link to the project' do + expect(subject).to match(%r{/#{group.full_path}/#{project.path}}) + expect(subject).to include('group name with space /') + expect(subject).to include('project name with space') + end + end + describe '#link_to_member_avatar' do let(:user) { build_stubbed(:user) } let(:expected) { double } -- cgit v1.2.1