From 26d3da4bfe5c4718d3dfb4407cee58da53b408aa Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Mon, 13 Mar 2017 16:00:07 +0000 Subject: removes n+1 query from tags and branches indexes --- app/controllers/projects/branches_controller.rb | 10 ++++++++-- app/controllers/projects/tags_controller.rb | 4 +++- app/models/ci/pipeline.rb | 6 ++++++ app/views/projects/branches/_branch.html.haml | 2 +- app/views/projects/buttons/_download.html.haml | 3 ++- app/views/projects/tags/_tag.html.haml | 2 +- .../unreleased/fix-slow-queries-for-branches-index.yml | 4 ++++ spec/features/projects/branches_spec.rb | 8 ++++++++ spec/features/tags/master_views_tags_spec.rb | 10 ++++++++++ spec/models/ci/pipeline_spec.rb | 13 +++++++++++++ 10 files changed, 56 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/fix-slow-queries-for-branches-index.yml diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb index 22714d9c5a4..840405f38cb 100644 --- a/app/controllers/projects/branches_controller.rb +++ b/app/controllers/projects/branches_controller.rb @@ -11,16 +11,18 @@ class Projects::BranchesController < Projects::ApplicationController @sort = params[:sort].presence || sort_value_name @branches = BranchesFinder.new(@repository, params).execute - @branches = Kaminari.paginate_array(@branches).page(params[:page]) unless params[:show_all].present? - respond_to do |format| format.html do + paginate_branches + @refs_pipelines = @project.pipelines.latest_successful_for_refs(@branches.map(&:name)) + @max_commits = @branches.reduce(0) do |memo, branch| diverging_commit_counts = repository.diverging_commit_counts(branch) [memo, diverging_commit_counts[:behind], diverging_commit_counts[:ahead]].max end end format.json do + paginate_branches unless params[:show_all] render json: @branches.map(&:name) end end @@ -91,6 +93,10 @@ class Projects::BranchesController < Projects::ApplicationController end end + def paginate_branches + @branches = Kaminari.paginate_array(@branches).page(params[:page]) + end + def url_to_autodeploy_setup(project, branch_name) namespace_project_new_blob_path( project.namespace, diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb index ea7e4d9f663..e13f0bde315 100644 --- a/app/controllers/projects/tags_controller.rb +++ b/app/controllers/projects/tags_controller.rb @@ -14,7 +14,9 @@ class Projects::TagsController < Projects::ApplicationController @tags = TagsFinder.new(@repository, params).execute @tags = Kaminari.paginate_array(@tags).page(params[:page]) - @releases = project.releases.where(tag: @tags.map(&:name)) + tag_names = @tags.map(&:name) + @tags_pipelines = @project.pipelines.latest_successful_for_refs(tag_names) + @releases = project.releases.where(tag: tag_names) end def show diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 65d08a22b4c..d1009f88549 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -115,6 +115,12 @@ module Ci success.latest(ref).order(id: :desc).first end + def self.latest_successful_for_refs(refs) + success.latest(refs).order(id: :desc).each_with_object({}) do |pipeline, hash| + hash[pipeline.ref] ||= pipeline + end + end + def self.truncate_sha(sha) sha[0...8] end diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml index ae63f8184df..9eb610ba9c0 100644 --- a/app/views/projects/branches/_branch.html.haml +++ b/app/views/projects/branches/_branch.html.haml @@ -27,7 +27,7 @@ = link_to namespace_project_compare_index_path(@project.namespace, @project, from: @repository.root_ref, to: branch.name), class: "btn btn-default #{'prepend-left-10' unless merge_project}", method: :post, title: "Compare" do Compare - = render 'projects/buttons/download', project: @project, ref: branch.name + = render 'projects/buttons/download', project: @project, ref: branch.name, pipeline: @refs_pipelines[branch.name] - if can?(current_user, :push_code, @project) = link_to namespace_project_branch_path(@project.namespace, @project, branch.name), diff --git a/app/views/projects/buttons/_download.html.haml b/app/views/projects/buttons/_download.html.haml index b560ed21f1d..d90d4a27cd6 100644 --- a/app/views/projects/buttons/_download.html.haml +++ b/app/views/projects/buttons/_download.html.haml @@ -1,3 +1,5 @@ +- pipeline = local_assigns.fetch(:pipeline) { project.pipelines.latest_successful_for(ref) } + - if !project.empty_repo? && can?(current_user, :download_code, project) .project-action-button.dropdown.inline> %button.btn{ 'data-toggle' => 'dropdown' } @@ -24,7 +26,6 @@ %i.fa.fa-download %span Download tar - - pipeline = project.pipelines.latest_successful_for(ref) - if pipeline - artifacts = pipeline.builds.latest.with_artifacts - if artifacts.any? diff --git a/app/views/projects/tags/_tag.html.haml b/app/views/projects/tags/_tag.html.haml index 8ef069b9e05..dffe908e85a 100644 --- a/app/views/projects/tags/_tag.html.haml +++ b/app/views/projects/tags/_tag.html.haml @@ -23,7 +23,7 @@ = markdown_field(release, :description) .row-fixed-content.controls - = render 'projects/buttons/download', project: @project, ref: tag.name + = render 'projects/buttons/download', project: @project, ref: tag.name, pipeline: @tags_pipelines[tag.name] - if can?(current_user, :push_code, @project) = link_to edit_namespace_project_tag_release_path(@project.namespace, @project, tag.name), class: 'btn has-tooltip', title: "Edit release notes", data: { container: "body" } do diff --git a/changelogs/unreleased/fix-slow-queries-for-branches-index.yml b/changelogs/unreleased/fix-slow-queries-for-branches-index.yml new file mode 100644 index 00000000000..f5bd7003615 --- /dev/null +++ b/changelogs/unreleased/fix-slow-queries-for-branches-index.yml @@ -0,0 +1,4 @@ +--- +title: Fixes n+1 query for tags and branches index page +merge_request: 9905 +author: diff --git a/spec/features/projects/branches_spec.rb b/spec/features/projects/branches_spec.rb index d26a0caf036..8e0306ce83b 100644 --- a/spec/features/projects/branches_spec.rb +++ b/spec/features/projects/branches_spec.rb @@ -17,6 +17,14 @@ describe 'Branches', feature: true do repository.branches { |branch| expect(page).to have_content("#{branch.name}") } expect(page).to have_content("Protected branches can be managed in project settings") end + + it 'avoids a N+1 query in branches index' do + control_count = ActiveRecord::QueryRecorder.new { visit namespace_project_branches_path(project.namespace, project) }.count + + %w(one two three four five).each { |ref| repository.add_branch(@user, ref, 'master') } + + expect { visit namespace_project_branches_path(project.namespace, project) }.not_to exceed_query_limit(control_count) + end end describe 'Find branches' do diff --git a/spec/features/tags/master_views_tags_spec.rb b/spec/features/tags/master_views_tags_spec.rb index 29d2c244720..555f84c4772 100644 --- a/spec/features/tags/master_views_tags_spec.rb +++ b/spec/features/tags/master_views_tags_spec.rb @@ -27,10 +27,20 @@ feature 'Master views tags', feature: true do context 'when project has tags' do let(:project) { create(:project, namespace: user.namespace) } + let(:repository) { project.repository } + before do visit namespace_project_tags_path(project.namespace, project) end + scenario 'avoids a N+1 query in branches index' do + control_count = ActiveRecord::QueryRecorder.new { visit namespace_project_tags_path(project.namespace, project) }.count + + %w(one two three four five).each { |tag| repository.add_tag(user, tag, 'master', 'foo') } + + expect { visit namespace_project_tags_path(project.namespace, project) }.not_to exceed_query_limit(control_count) + end + scenario 'views the tags list page' do expect(page).to have_content 'v1.0.0' end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 4a664e4fae2..53282b999dc 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -532,6 +532,19 @@ describe Ci::Pipeline, models: true do end end + describe '.latest_successful_for_refs' do + include_context 'with some outdated pipelines' + + let!(:latest_successful_pipeline1) { create_pipeline(:success, 'ref1', 'D') } + let!(:latest_successful_pipeline2) { create_pipeline(:success, 'ref2', 'D') } + + it 'returns the latest successful pipeline for both refs' do + refs = %w(ref1 ref2 ref3) + + expect(described_class.latest_successful_for_refs(refs)).to eq({ 'ref1' => latest_successful_pipeline1, 'ref2' => latest_successful_pipeline2 }) + end + end + describe '#status' do let(:build) do create(:ci_build, :created, pipeline: pipeline, name: 'test') -- cgit v1.2.1