Merge branch '330406_lfs_pointer_finder' into 'master'

Extract LfsPointersFinder from ExtractPath

See merge request gitlab-org/gitlab!66908

.rubocop_manual_todo.yml

@@ -1031,6 +1031,7 @@ Gitlab/NamespacedClass:
     - 'app/finders/joined_groups_finder.rb'
     - 'app/finders/keys_finder.rb'
     - 'app/finders/labels_finder.rb'
+    - 'app/finders/lfs_pointers_finder.rb'
     - 'app/finders/license_template_finder.rb'
     - 'app/finders/members_finder.rb'
     - 'app/finders/merge_request_target_project_finder.rb'

app/finders/lfs_pointers_finder.rb

+# frozen_string_literal: true
+
+class LfsPointersFinder
+  def initialize(repository, path)
+    @repository = repository
+    @path = path
+  end
+
+  def execute
+    return [] unless ref
+
+    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(current_blob&.id) if blob_ids.empty?
+
+    Gitlab::Git::Blob.batch_lfs_pointers(repository, blob_ids).map(&:id)
+  end
+
+  private
+
+  attr_reader :repository, :path
+
+  def ref
+    repository.root_ref
+  end
+
+  def tree
+    repository.tree(ref, path)
+  end
+
+  def current_blob
+    repository.blob_at(ref, path)
+  end
+end

ee/app/controllers/projects/path_locks_controller.rb

@@ -2,7 +2,6 @@
 
 class Projects::PathLocksController < Projects::ApplicationController
   include PathLocksHelper
-  include ExtractsPath
 
   # Authorize
   before_action :require_non_empty_project
@@ -10,8 +9,6 @@ class Projects::PathLocksController < Projects::ApplicationController
   before_action :authorize_push_code!, only: [:toggle]
 
   before_action :check_license
-  before_action :assign_ref_vars, only: :toggle
-  before_action :lfs_blob_ids, only: :toggle
 
   feature_category :source_code_management
 
@@ -21,7 +18,7 @@ class Projects::PathLocksController < Projects::ApplicationController
 
   # rubocop: disable CodeReuse/ActiveRecord
   def toggle
-    path_lock = @project.path_locks.find_by(path: params[:path])
+    path_lock = @project.path_locks.find_by(path: path)
 
     if path_lock
       unlock_file(path_lock)
@@ -62,13 +59,13 @@ class Projects::PathLocksController < Projects::ApplicationController
   end
 
   def lock_file
-    path_lock = PathLocks::LockService.new(project, current_user).execute(params[:path])
+    path_lock = PathLocks::LockService.new(project, current_user).execute(path)
 
     if path_lock.persisted? && sync_with_lfs?
       Lfs::LockFileService.new(
         project,
         current_user,
-        path: params[:path],
+        path: path,
         create_path_lock: false
       ).execute
     end
@@ -82,23 +79,21 @@ class Projects::PathLocksController < Projects::ApplicationController
     end
   end
 
-  # Override get_id from ExtractsPath.
-  # We don't support file locking per branch, that's why we use the root branch.
-  def get_id
-    id = project.repository.root_ref
-    id += "/#{params[:path]}" if params[:path].present?
-    id
-  end
-
   def lfs_file?
-    blob = project.repository.blob_at_branch(@ref, @path)
+    blob = repository.blob_at_branch(repository.root_ref, path)
 
     return false unless blob
 
-    @lfs_blob_ids.include?(blob.id)
+    lfs_blob_ids = LfsPointersFinder.new(repository, path).execute
+
+    lfs_blob_ids.include?(blob.id)
   end
 
   def sync_with_lfs?
     project.lfs_enabled? && lfs_file?
   end
+
+  def path
+    params[:path]
+  end
 end

ee/spec/controllers/projects/path_locks_controller_spec.rb

@@ -3,14 +3,19 @@
 require 'spec_helper'
 
 RSpec.describe Projects::PathLocksController do
-  let(:project) { create(:project, :repository, :public) }
-  let(:user)    { project.owner }
+  let_it_be(:project) { create(:project, :repository, :public) }
+  let_it_be(:user)    { project.owner }
+
   let(:file_path) { 'files/lfs/lfs_object.iso' }
+  let(:lfs_enabled) { true }
 
   before do
     sign_in(user)
 
     allow_any_instance_of(Repository).to receive(:root_ref).and_return('lfs')
+    allow_next_found_instance_of(Project) do |project|
+      allow(project).to receive(:lfs_enabled?) { lfs_enabled }
+    end
   end
 
   describe 'GET #index' do
