From eb67fe85b50c75f4547df0d8f7681ac8da6d7ebe Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 5 Nov 2018 15:37:21 -0800 Subject: Paginate Bitbucket Server importer projects To prevent delays in loading the page and reduce memory usage, limit the number of projects shown at 25 per page. Part of https://gitlab.com/gitlab-org/gitlab-ce/issues/50021 --- .../import/bitbucket_server_controller.rb | 26 +++++++++++++++---- app/views/import/bitbucket_server/status.html.haml | 2 ++ .../sh-paginate-bitbucket-server-imports.yml | 5 ++++ lib/bitbucket_server/client.rb | 8 +++--- lib/bitbucket_server/collection.rb | 24 ++++++++++++++++++ lib/bitbucket_server/paginator.rb | 27 +++++++++++++++----- .../import/bitbucket_server_controller_spec.rb | 11 ++++++-- spec/lib/bitbucket_server/client_spec.rb | 12 ++++++--- spec/lib/bitbucket_server/collection_spec.rb | 29 ++++++++++++++++++++++ spec/lib/bitbucket_server/paginator_spec.rb | 10 ++++++++ 10 files changed, 134 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/sh-paginate-bitbucket-server-imports.yml create mode 100644 spec/lib/bitbucket_server/collection_spec.rb diff --git a/app/controllers/import/bitbucket_server_controller.rb b/app/controllers/import/bitbucket_server_controller.rb index fdd1078cdf7..6b7cc2d8ad7 100644 --- a/app/controllers/import/bitbucket_server_controller.rb +++ b/app/controllers/import/bitbucket_server_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Import::BitbucketServerController < Import::BaseController + include PaginationHelper + before_action :verify_bitbucket_server_import_enabled before_action :bitbucket_auth, except: [:new, :configure] before_action :validate_import_params, only: [:create] @@ -54,14 +56,14 @@ class Import::BitbucketServerController < Import::BaseController # rubocop: disable CodeReuse/ActiveRecord def status - repos = bitbucket_client.repos - - @repos, @incompatible_repos = repos.partition { |repo| repo.valid? } + @collection = bitbucket_client.repos(page_offset: page_offset, limit: limit_per_page) + @repos, @incompatible_repos = @collection.partition { |repo| repo.valid? } - @already_added_projects = find_already_added_projects('bitbucket_server') + # Use the import URL to filter beyond what BaseService#find_already_added_projects + @already_added_projects = filter_added_projects('bitbucket_server', @repos.map(&:browse_url)) already_added_projects_names = @already_added_projects.pluck(:import_source) - @repos.to_a.reject! { |repo| already_added_projects_names.include?(repo.browse_url) } + @repos.reject! { |repo| already_added_projects_names.include?(repo.browse_url) } rescue BitbucketServer::Connection::ConnectionError, BitbucketServer::Client::ServerError => e flash[:alert] = "Unable to connect to server: #{e}" clear_session_data @@ -75,6 +77,12 @@ class Import::BitbucketServerController < Import::BaseController private + # rubocop: disable CodeReuse/ActiveRecord + def filter_added_projects(import_type, import_sources) + current_user.created_projects.where(import_type: import_type, import_source: import_sources).includes(:import_state) + end + # rubocop: enable CodeReuse/ActiveRecord + def bitbucket_client @bitbucket_client ||= BitbucketServer::Client.new(credentials) end @@ -130,4 +138,12 @@ class Import::BitbucketServerController < Import::BaseController password: session[personal_access_token_key] } end + + def page_offset + [0, params[:page].to_i].max + end + + def limit_per_page + BitbucketServer::Paginator::PAGE_LENGTH + end end diff --git a/app/views/import/bitbucket_server/status.html.haml b/app/views/import/bitbucket_server/status.html.haml index ae09e0dfa18..56d4f2ba881 100644 --- a/app/views/import/bitbucket_server/status.html.haml +++ b/app/views/import/bitbucket_server/status.html.haml @@ -84,4 +84,6 @@ = link_to 'import flow', status_import_bitbucket_server_path again. += paginate_without_count(@collection) + .js-importer-status{ data: { jobs_import_path: "#{jobs_import_bitbucket_server_path}", import_path: "#{import_bitbucket_server_path}" } } diff --git a/changelogs/unreleased/sh-paginate-bitbucket-server-imports.yml b/changelogs/unreleased/sh-paginate-bitbucket-server-imports.yml new file mode 100644 index 00000000000..b5743e71cf9 --- /dev/null +++ b/changelogs/unreleased/sh-paginate-bitbucket-server-imports.yml @@ -0,0 +1,5 @@ +--- +title: Paginate Bitbucket Server importer projects +merge_request: 22825 +author: +type: changed diff --git a/lib/bitbucket_server/client.rb b/lib/bitbucket_server/client.rb index 15e59f93141..83e8808db07 100644 --- a/lib/bitbucket_server/client.rb +++ b/lib/bitbucket_server/client.rb @@ -35,9 +35,9 @@ module BitbucketServer BitbucketServer::Representation::Repo.new(parsed_response) end - def repos + def repos(page_offset: 0, limit: nil) path = "/repos" - get_collection(path, :repo) + get_collection(path, :repo, page_offset: page_offset, limit: limit) end def create_branch(project_key, repo, branch_name, sha) @@ -61,8 +61,8 @@ module BitbucketServer private - def get_collection(path, type) - paginator = BitbucketServer::Paginator.new(connection, Addressable::URI.escape(path), type) + def get_collection(path, type, page_offset: 0, limit: nil) + paginator = BitbucketServer::Paginator.new(connection, Addressable::URI.escape(path), type, page_offset: page_offset, limit: limit) BitbucketServer::Collection.new(paginator) rescue *SERVER_ERRORS => e raise ServerError, e diff --git a/lib/bitbucket_server/collection.rb b/lib/bitbucket_server/collection.rb index b50c5dde352..7e4b2277bbe 100644 --- a/lib/bitbucket_server/collection.rb +++ b/lib/bitbucket_server/collection.rb @@ -2,7 +2,13 @@ module BitbucketServer class Collection < Enumerator + attr_reader :paginator + + delegate :page_offset, :has_next_page?, to: :paginator + def initialize(paginator) + @paginator = paginator + super() do |yielder| loop do paginator.items.each { |item| yielder << item } @@ -12,6 +18,24 @@ module BitbucketServer lazy end + def current_page + return 1 if page_offset <= 1 + + [1, page_offset].max + end + + def prev_page + return nil unless current_page > 1 + + current_page - 1 + end + + def next_page + return nil unless has_next_page? + + current_page + 1 + end + def method_missing(method, *args) return super unless self.respond_to?(method) diff --git a/lib/bitbucket_server/paginator.rb b/lib/bitbucket_server/paginator.rb index c351fb2f11f..aa5f84f44b3 100644 --- a/lib/bitbucket_server/paginator.rb +++ b/lib/bitbucket_server/paginator.rb @@ -4,34 +4,49 @@ module BitbucketServer class Paginator PAGE_LENGTH = 25 - def initialize(connection, url, type) + attr_reader :page_offset + + def initialize(connection, url, type, page_offset: 0, limit: nil) @connection = connection @type = type @url = url @page = nil + @page_offset = page_offset + @limit = limit || PAGE_LENGTH + @total = 0 end def items raise StopIteration unless has_next_page? + raise StopIteration if over_limit? @page = fetch_next_page + @total += @page.items.count @page.items end + def has_next_page? + page.nil? || page.next? + end + private - attr_reader :connection, :page, :url, :type + attr_reader :connection, :page, :url, :type, :limit - def has_next_page? - page.nil? || page.next? + def over_limit? + @limit.positive? && @total >= @limit end def next_offset - page.nil? ? 0 : page.next + page.nil? ? starting_offset : page.next + end + + def starting_offset + [0, page_offset - 1].max * limit end def fetch_next_page - parsed_response = connection.get(@url, start: next_offset, limit: PAGE_LENGTH) + parsed_response = connection.get(@url, start: next_offset, limit: @limit) Page.new(parsed_response, type) end end diff --git a/spec/controllers/import/bitbucket_server_controller_spec.rb b/spec/controllers/import/bitbucket_server_controller_spec.rb index 5024ef71771..77060fdc3be 100644 --- a/spec/controllers/import/bitbucket_server_controller_spec.rb +++ b/spec/controllers/import/bitbucket_server_controller_spec.rb @@ -121,12 +121,19 @@ describe Import::BitbucketServerController do @repo = double(slug: 'vim', project_key: 'asd', full_name: 'asd/vim', "valid?" => true, project_name: 'asd', browse_url: 'http://test', name: 'vim') @invalid_repo = double(slug: 'invalid', project_key: 'foobar', full_name: 'asd/foobar', "valid?" => false, browse_url: 'http://bad-repo') + @created_repo = double(slug: 'created', project_key: 'existing', full_name: 'group/created', "valid?" => true, browse_url: 'http://existing') assign_session_tokens end it 'assigns repository categories' do - created_project = create(:project, import_type: 'bitbucket_server', creator_id: user.id, import_source: 'foo/bar', import_status: 'finished') - expect(client).to receive(:repos).and_return([@repo, @invalid_repo]) + created_project = create(:project, import_type: 'bitbucket_server', creator_id: user.id, import_status: 'finished', import_source: @created_repo.browse_url) + repos = instance_double(BitbucketServer::Collection) + + expect(repos).to receive(:partition).and_return([[@repo, @created_repo], [@invalid_repo]]) + expect(repos).to receive(:current_page).and_return(1) + expect(repos).to receive(:next_page).and_return(2) + expect(repos).to receive(:prev_page).and_return(nil) + expect(client).to receive(:repos).and_return(repos) get :status diff --git a/spec/lib/bitbucket_server/client_spec.rb b/spec/lib/bitbucket_server/client_spec.rb index f926ae963a4..5de0a9a65b5 100644 --- a/spec/lib/bitbucket_server/client_spec.rb +++ b/spec/lib/bitbucket_server/client_spec.rb @@ -13,7 +13,7 @@ describe BitbucketServer::Client do let(:path) { "/projects/#{project}/repos/#{repo_slug}/pull-requests?state=ALL" } it 'requests a collection' do - expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :pull_request) + expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :pull_request, page_offset: 0, limit: nil) subject.pull_requests(project, repo_slug) end @@ -29,7 +29,7 @@ describe BitbucketServer::Client do let(:path) { "/projects/#{project}/repos/#{repo_slug}/pull-requests/1/activities" } it 'requests a collection' do - expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :activity) + expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :activity, page_offset: 0, limit: nil) subject.activities(project, repo_slug, 1) end @@ -52,10 +52,16 @@ describe BitbucketServer::Client do let(:path) { "/repos" } it 'requests a collection' do - expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :repo) + expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :repo, page_offset: 0, limit: nil) subject.repos end + + it 'requests a collection with an offset and limit' do + expect(BitbucketServer::Paginator).to receive(:new).with(anything, path, :repo, page_offset: 10, limit: 25) + + subject.repos(page_offset: 10, limit: 25) + end end describe '#create_branch' do diff --git a/spec/lib/bitbucket_server/collection_spec.rb b/spec/lib/bitbucket_server/collection_spec.rb new file mode 100644 index 00000000000..ddd02bac88a --- /dev/null +++ b/spec/lib/bitbucket_server/collection_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe BitbucketServer::Collection do + let(:connection) { instance_double(BitbucketServer::Connection) } + let(:page) { 1 } + let(:paginator) { BitbucketServer::Paginator.new(connection, 'http://more-data', :pull_request, page_offset: page) } + + subject { described_class.new(paginator) } + + describe '#current_page' do + it 'returns 1' do + expect(subject.current_page).to eq(1) + end + end + + describe '#prev_page' do + it 'returns nil' do + expect(subject.prev_page).to be_nil + end + end + + describe '#next_page' do + it 'returns 2' do + expect(subject.next_page).to eq(2) + end + end +end diff --git a/spec/lib/bitbucket_server/paginator_spec.rb b/spec/lib/bitbucket_server/paginator_spec.rb index 2de50eba3c4..d268d4f23cf 100644 --- a/spec/lib/bitbucket_server/paginator_spec.rb +++ b/spec/lib/bitbucket_server/paginator_spec.rb @@ -20,6 +20,16 @@ describe BitbucketServer::Paginator do expect { paginator.items }.to raise_error(StopIteration) end + it 'obeys limits' do + limited = described_class.new(connection, 'http://more-data', :pull_request, page_offset: 0, limit: 1) + allow(limited).to receive(:fetch_next_page).and_return(first_page) + + expect(limited.has_next_page?).to be_truthy + expect(limited.items).to match(['item_1']) + expect(limited.has_next_page?).to be_truthy + expect { limited.items }.to raise_error(StopIteration) + end + it 'calls the connection with different offsets' do expect(connection).to receive(:get).with('http://more-data', start: 0, limit: BitbucketServer::Paginator::PAGE_LENGTH).and_return(page_attrs) -- cgit v1.2.1