From bbcb7e5e96b030b4b2a3d03ee9c715853e6528be Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sun, 23 Apr 2017 22:27:13 -0700 Subject: Eliminate N+1 queries in loading namespaces for every issuable in milestones If we rely on the helper functions in `GitlabRoutingHelper` for `merge_request_path`, we end up calling a database query to look up the Namespace association for every merge request since `entity.project.namespace` is called. By reusing the project defined in the controller, we avoid that problem. Partial fix to #27387 --- app/views/shared/milestones/_issuable.html.haml | 2 +- changelogs/unreleased/sh-optimize-milestone-polymorphic-url.yml | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/sh-optimize-milestone-polymorphic-url.yml diff --git a/app/views/shared/milestones/_issuable.html.haml b/app/views/shared/milestones/_issuable.html.haml index 4c7d69d40d5..13a4175fd33 100644 --- a/app/views/shared/milestones/_issuable.html.haml +++ b/app/views/shared/milestones/_issuable.html.haml @@ -5,7 +5,7 @@ - base_url_args = [project.namespace.becomes(Namespace), project, issuable_type] - can_update = can?(current_user, :"update_#{issuable.to_ability_name}", issuable) -%li{ id: dom_id(issuable, 'sortable'), class: "issuable-row #{'is-disabled' unless can_update}", 'data-iid' => issuable.iid, 'data-id' => issuable.id, 'data-url' => polymorphic_path(issuable) } +%li{ id: dom_id(issuable, 'sortable'), class: "issuable-row #{'is-disabled' unless can_update}", 'data-iid' => issuable.iid, 'data-id' => issuable.id, 'data-url' => polymorphic_path([project.namespace.becomes(Namespace), project, issuable]) } %span - if show_project_name %strong #{project.name} · diff --git a/changelogs/unreleased/sh-optimize-milestone-polymorphic-url.yml b/changelogs/unreleased/sh-optimize-milestone-polymorphic-url.yml new file mode 100644 index 00000000000..ad62c896b04 --- /dev/null +++ b/changelogs/unreleased/sh-optimize-milestone-polymorphic-url.yml @@ -0,0 +1,4 @@ +--- +title: Eliminate N+1 queries in loading namespaces for every issuable in milestones +merge_request: +author: -- cgit v1.2.1 From 68bb2a53383d79e5e3e38f9894f0ea61c3701f46 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 24 Apr 2017 08:58:15 -0700 Subject: Optimize project namespace lookup for milestones and add specs --- app/controllers/projects/milestones_controller.rb | 1 + app/views/shared/milestones/_issuable.html.haml | 13 ++++++++----- spec/controllers/projects/milestones_controller_spec.rb | 15 +++++++++++++++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb index 408c0c60cb0..d0dd524c484 100644 --- a/app/controllers/projects/milestones_controller.rb +++ b/app/controllers/projects/milestones_controller.rb @@ -23,6 +23,7 @@ class Projects::MilestonesController < Projects::ApplicationController respond_to do |format| format.html do + @project_namespace = @project.namespace.becomes(Namespace) @milestones = @milestones.includes(:project) @milestones = @milestones.page(params[:page]) end diff --git a/app/views/shared/milestones/_issuable.html.haml b/app/views/shared/milestones/_issuable.html.haml index 13a4175fd33..5247d6a51e6 100644 --- a/app/views/shared/milestones/_issuable.html.haml +++ b/app/views/shared/milestones/_issuable.html.haml @@ -1,11 +1,14 @@ -# @project is present when viewing Project's milestone - project = @project || issuable.project +- namespace = @project_namespace || project.namespace.becomes(Namespace) - assignee = issuable.assignee - issuable_type = issuable.class.table_name -- base_url_args = [project.namespace.becomes(Namespace), project, issuable_type] +- base_url_args = [namespace, project] +- issuable_type_args = base_url_args + [issuable_type] +- issuable_url_args = base_url_args + [issuable] - can_update = can?(current_user, :"update_#{issuable.to_ability_name}", issuable) -%li{ id: dom_id(issuable, 'sortable'), class: "issuable-row #{'is-disabled' unless can_update}", 'data-iid' => issuable.iid, 'data-id' => issuable.id, 'data-url' => polymorphic_path([project.namespace.becomes(Namespace), project, issuable]) } +%li{ id: dom_id(issuable, 'sortable'), class: "issuable-row #{'is-disabled' unless can_update}", 'data-iid' => issuable.iid, 'data-id' => issuable.id, 'data-url' => polymorphic_path(issuable_url_args) } %span - if show_project_name %strong #{project.name} · @@ -13,17 +16,17 @@ %strong #{project.name_with_namespace} · - if issuable.is_a?(Issue) = confidential_icon(issuable) - = link_to_gfm issuable.title, [project.namespace.becomes(Namespace), project, issuable], title: issuable.title + = link_to_gfm issuable.title, issuable_url_args, title: issuable.title .issuable-detail = link_to [project.namespace.becomes(Namespace), project, issuable] do %span.issuable-number= issuable.to_reference - issuable.labels.each do |label| - = link_to polymorphic_path(base_url_args, { milestone_title: @milestone.title, label_name: label.title, state: 'all' }) do + = link_to polymorphic_path(issuable_type_args, { milestone_title: @milestone.title, label_name: label.title, state: 'all' }) do - render_colored_label(label) %span.assignee-icon - if assignee - = link_to polymorphic_path(base_url_args, { milestone_title: @milestone.title, assignee_id: issuable.assignee_id, state: 'all' }), + = link_to polymorphic_path(issuable_type_args, { milestone_title: @milestone.title, assignee_id: issuable.assignee_id, state: 'all' }), class: 'has-tooltip', title: "Assigned to #{assignee.name}", data: { container: 'body' } do - image_tag(avatar_icon(issuable.assignee, 16), class: "avatar s16", alt: '') diff --git a/spec/controllers/projects/milestones_controller_spec.rb b/spec/controllers/projects/milestones_controller_spec.rb index 14207bf6b7a..47e61c3cea8 100644 --- a/spec/controllers/projects/milestones_controller_spec.rb +++ b/spec/controllers/projects/milestones_controller_spec.rb @@ -5,6 +5,7 @@ describe Projects::MilestonesController do let(:user) { create(:user) } let(:milestone) { create(:milestone, project: project) } let(:issue) { create(:issue, project: project, milestone: milestone) } + let!(:label) { create(:label, project: project, title: 'Issue Label', issues: [issue]) } let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, milestone: milestone) } before do @@ -13,6 +14,20 @@ describe Projects::MilestonesController do controller.instance_variable_set(:@project, project) end + describe "#show" do + render_views + + def view_milestone + get :show, namespace_id: project.namespace.id, project_id: project.id, id: milestone.iid + end + + it 'shows milestone page' do + view_milestone + + expect(response).to have_http_status(200) + end + end + describe "#destroy" do it "removes milestone" do expect(issue.milestone_id).to eq(milestone.id) -- cgit v1.2.1