Commit c210ddab authored by Rubén Dávila's avatar Rubén Dávila

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.
parent 81331fa7
...@@ -20,6 +20,7 @@ module Files ...@@ -20,6 +20,7 @@ module Files
params[:actions].each do |action| params[:actions].each do |action|
validate_action!(action) validate_action!(action)
validate_file_status(action)
end end
end end
...@@ -28,5 +29,16 @@ module Files ...@@ -28,5 +29,16 @@ module Files
raise_error("Unknown action '#{action[:action]}'") raise_error("Unknown action '#{action[:action]}'")
end end
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
end end
---
title: 'Validate file status when commiting multiple files'
merge_request: 15922
author:
type: added
...@@ -84,6 +84,7 @@ POST /projects/:id/repository/commits ...@@ -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` | | `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` | | `content` | string | no | File content, required for all except `delete`. Optional for `move` |
| `encoding` | string | no | `text` or `base64`. `text` is default. | | `encoding` | string | no | `text` or `base64`. `text` is default. |
| `last_commit_id` | string | no | Last known file commit id |
```bash ```bash
PAYLOAD=$(cat << 'JSON' PAYLOAD=$(cat << 'JSON'
......
...@@ -6,14 +6,20 @@ describe Files::MultiService do ...@@ -6,14 +6,20 @@ describe Files::MultiService do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:branch_name) { project.default_branch } let(:branch_name) { project.default_branch }
let(:file_path) { 'files/ruby/popen.rb' }
let(:action) { 'update' } 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 let(:actions) do
[ [
{ {
action: action, action: action,
file_path: 'files/ruby/popen.rb', file_path: file_path,
content: 'New content' content: 'New content',
last_commit_id: original_commit_id
} }
] ]
end end
...@@ -50,5 +56,40 @@ describe Files::MultiService do ...@@ -50,5 +56,40 @@ describe Files::MultiService do
expect(results[:message]).to match(/Unknown action/) expect(results[:message]).to match(/Unknown action/)
end end
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
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment