Commit dc3aa6c6 authored by Igor Drozdov's avatar Igor Drozdov

Load limited number of diff commits from database

Also preload users and latest pipeline for the correct ref
parent 081ac7fc
# frozen_string_literal: true
module RendersCommits
def limited_commits(commits)
if commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
def limited_commits(commits, commits_count)
if commits_count > MergeRequestDiff::COMMITS_SAFE_SIZE
[
commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE),
commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE
commits_count - MergeRequestDiff::COMMITS_SAFE_SIZE
]
else
[commits, 0]
......@@ -14,9 +14,10 @@ module RendersCommits
# This is used as a helper method in a controller.
# rubocop: disable Gitlab/ModuleWithInstanceVariables
def set_commits_for_rendering(commits)
@total_commit_count = commits.size
limited, @hidden_commit_count = limited_commits(commits)
def set_commits_for_rendering(commits, commits_count: nil)
@total_commit_count = commits_count || commits.size
limited, @hidden_commit_count = limited_commits(commits, @total_commit_count)
commits.each(&:lazy_author) # preload authors
prepare_commits_for_rendering(limited)
end
# rubocop: enable Gitlab/ModuleWithInstanceVariables
......
......@@ -109,7 +109,13 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
@target_project = @merge_request.target_project
@source_project = @merge_request.source_project
@commits = set_commits_for_rendering(@merge_request.commits)
@commits =
set_commits_for_rendering(
@merge_request.recent_commits.with_latest_pipeline(@merge_request.source_branch),
commits_count: @merge_request.commits_count
)
@commit = @merge_request.diff_head_commit
# FIXME: We have to assign a presenter to another instance variable
......
......@@ -90,7 +90,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
# Get commits from repository
# or from cache if already merged
@commits =
set_commits_for_rendering(@merge_request.commits.with_latest_pipeline)
set_commits_for_rendering(
@merge_request.recent_commits.with_latest_pipeline(@merge_request.source_branch),
commits_count: @merge_request.commits_count
)
render json: { html: view_to_html_string('projects/merge_requests/_commits') }
end
......
......@@ -374,11 +374,12 @@ class MergeRequest < ApplicationRecord
"#{project.to_reference(from, full: full)}#{reference}"
end
def commits
return merge_request_diff.commits if persisted?
def commits(limit: nil)
return merge_request_diff.commits(limit: limit) if persisted?
commits_arr = if compare_commits
compare_commits.reverse
reversed_commits = compare_commits.reverse
limit ? reversed_commits.take(limit) : reversed_commits
else
[]
end
......@@ -386,6 +387,10 @@ class MergeRequest < ApplicationRecord
CommitCollection.new(source_project, commits_arr, source_branch)
end
def recent_commits
commits(limit: MergeRequestDiff::COMMITS_SAFE_SIZE)
end
def commits_count
if persisted?
merge_request_diff.commits_count
......
......@@ -213,8 +213,10 @@ class MergeRequestDiff < ApplicationRecord
end
end
def commits
@commits ||= load_commits
def commits(limit: nil)
strong_memoize(:"commits_#{limit || 'all'}") do
load_commits(limit: limit)
end
end
def last_commit_sha
......@@ -529,8 +531,9 @@ class MergeRequestDiff < ApplicationRecord
end
end
def load_commits
commits = merge_request_diff_commits.map { |commit| Commit.from_hash(commit.to_hash, project) }
def load_commits(limit: nil)
commits = merge_request_diff_commits.limit(limit)
.map { |commit| Commit.from_hash(commit.to_hash, project) }
CommitCollection
.new(merge_request.source_project, commits, merge_request.source_branch)
......
# frozen_string_literal: true
require 'spec_helper'
describe RendersCommits do
let_it_be(:project) { create(:project, :public, :repository) }
let_it_be(:merge_request) { create(:merge_request, source_project: project) }
let_it_be(:user) { create(:user) }
controller(ApplicationController) do
# `described_class` is not available in this context
include RendersCommits # rubocop:disable RSpec/DescribedClass
def index
@merge_request = MergeRequest.find(params[:id])
@commits = set_commits_for_rendering(
@merge_request.recent_commits.with_latest_pipeline(@merge_request.source_branch),
commits_count: @merge_request.commits_count
)
render json: { html: view_to_html_string('projects/merge_requests/_commits') }
end
end
before do
sign_in(user)
end
def go
get :index, params: { id: merge_request.id }
end
it 'sets instance variables for counts' do
stub_const("MergeRequestDiff::COMMITS_SAFE_SIZE", 10)
go
expect(assigns[:total_commit_count]).to eq(29)
expect(assigns[:hidden_commit_count]).to eq(19)
expect(assigns[:commits].size).to eq(10)
end
context 'rendering commits' do
render_views
it 'avoids N + 1' do
stub_const("MergeRequestDiff::COMMITS_SAFE_SIZE", 5)
control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do
go
end.count
stub_const("MergeRequestDiff::COMMITS_SAFE_SIZE", 15)
expect do
go
end.not_to exceed_all_query_limit(control_count)
end
end
end
......@@ -3484,4 +3484,67 @@ describe MergeRequest do
end
it_behaves_like 'versioned description'
describe '#commits' do
context 'persisted merge request' do
context 'with a limit' do
it 'returns a limited number of commits' do
expect(subject.commits(limit: 2).map(&:sha)).to eq(%w[
b83d6e391c22777fca1ed3012fce84f633d7fed0
498214de67004b1da3d820901307bed2a68a8ef6
])
expect(subject.commits(limit: 3).map(&:sha)).to eq(%w[
b83d6e391c22777fca1ed3012fce84f633d7fed0
498214de67004b1da3d820901307bed2a68a8ef6
1b12f15a11fc6e62177bef08f47bc7b5ce50b141
])
end
end
context 'without a limit' do
it 'returns all commits of the merge request diff' do
expect(subject.commits.size).to eq(29)
end
end
end
context 'new merge request' do
subject { build(:merge_request) }
context 'compare commits' do
let(:first_commit) { double }
let(:second_commit) { double }
before do
subject.compare_commits = [
first_commit, second_commit
]
end
context 'without a limit' do
it 'returns all the compare commits' do
expect(subject.commits.to_a).to eq([second_commit, first_commit])
end
end
context 'with a limit' do
it 'returns a limited number of commits' do
expect(subject.commits(limit: 1).to_a).to eq([second_commit])
end
end
end
end
end
describe '#recent_commits' do
before do
stub_const("#{MergeRequestDiff}::COMMITS_SAFE_SIZE", 2)
end
it 'returns the safe number of commits' do
expect(subject.recent_commits.map(&:sha)).to eq(%w[
b83d6e391c22777fca1ed3012fce84f633d7fed0 498214de67004b1da3d820901307bed2a68a8ef6
])
end
end
end
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