Commit 0fa511fd authored by Douwe Maan's avatar Douwe Maan

Merge branch 'fix-slow-queries-for-branches-index' into 'master'

removes n+1 query from tags and branches indexes

See merge request !9905
parents 63292383 26d3da4b
...@@ -11,16 +11,18 @@ class Projects::BranchesController < Projects::ApplicationController ...@@ -11,16 +11,18 @@ class Projects::BranchesController < Projects::ApplicationController
@sort = params[:sort].presence || sort_value_name @sort = params[:sort].presence || sort_value_name
@branches = BranchesFinder.new(@repository, params).execute @branches = BranchesFinder.new(@repository, params).execute
@branches = Kaminari.paginate_array(@branches).page(params[:page]) unless params[:show_all].present?
respond_to do |format| respond_to do |format|
format.html do format.html do
paginate_branches
@refs_pipelines = @project.pipelines.latest_successful_for_refs(@branches.map(&:name))
@max_commits = @branches.reduce(0) do |memo, branch| @max_commits = @branches.reduce(0) do |memo, branch|
diverging_commit_counts = repository.diverging_commit_counts(branch) diverging_commit_counts = repository.diverging_commit_counts(branch)
[memo, diverging_commit_counts[:behind], diverging_commit_counts[:ahead]].max [memo, diverging_commit_counts[:behind], diverging_commit_counts[:ahead]].max
end end
end end
format.json do format.json do
paginate_branches unless params[:show_all]
render json: @branches.map(&:name) render json: @branches.map(&:name)
end end
end end
...@@ -91,6 +93,10 @@ class Projects::BranchesController < Projects::ApplicationController ...@@ -91,6 +93,10 @@ class Projects::BranchesController < Projects::ApplicationController
end end
end end
def paginate_branches
@branches = Kaminari.paginate_array(@branches).page(params[:page])
end
def url_to_autodeploy_setup(project, branch_name) def url_to_autodeploy_setup(project, branch_name)
namespace_project_new_blob_path( namespace_project_new_blob_path(
project.namespace, project.namespace,
......
...@@ -14,7 +14,9 @@ class Projects::TagsController < Projects::ApplicationController ...@@ -14,7 +14,9 @@ class Projects::TagsController < Projects::ApplicationController
@tags = TagsFinder.new(@repository, params).execute @tags = TagsFinder.new(@repository, params).execute
@tags = Kaminari.paginate_array(@tags).page(params[:page]) @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 end
def show def show
......
...@@ -115,6 +115,12 @@ module Ci ...@@ -115,6 +115,12 @@ module Ci
success.latest(ref).order(id: :desc).first success.latest(ref).order(id: :desc).first
end 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) def self.truncate_sha(sha)
sha[0...8] sha[0...8]
end end
......
...@@ -27,7 +27,7 @@ ...@@ -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 = 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 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) - if can?(current_user, :push_code, @project)
= link_to namespace_project_branch_path(@project.namespace, @project, branch.name), = link_to namespace_project_branch_path(@project.namespace, @project, branch.name),
......
- pipeline = local_assigns.fetch(:pipeline) { project.pipelines.latest_successful_for(ref) }
- if !project.empty_repo? && can?(current_user, :download_code, project) - if !project.empty_repo? && can?(current_user, :download_code, project)
.project-action-button.dropdown.inline> .project-action-button.dropdown.inline>
%button.btn{ 'data-toggle' => 'dropdown' } %button.btn{ 'data-toggle' => 'dropdown' }
...@@ -24,7 +26,6 @@ ...@@ -24,7 +26,6 @@
%i.fa.fa-download %i.fa.fa-download
%span Download tar %span Download tar
- pipeline = project.pipelines.latest_successful_for(ref)
- if pipeline - if pipeline
- artifacts = pipeline.builds.latest.with_artifacts - artifacts = pipeline.builds.latest.with_artifacts
- if artifacts.any? - if artifacts.any?
......
...@@ -23,7 +23,7 @@ ...@@ -23,7 +23,7 @@
= markdown_field(release, :description) = markdown_field(release, :description)
.row-fixed-content.controls .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) - 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 = 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
......
---
title: Fixes n+1 query for tags and branches index page
merge_request: 9905
author:
...@@ -17,6 +17,14 @@ describe 'Branches', feature: true do ...@@ -17,6 +17,14 @@ describe 'Branches', feature: true do
repository.branches { |branch| expect(page).to have_content("#{branch.name}") } repository.branches { |branch| expect(page).to have_content("#{branch.name}") }
expect(page).to have_content("Protected branches can be managed in project settings") expect(page).to have_content("Protected branches can be managed in project settings")
end 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 end
describe 'Find branches' do describe 'Find branches' do
......
...@@ -27,10 +27,20 @@ feature 'Master views tags', feature: true do ...@@ -27,10 +27,20 @@ feature 'Master views tags', feature: true do
context 'when project has tags' do context 'when project has tags' do
let(:project) { create(:project, namespace: user.namespace) } let(:project) { create(:project, namespace: user.namespace) }
let(:repository) { project.repository }
before do before do
visit namespace_project_tags_path(project.namespace, project) visit namespace_project_tags_path(project.namespace, project)
end 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 scenario 'views the tags list page' do
expect(page).to have_content 'v1.0.0' expect(page).to have_content 'v1.0.0'
end end
......
...@@ -532,6 +532,19 @@ describe Ci::Pipeline, models: true do ...@@ -532,6 +532,19 @@ describe Ci::Pipeline, models: true do
end end
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 describe '#status' do
let(:build) do let(:build) do
create(:ci_build, :created, pipeline: pipeline, name: 'test') create(:ci_build, :created, pipeline: pipeline, name: 'test')
......
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