Merge branch '288827-feature-flag-rollout-of-diff_check_with_paths_changed_rpc' into 'master'

Resolve "[Feature flag] Rollout of `diff_check_with_paths_changed_rpc`"

See merge request gitlab-org/gitlab!50623

config/feature_flags/development/diff_check_with_paths_changed_rpc.yml

----
-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: true

ee/lib/ee/gitlab/checks/diff_check.rb

@@ -9,7 +9,7 @@ module EE
 
         private
 
-        def path_validations
+        def file_paths_validations
           validations = [super].flatten
 
           if validate_code_owners?
@@ -48,8 +48,8 @@ module EE
           push_rule.file_name_regex.present? || push_rule.prevent_secrets
         end
 
-        override :validations_for_diff
-        def validations_for_diff
+        override :validations_for_path
+        def validations_for_path
           super.tap do |validations|
             validations.push(path_locks_validation) if validate_path_locks?
             validations.push(file_name_validation) if push_rule_checks_commit?
@@ -57,17 +57,8 @@ module EE
         end
 
         def path_locks_validation
-          lambda do |diff|
-            path = if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project, default_enabled: true)
-                     diff.path
-                   else
-                     if diff.renamed_file?
-                       diff.old_path
-                     else
-                       diff.new_path || diff.old_path
-                     end
-                   end
-
+          lambda do |changed_path|
+            path = changed_path.path
             lock_info = project.find_path_lock(path)
 
             if lock_info && lock_info.user != user_access.user
@@ -76,24 +67,12 @@ module EE
           end
         end
 
-        def new_file?(path)
-          path.status == :ADDED
-        end
-
         def file_name_validation
-          lambda do |diff|
-            if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project, default_enabled: true)
-              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
+          lambda do |changed_path|
+            if changed_path.new_file? && denylisted_regex = push_rule.filename_denylisted?(changed_path.path)
+              return unless denylisted_regex.present?
+
+              "File name #{changed_path.path} was blacklisted by the pattern #{denylisted_regex}."
             end
           rescue ::PushRule::MatchError => e
             raise ::Gitlab::GitAccess::ForbiddenError, e.message

lib/gitlab/checks/diff_check.rb

@@ -6,37 +6,20 @@ module Gitlab
       include Gitlab::Utils::StrongMemoize
 
       LOG_MESSAGES = {
-        validate_file_paths: "Validating diffs' file paths...",
-        diff_content_check: "Validating diff contents..."
+        validate_file_paths: "Validating diffs' file paths..."
       }.freeze
 
       def validate!
         return if deletion?
-        return unless should_run_diff_validations?
+        return unless should_run_validations?
         return if commits.empty?
 
-        file_paths = []
-
-        if ::Feature.enabled?(:diff_check_with_paths_changed_rpc, project, default_enabled: true)
-          paths = project.repository.find_changed_paths(commits.map(&:sha))
-          paths.each do |path|
-            file_paths.concat([path.path])
-
-            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
+        paths = project.repository.find_changed_paths(commits.map(&:sha))
+        paths.each do |path|
+          validate_path(path)
         end
 
-        validate_file_paths(file_paths.uniq)
+        validate_file_paths(paths.map(&:path).uniq)
       end
 
       private
@@ -47,43 +30,30 @@ module Gitlab
         end
       end
 
-      def should_run_diff_validations?
-        validations_for_diff.present? || path_validations.present?
+      def should_run_validations?
+        validations_for_path.present? || file_paths_validations.present?
       end
 
-      def validate_diff(diff)
-        validations_for_diff.each do |validation|
-          if error = validation.call(diff)
+      def validate_path(path)
+        validations_for_path.each do |validation|
+          if error = validation.call(path)
             raise ::Gitlab::GitAccess::ForbiddenError, error
           end
         end
       end
 
       # Method overwritten in EE to inject custom validations
-      def validations_for_diff
+      def validations_for_path
         []
       end
 
-      def path_validations
+      def file_paths_validations
         validate_lfs_file_locks? ? [lfs_file_locks_validation] : []
       end
 
