Commit f876eb84 authored by Marc Shaw's avatar Marc Shaw

Use the new gitaly RPC when validating the diff

Merge Request: gitlab.com/gitlab-org/gitlab/-/merge_requests/46116
Issue: gitlab.com/gitlab-org/gitlab/-/issues/227572
parent 66649862
......@@ -465,7 +465,7 @@ group :ed25519 do
end
# Gitaly GRPC protocol definitions
gem 'gitaly', '~> 13.6.1'
gem 'gitaly', '~> 13.7.0.pre.rc1'
gem 'grpc', '~> 1.30.2'
......
......@@ -420,7 +420,7 @@ GEM
rails (>= 3.2.0)
git (1.7.0)
rchardet (~> 1.8)
gitaly (13.6.1)
gitaly (13.7.0.pre.rc1)
grpc (~> 1.0)
github-markup (1.7.0)
gitlab-chronic (0.10.5)
......@@ -1345,7 +1345,7 @@ DEPENDENCIES
gettext (~> 3.3)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
gitaly (~> 13.6.1)
gitaly (~> 13.7.0.pre.rc1)
github-markup (~> 1.7.0)
gitlab-chronic (~> 0.10.5)
gitlab-fog-azure-rm (~> 1.0)
......
---
name: diff_check_with_paths_changed_rpc
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46116
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/288827
milestone: '13.7'
type: development
group: group::code review
default_enabled: false
......@@ -58,10 +58,14 @@ module EE
def path_locks_validation
lambda do |diff|
path = if diff.renamed_file?
diff.old_path
path = if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project)
diff.path
else
diff.new_path || diff.old_path
if diff.renamed_file?
diff.old_path
else
diff.new_path || diff.old_path
end
end
lock_info = project.find_path_lock(path)
......@@ -72,12 +76,24 @@ module EE
end
end
def new_file?(path)
path.status == :ADDED
end
def file_name_validation
lambda do |diff|
if (diff.renamed_file || diff.new_file) && blacklisted_regex = push_rule.filename_denylisted?(diff.new_path)
return unless blacklisted_regex.present?
"File name #{diff.new_path} was blacklisted by the pattern #{blacklisted_regex}."
if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project)
if new_file?(diff) && denylisted_regex = push_rule.filename_denylisted?(diff.path)
return unless denylisted_regex.present?
"File name #{diff.path} was blacklisted by the pattern #{denylisted_regex}."
end
else
if (diff.renamed_file || diff.new_file) && denylisted_regex = push_rule.filename_denylisted?(diff.new_path)
return unless denylisted_regex.present?
"File name #{diff.new_path} was blacklisted by the pattern #{denylisted_regex}."
end
end
rescue ::PushRule::MatchError => e
raise ::Gitlab::GitAccess::ForbiddenError, e.message
......
......@@ -17,17 +17,26 @@ module Gitlab
file_paths = []
process_commits do |commit|
validate_once(commit) do
commit.raw_deltas.each do |diff|
file_paths.concat([diff.new_path, diff.old_path].compact)
if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project)
paths = project.repository.find_changed_paths(commits.map(&:sha))
paths.each do |path|
file_paths.concat([path.path])
validate_diff(diff)
validate_diff(path)
end
else
process_commits do |commit|
validate_once(commit) do
commit.raw_deltas.each do |diff|
file_paths.concat([diff.new_path, diff.old_path].compact)
validate_diff(diff)
end
end
end
end
validate_file_paths(file_paths)
validate_file_paths(file_paths.uniq)
end
private
......
......@@ -467,6 +467,18 @@ module Gitlab
empty_diff_stats
end
def find_changed_paths(commits)
processed_commits = commits.reject { |ref| ref.blank? || Gitlab::Git.blank_ref?(ref) }
return [] if processed_commits.empty?
wrapped_gitaly_errors do
gitaly_commit_client.find_changed_paths(processed_commits)
end
rescue CommandError, TypeError, NoRepository
[]
end
# Returns a RefName for a given SHA
def ref_name_for_sha(ref_path, sha)
raise ArgumentError, "sha can't be empty" unless sha.present?
......
......@@ -216,6 +216,23 @@ module Gitlab
response.flat_map(&:stats)
end
def find_changed_paths(commits)
request = Gitaly::FindChangedPathsRequest.new(
repository: @gitaly_repo,
commits: commits
)
response = GitalyClient.call(@repository.storage, :diff_service, :find_changed_paths, request, timeout: GitalyClient.medium_timeout)
response.flat_map do |msg|
msg.paths.map do |path|
OpenStruct.new(
status: path.status,
path: EncodingHelper.encode!(path.path)
)
end
end
end
def find_all_commits(opts = {})
request = Gitaly::FindAllCommitsRequest.new(
repository: @gitaly_repo,
......
......@@ -7,7 +7,6 @@ RSpec.describe Gitlab::Checks::DiffCheck do
describe '#validate!' do
let(:owner) { create(:user) }
let!(:lock) { create(:lfs_file_lock, user: owner, project: project, path: 'README') }
before do
allow(project.repository).to receive(:new_commits).and_return(
......@@ -28,13 +27,27 @@ RSpec.describe Gitlab::Checks::DiffCheck do
end
context 'with LFS enabled' do
let!(:lock) { create(:lfs_file_lock, user: owner, project: project, path: 'README') }
before do
allow(project).to receive(:lfs_enabled?).and_return(true)
end
context 'when change is sent by a different user' do
it 'raises an error if the user is not allowed to update the file' do
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "The path 'README' is locked in Git LFS by #{lock.user.name}")
context 'when diff check with paths rpc feature flag is true' do
it 'raises an error if the user is not allowed to update the file' do
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "The path 'README' is locked in Git LFS by #{lock.user.name}")
end
end
context 'when diff check with paths rpc feature flag is false' do
before do
stub_feature_flags(diff_check_with_paths_changed_rpc: false)
end
it 'raises an error if the user is not allowed to update the file' do
expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "The path 'README' is locked in Git LFS by #{lock.user.name}")
end
end
end
......@@ -53,6 +66,8 @@ RSpec.describe Gitlab::Checks::DiffCheck do
expect_any_instance_of(Commit).to receive(:raw_deltas).and_call_original
stub_feature_flags(diff_check_with_paths_changed_rpc: false)
subject.validate!
end
......
......@@ -1185,6 +1185,66 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
end
end
describe '#find_changed_paths' do
let(:commit_1) { 'fa1b1e6c004a68b7d8763b86455da9e6b23e36d6' }
let(:commit_2) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
let(:commit_3) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' }
let(:commit_1_files) do
[
OpenStruct.new(status: :ADDED, path: "files/executables/ls"),
OpenStruct.new(status: :ADDED, path: "files/executables/touch"),
OpenStruct.new(status: :ADDED, path: "files/links/regex.rb"),
OpenStruct.new(status: :ADDED, path: "files/links/ruby-style-guide.md"),
OpenStruct.new(status: :ADDED, path: "files/links/touch"),
OpenStruct.new(status: :MODIFIED, path: ".gitmodules"),
OpenStruct.new(status: :ADDED, path: "deeper/nested/six"),
OpenStruct.new(status: :ADDED, path: "nested/six")
]
end
let(:commit_2_files) do
[OpenStruct.new(status: :ADDED, path: "bin/executable")]
end
let(:commit_3_files) do
[
OpenStruct.new(status: :MODIFIED, path: ".gitmodules"),
OpenStruct.new(status: :ADDED, path: "gitlab-shell")
]
end
it 'returns a list of paths' do
collection = repository.find_changed_paths([commit_1, commit_2, commit_3])
expect(collection).to be_a(Enumerable)
expect(collection.to_a).to eq(commit_1_files + commit_2_files + commit_3_files)
end
it 'returns no paths when SHAs are invalid' do
collection = repository.find_changed_paths(['invalid', commit_1])
expect(collection).to be_a(Enumerable)
expect(collection.to_a).to be_empty
end
it 'returns a list of paths even when containing a blank ref' do
collection = repository.find_changed_paths([nil, commit_1])
expect(collection).to be_a(Enumerable)
expect(collection.to_a).to eq(commit_1_files)
end
it 'returns no paths when the commits are nil' do
expect_any_instance_of(Gitlab::GitalyClient::CommitService)
.not_to receive(:find_changed_paths)
collection = repository.find_changed_paths([nil, nil])
expect(collection).to be_a(Enumerable)
expect(collection.to_a).to be_empty
end
end
describe "#ls_files" do
let(:master_file_paths) { repository.ls_files("master") }
let(:utf8_file_paths) { repository.ls_files("ls-files-utf8") }
......
......@@ -145,6 +145,31 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
end
end
describe '#find_changed_paths' do
let(:commits) { %w[1a0b36b3cdad1d2ee32457c102a8c0b7056fa863 cfe32cf61b73a0d5e9f13e774abde7ff789b1660] }
it 'sends an RPC request and returns the stats' do
request = Gitaly::FindChangedPathsRequest.new(repository: repository_message,
commits: commits)
changed_paths_response = Gitaly::FindChangedPathsResponse.new(
paths: [{
path: "app/assets/javascripts/boards/components/project_select.vue",
status: :MODIFIED
}])
expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:find_changed_paths)
.with(request, kind_of(Hash)).and_return([changed_paths_response])
returned_value = described_class.new(repository).find_changed_paths(commits)
mapped_returned_value = returned_value.map(&:to_h)
mapped_expected_value = changed_paths_response.paths.map(&:to_h)
expect(mapped_returned_value).to eq(mapped_expected_value)
end
end
describe '#tree_entries' do
let(:path) { '/' }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment