Fix error when locking/unlocking a directory through the File Locking feature

When the Project has LFS enabled, we try to lock the file through the
LFS locking feature too, given we can only lock files through LFS, this
operation was raising an error.

Additionally a couple of errors has been fixed:

- The previous implementation was only looking for LFS files in the root
  of the project given `@path` was `nil`
- We were trying to create the PathLock record twice, it was silently
  failing due to the usage of `create` instead of `create!`

ee/app/controllers/projects/path_locks_controller.rb

@@ -58,7 +58,12 @@ class Projects::PathLocksController < Projects::ApplicationController
     path_lock = PathLocks::LockService.new(project, current_user).execute(params[:path])
 
     if path_lock.persisted? && sync_with_lfs?
-      Lfs::LockFileService.new(project, current_user, path: params[:path]).execute
+      Lfs::LockFileService.new(
+        project,
+        current_user,
+        path: params[:path],
+        create_path_lock: false
+      ).execute
     end
   end
 
@@ -70,13 +75,18 @@ class Projects::PathLocksController < Projects::ApplicationController
     end
   end
 
-  # Override get_id from ExtractsPath in this case is just the root of the default branch.
+  # Override get_id from ExtractsPath.
+  # We don't support file locking per branch, that's why we use the root branch.
   def get_id
-    @ref ||= project.repository.root_ref
+    id = project.repository.root_ref
+    id += "/#{params[:path]}" if params[:path].present?
+    id
   end
 
   def lfs_file?
-    blob = project.repository.blob_at_branch(get_id, params[:path])
+    blob = project.repository.blob_at_branch(@ref, @path)
+
+    return false unless blob
 
     @lfs_blob_ids.include?(blob.id)
   end

ee/app/services/ee/lfs/lock_file_service.rb

@@ -4,12 +4,20 @@ module EE
       def execute
         result = super
 
-        if (result[:status] == :success) && project.feature_available?(:file_locks)
+        if (result[:status] == :success) &&
+            create_path_lock? &&
+            project.feature_available?(:file_locks)
           PathLocks::LockService.new(project, current_user).execute(result[:lock].path)
         end
 
         result
       end
+
+      private
+
+      def create_path_lock?
+        params[:create_path_lock] != false
+      end
     end
   end
 end

ee/changelogs/unreleased/rd-6192-fix-error-when-trying-to-lock-or-unlock-a-directory-through-lfs.yml

+---
+title: Fix error when locking/unlocking directories
+merge_request: 5862
+author:
+type: fixed

ee/spec/requests/projects/path_locks_controller_spec.rb

@@ -4,69 +4,108 @@ describe Projects::PathLocksController, type: :request do
   let(:project) { create(:project, :repository) }
   let(:user)    { project.owner }
   let(:viewer)  { user }
+  let(:file_path) { 'files/lfs/lfs_object.iso' }
+  let(:blob_object) { project.repository.blob_at_branch('lfs', file_path) }
+  let!(:lfs_object) { create(:lfs_object, oid: blob_object.lfs_oid) }
+  let!(:lfs_objects_project) { create(:lfs_objects_project, project: project, lfs_object: lfs_object) }
 
   before do
     login_as(viewer)
+
+    allow_any_instance_of(Repository).to receive(:root_ref).and_return('lfs')
   end
 
   describe 'POST #toggle' do
-    context 'when locking a file' do
-      context 'when LFS is enabled' do
-        before do
-          allow_any_instance_of(Projects::PathLocksController).to receive(:sync_with_lfs?).and_return(true)
-        end
+    context 'when LFS is enabled' do
+      before do
+        allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(true)
+      end
 
+      context 'when locking a file' do
         it 'locks the file' do
-          expect { toggle_lock('README.md') }.to change { PathLock.count }.to(1)
+          toggle_lock(file_path)
 
+          expect(PathLock.count).to eq(1)
           expect(response).to have_gitlab_http_status(200)
         end
 
         it "locks the file in LFS" do
