Merge branch '9490-store-designs-in-lfs' into 'master'

Support storing design blobs using LFS

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

ee/app/services/design_management/save_designs_service.rb

@@ -50,7 +50,7 @@ module DesignManagement
       {
         action: new_file?(design) ? :create : :update,
         file_path: design.full_path,
-        content: file.to_io
+        content: file_content(file, design.full_path)
       }
     end
 
@@ -98,6 +98,13 @@ module DesignManagement
       design.new_design? && existing_metadata.none? { |blob| blob.path == design.full_path }
     end
 
+    def file_content(file, full_path)
+      return file.to_io if Feature.disabled?(:store_designs_in_lfs)
+
+      transformer = Lfs::FileTransformer.new(project, target_branch)
+      transformer.new_file(full_path, file.to_io).content
+    end
+
     def existing_metadata
       @existing_metadata ||= begin
                                paths = updated_designs.map(&:full_path)

ee/changelogs/unreleased/9490-store-designs-in-lfs.yml

+---
+title: Allow design blobs to be stored in Git LFS
+merge_request: 13389
+author:
+type: added

ee/spec/controllers/projects/designs_controller_spec.rb

@@ -12,7 +12,7 @@ describe Projects::DesignsController do
   end
 
   describe 'GET #show' do
-    it 'serves the file using workhorse' do
+    subject do
       get(:show,
         params: {
           namespace_id: project.namespace,
@@ -20,11 +20,22 @@ describe Projects::DesignsController do
           id: design.id,
           ref: 'HEAD'
       })
+    end
+
+    it 'serves the file using workhorse' do
+      subject
 
       expect(response).to have_gitlab_http_status(200)
       expect(response.header['Content-Disposition']).to eq('inline')
       expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
       expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
     end
+
+    it_behaves_like 'a controller that can serve LFS files' do
+      let(:design) { create(:design, :with_lfs_file, issue: issue) }
+      let(:lfs_oid) { project.design_repository.blob_at('HEAD', design.full_path).lfs_oid }
+      let(:filename) { design.filename }
+      let(:filepath) { design.full_path }
+    end
   end
 end

ee/spec/factories/design_management/designs.rb

@@ -6,6 +6,14 @@ FactoryBot.define do
     project { issue.project }
     sequence(:filename) { |n| "homescreen-#{n}.jpg" }
 
+    trait :with_lfs_file do
+      with_file
+
+      transient do
+        file Gitlab::Git::LfsPointerFile.new('').pointer
+      end
+    end
+
     trait :with_file do
       transient do
         versions_count 1

ee/spec/services/design_management/save_designs_service_spec.rb

@@ -131,6 +131,36 @@ describe DesignManagement::SaveDesignsService do
         end
       end
 
+      context 'when LFS is enabled' do
+        before do
+          allow(project).to receive(:lfs_enabled?).and_return(true)
+        end
+
+        context 'and the `store_designs_in_lfs` feature is enabled' do
+          before do
+            stub_feature_flags(store_designs_in_lfs: true)
+
+            expect_next_instance_of(Lfs::FileTransformer) do |transformer|
+              expect(transformer).to receive(:lfs_file?).and_return(true)
+            end
+          end
+
+          it 'saves the design to LFS' do
+            expect { service.execute }.to change { LfsObject.count }.by(1)
+          end
+        end
+
+        context 'and the `store_designs_in_lfs` feature is not enabled' do
+          before do
+            stub_feature_flags(store_designs_in_lfs: false)
+          end
+
+          it 'does not save the design to LFS' do
+            expect { service.execute }.not_to change { LfsObject.count }
+          end
+        end
+      end
+
       context 'when the user is not allowed to upload designs' do
         let(:user) { create(:user) }
 

spec/controllers/projects/avatars_controller_spec.rb

@@ -39,7 +39,7 @@ describe Projects::AvatarsController do
       end
 
       context 'when the avatar is stored in lfs' do
-        it_behaves_like 'repository lfs file load' do
+        it_behaves_like 'a controller that can serve LFS files' do
           let(:filename) { 'lfs_object.iso' }
           let(:filepath) { "files/lfs/#{filename}" }
         end

spec/controllers/projects/raw_controller_spec.rb

@@ -42,7 +42,7 @@ describe Projects::RawController do
       end
     end
 
-    it_behaves_like 'repository lfs file load' do
+    it_behaves_like 'a controller that can serve LFS files' do
       let(:filename) { 'lfs_object.iso' }
       let(:filepath) { "be93687/files/lfs/#{filename}" }
     end

spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb

@@ -9,87 +9,87 @@
 # - `filepath`: path of the file (contains filename)
 # - `subject`: the request to be made to the controller. Example:
 # subject { get :show, namespace_id: project.namespace, project_id: project }