-      def process_commits
-        logger.log_timed(LOG_MESSAGES[:diff_content_check]) do
-          # n+1: https://gitlab.com/gitlab-org/gitlab/issues/3593
-          ::Gitlab::GitalyClient.allow_n_plus_1_calls do
-            commits.each do |commit|
-              logger.check_timeout_reached
-
-              yield(commit)
-            end
-          end
-        end
-      end
-
       def validate_file_paths(file_paths)
         logger.log_timed(LOG_MESSAGES[__method__]) do
-          path_validations.each do |validation|
+          file_paths_validations.each do |validation|
             if error = validation.call(file_paths)
               raise ::Gitlab::GitAccess::ForbiddenError, error
             end

lib/gitlab/git/changed_path.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Git
+    class ChangedPath
+      attr_reader :status, :path
+
+      def initialize(status:, path:)
+        @status = status
+        @path = path
+      end
+
+      def new_file?
+        status == :ADDED
+      end
+    end
+  end
+end

lib/gitlab/gitaly_client/commit_service.rb

@@ -225,7 +225,7 @@ module Gitlab
         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(
+            Gitlab::Git::ChangedPath.new(
               status: path.status,
               path:  EncodingHelper.encode!(path.path)
             )

spec/lib/gitlab/checks/diff_check_spec.rb

@@ -6,96 +6,63 @@ RSpec.describe Gitlab::Checks::DiffCheck do
   include_context 'change access checks context'
 
   describe '#validate!' do
-    let(:owner) { create(:user) }
-
-    before do
-      allow(project.repository).to receive(:new_commits).and_return(
-        project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51')
-      )
-    end
-
-    context 'with LFS not enabled' do
-      before do
-        allow(project).to receive(:lfs_enabled?).and_return(false)
-      end
-
-      it 'does not invoke :lfs_file_locks_validation' do
-        expect(subject).not_to receive(:lfs_file_locks_validation)
+    context 'when commits is empty' do
+      it 'does not call find_changed_paths' do
+        expect(project.repository).not_to receive(:find_changed_paths)
 
         subject.validate!
       end
     end
 
-    context 'with LFS enabled' do
-      let!(:lock) { create(:lfs_file_lock, user: owner, project: project, path: 'README') }
-
+    context 'when commits is not empty' do
       before do
-        allow(project).to receive(:lfs_enabled?).and_return(true)
+        allow(project.repository).to receive(:new_commits).and_return(
+          project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51')
+        )
       end
 
-      context 'when change is sent by a different user' do
-        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 deletion is true' do
+        let(:newrev) { Gitlab::Git::BLANK_SHA }
 
-        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 'does not call find_changed_paths' do
+          expect(project.repository).not_to receive(:find_changed_paths)
 
-          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
+          subject.validate!
         end
       end
 
-      context 'when change is sent by the author of the lock' do
-        let(:user) { owner }
-
-        it "doesn't raise any error" do
-          expect { subject.validate! }.not_to raise_error
+      context 'with LFS not enabled' do
+        before do
+          allow(project).to receive(:lfs_enabled?).and_return(false)
         end
-      end
-    end
-
-    context 'commit diff validations' do
-      before do
-        allow(subject).to receive(:validations_for_diff).and_return([lambda { |diff| return }])
-
-        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
 
-      context 'when request store is inactive' do
-        it 'are run for every commit' do
-          expect_any_instance_of(Commit).to receive(:raw_deltas).and_call_original
+        it 'does not invoke :lfs_file_locks_validation' do
+          expect(subject).not_to receive(:lfs_file_locks_validation)
 
           subject.validate!
         end
       end
 
-      context 'when request store is active', :request_store do
-        it 'are cached for every commit' do
-          expect_any_instance_of(Commit).not_to receive(:raw_deltas)
+      context 'with LFS enabled' do
+        let(:owner) { create(:user) }
+        let!(:lock) { create(:lfs_file_lock, user: owner, project: project, path: 'README') }
 
-          subject.validate!
+        before do
+          allow(project).to receive(:lfs_enabled?).and_return(true)
         end
 
-        it 'are run for not cached commits' do
-          allow(project.repository).to receive(:new_commits).and_return(
-            project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', 'a5391128b0ef5d21df5dd23d98557f4ef12fae20')
-          )
-          change_access.instance_variable_set(:@commits, project.repository.new_commits)
+        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}")
+          end
+        end
 
