From 1baac9211238f60d2d2a50cccd0bea6979bfa6ba Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Thu, 11 Jan 2018 23:12:34 +0000 Subject: [PATCH] Multi-file upload and Commit API obey LFS filters Updates Files::MultiService for the commits API which is in turn used by the multi-file upload web UI Ensures that files which should be in LFS are transformed into LFS pointers Uses Lfs::Transformer which then links LfsObjectProjects on success --- app/services/files/create_service.rb | 5 +- app/services/files/multi_service.rb | 25 ++++- app/services/lfs/file_transformer.rb | 35 +++++- .../unreleased/jej-commit-api-tracks-lfs.yml | 5 + spec/services/files/multi_service_spec.rb | 34 +++++- spec/services/lfs/file_transformer_spec.rb | 100 ++++++++++++++++++ 6 files changed, 193 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/jej-commit-api-tracks-lfs.yml create mode 100644 spec/services/lfs/file_transformer_spec.rb diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index 675b05e8fc4..b8ae03842a3 100644 --- a/app/services/files/create_service.rb +++ b/app/services/files/create_service.rb @@ -1,9 +1,8 @@ module Files class CreateService < Files::BaseService def create_commit! - handler = Lfs::FileTransformer.new(project, @branch_name) - - handler.new_file(@file_path, @file_content) do |content_or_lfs_pointer| + Lfs::FileTransformer.link_lfs_objects(project, @branch_name) do |transformer| + content_or_lfs_pointer = transformer.new_file(@file_path, @file_content) create_transformed_commit(content_or_lfs_pointer) end end diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb index a03c59f569d..bd88c46e4af 100644 --- a/app/services/files/multi_service.rb +++ b/app/services/files/multi_service.rb @@ -3,11 +3,32 @@ module Files UPDATE_FILE_ACTIONS = %w(update move delete).freeze def create_commit! + Lfs::FileTransformer.link_lfs_objects(project, @branch_name) do |transformer| + actions = actions_after_lfs_transformation(transformer, params[:actions]) + + commit_actions!(actions) + end + end + + private + + def actions_after_lfs_transformation(transformer, actions) + actions.map do |action| + if action[:action] == 'create' + content = transformer.new_file(action[:file_path], action[:content]) + action[:content] = content + end + + action + end + end + + def commit_actions!(actions) repository.multi_action( current_user, message: @commit_message, branch_name: @branch_name, - actions: params[:actions], + actions: actions, author_email: @author_email, author_name: @author_name, start_project: @start_project, @@ -17,8 +38,6 @@ module Files raise_error(e) end - private - def validate! super diff --git a/app/services/lfs/file_transformer.rb b/app/services/lfs/file_transformer.rb index 030f2c6aeba..7c93659b60d 100644 --- a/app/services/lfs/file_transformer.rb +++ b/app/services/lfs/file_transformer.rb @@ -1,4 +1,16 @@ module Lfs + # Usage: Open a `.link_lfs_objects` block, call `new_file` on the yielded object + # as many times as needed. LfsObjectProject links will be saved if the + # return value of the block is truthy. + # + # Calling `new_file` will check the path to see if it should be in LFS, + # save and LFS pointer of needed and return its content to be saved in + # a commit. If the file isn't LFS the untransformed content is returned. + # + # Lfs::FileTransformer.link_lfs_objects(project, @branch_name) do |transformer| + # content_or_lfs_pointer = transformer.new_file(file_path, file_content) + # create_transformed_commit(content_or_lfs_pointer) + # end class FileTransformer attr_reader :project, :branch_name @@ -9,26 +21,41 @@ module Lfs @branch_name = branch_name end + def self.link_lfs_objects(project, branch_name) + transformer = new(project, branch_name) + result = yield(transformer) + transformer.after_transform! if result + + result + end + def new_file(file_path, file_content) if project.lfs_enabled? && lfs_file?(file_path) lfs_pointer_file = Gitlab::Git::LfsPointerFile.new(file_content) lfs_object = create_lfs_object!(lfs_pointer_file, file_content) - content = lfs_pointer_file.pointer - success = yield(content) + on_success_actions << -> { link_lfs_object!(lfs_object) } - link_lfs_object!(lfs_object) if success + lfs_pointer_file.pointer else - yield(file_content) + file_content end end + def after_transform! + on_success_actions.map(&:call) + end + private def lfs_file?(file_path) repository.attributes_at(branch_name, file_path)['filter'] == 'lfs' end + def on_success_actions + @on_success_actions ||= [] + end + def create_lfs_object!(lfs_pointer_file, file_content) LfsObject.find_or_create_by(oid: lfs_pointer_file.sha256, size: lfs_pointer_file.size) do |lfs_object| lfs_object.file = CarrierWaveStringFile.new(file_content) diff --git a/changelogs/unreleased/jej-commit-api-tracks-lfs.yml b/changelogs/unreleased/jej-commit-api-tracks-lfs.yml new file mode 100644 index 00000000000..8284abf9f28 --- /dev/null +++ b/changelogs/unreleased/jej-commit-api-tracks-lfs.yml @@ -0,0 +1,5 @@ +--- +title: Create commit API and Web IDE obey LFS filters +merge_request: 16718 +author: +type: fixed diff --git a/spec/services/files/multi_service_spec.rb b/spec/services/files/multi_service_spec.rb index b9971776b33..7b47df32b43 100644 --- a/spec/services/files/multi_service_spec.rb +++ b/spec/services/files/multi_service_spec.rb @@ -4,10 +4,12 @@ describe Files::MultiService do subject { described_class.new(project, user, commit_params) } let(:project) { create(:project, :repository) } + let(:repository) { project.repository } let(:user) { create(:user) } let(:branch_name) { project.default_branch } let(:original_file_path) { 'files/ruby/popen.rb' } let(:new_file_path) { 'files/ruby/popen.rb' } + let(:file_content) { 'New content' } let(:action) { 'update' } let!(:original_commit_id) do @@ -20,7 +22,7 @@ describe Files::MultiService do action: action, file_path: new_file_path, previous_path: original_file_path, - content: 'New content', + content: file_content, last_commit_id: original_commit_id } ] @@ -110,6 +112,36 @@ describe Files::MultiService do end end + context 'when creating a file matching an LFS filter' do + let(:action) { 'create' } + let(:branch_name) { 'lfs' } + let(:new_file_path) { 'test_file.lfs' } + + before do + allow(project).to receive(:lfs_enabled?).and_return(true) + end + + it 'creates an LFS pointer' do + subject.execute + + blob = repository.blob_at('lfs', new_file_path) + + expect(blob.data).to start_with('version https://git-lfs.github.com/spec/v1') + end + + it "creates an LfsObject with the file's content" do + subject.execute + + expect(LfsObject.last.file.read).to eq file_content + end + + it 'links the LfsObject to the project' do + expect do + subject.execute + end.to change { project.lfs_objects.count }.by(1) + end + end + context 'when file status validation is skipped' do let(:action) { 'create' } let(:new_file_path) { 'files/ruby/new_file.rb' } diff --git a/spec/services/lfs/file_transformer_spec.rb b/spec/services/lfs/file_transformer_spec.rb new file mode 100644 index 00000000000..f469f5e76dd --- /dev/null +++ b/spec/services/lfs/file_transformer_spec.rb @@ -0,0 +1,100 @@ +require "spec_helper" + +describe Lfs::FileTransformer do + let(:project) { create(:project, :repository) } + let(:repository) { project.repository } + let(:file_content) { 'Test file content' } + let(:branch_name) { 'lfs' } + let(:file_path) { 'test_file.lfs' } + + subject { described_class.new(project, branch_name) } + + describe '#new_file' do + context 'with lfs disabled' do + it 'skips gitattributes check' do + expect(repository.raw).not_to receive(:blob_at) + + subject.new_file(file_path, file_content) + end + end + + context 'with lfs enabled' do + before do + allow(project).to receive(:lfs_enabled?).and_return(true) + end + + it 'reuses cached gitattributes' do + second_file = 'another_file.lfs' + + expect(repository.raw).to receive(:blob_at).with(branch_name, '.gitattributes').once + + subject.new_file(file_path, file_content) + subject.new_file(second_file, file_content) + end + + it "creates an LfsObject with the file's content" do + subject.new_file(file_path, file_content) + + expect(LfsObject.last.file.read).to eq file_content + end + + it 'creates an LFS pointer' do + new_content = subject.new_file(file_path, file_content) + + expect(new_content).to start_with('version https://git-lfs.github.com/spec/v1') + end + + context "when doesn't use LFS" do + let(:file_path) { 'other.filetype' } + + it "doesn't create LFS pointers" do + new_content = subject.new_file(file_path, file_content) + + expect(new_content).not_to start_with('version https://git-lfs.github.com/spec/v1') + expect(new_content).to eq(file_content) + end + end + + it 'sets up after_transform! to link LfsObjects to project' do + subject.new_file(file_path, file_content) + + expect { subject.after_transform! }.to change { project.lfs_objects.count }.by(1) + end + end + end + + describe '.link_lfs_objects' do + context 'with lfs enabled' do + before do + allow(project).to receive(:lfs_enabled?).and_return(true) + end + + context 'when given a block' do + it 'links LfsObject to the project automatically' do + expect do + described_class.link_lfs_objects(project, branch_name) do |t| + t.new_file(file_path, file_content) + end + end.to change { project.lfs_objects.count }.by(1) + end + + it 'skips linking LfsObjects if the block returns falsey' do + expect do + described_class.link_lfs_objects(project, branch_name) do |t| + t.new_file(file_path, file_content) + false + end + end.not_to change { project.lfs_objects.count } + end + + it 'returns the result of the block' do + result = described_class.link_lfs_objects(project, branch_name) do |t| + :dummy_commit + end + + expect(result).to eq :dummy_commit + end + end + end + end +end