diff options
author | Nick Thomas <nick@gitlab.com> | 2018-09-04 19:30:52 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2018-09-06 18:04:21 +0300 |
commit | bc3d1afe1398a32e3fab42b05092bca5f2c6c745 (patch) | |
tree | 323cbce1e75c2f8e1f33af8086c44c9174c78d87 | |
parent | 43b91111d698c97c9d07617bb36db8d92bb9a0d9 (diff) |
Extract tree summary logic out of RefsController#logs_tree
-rw-r--r-- | app/controllers/projects/refs_controller.rb | 65 | ||||
-rw-r--r-- | lib/gitlab/tree_summary.rb | 115 | ||||
-rw-r--r-- | spec/lib/gitlab/tree_summary_spec.rb | 202 |
3 files changed, 337 insertions, 45 deletions
diff --git a/app/controllers/projects/refs_controller.rb b/app/controllers/projects/refs_controller.rb index 97231b09c05..200054b4746 100644 --- a/app/controllers/projects/refs_controller.rb +++ b/app/controllers/projects/refs_controller.rb @@ -36,65 +36,40 @@ class Projects::RefsController < Projects::ApplicationController end def logs_tree - @resolved_commits ||= {} - @limit = 25 - @offset = params[:offset].to_i - @path = params[:path] + summary = ::Gitlab::TreeSummary.new( + @commit, + @project, + path: @path, + offset: params[:offset], + limit: 25 + ) - contents = [] - contents.push(*tree.trees) - contents.push(*tree.blobs) - contents.push(*tree.submodules) - - # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37433 - @logs = Gitlab::GitalyClient.allow_n_plus_1_calls do - contents[@offset, @limit].to_a.map do |content| - file = @path ? File.join(@path, content.name) : content.name - last_commit = @repo.last_commit_for_path(@commit.id, file) - commit_path = project_commit_path(@project, last_commit) if last_commit - - { - file_name: content.name, - commit: resolve_commit(last_commit), - type: content.type, - commit_path: commit_path - } - end - end - - prerender_commits! - - offset = (@offset + @limit) - if contents.size > offset - @more_log_url = logs_file_project_ref_path(@project, @ref, @path || '', offset: offset) - end + @logs, commits = summary.summarize + @more_log_url = more_url(summary.next_offset) if summary.more? respond_to do |format| format.html { render_404 } format.json do - response.headers["More-Logs-Url"] = @more_log_url - + response.headers["More-Logs-Url"] = @more_log_url if summary.more? render json: @logs end - format.js + + # The commit titles must be rendered and redacted before being shown. + # Doing it here allows us to apply performance optimizations that avoid + # N+1 problems + format.js do + prerender_commit_full_titles!(commits) + end end end private - # Ensure that if multiple tree entries share the same last commit, they share - # a ::Commit instance. This prevents us from rendering the same commit title - # multiple times - def resolve_commit(raw_commit) - return nil unless raw_commit.present? - - @resolved_commits[raw_commit.id] ||= Commit.new(raw_commit, project) + def more_url(offset) + logs_file_project_ref_path(@project, @ref, @path, offset: offset) end - # The commit titles must be passed through Banzai before being shown. - # Doing this here in bulk allows significant database work to be skipped. - def prerender_commits! - commits = @resolved_commits.values + def prerender_commit_full_titles!(commits) renderer = Banzai::ObjectRenderer.new(user: current_user, default_project: @project) renderer.render(commits, :full_title) end diff --git a/lib/gitlab/tree_summary.rb b/lib/gitlab/tree_summary.rb new file mode 100644 index 00000000000..b05d408b1c0 --- /dev/null +++ b/lib/gitlab/tree_summary.rb @@ -0,0 +1,115 @@ +module Gitlab + class TreeSummary + include ::Gitlab::Utils::StrongMemoize + + attr_reader :commit, :project, :path, :offset, :limit + + attr_reader :resolved_commits + private :resolved_commits + + def initialize(commit, project, params = {}) + @commit = commit + @project = project + + @path = params.fetch(:path, nil).presence + @offset = params.fetch(:offset, 0).to_i + @limit = (params.fetch(:limit, 25) || 25).to_i + + # Ensure that if multiple tree entries share the same last commit, they share + # a ::Commit instance. This prevents us from rendering the same commit title + # multiple times + @resolved_commits = {} + end + + # Creates a summary of the tree entries for a commit, within the window of + # entries defined by the offset and limit parameters. This consists of two + # return values: + # + # - An Array of Hashes containing the following keys: + # - file_name: The full path of the tree entry + # - type: One of :blob, :tree, or :submodule + # - commit: The last ::Commit to touch this entry in the tree + # - commit_path: URI of the commit in the web interface + # - An Array of the unique ::Commit objects in the first value + def summarize + summary = contents + .map { |content| build_entry(content) } + .tap { |summary| fill_last_commits!(summary) } + + [summary, commits] + end + + # Does the tree contain more entries after the given offset + limit? + def more? + all_contents[next_offset].present? + end + + # The offset of the next batch of tree entries. If more? returns false, this + # batch will be empty + def next_offset + [all_contents.size + 1, offset + limit].min + end + + private + + def contents + all_contents[offset, limit] + end + + def commits + resolved_commits.values + end + + def repository + project.repository + end + + def entry_path(entry) + File.join(*[path, entry[:file_name]].compact) + end + + def build_entry(entry) + { file_name: entry.name, type: entry.type } + end + + def fill_last_commits!(entries) + # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37433 + Gitlab::GitalyClient.allow_n_plus_1_calls do + entries.each do |entry| + raw_commit = repository.last_commit_for_path(commit.id, entry_path(entry)) + + if raw_commit + commit = resolve_commit(raw_commit) + + entry[:commit] = commit + entry[:commit_path] = commit_path(commit) + end + end + end + end + + def resolve_commit(raw_commit) + return nil unless raw_commit.present? + + resolved_commits[raw_commit.id] ||= ::Commit.new(raw_commit, project) + end + + def commit_path(commit) + Gitlab::Routing.url_helpers.project_commit_path(project, commit) + end + + def all_contents + strong_memoize(:all_contents) do + [ + *tree.trees, + *tree.blobs, + *tree.submodules + ] + end + end + + def tree + strong_memoize(:tree) { repository.tree(commit.id, path) } + end + end +end diff --git a/spec/lib/gitlab/tree_summary_spec.rb b/spec/lib/gitlab/tree_summary_spec.rb new file mode 100644 index 00000000000..7ffcef2baef --- /dev/null +++ b/spec/lib/gitlab/tree_summary_spec.rb @@ -0,0 +1,202 @@ +require 'spec_helper' + +describe Gitlab::TreeSummary do + using RSpec::Parameterized::TableSyntax + + let(:project) { create(:project, :empty_repo) } + let(:repo) { project.repository } + let(:commit) { repo.head_commit } + + let(:path) { nil } + let(:offset) { nil } + let(:limit) { nil } + + subject(:summary) { described_class.new(commit, project, path: path, offset: offset, limit: limit) } + + describe '#initialize' do + it 'defaults offset to 0' do + expect(summary.offset).to eq(0) + end + + it 'defaults limit to 25' do + expect(summary.limit).to eq(25) + end + end + + describe '#summarize' do + let(:project) { create(:project, :custom_repo, files: { 'a.txt' => '' }) } + + subject(:summarized) { summary.summarize } + + it 'returns an array of entries, and an array of commits' do + expect(summarized).to be_a(Array) + expect(summarized.size).to eq(2) + + entries, commits = *summarized + aggregate_failures do + expect(entries).to contain_exactly( + a_hash_including(file_name: 'a.txt', commit: have_attributes(id: commit.id)) + ) + + expect(commits).to match_array(entries.map { |entry| entry[:commit] }) + end + end + end + + describe '#summarize (entries)' do + let(:limit) { 2 } + + custom_files = { + 'a.txt' => '', + 'b.txt' => '', + 'directory/c.txt' => '' + } + + let(:project) { create(:project, :custom_repo, files: custom_files) } + let(:commit) { repo.head_commit } + + subject(:entries) { summary.summarize.first } + + it 'summarizes the entries within the window' do + is_expected.to contain_exactly( + a_hash_including(type: :tree, file_name: 'directory'), + a_hash_including(type: :blob, file_name: 'a.txt') + # b.txt is excluded by the limit + ) + end + + it 'references the commit and commit path in entries' do + entry = entries.first + expected_commit_path = Gitlab::Routing.url_helpers.project_commit_path(project, commit) + + expect(entry[:commit]).to be_a(::Commit) + expect(entry[:commit_path]).to eq expected_commit_path + end + + context 'in a good subdirectory' do + let(:path) { 'directory' } + + it 'summarizes the entries in the subdirectory' do + is_expected.to contain_exactly(a_hash_including(type: :blob, file_name: 'c.txt')) + end + end + + context 'in a non-existent subdirectory' do + let(:path) { 'tmp' } + + it { is_expected.to be_empty } + end + + context 'custom offset and limit' do + let(:offset) { 2 } + + it 'returns entries from the offset' do + is_expected.to contain_exactly(a_hash_including(type: :blob, file_name: 'b.txt')) + end + end + end + + describe '#summarize (commits)' do + # This is a commit in the master branch of the gitlab-test repository that + # satisfies certain assumptions these tests depend on + let(:test_commit_sha) { '7975be0116940bf2ad4321f79d02a55c5f7779aa' } + let(:whitespace_commit_sha) { '66eceea0db202bb39c4e445e8ca28689645366c5' } + + let(:project) { create(:project, :repository) } + let(:commit) { repo.commit(test_commit_sha) } + let(:limit) { nil } + let(:entries) { summary.summarize.first } + + subject(:commits) do + summary.summarize.last + end + + it 'returns an Array of ::Commit objects' do + is_expected.not_to be_empty + is_expected.to all(be_kind_of(::Commit)) + end + + it 'deduplicates commits when multiple entries reference the same commit' do + expect(commits.size).to be < entries.size + end + + context 'in a subdirectory' do + let(:path) { 'files' } + + it 'returns commits for entries in the subdirectory' do + expect(commits).to satisfy_one { |c| c.id == whitespace_commit_sha } + end + end + end + + describe '#more?' do + let(:path) { 'tmp/more' } + + where(:num_entries, :offset, :limit, :expected_result) do + 0 | 0 | 0 | false + 0 | 0 | 1 | false + + 1 | 0 | 0 | true + 1 | 0 | 1 | false + 1 | 1 | 0 | false + 1 | 1 | 1 | false + + 2 | 0 | 0 | true + 2 | 0 | 1 | true + 2 | 0 | 2 | false + 2 | 0 | 3 | false + 2 | 1 | 0 | true + 2 | 1 | 1 | false + 2 | 2 | 0 | false + 2 | 2 | 1 | false + end + + with_them do + before do + create_file('dummy', path: 'other') if num_entries.zero? + 1.upto(num_entries) { |n| create_file(n, path: path) } + end + + subject { summary.more? } + + it { is_expected.to eq(expected_result) } + end + end + + describe '#next_offset' do + let(:path) { 'tmp/next_offset' } + + where(:num_entries, :offset, :limit, :expected_result) do + 0 | 0 | 0 | 0 + 0 | 0 | 1 | 1 + 0 | 1 | 0 | 1 + 0 | 1 | 1 | 1 + + 1 | 0 | 0 | 0 + 1 | 0 | 1 | 1 + 1 | 1 | 0 | 1 + 1 | 1 | 1 | 2 + end + + with_them do + before do + create_file('dummy', path: 'other') if num_entries.zero? + 1.upto(num_entries) { |n| create_file(n, path: path) } + end + + subject { summary.next_offset } + + it { is_expected.to eq(expected_result) } + end + end + + def create_file(unique, path:) + repo.create_file( + project.creator, + "#{path}/file-#{unique}.txt", + 'content', + message: "Commit message #{unique}", + branch_name: 'master' + ) + end +end |