Address feedback from last code review

* N+1 queries were fixed for LFS File Locking operations
* Small update for docs

changelogs/unreleased-ee/1830-implement-lfs-file-locking.yml → changelogs/unreleased-ee/1830-integrate-file-locking-with-lfs-file-locking.yml

 ---
-title: Add support for LFS File Locking API
+title: Integrate current File Locking feature with LFS File Locking
 merge_request: 4091
 author:
 type: added

changelogs/unreleased/35856-implement-file-locking-api.yml

+---
+title: Backport of LFS File Locking API
+merge_request: 16935
+author:
+type: added

doc/workflow/lfs/manage_large_binaries_with_git_lfs.md

@@ -100,8 +100,9 @@ created or updated with the following content:
 *.png filter=lfs diff=lfs merge=lfs -text lockable
 ```
 
-You can also register a file type as lockable without using LFS, in
-order to do that you can edit the `.gitattributes` file manually:
+You can also register a file type as lockable without using LFS
+(In order to be able to lock/unlock a file you need a remote server that implements the  LFS File Locking API),
+in order to do that you can edit the `.gitattributes` file manually:
 
 ```bash
 *.pdf lockable

ee/lib/ee/gitlab/checks/change_access.rb

@@ -59,11 +59,8 @@ module EE
           # if newrev is blank, the branch was deleted
           return if deletion? || !(commit_validation || validate_path_locks?)
 
-          # n+1: https://gitlab.com/gitlab-org/gitlab-ee/issues/3593
-          ::Gitlab::GitalyClient.allow_n_plus_1_calls do
-            commits.each do |commit|
-              push_rule_commit_check(commit)
-            end
+          commits.each do |commit|
+            push_rule_commit_check(commit)
           end
         rescue ::PushRule::MatchError => e
           raise ::Gitlab::GitAccess::UnauthorizedError, e.message

lib/gitlab/checks/change_access.rb

@@ -120,21 +120,22 @@ module Gitlab
         end
       end
 
-      def commit_check
-        @commit_check ||= Gitlab::Checks::CommitCheck.new(project, user_access.user, newrev, oldrev)
-      end
-
       def commits_check
         return if deletion? || newrev.nil?
 
-        commits.each do |commit|
-          commit_check.validate(commit, validations_for_commit(commit))
+        # n+1: https://gitlab.com/gitlab-org/gitlab-ee/issues/3593
+        ::Gitlab::GitalyClient.allow_n_plus_1_calls do
+          commits.each do |commit|
+            commit_check.validate(commit, validations_for_commit(commit))
+          end
         end
+
+        commit_check.validate_file_paths
       end
 
       # Method overwritten in EE to inject custom validations
       def validations_for_commit(_)
-        commit_check.validations
+        []
       end
 
       private
@@ -171,6 +172,10 @@ module Gitlab
         end
       end
 
+      def commit_check
+        @commit_check ||= Gitlab::Checks::CommitCheck.new(project, user_access.user, newrev, oldrev)
+      end
+
       def commits
         project.repository.new_commits(newrev)
       end

lib/gitlab/checks/commit_check.rb

 module Gitlab
   module Checks
     class CommitCheck
+      include Gitlab::Utils::StrongMemoize
+
       attr_reader :project, :user, :newrev, :oldrev
 
       def initialize(project, user, newrev, oldrev)
@@ -8,43 +10,52 @@ module Gitlab
         @user = user
         @newrev = user
         @oldrev = user
+        @file_paths = []
       end
 
       def validate(commit, validations)
-        return if validations.empty?
+        return if validations.empty? && path_validations.empty?
 
         commit.raw_deltas.each do |diff|
+          @file_paths << (diff.new_path || diff.old_path)
+
           validations.each do |validation|
             if error = validation.call(diff)
               raise ::Gitlab::GitAccess::UnauthorizedError, error
             end
           end
         end
-
-        nil
       end
 
-      def validations
-        validate_lfs_file_locks? ? [lfs_file_locks_validation] : []
+      def validate_file_paths
+        path_validations.each do |validation|
+          if error = validation.call(@file_paths)
+            raise ::Gitlab::GitAccess::UnauthorizedError, error
+          end
+        end
       end
 
       private
 
       def validate_lfs_file_locks?
-        project.lfs_enabled? && project.lfs_file_locks.any? && newrev && oldrev
+        strong_memoize(:validate_lfs_file_locks) do
+          project.lfs_enabled? && project.lfs_file_locks.any? && newrev && oldrev
+        end
       end
 
       def lfs_file_locks_validation
-        lambda do |diff|
-          path = diff.new_path || diff.old_path
-
-          lfs_lock = project.lfs_file_locks.find_by(path: path)
+        lambda do |paths|
+          lfs_lock = project.lfs_file_locks.where(path: paths).where.not(user_id: user.id).first
 
-          if lfs_lock && lfs_lock.user != user
+          if lfs_lock
             return "The path '#{lfs_lock.path}' is locked in Git LFS by #{lfs_lock.user.name}"
           end
         end
       end
+
+      def path_validations
+        validate_lfs_file_locks? ? [lfs_file_locks_validation] : []
+      end
     end
   end
 end

spec/services/lfs/lock_file_service_spec.rb

@@ -57,6 +57,26 @@ describe Lfs::LockFileService do
           expect(subject.execute[:status]).to eq(:error)
         end
       end
+
+      context 'when File Locking is available' do
+        before do
+          stub_licensed_features(file_locks: true)
+        end
+
+        it 'creates the Path Lock' do
+          expect { subject.execute }.to change { PathLock.count }.to(1)
+        end
+      end
+
+      context 'when File Locking is not available' do
+        before do
+          stub_licensed_features(file_locks: false)
+        end
+
+        it 'creates the Path Lock' do
+          expect { subject.execute }.not_to change { PathLock.count }
+        end
+      end
     end
   end
 end

spec/services/lfs/unlock_file_service_spec.rb

@@ -100,6 +100,34 @@ describe Lfs::UnlockFileService do
           end
         end
       end
+
+      describe 'File Locking integraction' do
+        let(:params) { { id: lock.id } }
+        let(:current_user) { lock_author }
+
+        before do
+          project.add_developer(lock_author)
+          project.path_locks.create(path: lock.path, user: lock_author)
+        end
+
+        context 'when File Locking is available' do
+          it 'deletes the Path Lock' do
+            expect { subject.execute }.to change { PathLock.count }.to(0)
+          end
+        end
+
+        context 'when File Locking is not available' do
+          before do
+            stub_licensed_features(file_locks: false)
+          end
+
+          # For some reason RSpec is reseting the mock and
+          # License.feature_available?(:file_locks) returns true when the spec runs.
+          xit 'does not delete the Path Lock' do
+            expect { subject.execute }.not_to change { PathLock.count }
+          end
+        end
+      end
     end
   end
 end