-          expect(project.repository.new_commits.first).not_to receive(:raw_deltas).and_call_original
-          expect(project.repository.new_commits.last).to receive(:raw_deltas).and_call_original
+        context 'when change is sent by the author of the lock' do
+          let(:user) { owner }
 
-          subject.validate!
+          it "doesn't raise any error" do
+            expect { subject.validate! }.not_to raise_error
+          end
         end
       end
     end

spec/lib/gitlab/git/changed_path_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe Gitlab::Git::ChangedPath do
+  subject(:changed_path) { described_class.new(path: path, status: status) }
+
+  let(:path) { 'test_path' }
+
+  describe '#new_file?' do
+    subject(:new_file?) { changed_path.new_file? }
+
+    context 'when it is a new file' do
+      let(:status) { :ADDED }
+
+      it 'returns true' do
+        expect(new_file?).to eq(true)
+      end
+    end
+
+    context 'when it is not a new file' do
+      let(:status) { :MODIFIED }
+
+      it 'returns false' do
+        expect(new_file?).to eq(false)
+      end
+    end
+  end
+end

spec/lib/gitlab/git/repository_spec.rb

@@ -1191,25 +1191,25 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
     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")
+        Gitlab::Git::ChangedPath.new(status: :ADDED, path: "files/executables/ls"),
+        Gitlab::Git::ChangedPath.new(status: :ADDED, path: "files/executables/touch"),
+        Gitlab::Git::ChangedPath.new(status: :ADDED, path: "files/links/regex.rb"),
+        Gitlab::Git::ChangedPath.new(status: :ADDED, path: "files/links/ruby-style-guide.md"),
+        Gitlab::Git::ChangedPath.new(status: :ADDED, path: "files/links/touch"),
+        Gitlab::Git::ChangedPath.new(status: :MODIFIED, path: ".gitmodules"),
+        Gitlab::Git::ChangedPath.new(status: :ADDED, path: "deeper/nested/six"),
+        Gitlab::Git::ChangedPath.new(status: :ADDED, path: "nested/six")
       ]
     end
 
     let(:commit_2_files) do
-      [OpenStruct.new(status: :ADDED, path: "bin/executable")]
+      [Gitlab::Git::ChangedPath.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")
+        Gitlab::Git::ChangedPath.new(status: :MODIFIED, path: ".gitmodules"),
+        Gitlab::Git::ChangedPath.new(status: :ADDED, path: "gitlab-shell")
       ]
     end
 
@@ -1217,7 +1217,7 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper 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)
+      expect(collection.as_json).to eq((commit_1_files + commit_2_files + commit_3_files).as_json)
     end
 
     it 'returns no paths when SHAs are invalid' do
@@ -1231,7 +1231,7 @@ RSpec.describe Gitlab::Git::Repository, :seed_helper do
       collection = repository.find_changed_paths([nil, commit_1])
 
       expect(collection).to be_a(Enumerable)
-      expect(collection.to_a).to eq(commit_1_files)
+      expect(collection.as_json).to eq(commit_1_files.as_json)
     end
 
     it 'returns no paths when the commits are nil' do

spec/lib/gitlab/gitaly_client/commit_service_spec.rb

@@ -162,11 +162,9 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
         .with(request, kind_of(Hash)).and_return([changed_paths_response])
 
       returned_value = described_class.new(repository).find_changed_paths(commits)
+      mapped_expected_value = changed_paths_response.paths.map { |path| Gitlab::Git::ChangedPath.new(status: path.status, path: path.path) }
 
-      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)
+      expect(returned_value.as_json).to eq(mapped_expected_value.as_json)
     end
   end