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 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'app') 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 -- 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 +-------------------- 4 files changed, 33 insertions(+), 27 deletions(-) (limited to 'app') 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 -- cgit v1.2.1