From 5816ad4d588a2b711f82c41c5d19d2dba739ae7e Mon Sep 17 00:00:00 2001
From: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Date: Fri, 21 Oct 2022 22:43:41 +0700
Subject: [PATCH] Add server-side pagination to SearchFilesByName
In prior versions, Wiki operations use SearchFilesByName RPC to
access files in Wiki repositories. Gitaly did not support limit
and offset. Rails has to fetch all available pages and perform
pagination from its side with Karinami. This commit adds those
fields to the corresponding request after Gitaly adds such
functionality.
Changelog: performance
Signed-off-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
---
Gemfile | 2 +-
Gemfile.checksum | 2 +-
Gemfile.lock | 4 +-
app/models/wiki.rb | 16 +++---
lib/gitlab/git/repository.rb | 8 +--
.../gitaly_client/repository_service.rb | 8 +--
.../models/wiki_shared_examples.rb | 49 ++++++++++++++++---
7 files changed, 62 insertions(+), 27 deletions(-)
diff --git a/Gemfile b/Gemfile
index 4853249265349..b75090bfef592 100644
--- a/Gemfile
+++ b/Gemfile
@@ -503,7 +503,7 @@ gem 'ssh_data', '~> 1.3'
gem 'spamcheck', '~> 1.0.0'
# Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 15.4.0-rc2'
+gem 'gitaly', '~> 15.5.0'
# KAS GRPC protocol definitions
gem 'kas-grpc', '~> 0.0.2'
diff --git a/Gemfile.checksum b/Gemfile.checksum
index 50463fe271522..620934a40043a 100644
--- a/Gemfile.checksum
+++ b/Gemfile.checksum
@@ -198,7 +198,7 @@
{"name":"gettext_i18n_rails","version":"1.8.0","platform":"ruby","checksum":"95e5cf8440b1e08705b27f2bccb56143272c5a7a0dabcf54ea1bd701140a496f"},
{"name":"gettext_i18n_rails_js","version":"1.3.0","platform":"ruby","checksum":"5d10afe4be3639bff78c50a56768c20f39aecdabc580c08aa45573911c2bd687"},
{"name":"git","version":"1.11.0","platform":"ruby","checksum":"7e95ba4da8298a0373ef1a6862aa22007d761f3c8274b675aa787966fecea0f1"},
-{"name":"gitaly","version":"15.4.0.pre.rc2","platform":"ruby","checksum":"48764528a730605a46f00cf86c7cfcb92d25f4f3d8cb9e09557baac3e9f3f8e3"},
+{"name":"gitaly","version":"15.5.0","platform":"ruby","checksum":"d85dd4890a1f0fd95f935c848bcedf03f19b78872f20f04b9811e602bea4ef42"},
{"name":"github-markup","version":"1.7.0","platform":"ruby","checksum":"97eb27c70662d9cc1d5997cd6c99832026fae5d4913b5dce1ce6c9f65078e69d"},
{"name":"gitlab","version":"4.16.1","platform":"ruby","checksum":"13fd7059cbdad5a1a21b15fa2cf9070b97d92e27f8c688581fe3d84dc038074f"},
{"name":"gitlab-chronic","version":"0.10.5","platform":"ruby","checksum":"f80f18dc699b708870a80685243331290bc10cfeedb6b99c92219722f729c875"},
diff --git a/Gemfile.lock b/Gemfile.lock
index dc1a39776bc2a..36c5ad429c19c 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -547,7 +547,7 @@ GEM
rails (>= 3.2.0)
git (1.11.0)
rchardet (~> 1.8)
- gitaly (15.4.0.pre.rc2)
+ gitaly (15.5.0)
grpc (~> 1.0)
github-markup (1.7.0)
gitlab (4.16.1)
@@ -1624,7 +1624,7 @@ DEPENDENCIES
gettext (~> 3.3)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
- gitaly (~> 15.4.0.pre.rc2)
+ gitaly (~> 15.5.0)
github-markup (~> 1.7.0)
gitlab-chronic (~> 0.10.5)
gitlab-dangerfiles (~> 3.6.1)
diff --git a/app/models/wiki.rb b/app/models/wiki.rb
index b718c3a096fa9..4fe8170a4ab03 100644
--- a/app/models/wiki.rb
+++ b/app/models/wiki.rb
@@ -190,7 +190,7 @@ def has_home_page?
end
def empty?
- !repository_exists? || list_page_paths.empty?
+ !repository_exists? || list_page_paths(limit: 1).empty?
end
def exists?
@@ -207,9 +207,9 @@ def exists?
#
# Returns an Array of GitLab WikiPage instances or an
# empty Array if this Wiki has no pages.
- def list_pages(limit: 0, direction: DIRECTION_ASC, load_content: false)
+ def list_pages(direction: DIRECTION_ASC, load_content: false, limit: 0, offset: 0)
create_wiki_repository unless repository_exists?
- list_pages_with_repository_rpcs(limit: limit, direction: direction, load_content: load_content)
+ list_pages_with_repository_rpcs(direction: direction, load_content: load_content, limit: limit, offset: offset)
end
def sidebar_entries(limit: Gitlab::WikiPages::MAX_SIDEBAR_PAGES, **options)
@@ -457,7 +457,7 @@ def find_matched_file(title, version)
escaped_path = RE2::Regexp.escape(sluggified_title(title))
path_regexp = Gitlab::EncodingHelper.encode_utf8_no_detect("(?i)^#{escaped_path}\\.(#{file_extension_regexp})$")
- matched_files = repository.search_files_by_regexp(path_regexp, version)
+ matched_files = repository.search_files_by_regexp(path_regexp, version, limit: 1)
return if matched_files.blank?
Gitlab::EncodingHelper.encode_utf8_no_detect(matched_files.first)
@@ -509,15 +509,15 @@ def strip_extension(path)
path.sub(/\.[^.]+\z/, "")
end
- def list_page_paths
+ def list_page_paths(limit: 0, offset: 0)
return [] if repository.empty?
path_regexp = Gitlab::EncodingHelper.encode_utf8_no_detect("(?i)\\.(#{file_extension_regexp})$")
- repository.search_files_by_regexp(path_regexp, default_branch)
+ repository.search_files_by_regexp(path_regexp, default_branch, limit: limit, offset: offset)
end
- def list_pages_with_repository_rpcs(limit:, direction:, load_content:)
- paths = list_page_paths
+ def list_pages_with_repository_rpcs(direction:, load_content:, limit:, offset:)
+ paths = list_page_paths(limit: limit, offset: offset)
return [] if paths.empty?
pages = paths.map do |path|
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 9bbe17dcad1f4..a5eab9aae8cc3 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1054,19 +1054,19 @@ def can_be_merged?(source_sha, target_branch)
end
end
- def search_files_by_name(query, ref)
+ def search_files_by_name(query, ref, limit: 0, offset: 0)
safe_query = query.sub(%r{^/*}, "")
ref ||= root_ref
return [] if empty? || safe_query.blank?
- gitaly_repository_client.search_files_by_name(ref, safe_query).map do |file|
+ gitaly_repository_client.search_files_by_name(ref, safe_query, limit: limit, offset: offset).map do |file|
Gitlab::EncodingHelper.encode_utf8(file)
end
end
- def search_files_by_regexp(filter, ref = 'HEAD')
- gitaly_repository_client.search_files_by_regexp(ref, filter).map do |file|
+ def search_files_by_regexp(filter, ref = 'HEAD', limit: 0, offset: 0)
+ gitaly_repository_client.search_files_by_regexp(ref, filter, limit: limit, offset: offset).map do |file|
Gitlab::EncodingHelper.encode_utf8(file)
end
end
diff --git a/lib/gitlab/gitaly_client/repository_service.rb b/lib/gitlab/gitaly_client/repository_service.rb
index f11437552e1fd..8934067551c20 100644
--- a/lib/gitlab/gitaly_client/repository_service.rb
+++ b/lib/gitlab/gitaly_client/repository_service.rb
@@ -303,8 +303,8 @@ def raw_changes_between(from, to)
GitalyClient.call(@storage, :repository_service, :get_raw_changes, request, timeout: GitalyClient.fast_timeout)
end
- def search_files_by_name(ref, query)
- request = Gitaly::SearchFilesByNameRequest.new(repository: @gitaly_repo, ref: ref, query: query)
+ def search_files_by_name(ref, query, limit: 0, offset: 0)
+ request = Gitaly::SearchFilesByNameRequest.new(repository: @gitaly_repo, ref: ref, query: query, limit: limit, offset: offset)
GitalyClient.call(@storage, :repository_service, :search_files_by_name, request, timeout: GitalyClient.fast_timeout).flat_map(&:files)
end
@@ -314,8 +314,8 @@ def search_files_by_content(ref, query, options = {})
search_results_from_response(response, options)
end
- def search_files_by_regexp(ref, filter)
- request = Gitaly::SearchFilesByNameRequest.new(repository: @gitaly_repo, ref: ref, query: '.', filter: filter)
+ def search_files_by_regexp(ref, filter, limit: 0, offset: 0)
+ request = Gitaly::SearchFilesByNameRequest.new(repository: @gitaly_repo, ref: ref, query: '.', filter: filter, limit: limit, offset: offset)
GitalyClient.call(@storage, :repository_service, :search_files_by_name, request, timeout: GitalyClient.fast_timeout).flat_map(&:files)
end
diff --git a/spec/support/shared_examples/models/wiki_shared_examples.rb b/spec/support/shared_examples/models/wiki_shared_examples.rb
index b1aa90449e177..7f0a87be939d8 100644
--- a/spec/support/shared_examples/models/wiki_shared_examples.rb
+++ b/spec/support/shared_examples/models/wiki_shared_examples.rb
@@ -161,9 +161,10 @@
let(:wiki_pages) { subject.list_pages }
before do
- subject.create_page('index', 'This is an index')
+ # The order is intentional
subject.create_page('index2', 'This is an index2')
- subject.create_page('an index3', 'This is an index3')
+ subject.create_page('index', 'This is an index')
+ subject.create_page('index3', 'This is an index3')
end
it 'returns an array of WikiPage instances' do
@@ -183,13 +184,47 @@
context 'with limit option' do
it 'returns limited set of pages' do
- expect(subject.list_pages(limit: 1).count).to eq(1)
+ expect(
+ subject.list_pages(limit: 1).map(&:title)
+ ).to eql(%w[index])
+ end
+
+ it 'returns all set of pages if limit is more than the total pages' do
+ expect(subject.list_pages(limit: 4).count).to eq(3)
+ end
+
+ it 'returns all set of pages if limit is 0' do
+ expect(subject.list_pages(limit: 0).count).to eq(3)
+ end
+ end
+
+ context 'with offset option' do
+ it 'returns offset-ed set of pages' do
+ expect(
+ subject.list_pages(offset: 1).map(&:title)
+ ).to eq(%w[index2 index3])
+
+ expect(
+ subject.list_pages(offset: 2).map(&:title)
+ ).to eq(["index3"])
+ expect(subject.list_pages(offset: 3).count).to eq(0)
+ expect(subject.list_pages(offset: 4).count).to eq(0)
+ end
+
+ it 'returns all set of pages if offset is 0' do
+ expect(subject.list_pages(offset: 0).count).to eq(3)
+ end
+
+ it 'can combines with limit' do
+ expect(
+ subject.list_pages(offset: 1, limit: 1).map(&:title)
+ ).to eq(["index2"])
end
end
context 'with sorting options' do
it 'returns pages sorted by title by default' do
- pages = ['an index3', 'index', 'index2']
+ pages = %w[index index2 index3]
expect(subject.list_pages.map(&:title)).to eq(pages)
expect(subject.list_pages(direction: 'desc').map(&:title)).to eq(pages.reverse)
@@ -200,9 +235,9 @@
let(:pages) { subject.list_pages(load_content: true) }
it 'loads WikiPage content' do
- expect(pages.first.content).to eq('This is an index3')
- expect(pages.second.content).to eq('This is an index')
- expect(pages.third.content).to eq('This is an index2')
+ expect(pages.first.content).to eq('This is an index')
+ expect(pages.second.content).to eq('This is an index2')
+ expect(pages.third.content).to eq('This is an index3')
end
end
end
--
GitLab