@@ -34,9 +39,7 @@ RSpec.describe Projects::PathLocksController do
 
   describe 'POST #toggle' do
     context 'when LFS is enabled' do
-      before do
-        allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(true)
-      end
+      let(:lfs_enabled) { true }
 
       context 'when locking a file' do
         it 'locks the file' do
@@ -71,6 +74,21 @@ RSpec.describe Projects::PathLocksController do
         end
       end
 
+      context 'when file does not exist' do
+        let(:file_path) { 'unknown-file' }
+
+        it 'locks the file' do
+          toggle_lock(file_path)
+
+          expect(PathLock.count).to eq(1)
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+
+        it 'does not lock the file in LFS' do
+          expect { toggle_lock(file_path) }.not_to change { LfsFileLock.count }
+        end
+      end
+
       context 'when unlocking a file' do
         context 'with files' do
           before do
@@ -87,6 +105,24 @@ RSpec.describe Projects::PathLocksController do
             expect { toggle_lock(file_path) }.to change { LfsFileLock.count }.to(0)
           end
         end
+
+        context 'when file does not exist' do
+          let(:file_path) { 'unknown-file' }
+
+          before do
+            toggle_lock(file_path)
+          end
+
+          it 'unlocks the file' do
+            expect { toggle_lock(file_path) }.to change { PathLock.count }.to(0)
+
+            expect(response).to have_gitlab_http_status(:ok)
+          end
+
+          it 'does not unlock the file in LFS' do
+            expect { toggle_lock(file_path) }.not_to change { LfsFileLock.count }
+          end
+        end
       end
 
       context 'when unlocking a directory' do
@@ -109,6 +145,8 @@ RSpec.describe Projects::PathLocksController do
     end
 
     context 'when LFS is not enabled' do
+      let(:lfs_enabled) { false }
+
       it 'locks the file' do
         expect { toggle_lock(file_path) }.to change { PathLock.count }.to(1)
 

lib/extracts_path.rb

@@ -47,16 +47,6 @@ module ExtractsPath
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
-  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(@commit.id, @path)&.id) if blob_ids.empty? # rubocop:disable Gitlab/ModuleWithInstanceVariables
-
-    @lfs_blob_ids = Gitlab::Git::Blob.batch_lfs_pointers(repository_container.repository, blob_ids).map(&:id) # rubocop:disable Gitlab/ModuleWithInstanceVariables
-  end
-
   private
 
   # Override in controllers to determine which actions are subject to the redirect

spec/finders/lfs_pointers_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe LfsPointersFinder do
+  subject(:finder) { described_class.new(repository, path) }
+
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:repository) { project.repository }
+
+  let(:path) { nil }
+
+  describe '#execute' do
+    subject { finder.execute }
+
+    let(:expected_blob_id) { '0c304a93cb8430108629bbbcaa27db3343299bc0' }
+
+    context 'when path has no LFS files' do
+      it { is_expected.to eq([]) }
+    end
+
+    context 'when path points to LFS file' do
+      let(:path) { 'files/lfs/lfs_object.iso' }
+
+      it 'returns LFS blob ids' do
+        is_expected.to eq([expected_blob_id])
+      end
+    end
+
+    context 'when path points to directory with LFS files' do
+      let(:path) { 'files/lfs/' }
+
+      it 'returns LFS blob ids' do
+        is_expected.to eq([expected_blob_id])
+      end
+    end
+
+    context 'when repository is empty' do
+      let(:project) { create(:project, :empty_repo) }
+
+      it { is_expected.to eq([]) }
+    end
+  end
+end

spec/lib/extracts_path_spec.rb

@@ -213,20 +213,4 @@ RSpec.describe ExtractsPath do
       expect(extract_ref_without_atom('foo.atom')).to eq(nil)
     end
   end
-
-  describe '#lfs_blob_ids' do
-    let(:tag) { @project.repository.add_tag(@project.owner, 'my-annotated-tag', 'master', 'test tag') }
-    let(:ref) { tag.target }
-    let(:params) { { ref: ref, path: 'README.md' } }
-
-    before do
-      @project = create(:project, :repository)
-    end
-
-    it 'handles annotated tags' do
-      assign_ref_vars
-
-      expect(lfs_blob_ids).to eq([])
-    end
-  end
 end

spec/views/projects/tree/show.html.haml_spec.rb

@@ -15,7 +15,6 @@ RSpec.describe 'projects/tree/show' do
   before do
     assign(:project, project)
     assign(:repository, repository)
-    assign(:lfs_blob_ids, [])
 
     allow(view).to receive(:can?).and_return(true)
     allow(view).to receive(:can_collaborate_with_project?).and_return(true)