-          expect { toggle_lock('README.md') }.to change { LfsFileLock.count }.to(1)
+          expect { toggle_lock(file_path) }.to change { LfsFileLock.count }.to(1)
+        end
+
+        it "tries to create the PathLock only once" do
+          expect(PathLocks::LockService).to receive(:new).once.and_return(double.as_null_object)
+
+          toggle_lock(file_path)
         end
       end
 
-      context 'when LFS is not enabled' do
-        it 'locks the file' do
-          expect { toggle_lock('README.md') }.to change { PathLock.count }.to(1)
+      context 'when locking a directory' do
+        it 'locks the directory' do
+          expect { toggle_lock('bar/') }.to change { PathLock.count }.to(1)
 
           expect(response).to have_gitlab_http_status(200)
         end
 
-        it "doesn't lock the file in LFS" do
-          expect { toggle_lock('README.md') }.not_to change { LfsFileLock.count }
+        it 'does not locks the directory through LFS' do
+          expect { toggle_lock('bar/') }.not_to change { LfsFileLock.count }
+
+          expect(response).to have_gitlab_http_status(200)
         end
       end
-    end
 
-    context 'when unlocking a file' do
-      context 'when LFS is enabled' do
-        before do
-          allow_any_instance_of(Projects::PathLocksController).to receive(:sync_with_lfs?).and_return(true)
+      context 'when unlocking a file' do
+        context 'with files' do
+          before do
+            toggle_lock(file_path)
+          end
 
-          toggle_lock('README.md')
+          it 'unlocks the file' do
+            expect { toggle_lock(file_path) }.to change { PathLock.count }.to(0)
+
+            expect(response).to have_gitlab_http_status(200)
+          end
+
+          it "unlocks the file in LFS" do
+            expect { toggle_lock(file_path) }.to change { LfsFileLock.count }.to(0)
+          end
         end
+      end
 
-        it 'unlocks the file' do
-          expect { toggle_lock('README.md') }.to change { PathLock.count }.to(0)
+      context 'when unlocking a directory' do
+        before do
+          toggle_lock('bar')
+        end
+
+        it 'unlocks the directory' do
+          expect { toggle_lock('bar') }.to change { PathLock.count }.to(0)
 
           expect(response).to have_gitlab_http_status(200)
         end
 
-        it "unlocks the file in LFS" do
-          expect { toggle_lock('README.md') }.to change { LfsFileLock.count }.to(0)
+        it 'does not call the LFS unlock service' do
+          expect(Lfs::UnlockFileService).not_to receive(:new)
+
+          toggle_lock('bar')
         end
       end
     end
 
     context 'when LFS is not enabled' do
-      before do
-        toggle_lock('README.md')
+      it 'locks the file' do
+        expect { toggle_lock(file_path) }.to change { PathLock.count }.to(1)
+
+        expect(response).to have_gitlab_http_status(200)
+      end
+
+      it "doesn't lock the file in LFS" do
+        expect { toggle_lock(file_path) }.not_to change { LfsFileLock.count }
       end
 
       it 'unlocks the file' do
-        expect { toggle_lock('README.md') }.to change { PathLock.count }.to(0)
+        toggle_lock(file_path)
+
+        expect { toggle_lock(file_path) }.to change { PathLock.count }.to(0)
 
         expect(response).to have_gitlab_http_status(200)
       end

lib/extracts_path.rb

@@ -139,6 +139,11 @@ module ExtractsPath
 
   def lfs_blob_ids
     blob_ids = tree.blobs.map(&:id)
+
+    # When current endpoint is a Blob then `tree.blobs` will be empty, it means we need to analyze
+    # the current Blob in order to determine if it's a LFS object
+    blob_ids = Array.wrap(@repo.blob_at(@ref, @path)&.id) if blob_ids.empty? # rubocop:disable Gitlab/ModuleWithInstanceVariables
+
     @lfs_blob_ids = Gitlab::Git::Blob.batch_lfs_pointers(@project.repository, blob_ids).map(&:id) # rubocop:disable Gitlab/ModuleWithInstanceVariables
   end