-shared_examples 'repository lfs file load' do
-  context 'when file is stored in lfs' do
-    let(:lfs_oid) { '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897' }
-    let(:lfs_size) { '1575078' }
-    let!(:lfs_object) { create(:lfs_object, oid: lfs_oid, size: lfs_size) }
+shared_examples 'a controller that can serve LFS files' do
+  let(:lfs_oid) { '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897' }
+  let(:lfs_size) { '1575078' }
+  let!(:lfs_object) { create(:lfs_object, oid: lfs_oid, size: lfs_size) }
+
+  context 'when lfs is enabled' do
+    before do
+      allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(true)
+    end
 
-    context 'when lfs is enabled' do
+    context 'when project has access' do
       before do
-        allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(true)
+        project.lfs_objects << lfs_object
+        allow_any_instance_of(LfsObjectUploader).to receive(:exists?).and_return(true)
+        allow(controller).to receive(:send_file) { controller.head :ok }
       end
 
-      context 'when project has access' do
-        before do
-          project.lfs_objects << lfs_object
-          allow_any_instance_of(LfsObjectUploader).to receive(:exists?).and_return(true)
-          allow(controller).to receive(:send_file) { controller.head :ok }
-        end
+      it 'serves the file' do
+        lfs_uploader = LfsObjectUploader.new(lfs_object)
 
-        it 'serves the file' do
-          # Notice the filename= is omitted from the disposition; this is because
-          # Rails 5 will append this header in send_file
-          expect(controller).to receive(:send_file)
-                            .with(
-                              "#{LfsObjectUploader.root}/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897",
-                              filename: filename,
-                              disposition: %Q(attachment; filename*=UTF-8''#{filename}))
+        # Notice the filename= is omitted from the disposition; this is because
+        # Rails 5 will append this header in send_file
+        expect(controller).to receive(:send_file)
+                          .with(
+                            File.join(lfs_uploader.root, lfs_uploader.store_dir, lfs_uploader.filename),
+                            filename: filename,
+                            disposition: %Q(attachment; filename*=UTF-8''#{filename}))
 
-          subject
+        subject
 
-          expect(response).to have_gitlab_http_status(200)
-        end
+        expect(response).to have_gitlab_http_status(200)
+      end
 
-        context 'and lfs uses object storage' do
-          let(:lfs_object) { create(:lfs_object, :with_file, oid: lfs_oid, size: lfs_size) }
+      context 'and lfs uses object storage' do
+        let(:lfs_object) { create(:lfs_object, :with_file, oid: lfs_oid, size: lfs_size) }
 
-          before do
-            stub_lfs_object_storage
-            lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE)
-          end
+        before do
+          stub_lfs_object_storage
+          lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE)
+        end
 
-          it 'responds with redirect to file' do
-            subject
+        it 'responds with redirect to file' do
+          subject
 
-            expect(response).to have_gitlab_http_status(302)
-            expect(response.location).to include(lfs_object.reload.file.path)
-          end
+          expect(response).to have_gitlab_http_status(302)
+          expect(response.location).to include(lfs_object.reload.file.path)
+        end
 
-          it 'sets content disposition' do
-            subject
+        it 'sets content disposition' do
+          subject
 
-            file_uri = URI.parse(response.location)
-            params = CGI.parse(file_uri.query)
+          file_uri = URI.parse(response.location)
+          params = CGI.parse(file_uri.query)
 
-            expect(params["response-content-disposition"].first).to eq(%q(attachment; filename="lfs_object.iso"; filename*=UTF-8''lfs_object.iso))
-          end
+          expect(params["response-content-disposition"].first).to eq(%Q(attachment; filename="#{filename}"; filename*=UTF-8''#{filename}))
         end
       end
+    end
 
-      context 'when project does not have access' do
-        it 'does not serve the file' do
-          subject
+    context 'when project does not have access' do
+      it 'does not serve the file' do
+        subject
 
-          expect(response).to have_gitlab_http_status(404)
-        end
+        expect(response).to have_gitlab_http_status(404)
       end
     end
+  end
 
-    context 'when lfs is not enabled' do
-      before do
-        allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(false)
-      end
+  context 'when lfs is not enabled' do
+    before do
+      allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(false)
+    end
 
-      it 'delivers ASCII file' do
-        subject
+    it 'delivers ASCII file' do
+      subject
 
-        expect(response).to have_gitlab_http_status(200)
-        expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
-        expect(response.header['Content-Disposition'])
-            .to eq('inline')
-        expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
-      end
+      expect(response).to have_gitlab_http_status(200)
+      expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
+      expect(response.header['Content-Disposition'])
+          .to eq('inline')
+      expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
     end
   end
 end