From 3b537b278bbcf4ddacf41bc16ef6d66c5b6f76f0 Mon Sep 17 00:00:00 2001
From: karthik nayak <knayak@gitlab.com>
Date: Thu, 15 Jun 2023 18:39:20 +0000
Subject: [PATCH] gemfile: Update `gitaly` to `16.1.0-rc2`
Update the gitaly gem from `16.1.0-rc1` to `16.1.0-rc2`. The new gem has
the updated protobuf for `GetTreeEntries` RPC, which includes new
structured error type `GetTreeEntriesError`.
In the following commit[s], we will use this error for better response
handling.
The new release of the gitaly gem also deprecates the
`Gitaly::FindLocalBranchResponse` field. So modify `ref_service_spec.rb`
to remove any usage of the field.
---
Gemfile | 2 +-
Gemfile.checksum | 2 +-
Gemfile.lock | 4 +-
lib/gitlab/gitaly_client/commit_service.rb | 26 ++++
.../gitaly_client/commit_service_spec.rb | 123 ++++++++++++++++++
.../gitlab/gitaly_client/ref_service_spec.rb | 105 +++++----------
6 files changed, 186 insertions(+), 76 deletions(-)
diff --git a/Gemfile b/Gemfile
index a66c0433c7bb5..c7d487f128e88 100644
--- a/Gemfile
+++ b/Gemfile
@@ -509,7 +509,7 @@ gem 'ssh_data', '~> 1.3'
gem 'spamcheck', '~> 1.3.0'
# Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 16.1.0-rc1'
+gem 'gitaly', '~> 16.1.0-rc2'
# KAS GRPC protocol definitions
gem 'kas-grpc', '~> 0.1.0'
diff --git a/Gemfile.checksum b/Gemfile.checksum
index a0cc86f4a0166..932fdf01fc101 100644
--- a/Gemfile.checksum
+++ b/Gemfile.checksum
@@ -204,7 +204,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":"16.1.0.pre.rc1","platform":"ruby","checksum":"3f61ecd96baf283a3365943dac58c3fb81221fee74d1ccc7931e1adea5403405"},
+{"name":"gitaly","version":"16.1.0.pre.rc2","platform":"ruby","checksum":"0007db161bd3f12321813e7095c8d3266d74b0a22b84d7cba8df864c0b88e3af"},
{"name":"gitlab","version":"4.19.0","platform":"ruby","checksum":"3f645e3e195dbc24f0834fbf83e8ccfb2056d8e9712b01a640aad418a6949679"},
{"name":"gitlab-chronic","version":"0.10.5","platform":"ruby","checksum":"f80f18dc699b708870a80685243331290bc10cfeedb6b99c92219722f729c875"},
{"name":"gitlab-dangerfiles","version":"3.10.0","platform":"ruby","checksum":"df4cfe051f52529c0256346d89d06d5ef2bb630928754eb620b5233eb9b14041"},
diff --git a/Gemfile.lock b/Gemfile.lock
index 9eab52f40a32e..ce718a641885c 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -575,7 +575,7 @@ GEM
rails (>= 3.2.0)
git (1.11.0)
rchardet (~> 1.8)
- gitaly (16.1.0.pre.rc1)
+ gitaly (16.1.0.pre.rc2)
grpc (~> 1.0)
gitlab (4.19.0)
httparty (~> 0.20)
@@ -1748,7 +1748,7 @@ DEPENDENCIES
gettext (~> 3.3)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
- gitaly (~> 16.1.0.pre.rc1)
+ gitaly (~> 16.1.0.pre.rc2)
gitlab-chronic (~> 0.10.5)
gitlab-dangerfiles (~> 3.10.0)
gitlab-experiment (~> 0.7.1)
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index f7d6b2402e439..b1f3c2ca38646 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -159,6 +159,17 @@ def tree_entries(repository, revision, path, recursive, skip_flat_paths, paginat
end
[entries, cursor]
+ rescue GRPC::BadStatus => e
+ detailed_error = GitalyClient.decode_detailed_error(e)
+
+ case detailed_error.try(:error)
+ when :path
+ raise Gitlab::Git::Index::IndexError, path_error_message(detailed_error.path)
+ when :resolve_tree
+ raise Gitlab::Git::Index::IndexError, e.details
+ else
+ raise e
+ end
end
def commit_count(ref, options = {})
@@ -608,6 +619,21 @@ def find_changed_paths_request(commits)
Gitaly::FindChangedPathsRequest.new(repository: @gitaly_repo, requests: commit_requests)
end
+
+ def path_error_message(path_error)
+ case path_error.error_type
+ when :ERROR_TYPE_EMPTY_PATH
+ "You must provide a file path"
+ when :ERROR_TYPE_RELATIVE_PATH_ESCAPES_REPOSITORY
+ "Path cannot include traversal syntax"
+ when :ERROR_TYPE_ABSOLUTE_PATH
+ "Only relative path is accepted"
+ when :ERROR_TYPE_LONG_PATH
+ "Path is too long"
+ else
+ "Unknown path error"
+ end
+ end
end
end
end
diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index 0bdb9f7938d5f..ddec335a54ad3 100644
--- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
@@ -287,6 +287,129 @@
is_expected.to eq([[], pagination_cursor])
end
end
+
+ context 'with structured errors' do
+ context 'with ResolveTree error' do
+ before do
+ expect_any_instance_of(Gitaly::CommitService::Stub)
+ .to receive(:get_tree_entries)
+ .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+ .and_raise(raised_error)
+ end
+
+ let(:raised_error) do
+ new_detailed_error(
+ GRPC::Core::StatusCodes::INVALID_ARGUMENT,
+ "invalid revision or path",
+ Gitaly::GetTreeEntriesError.new(
+ resolve_tree: Gitaly::ResolveRevisionError.new(
+ revision: "incorrect revision"
+ )))
+ end
+
+ it 'raises an IndexError' do
+ expect { subject }.to raise_error do |error|
+ expect(error).to be_a(Gitlab::Git::Index::IndexError)
+ expect(error.message).to eq("invalid revision or path")
+ end
+ end
+ end
+
+ context 'with Path error' do
+ let(:status_code) { nil }
+ let(:expected_error) { nil }
+
+ let(:structured_error) do
+ new_detailed_error(
+ status_code,
+ "invalid revision or path",
+ expected_error)
+ end
+
+ shared_examples '#get_tree_entries path failure' do
+ it 'raises an IndexError' do
+ expect_any_instance_of(Gitaly::CommitService::Stub)
+ .to receive(:get_tree_entries).with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+ .and_raise(structured_error)
+
+ expect { subject }.to raise_error do |error|
+ expect(error).to be_a(Gitlab::Git::Index::IndexError)
+ expect(error.message).to eq(expected_message)
+ end
+ end
+ end
+
+ context 'with missing file' do
+ let(:status_code) { GRPC::Core::StatusCodes::INVALID_ARGUMENT }
+ let(:expected_message) { "You must provide a file path" }
+ let(:expected_error) do
+ Gitaly::GetTreeEntriesError.new(
+ path: Gitaly::PathError.new(
+ path: "random path",
+ error_type: :ERROR_TYPE_EMPTY_PATH
+ ))
+ end
+
+ it_behaves_like '#get_tree_entries path failure'
+ end
+
+ context 'with path including traversal' do
+ let(:status_code) { GRPC::Core::StatusCodes::INVALID_ARGUMENT }
+ let(:expected_message) { "Path cannot include traversal syntax" }
+ let(:expected_error) do
+ Gitaly::GetTreeEntriesError.new(
+ path: Gitaly::PathError.new(
+ path: "foo/../bar",
+ error_type: :ERROR_TYPE_RELATIVE_PATH_ESCAPES_REPOSITORY
+ ))
+ end
+
+ it_behaves_like '#get_tree_entries path failure'
+ end
+
+ context 'with absolute path' do
+ let(:status_code) { GRPC::Core::StatusCodes::INVALID_ARGUMENT }
+ let(:expected_message) { "Only relative path is accepted" }
+ let(:expected_error) do
+ Gitaly::GetTreeEntriesError.new(
+ path: Gitaly::PathError.new(
+ path: "/bar/foo",
+ error_type: :ERROR_TYPE_ABSOLUTE_PATH
+ ))
+ end
+
+ it_behaves_like '#get_tree_entries path failure'
+ end
+
+ context 'with long path' do
+ let(:status_code) { GRPC::Core::StatusCodes::INVALID_ARGUMENT }
+ let(:expected_message) { "Path is too long" }
+ let(:expected_error) do
+ Gitaly::GetTreeEntriesError.new(
+ path: Gitaly::PathError.new(
+ path: "long/path/",
+ error_type: :ERROR_TYPE_LONG_PATH
+ ))
+ end
+
+ it_behaves_like '#get_tree_entries path failure'
+ end
+
+ context 'with unkown path error' do
+ let(:status_code) { GRPC::Core::StatusCodes::INVALID_ARGUMENT }
+ let(:expected_message) { "Unknown path error" }
+ let(:expected_error) do
+ Gitaly::GetTreeEntriesError.new(
+ path: Gitaly::PathError.new(
+ path: "unkown error",
+ error_type: :ERROR_TYPE_UNSPECIFIED
+ ))
+ end
+
+ it_behaves_like '#get_tree_entries path failure'
+ end
+ end
+ end
end
describe '#commit_count' do
diff --git a/spec/lib/gitlab/gitaly_client/ref_service_spec.rb b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb
index 7bdfa8922d363..89ac0c119bfd8 100644
--- a/spec/lib/gitlab/gitaly_client/ref_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb
@@ -147,92 +147,53 @@
describe '#local_branches' do
let(:remote_name) { 'my_remote' }
- shared_examples 'common examples' do
- it 'sends a find_local_branches message' do
- target_commits = create_list(:gitaly_commit, 4)
- branches = target_commits.each_with_index.map do |gitaly_commit, i|
- Gitaly::FindLocalBranchResponse.new(
- name: "#{remote_name}/#{i}",
- commit: gitaly_commit,
- commit_author: Gitaly::FindLocalBranchCommitAuthor.new(
- name: gitaly_commit.author.name,
- email: gitaly_commit.author.email,
- date: gitaly_commit.author.date,
- timezone: gitaly_commit.author.timezone
- ),
- commit_committer: Gitaly::FindLocalBranchCommitAuthor.new(
- name: gitaly_commit.committer.name,
- email: gitaly_commit.committer.email,
- date: gitaly_commit.committer.date,
- timezone: gitaly_commit.committer.timezone
- )
- )
- end
-
- local_branches = target_commits.each_with_index.map do |gitaly_commit, i|
- Gitaly::Branch.new(name: "#{remote_name}/#{i}", target_commit: gitaly_commit)
- end
-
- response = if set_local_branches
- [
- Gitaly::FindLocalBranchesResponse.new(local_branches: local_branches[0, 2]),
- Gitaly::FindLocalBranchesResponse.new(local_branches: local_branches[2, 2])
- ]
- else
- [
- Gitaly::FindLocalBranchesResponse.new(branches: branches[0, 2]),
- Gitaly::FindLocalBranchesResponse.new(branches: branches[2, 2])
- ]
- end
-
- expect_any_instance_of(Gitaly::RefService::Stub)
- .to receive(:find_local_branches)
- .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
- .and_return(response)
-
- subject = client.local_branches
+ it 'sends a find_local_branches message' do
+ target_commits = create_list(:gitaly_commit, 4)
- expect(subject.length).to be(target_commits.length)
+ local_branches = target_commits.each_with_index.map do |gitaly_commit, i|
+ Gitaly::Branch.new(name: "#{remote_name}/#{i}", target_commit: gitaly_commit)
end
- it 'parses and sends the sort parameter' do
- expect_any_instance_of(Gitaly::RefService::Stub)
- .to receive(:find_local_branches)
- .with(gitaly_request_with_params(sort_by: :UPDATED_DESC), kind_of(Hash))
- .and_return([])
+ response = [
+ Gitaly::FindLocalBranchesResponse.new(local_branches: local_branches[0, 2]),
+ Gitaly::FindLocalBranchesResponse.new(local_branches: local_branches[2, 2])
+ ]
- client.local_branches(sort_by: 'updated_desc')
- end
+ expect_any_instance_of(Gitaly::RefService::Stub)
+ .to receive(:find_local_branches)
+ .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+ .and_return(response)
- it 'translates known mismatches on sort param values' do
- expect_any_instance_of(Gitaly::RefService::Stub)
- .to receive(:find_local_branches)
- .with(gitaly_request_with_params(sort_by: :NAME), kind_of(Hash))
- .and_return([])
+ subject = client.local_branches
- client.local_branches(sort_by: 'name_asc')
- end
+ expect(subject.length).to be(target_commits.length)
+ end
- it 'uses default sort by name' do
- expect_any_instance_of(Gitaly::RefService::Stub)
- .to receive(:find_local_branches)
- .with(gitaly_request_with_params(sort_by: :NAME), kind_of(Hash))
- .and_return([])
+ it 'parses and sends the sort parameter' do
+ expect_any_instance_of(Gitaly::RefService::Stub)
+ .to receive(:find_local_branches)
+ .with(gitaly_request_with_params(sort_by: :UPDATED_DESC), kind_of(Hash))
+ .and_return([])
- client.local_branches(sort_by: 'invalid')
- end
+ client.local_branches(sort_by: 'updated_desc')
end
- context 'when local_branches variable is not set' do
- let(:set_local_branches) { false }
+ it 'translates known mismatches on sort param values' do
+ expect_any_instance_of(Gitaly::RefService::Stub)
+ .to receive(:find_local_branches)
+ .with(gitaly_request_with_params(sort_by: :NAME), kind_of(Hash))
+ .and_return([])
- it_behaves_like 'common examples'
+ client.local_branches(sort_by: 'name_asc')
end
- context 'when local_branches variable is set' do
- let(:set_local_branches) { true }
+ it 'uses default sort by name' do
+ expect_any_instance_of(Gitaly::RefService::Stub)
+ .to receive(:find_local_branches)
+ .with(gitaly_request_with_params(sort_by: :NAME), kind_of(Hash))
+ .and_return([])
- it_behaves_like 'common examples'
+ client.local_branches(sort_by: 'invalid')
end
end
--
GitLab