Merge branch...

Merge branch 'rd-6192-fix-error-when-trying-to-lock-or-unlock-a-directory-through-lfs' into 'master'

Fix error when trying to lock/unlock directories through LFS

Closes #6192

See merge request gitlab-org/gitlab-ee!5862

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