From 81331fa77e181d193f444c34443d6ef0c5b01ed6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 13 Dec 2017 22:58:10 -0500 Subject: Add some initial specs for Files::MultiService class --- spec/services/files/multi_service_spec.rb | 54 +++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 spec/services/files/multi_service_spec.rb diff --git a/spec/services/files/multi_service_spec.rb b/spec/services/files/multi_service_spec.rb new file mode 100644 index 00000000000..34457ee796a --- /dev/null +++ b/spec/services/files/multi_service_spec.rb @@ -0,0 +1,54 @@ +require "spec_helper" + +describe Files::MultiService do + subject { described_class.new(project, user, commit_params) } + + let(:project) { create(:project, :repository) } + let(:user) { create(:user) } + let(:branch_name) { project.default_branch } + let(:action) { 'update' } + + let(:actions) do + [ + { + action: action, + file_path: 'files/ruby/popen.rb', + content: 'New content' + } + ] + end + + let(:commit_params) do + { + commit_message: "Update File", + branch_name: branch_name, + start_branch: branch_name, + actions: actions + } + end + + before do + project.team << [user, :master] + end + + describe '#execute' do + context 'with a valid action' do + it 'returns a hash with the :success status ' do + results = subject.execute + + expect(results[:status]).to eq(:success) + end + end + + context 'with an invalid action' do + let(:action) { 'rename' } + + it 'returns a hash with the :error status ' do + results = subject.execute + + expect(results[:status]).to eq(:error) + expect(results[:message]).to match(/Unknown action/) + end + end + end +end -- cgit v1.2.1 From c210ddab9289e29fadc9a5a0462ffbe864af736c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Fri, 15 Dec 2017 23:24:12 -0500 Subject: Check if file has been modified for each action provided. When commiting multiple files we're now checking if any of those files has been modified by another commit and we're rejecting the new commit in this case. --- app/services/files/multi_service.rb | 12 ++++++ ...e-file-status-when-commiting-multiple-files.yml | 5 +++ doc/api/commits.md | 1 + spec/services/files/multi_service_spec.rb | 45 +++++++++++++++++++++- 4 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/15922-validate-file-status-when-commiting-multiple-files.yml diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb index bfacc462847..eb2773733b1 100644 --- a/app/services/files/multi_service.rb +++ b/app/services/files/multi_service.rb @@ -20,6 +20,7 @@ module Files params[:actions].each do |action| validate_action!(action) + validate_file_status(action) end end @@ -28,5 +29,16 @@ module Files raise_error("Unknown action '#{action[:action]}'") end end + + def validate_file_status(action) + return unless action[:last_commit_id] + + current_commit = Gitlab::Git::Commit.last_for_path( + @start_project.repository, @start_branch, action[:file_path]) + + if current_commit.sha != action[:last_commit_id] + raise_error("The file has changed since you started editing it: #{action[:file_path]}") + end + end end end diff --git a/changelogs/unreleased/15922-validate-file-status-when-commiting-multiple-files.yml b/changelogs/unreleased/15922-validate-file-status-when-commiting-multiple-files.yml new file mode 100644 index 00000000000..db2bd6e692b --- /dev/null +++ b/changelogs/unreleased/15922-validate-file-status-when-commiting-multiple-files.yml @@ -0,0 +1,5 @@ +--- +title: 'Validate file status when commiting multiple files' +merge_request: 15922 +author: +type: added diff --git a/doc/api/commits.md b/doc/api/commits.md index 5a4a8d888b3..7de08d230dd 100644 --- a/doc/api/commits.md +++ b/doc/api/commits.md @@ -84,6 +84,7 @@ POST /projects/:id/repository/commits | `previous_path` | string | no | Original full path to the file being moved. Ex. `lib/class1.rb` | | `content` | string | no | File content, required for all except `delete`. Optional for `move` | | `encoding` | string | no | `text` or `base64`. `text` is default. | +| `last_commit_id` | string | no | Last known file commit id | ```bash PAYLOAD=$(cat << 'JSON' diff --git a/spec/services/files/multi_service_spec.rb b/spec/services/files/multi_service_spec.rb index 34457ee796a..6e11f126f14 100644 --- a/spec/services/files/multi_service_spec.rb +++ b/spec/services/files/multi_service_spec.rb @@ -6,14 +6,20 @@ describe Files::MultiService do let(:project) { create(:project, :repository) } let(:user) { create(:user) } let(:branch_name) { project.default_branch } + let(:file_path) { 'files/ruby/popen.rb' } let(:action) { 'update' } + let!(:original_commit_id) do + Gitlab::Git::Commit.last_for_path(project.repository, branch_name, file_path).sha + end + let(:actions) do [ { action: action, - file_path: 'files/ruby/popen.rb', - content: 'New content' + file_path: file_path, + content: 'New content', + last_commit_id: original_commit_id } ] end @@ -50,5 +56,40 @@ describe Files::MultiService do expect(results[:message]).to match(/Unknown action/) end end + + describe 'Updating files' do + context 'when the file has been previously updated' do + before do + update_file(file_path) + end + + it 'rejects the commit' do + results = subject.execute + + expect(results[:status]).to eq(:error) + expect(results[:message]).to match(file_path) + end + end + + context 'when the file have not been modified' do + it 'accepts the commit' do + results = subject.execute + + expect(results[:status]).to eq(:success) + end + end + end + end + + def update_file(path) + params = { + file_path: path, + start_branch: branch_name, + branch_name: branch_name, + commit_message: 'Update file', + file_content: 'New content' + } + + Files::UpdateService.new(project, user, params).execute end end -- cgit v1.2.1 From c927e57466b6c705891f09c95f566259d8e1ec0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 20 Dec 2017 10:57:27 -0500 Subject: Updates from last code review: - Apply some refactoring for code reuse - Add file status validation for Files::DeleteService - Write additional specs --- app/services/files/base_service.rb | 14 +++++++ app/services/files/delete_service.rb | 10 +++++ app/services/files/multi_service.rb | 15 +++---- app/services/files/update_service.rb | 21 +--------- doc/api/commits.md | 2 +- doc/api/repository_files.md | 1 + spec/services/files/delete_service_spec.rb | 64 ++++++++++++++++++++++++++++++ spec/services/files/multi_service_spec.rb | 59 ++++++++++++++++++++++++--- 8 files changed, 153 insertions(+), 33 deletions(-) create mode 100644 spec/services/files/delete_service_spec.rb diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index 38231f66009..8d4b9f14780 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -1,11 +1,14 @@ module Files class BaseService < Commits::CreateService + FileChangedError = Class.new(StandardError) + def initialize(*args) super @author_email = params[:author_email] @author_name = params[:author_name] @commit_message = params[:commit_message] + @last_commit_sha = params[:last_commit_sha] @file_path = params[:file_path] @previous_path = params[:previous_path] @@ -13,5 +16,16 @@ module Files @file_content = params[:file_content] @file_content = Base64.decode64(@file_content) if params[:file_content_encoding] == 'base64' end + + def file_has_changed?(path, commit_id) + return false unless commit_id + + last_commit = Gitlab::Git::Commit + .last_for_path(@start_project.repository, @start_branch, path) + + return false unless last_commit + + last_commit.sha != commit_id + end end end diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb index 7952e5c95d4..32a57484d4e 100644 --- a/app/services/files/delete_service.rb +++ b/app/services/files/delete_service.rb @@ -11,5 +11,15 @@ module Files start_project: @start_project, start_branch_name: @start_branch) end + + private + + def validate! + super + + if file_has_changed?(@file_path, @last_commit_sha) + raise FileChangedError, "You are attempting to delete a file that has been previously updated." + end + end end end diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb index eb2773733b1..98a3e83c130 100644 --- a/app/services/files/multi_service.rb +++ b/app/services/files/multi_service.rb @@ -1,5 +1,7 @@ module Files class MultiService < Files::BaseService + UPDATE_FILE_ACTIONS = %w(update move delete).freeze + def create_commit! repository.multi_action( user: current_user, @@ -20,7 +22,7 @@ module Files params[:actions].each do |action| validate_action!(action) - validate_file_status(action) + validate_file_status!(action) end end @@ -30,14 +32,13 @@ module Files end end - def validate_file_status(action) - return unless action[:last_commit_id] + def validate_file_status!(action) + return unless UPDATE_FILE_ACTIONS.include?(action[:action]) - current_commit = Gitlab::Git::Commit.last_for_path( - @start_project.repository, @start_branch, action[:file_path]) + file_path = action[:previous_path] || action[:file_path] - if current_commit.sha != action[:last_commit_id] - raise_error("The file has changed since you started editing it: #{action[:file_path]}") + if file_has_changed?(file_path, action[:last_commit_id]) + raise_error("The file has changed since you started editing it: #{file_path}") end end end diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index bcca1386bed..1902d1cea72 100644 --- a/app/services/files/update_service.rb +++ b/app/services/files/update_service.rb @@ -1,13 +1,5 @@ module Files class UpdateService < Files::BaseService - FileChangedError = Class.new(StandardError) - - def initialize(*args) - super - - @last_commit_sha = params[:last_commit_sha] - end - def create_commit! repository.update_file(current_user, @file_path, @file_content, message: @commit_message, @@ -21,21 +13,10 @@ module Files private - def file_has_changed? - return false unless @last_commit_sha && last_commit - - @last_commit_sha != last_commit.sha - end - - def last_commit - @last_commit ||= Gitlab::Git::Commit - .last_for_path(@start_project.repository, @start_branch, @file_path) - end - def validate! super - if file_has_changed? + if file_has_changed?(@file_path, @last_commit_sha) raise FileChangedError, "You are attempting to update a file that has changed since you started editing it." end end diff --git a/doc/api/commits.md b/doc/api/commits.md index 7de08d230dd..c9b72d4a1dd 100644 --- a/doc/api/commits.md +++ b/doc/api/commits.md @@ -84,7 +84,7 @@ POST /projects/:id/repository/commits | `previous_path` | string | no | Original full path to the file being moved. Ex. `lib/class1.rb` | | `content` | string | no | File content, required for all except `delete`. Optional for `move` | | `encoding` | string | no | `text` or `base64`. `text` is default. | -| `last_commit_id` | string | no | Last known file commit id | +| `last_commit_id` | string | no | Last known file commit id. Will be only considered in update, move and delete actions. | ```bash PAYLOAD=$(cat << 'JSON' diff --git a/doc/api/repository_files.md b/doc/api/repository_files.md index c517a38a8ba..a1a0b1b756c 100644 --- a/doc/api/repository_files.md +++ b/doc/api/repository_files.md @@ -151,3 +151,4 @@ Parameters: - `author_email` (optional) - Specify the commit author's email address - `author_name` (optional) - Specify the commit author's name - `commit_message` (required) - Commit message +- `last_commit_id` (optional) - Last known file commit id diff --git a/spec/services/files/delete_service_spec.rb b/spec/services/files/delete_service_spec.rb new file mode 100644 index 00000000000..e9f8f0efe6b --- /dev/null +++ b/spec/services/files/delete_service_spec.rb @@ -0,0 +1,64 @@ +require "spec_helper" + +describe Files::DeleteService do + subject { described_class.new(project, user, commit_params) } + + let(:project) { create(:project, :repository) } + let(:user) { create(:user) } + let(:file_path) { 'files/ruby/popen.rb' } + let(:branch_name) { project.default_branch } + let(:last_commit_sha) { nil } + + let(:commit_params) do + { + file_path: file_path, + commit_message: "Delete File", + last_commit_sha: last_commit_sha, + start_project: project, + start_branch: project.default_branch, + branch_name: branch_name + } + end + + shared_examples 'successfully deletes the file' do + it 'returns a hash with the :success status' do + results = subject.execute + + expect(results[:status]).to match(:success) + end + + it 'deletes the file' do + subject.execute + + blob = project.repository.blob_at_branch(project.default_branch, file_path) + + expect(blob).to be_nil + end + end + + before do + project.team << [user, :master] + end + + describe "#execute" do + context "when the file's last commit sha does not match the supplied last_commit_sha" do + let(:last_commit_sha) { "foo" } + + it "returns a hash with the correct error message and a :error status " do + expect { subject.execute } + .to raise_error(Files::UpdateService::FileChangedError, + "You are attempting to delete a file that has been previously updated.") + end + end + + context "when the file's last commit sha does match the supplied last_commit_sha" do + let(:last_commit_sha) { Gitlab::Git::Commit.last_for_path(project.repository, project.default_branch, file_path).sha } + + it_behaves_like 'successfully deletes the file' + end + + context "when the last_commit_sha is not supplied" do + it_behaves_like 'successfully deletes the file' + end + end +end diff --git a/spec/services/files/multi_service_spec.rb b/spec/services/files/multi_service_spec.rb index 6e11f126f14..085a28d267f 100644 --- a/spec/services/files/multi_service_spec.rb +++ b/spec/services/files/multi_service_spec.rb @@ -6,18 +6,20 @@ describe Files::MultiService do let(:project) { create(:project, :repository) } let(:user) { create(:user) } let(:branch_name) { project.default_branch } - let(:file_path) { 'files/ruby/popen.rb' } + let(:original_file_path) { 'files/ruby/popen.rb' } + let(:new_file_path) { 'files/ruby/popen.rb' } let(:action) { 'update' } let!(:original_commit_id) do - Gitlab::Git::Commit.last_for_path(project.repository, branch_name, file_path).sha + Gitlab::Git::Commit.last_for_path(project.repository, branch_name, original_file_path).sha end let(:actions) do [ { action: action, - file_path: file_path, + file_path: new_file_path, + previous_path: original_file_path, content: 'New content', last_commit_id: original_commit_id } @@ -60,14 +62,14 @@ describe Files::MultiService do describe 'Updating files' do context 'when the file has been previously updated' do before do - update_file(file_path) + update_file(original_file_path) end it 'rejects the commit' do results = subject.execute expect(results[:status]).to eq(:error) - expect(results[:message]).to match(file_path) + expect(results[:message]).to match(new_file_path) end end @@ -79,6 +81,53 @@ describe Files::MultiService do end end end + + context 'when moving a file' do + let(:action) { 'move' } + let(:new_file_path) { 'files/ruby/new_popen.rb' } + + context 'when original file has been updated' do + before do + update_file(original_file_path) + end + + it 'rejects the commit' do + results = subject.execute + + expect(results[:status]).to eq(:error) + expect(results[:message]).to match(original_file_path) + end + end + + context 'when original file have not been updated' do + it 'moves the file' do + results = subject.execute + blob = project.repository.blob_at_branch(branch_name, new_file_path) + + expect(results[:status]).to eq(:success) + expect(blob).to be_present + end + end + end + + context 'when file status validation is skipped' do + let(:action) { 'create' } + let(:new_file_path) { 'files/ruby/new_file.rb' } + + it 'does not check the last commit' do + expect(Gitlab::Git::Commit).not_to receive(:last_for_path) + + subject.execute + end + + it 'creates the file' do + subject.execute + + blob = project.repository.blob_at_branch(branch_name, new_file_path) + + expect(blob).to be_present + end + end end def update_file(path) -- cgit v1.2.1