From 4b28d5ae770c6bd332283a3f13ceae06329c409b Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 9 Oct 2019 21:06:24 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .../components/sidebar/labels_select/base.vue | 2 +- .../sidebar/labels_select/dropdown_title.vue | 6 +- .../merge_requests/application_controller.rb | 6 +- .../merge_requests/diffs_controller.rb | 27 ++- .../projects/merge_requests_controller.rb | 3 + app/models/merge_request_diff.rb | 7 + app/models/merge_request_diff_file.rb | 1 + app/serializers/paginated_diff_entity.rb | 50 ++++++ app/serializers/paginated_diff_serializer.rb | 5 + .../unreleased/31290-mr-diffs-batch-load.yml | 5 + config/routes/project.rb | 1 + ...reate_analytics_repository_file_commits.rb | 19 +++ ..._unused_analytics_repository_file_edits.rb | 32 ++++ db/schema.rb | 16 +- .../file_collection/merge_request_diff.rb | 14 +- .../merge_request_diff_base.rb | 21 +++ .../merge_request_diff_batch.rb | 69 ++++++++ lib/gitlab/git/diff_collection.rb | 11 +- .../merge_requests/diffs_controller_spec.rb | 161 +++++++++++++++--- .../merge_request_diff_batch_spec.rb | 126 ++++++++++++++ spec/lib/gitlab/git/diff_collection_spec.rb | 64 +++++++ .../serializers/paginated_diff_entity_spec.rb | 33 ++++ .../shared_examples/diff_file_collections.rb | 8 +- 23 files changed, 623 insertions(+), 64 deletions(-) create mode 100644 app/serializers/paginated_diff_entity.rb create mode 100644 app/serializers/paginated_diff_serializer.rb create mode 100644 changelogs/unreleased/31290-mr-diffs-batch-load.yml create mode 100644 db/migrate/20191004133612_create_analytics_repository_file_commits.rb create mode 100644 db/post_migrate/20191004134055_drop_unused_analytics_repository_file_edits.rb create mode 100644 lib/gitlab/diff/file_collection/merge_request_diff_base.rb create mode 100644 lib/gitlab/diff/file_collection/merge_request_diff_batch.rb create mode 100644 spec/lib/gitlab/diff/file_collection/merge_request_diff_batch_spec.rb create mode 100644 spec/serializers/paginated_diff_entity_spec.rb diff --git a/app/assets/javascripts/vue_shared/components/sidebar/labels_select/base.vue b/app/assets/javascripts/vue_shared/components/sidebar/labels_select/base.vue index 9c258c4651f..13795eff714 100644 --- a/app/assets/javascripts/vue_shared/components/sidebar/labels_select/base.vue +++ b/app/assets/javascripts/vue_shared/components/sidebar/labels_select/base.vue @@ -167,7 +167,7 @@ dropdown-menu-labels dropdown-menu-selectable"
-
+
- diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index edffeb32203..b7e99cb7ed0 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -14,7 +14,11 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont end def merge_request_includes(association) - association.includes(:metrics, :assignees, author: :status) # rubocop:disable CodeReuse/ActiveRecord + association.includes(preloadable_mr_relations) # rubocop:disable CodeReuse/ActiveRecord + end + + def preloadable_mr_relations + [:metrics, :assignees, { author: :status }] end def merge_request_params diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 9c5caf7719e..23ef9157363 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_controller.rb @@ -5,9 +5,9 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic include RendersNotes before_action :apply_diff_view_cookie! - before_action :commit - before_action :define_diff_vars - before_action :define_diff_comment_vars + before_action :commit, except: :diffs_batch + before_action :define_diff_vars, except: :diffs_batch + before_action :define_diff_comment_vars, except: :diffs_batch def show render_diffs @@ -17,8 +17,29 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic render_diffs end + def diffs_batch + return render_404 unless Feature.enabled?(:diffs_batch_load, @merge_request.project) + + diffable = @merge_request.merge_request_diff + + return render_404 unless diffable + + diffs = diffable.diffs_in_batch(params[:page], params[:per_page], diff_options: diff_options) + + options = { + merge_request: @merge_request, + pagination_data: diffs.pagination_data + } + + render json: PaginatedDiffSerializer.new(current_user: current_user).represent(diffs, options) + end + private + def preloadable_mr_relations + [{ source_project: :namespace }, { target_project: :namespace }] + end + def render_diffs @environment = @merge_request.environments_for(current_user).last diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 7ddff9c1893..bd22226da5c 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -17,6 +17,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo before_action :set_issuables_index, only: [:index] before_action :authenticate_user!, only: [:assign_related_issues] before_action :check_user_can_push_to_source_branch!, only: [:rebase] + before_action only: [:show] do + push_frontend_feature_flag(:diffs_batch_load, @project) + end around_action :allow_gitaly_ref_name_caching, only: [:index, :show, :discussions] diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 8b5f10ce159..7706119681d 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -297,6 +297,13 @@ class MergeRequestDiff < ApplicationRecord base_commit_sha? && head_commit_sha? && start_commit_sha? end + def diffs_in_batch(batch_page, batch_size, diff_options:) + Gitlab::Diff::FileCollection::MergeRequestDiffBatch.new(self, + batch_page, + batch_size, + diff_options: diff_options) + end + def diffs(diff_options = nil) if without_files? && comparison = diff_refs&.compare_in(project) # It should fetch the repository when diffs are cleaned by the system. diff --git a/app/models/merge_request_diff_file.rb b/app/models/merge_request_diff_file.rb index a532c1e6356..14c86ec69da 100644 --- a/app/models/merge_request_diff_file.rb +++ b/app/models/merge_request_diff_file.rb @@ -5,6 +5,7 @@ class MergeRequestDiffFile < ApplicationRecord include DiffFile belongs_to :merge_request_diff, inverse_of: :merge_request_diff_files + alias_attribute :index, :relative_order def utf8_diff return '' if diff.blank? diff --git a/app/serializers/paginated_diff_entity.rb b/app/serializers/paginated_diff_entity.rb new file mode 100644 index 00000000000..622da926c69 --- /dev/null +++ b/app/serializers/paginated_diff_entity.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +# Serializes diffs with pagination data. +# +# Avoid adding more keys to this serializer as processing the +# diff file serialization is not cheap. +# +class PaginatedDiffEntity < Grape::Entity + include RequestAwareEntity + + expose :diff_files do |diffs, options| + submodule_links = Gitlab::SubmoduleLinks.new(merge_request.project.repository) + DiffFileEntity.represent(diffs.diff_files, options.merge(submodule_links: submodule_links)) + end + + expose :pagination do + expose :current_page + expose :next_page + expose :total_pages + expose :next_page_href do |diffs| + next unless next_page + + project = merge_request.target_project + + diffs_batch_namespace_project_json_merge_request_path( + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: merge_request.iid, + page: next_page, + format: :json + ) + end + end + + private + + %i[current_page next_page total_pages].each do |method| + define_method method do + pagination_data[method] + end + end + + def pagination_data + options.fetch(:pagination_data, {}) + end + + def merge_request + options[:merge_request] + end +end diff --git a/app/serializers/paginated_diff_serializer.rb b/app/serializers/paginated_diff_serializer.rb new file mode 100644 index 00000000000..9b40fbf7843 --- /dev/null +++ b/app/serializers/paginated_diff_serializer.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class PaginatedDiffSerializer < BaseSerializer + entity PaginatedDiffEntity +end diff --git a/changelogs/unreleased/31290-mr-diffs-batch-load.yml b/changelogs/unreleased/31290-mr-diffs-batch-load.yml new file mode 100644 index 00000000000..933493e80d7 --- /dev/null +++ b/changelogs/unreleased/31290-mr-diffs-batch-load.yml @@ -0,0 +1,5 @@ +--- +title: Introduce diffs_batch JSON endpoint for paginated diffs +merge_request: 17651 +author: +type: added diff --git a/config/routes/project.rb b/config/routes/project.rb index 3675a4600fe..f9331a22bc8 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -281,6 +281,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do get :commits get :pipelines get :diffs, to: 'merge_requests/diffs#show' + get :diffs_batch, to: 'merge_requests/diffs#diffs_batch' get :widget, to: 'merge_requests/content#widget' get :cached_widget, to: 'merge_requests/content#cached_widget' end diff --git a/db/migrate/20191004133612_create_analytics_repository_file_commits.rb b/db/migrate/20191004133612_create_analytics_repository_file_commits.rb new file mode 100644 index 00000000000..f2064b2b301 --- /dev/null +++ b/db/migrate/20191004133612_create_analytics_repository_file_commits.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class CreateAnalyticsRepositoryFileCommits < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + create_table :analytics_repository_file_commits do |t| + t.references :analytics_repository_file, index: { name: 'index_analytics_repository_file_commits_file_id' }, foreign_key: { on_delete: :cascade }, null: false + t.references :project, index: false, foreign_key: { on_delete: :cascade }, null: false + t.date :committed_date, null: false + t.integer :commit_count, limit: 2, null: false + end + + add_index :analytics_repository_file_commits, + [:project_id, :committed_date, :analytics_repository_file_id], + name: 'index_file_commits_on_committed_date_file_id_and_project_id', + unique: true + end +end diff --git a/db/post_migrate/20191004134055_drop_unused_analytics_repository_file_edits.rb b/db/post_migrate/20191004134055_drop_unused_analytics_repository_file_edits.rb new file mode 100644 index 00000000000..7bf92080fe6 --- /dev/null +++ b/db/post_migrate/20191004134055_drop_unused_analytics_repository_file_edits.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class DropUnusedAnalyticsRepositoryFileEdits < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def up + # The table was never used, there is no app code that writes or reads the table. Safe to remove. + drop_table :analytics_repository_file_edits + end + + def down + create_table :analytics_repository_file_edits do |t| + t.references :project, + index: true, + foreign_key: { on_delete: :cascade }, null: false + t.references :analytics_repository_file, + index: false, + foreign_key: { on_delete: :cascade }, + null: false + t.date :committed_date, + null: false + t.integer :num_edits, + null: false, + default: 0 + end + + add_index :analytics_repository_file_edits, + [:analytics_repository_file_id, :committed_date, :project_id], + name: 'index_file_edits_on_committed_date_file_id_and_project_id', + unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 3a1d5d73c89..352023ac40d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_09_30_025655) do +ActiveRecord::Schema.define(version: 2019_10_04_134055) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -93,13 +93,13 @@ ActiveRecord::Schema.define(version: 2019_09_30_025655) do t.index ["project_id"], name: "analytics_repository_languages_on_project_id" end - create_table "analytics_repository_file_edits", force: :cascade do |t| - t.bigint "project_id", null: false + create_table "analytics_repository_file_commits", force: :cascade do |t| t.bigint "analytics_repository_file_id", null: false + t.bigint "project_id", null: false t.date "committed_date", null: false - t.integer "num_edits", default: 0, null: false - t.index ["analytics_repository_file_id", "committed_date", "project_id"], name: "index_file_edits_on_committed_date_file_id_and_project_id", unique: true - t.index ["project_id"], name: "index_analytics_repository_file_edits_on_project_id" + t.integer "commit_count", limit: 2, null: false + t.index ["analytics_repository_file_id"], name: "index_analytics_repository_file_commits_file_id" + t.index ["project_id", "committed_date", "analytics_repository_file_id"], name: "index_file_commits_on_committed_date_file_id_and_project_id", unique: true end create_table "analytics_repository_files", force: :cascade do |t| @@ -3894,8 +3894,8 @@ ActiveRecord::Schema.define(version: 2019_09_30_025655) do add_foreign_key "analytics_cycle_analytics_project_stages", "projects", on_delete: :cascade add_foreign_key "analytics_language_trend_repository_languages", "programming_languages", on_delete: :cascade add_foreign_key "analytics_language_trend_repository_languages", "projects", on_delete: :cascade - add_foreign_key "analytics_repository_file_edits", "analytics_repository_files", on_delete: :cascade - add_foreign_key "analytics_repository_file_edits", "projects", on_delete: :cascade + add_foreign_key "analytics_repository_file_commits", "analytics_repository_files", on_delete: :cascade + add_foreign_key "analytics_repository_file_commits", "projects", on_delete: :cascade add_foreign_key "analytics_repository_files", "projects", on_delete: :cascade add_foreign_key "application_settings", "namespaces", column: "custom_project_templates_group_id", on_delete: :nullify add_foreign_key "application_settings", "projects", column: "file_template_project_id", name: "fk_ec757bd087", on_delete: :nullify diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb index e29bf75f341..c4288ca6408 100644 --- a/lib/gitlab/diff/file_collection/merge_request_diff.rb +++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb @@ -3,19 +3,7 @@ module Gitlab module Diff module FileCollection - class MergeRequestDiff < Base - extend ::Gitlab::Utils::Override - - def initialize(merge_request_diff, diff_options:) - @merge_request_diff = merge_request_diff - - super(merge_request_diff, - project: merge_request_diff.project, - diff_options: diff_options, - diff_refs: merge_request_diff.diff_refs, - fallback_diff_refs: merge_request_diff.fallback_diff_refs) - end - + class MergeRequestDiff < MergeRequestDiffBase def diff_files diff_files = super diff --git a/lib/gitlab/diff/file_collection/merge_request_diff_base.rb b/lib/gitlab/diff/file_collection/merge_request_diff_base.rb new file mode 100644 index 00000000000..a747a6ed475 --- /dev/null +++ b/lib/gitlab/diff/file_collection/merge_request_diff_base.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Gitlab + module Diff + module FileCollection + class MergeRequestDiffBase < Base + extend ::Gitlab::Utils::Override + + def initialize(merge_request_diff, diff_options:) + @merge_request_diff = merge_request_diff + + super(merge_request_diff, + project: merge_request_diff.project, + diff_options: diff_options, + diff_refs: merge_request_diff.diff_refs, + fallback_diff_refs: merge_request_diff.fallback_diff_refs) + end + end + end + end +end diff --git a/lib/gitlab/diff/file_collection/merge_request_diff_batch.rb b/lib/gitlab/diff/file_collection/merge_request_diff_batch.rb new file mode 100644 index 00000000000..c6d1e0b93a7 --- /dev/null +++ b/lib/gitlab/diff/file_collection/merge_request_diff_batch.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +module Gitlab + module Diff + module FileCollection + # Builds a paginated diff file collection and collects pagination + # metadata. + # + # It doesn't handle caching yet as we're not prepared to write/read + # separate file keys (https://gitlab.com/gitlab-org/gitlab/issues/30550). + # + class MergeRequestDiffBatch < MergeRequestDiffBase + DEFAULT_BATCH_PAGE = 1 + DEFAULT_BATCH_SIZE = 20 + + attr_reader :pagination_data + + def initialize(merge_request_diff, batch_page, batch_size, diff_options:) + super(merge_request_diff, diff_options: diff_options) + + batch_page ||= DEFAULT_BATCH_PAGE + batch_size ||= DEFAULT_BATCH_SIZE + + @paginated_collection = relation.page(batch_page).per(batch_size) + @pagination_data = { + current_page: @paginated_collection.current_page, + next_page: @paginated_collection.next_page, + total_pages: @paginated_collection.total_pages + } + end + + override :diffs + def diffs + strong_memoize(:diffs) do + @merge_request_diff.opening_external_diff do + # Avoiding any extra queries. + collection = @paginated_collection.to_a + + # The offset collection and calculation is required so that we + # know how much has been loaded in previous batches, collapsing + # the current paginated set accordingly (collection limit calculation). + # See: https://docs.gitlab.com/ee/development/diffs.html#diff-collection-limits + # + offset_index = collection.first&.index + options = diff_options.dup + + collection = + if offset_index && offset_index > 0 + offset_collection = relation.limit(offset_index) # rubocop:disable CodeReuse/ActiveRecord + options[:offset_index] = offset_index + offset_collection + collection + else + collection + end + + Gitlab::Git::DiffCollection.new(collection.map(&:to_hash), options) + end + end + end + + private + + def relation + @merge_request_diff.merge_request_diff_files + end + end + end + end +end diff --git a/lib/gitlab/git/diff_collection.rb b/lib/gitlab/git/diff_collection.rb index cb9154cb1e8..b79e30bff78 100644 --- a/lib/gitlab/git/diff_collection.rb +++ b/lib/gitlab/git/diff_collection.rb @@ -31,6 +31,7 @@ module Gitlab @limits = self.class.limits(options) @enforce_limits = !!options.fetch(:limits, true) @expanded = !!options.fetch(:expanded, true) + @offset_index = options.fetch(:offset_index, 0) @line_count = 0 @byte_count = 0 @@ -128,7 +129,7 @@ module Gitlab def each_serialized_patch i = @array.length - @iterator.each do |raw| + @iterator.each_with_index do |raw, iterator_index| @empty = false if @enforce_limits && i >= max_files @@ -154,8 +155,12 @@ module Gitlab break end - yield @array[i] = diff - i += 1 + # We should not yield / memoize diffs before the offset index. Though, + # we still consider the limit buffers for diffs before it. + if iterator_index >= @offset_index + yield @array[i] = diff + i += 1 + end end end end diff --git a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb index ac3e9901123..ca3c802777b 100644 --- a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb +++ b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb @@ -5,6 +5,49 @@ require 'spec_helper' describe Projects::MergeRequests::DiffsController do include ProjectForksHelper + shared_examples 'forked project with submodules' do + render_views + + let(:project) { create(:project, :repository) } + let(:forked_project) { fork_project_with_submodules(project) } + let(:merge_request) { create(:merge_request_with_diffs, source_project: forked_project, source_branch: 'add-submodule-version-bump', target_branch: 'master', target_project: project) } + + before do + project.add_developer(user) + + merge_request.reload + go + end + + it 'renders' do + expect(response).to be_successful + expect(response.body).to have_content('Subproject commit') + end + end + + shared_examples 'persisted preferred diff view cookie' do + context 'with view param' do + before do + go(view: 'parallel') + end + + it 'saves the preferred diff view in a cookie' do + expect(response.cookies['diff_view']).to eq('parallel') + end + end + + context 'when the user cannot view the merge request' do + before do + project.team.truncate + go + end + + it 'returns a 404' do + expect(response).to have_gitlab_http_status(404) + end + end + end + let(:project) { create(:project, :repository) } let(:user) { create(:user) } let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } @@ -51,36 +94,10 @@ describe Projects::MergeRequests::DiffsController do end end - context 'with forked projects with submodules' do - render_views - - let(:project) { create(:project, :repository) } - let(:forked_project) { fork_project_with_submodules(project) } - let(:merge_request) { create(:merge_request_with_diffs, source_project: forked_project, source_branch: 'add-submodule-version-bump', target_branch: 'master', target_project: project) } - - before do - project.add_developer(user) - - merge_request.reload - go - end - - it 'renders' do - expect(response).to be_successful - expect(response.body).to have_content('Subproject commit') - end - end + it_behaves_like 'forked project with submodules' end - context 'with view' do - before do - go(view: 'parallel') - end - - it 'saves the preferred diff view in a cookie' do - expect(response.cookies['diff_view']).to eq('parallel') - end - end + it_behaves_like 'persisted preferred diff view cookie' end describe 'GET diff_for_path' do @@ -154,4 +171,92 @@ describe Projects::MergeRequests::DiffsController do end end end + + describe 'GET diffs_batch' do + def go(extra_params = {}) + params = { + namespace_id: project.namespace.to_param, + project_id: project, + id: merge_request.iid, + format: 'json' + } + + get :diffs_batch, params: params.merge(extra_params) + end + + context 'when feature is disabled' do + before do + stub_feature_flags(diffs_batch_load: false) + end + + it 'returns 404' do + go + + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when not authorized' do + let(:other_user) { create(:user) } + + before do + sign_in(other_user) + end + + it 'returns 404' do + go + + expect(response).to have_gitlab_http_status(404) + end + end + + context 'with default params' do + let(:expected_options) do + { + merge_request: merge_request, + pagination_data: { + current_page: 1, + next_page: nil, + total_pages: 1 + } + } + end + + it 'serializes paginated merge request diff collection' do + expect_next_instance_of(PaginatedDiffSerializer) do |instance| + expect(instance).to receive(:represent) + .with(an_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiffBatch), expected_options) + .and_call_original + end + + go + end + end + + context 'with smaller diff batch params' do + let(:expected_options) do + { + merge_request: merge_request, + pagination_data: { + current_page: 2, + next_page: 3, + total_pages: 4 + } + } + end + + it 'serializes paginated merge request diff collection' do + expect_next_instance_of(PaginatedDiffSerializer) do |instance| + expect(instance).to receive(:represent) + .with(an_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiffBatch), expected_options) + .and_call_original + end + + go(page: 2, per_page: 5) + end + end + + it_behaves_like 'forked project with submodules' + it_behaves_like 'persisted preferred diff view cookie' + end end diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_batch_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_batch_spec.rb new file mode 100644 index 00000000000..265c6260ca9 --- /dev/null +++ b/spec/lib/gitlab/diff/file_collection/merge_request_diff_batch_spec.rb @@ -0,0 +1,126 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do + let(:merge_request) { create(:merge_request) } + let(:batch_page) { 1 } + let(:batch_size) { 10 } + let(:diffable) { merge_request.merge_request_diff } + let(:diff_files_relation) { diffable.merge_request_diff_files } + + subject do + described_class.new(diffable, + batch_page, + batch_size, + diff_options: nil) + end + + let(:diff_files) { subject.diff_files } + + describe 'initialize' do + it 'memoizes pagination_data' do + expect(subject.pagination_data).to eq(current_page: 1, next_page: 2, total_pages: 2) + end + end + + describe '#diff_files' do + let(:batch_size) { 3 } + let(:paginated_rel) { diff_files_relation.page(batch_page).per(batch_size) } + + let(:expected_batch_files) do + paginated_rel.map(&:new_path) + end + + it 'returns paginated diff files' do + expect(diff_files.size).to eq(3) + end + + it 'returns a valid instance of a DiffCollection' do + expect(diff_files).to be_a(Gitlab::Git::DiffCollection) + end + + context 'first page' do + it 'returns correct diff files' do + expect(diff_files.map(&:new_path)).to eq(expected_batch_files) + end + end + + context 'another page' do + let(:batch_page) { 2 } + + it 'returns correct diff files' do + expect(diff_files.map(&:new_path)).to eq(expected_batch_files) + end + end + + context 'nil batch_page' do + let(:batch_page) { nil } + + it 'returns correct diff files' do + expected_batch_files = + diff_files_relation.page(described_class::DEFAULT_BATCH_PAGE).per(batch_size).map(&:new_path) + + expect(diff_files.map(&:new_path)).to eq(expected_batch_files) + end + end + + context 'nil batch_size' do + let(:batch_size) { nil } + + it 'returns correct diff files' do + expected_batch_files = + diff_files_relation.page(batch_page).per(described_class::DEFAULT_BATCH_SIZE).map(&:new_path) + + expect(diff_files.map(&:new_path)).to eq(expected_batch_files) + end + end + + context 'invalid page' do + let(:batch_page) { 999 } + + it 'returns correct diff files' do + expect(diff_files.map(&:new_path)).to be_empty + end + end + + context 'last page' do + it 'returns correct diff files' do + last_page = paginated_rel.total_pages + collection = described_class.new(diffable, + last_page, + batch_size, + diff_options: nil) + + expected_batch_files = diff_files_relation.page(last_page).per(batch_size).map(&:new_path) + + expect(collection.diff_files.map(&:new_path)).to eq(expected_batch_files) + end + end + end + + it_behaves_like 'unfoldable diff' do + subject do + described_class.new(merge_request.merge_request_diff, + batch_page, + batch_size, + diff_options: nil) + end + end + + it_behaves_like 'diff statistics' do + let(:collection_default_args) do + { diff_options: {} } + end + + let(:diffable) { merge_request.merge_request_diff } + let(:stub_path) { '.gitignore' } + + subject do + described_class.new(merge_request.merge_request_diff, + batch_page, + batch_size, + collection_default_args) + end + end +end diff --git a/spec/lib/gitlab/git/diff_collection_spec.rb b/spec/lib/gitlab/git/diff_collection_spec.rb index be6ab0c1200..ded173c49ef 100644 --- a/spec/lib/gitlab/git/diff_collection_spec.rb +++ b/spec/lib/gitlab/git/diff_collection_spec.rb @@ -537,6 +537,70 @@ describe Gitlab::Git::DiffCollection, :seed_helper do end end end + + context 'when offset_index is given' do + subject do + Gitlab::Git::DiffCollection.new( + iterator, + max_files: max_files, + max_lines: max_lines, + limits: limits, + offset_index: 2, + expanded: expanded + ) + end + + def diff(raw) + raw['diff'] + end + + let(:iterator) do + [ + fake_diff(1, 1), + fake_diff(2, 2), + fake_diff(3, 3), + fake_diff(4, 4) + ] + end + + it 'does not yield diffs before the offset' do + expect(subject.to_a.map(&:diff)).to eq( + [ + diff(fake_diff(3, 3)), + diff(fake_diff(4, 4)) + ] + ) + end + + context 'when go over safe limits on bytes' do + let(:iterator) do + [ + fake_diff(1, 10), # 10 + fake_diff(1, 10), # 20 + fake_diff(1, 15), # 35 + fake_diff(1, 20), # 55 + fake_diff(1, 45), # 100 - limit hit + fake_diff(1, 45), + fake_diff(1, 20480), + fake_diff(1, 1) + ] + end + + before do + stub_const('Gitlab::Git::DiffCollection::DEFAULT_LIMITS', + { max_files: max_files, max_lines: 80 }) + end + + it 'considers size of diffs before the offset for prunning' do + expect(subject.to_a.map(&:diff)).to eq( + [ + diff(fake_diff(1, 15)), + diff(fake_diff(1, 20)) + ] + ) + end + end + end end def fake_diff(line_length, line_count) diff --git a/spec/serializers/paginated_diff_entity_spec.rb b/spec/serializers/paginated_diff_entity_spec.rb new file mode 100644 index 00000000000..7432e072318 --- /dev/null +++ b/spec/serializers/paginated_diff_entity_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe PaginatedDiffEntity do + let(:user) { create(:user) } + let(:request) { double('request', current_user: user) } + let(:merge_request) { create(:merge_request, :with_diffs) } + let(:diff_batch) { merge_request.merge_request_diff.diffs_in_batch(2, 3, diff_options: nil) } + let(:options) do + { + request: request, + merge_request: merge_request, + pagination_data: diff_batch.pagination_data + } + end + let(:entity) { described_class.new(diff_batch, options) } + + subject { entity.as_json } + + it 'exposes diff_files' do + expect(subject[:diff_files]).to be_present + end + + it 'exposes pagination data' do + expect(subject[:pagination]).to eq( + current_page: 2, + next_page: 3, + next_page_href: "/#{merge_request.project.full_path}/merge_requests/#{merge_request.iid}/diffs_batch.json?page=3", + total_pages: 7 + ) + end +end diff --git a/spec/support/shared_examples/diff_file_collections.rb b/spec/support/shared_examples/diff_file_collections.rb index 367ddf06c28..4c64abd2a97 100644 --- a/spec/support/shared_examples/diff_file_collections.rb +++ b/spec/support/shared_examples/diff_file_collections.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true shared_examples 'diff statistics' do |test_include_stats_flag: true| + subject { described_class.new(diffable, collection_default_args) } + def stub_stats_find_by_path(path, stats_mock) expect_next_instance_of(Gitlab::Git::DiffStatsCollection) do |collection| allow(collection).to receive(:find_by_path).and_call_original @@ -10,8 +12,6 @@ shared_examples 'diff statistics' do |test_include_stats_flag: true| context 'when should request diff stats' do it 'Repository#diff_stats is called' do - subject = described_class.new(diffable, collection_default_args) - expect(diffable.project.repository) .to receive(:diff_stats) .with(diffable.diff_refs.base_sha, diffable.diff_refs.head_sha) @@ -21,8 +21,6 @@ shared_examples 'diff statistics' do |test_include_stats_flag: true| end it 'Gitlab::Diff::File is initialized with diff stats' do - subject = described_class.new(diffable, collection_default_args) - stats_mock = double(Gitaly::DiffStats, path: '.gitignore', additions: 758, deletions: 120) stub_stats_find_by_path(stub_path, stats_mock) @@ -37,8 +35,6 @@ shared_examples 'diff statistics' do |test_include_stats_flag: true| it 'Repository#diff_stats is not called' do collection_default_args[:diff_options][:include_stats] = false - subject = described_class.new(diffable, collection_default_args) - expect(diffable.project.repository).not_to receive(:diff_stats) subject.diff_files -- GitLab