Merge branch '38356-add-last_commit_sha-to-the-commit-api' into 'master'
Add new last_commit_id param for create commit endpoint Closes #38356 See merge request gitlab-org/gitlab-ce!15922
This commit is contained in:
commit
a24b6dbfca
|
@ -1,11 +1,14 @@
|
||||||
module Files
|
module Files
|
||||||
class BaseService < Commits::CreateService
|
class BaseService < Commits::CreateService
|
||||||
|
FileChangedError = Class.new(StandardError)
|
||||||
|
|
||||||
def initialize(*args)
|
def initialize(*args)
|
||||||
super
|
super
|
||||||
|
|
||||||
@author_email = params[:author_email]
|
@author_email = params[:author_email]
|
||||||
@author_name = params[:author_name]
|
@author_name = params[:author_name]
|
||||||
@commit_message = params[:commit_message]
|
@commit_message = params[:commit_message]
|
||||||
|
@last_commit_sha = params[:last_commit_sha]
|
||||||
|
|
||||||
@file_path = params[:file_path]
|
@file_path = params[:file_path]
|
||||||
@previous_path = params[:previous_path]
|
@previous_path = params[:previous_path]
|
||||||
|
@ -13,5 +16,16 @@ module Files
|
||||||
@file_content = params[:file_content]
|
@file_content = params[:file_content]
|
||||||
@file_content = Base64.decode64(@file_content) if params[:file_content_encoding] == 'base64'
|
@file_content = Base64.decode64(@file_content) if params[:file_content_encoding] == 'base64'
|
||||||
end
|
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
|
||||||
end
|
end
|
||||||
|
|
|
@ -11,5 +11,15 @@ module Files
|
||||||
start_project: @start_project,
|
start_project: @start_project,
|
||||||
start_branch_name: @start_branch)
|
start_branch_name: @start_branch)
|
||||||
end
|
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
|
||||||
end
|
end
|
||||||
|
|
|
@ -1,5 +1,7 @@
|
||||||
module Files
|
module Files
|
||||||
class MultiService < Files::BaseService
|
class MultiService < Files::BaseService
|
||||||
|
UPDATE_FILE_ACTIONS = %w(update move delete).freeze
|
||||||
|
|
||||||
def create_commit!
|
def create_commit!
|
||||||
repository.multi_action(
|
repository.multi_action(
|
||||||
user: current_user,
|
user: current_user,
|
||||||
|
@ -20,6 +22,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 +31,15 @@ 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 UPDATE_FILE_ACTIONS.include?(action[:action])
|
||||||
|
|
||||||
|
file_path = action[:previous_path] || 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
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -1,13 +1,5 @@
|
||||||
module Files
|
module Files
|
||||||
class UpdateService < Files::BaseService
|
class UpdateService < Files::BaseService
|
||||||
FileChangedError = Class.new(StandardError)
|
|
||||||
|
|
||||||
def initialize(*args)
|
|
||||||
super
|
|
||||||
|
|
||||||
@last_commit_sha = params[:last_commit_sha]
|
|
||||||
end
|
|
||||||
|
|
||||||
def create_commit!
|
def create_commit!
|
||||||
repository.update_file(current_user, @file_path, @file_content,
|
repository.update_file(current_user, @file_path, @file_content,
|
||||||
message: @commit_message,
|
message: @commit_message,
|
||||||
|
@ -21,21 +13,10 @@ module Files
|
||||||
|
|
||||||
private
|
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!
|
def validate!
|
||||||
super
|
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."
|
raise FileChangedError, "You are attempting to update a file that has changed since you started editing it."
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: 'Validate file status when commiting multiple files'
|
||||||
|
merge_request: 15922
|
||||||
|
author:
|
||||||
|
type: added
|
|
@ -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. Will be only considered in update, move and delete actions. |
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
PAYLOAD=$(cat << 'JSON'
|
PAYLOAD=$(cat << 'JSON'
|
||||||
|
|
|
@ -151,3 +151,4 @@ Parameters:
|
||||||
- `author_email` (optional) - Specify the commit author's email address
|
- `author_email` (optional) - Specify the commit author's email address
|
||||||
- `author_name` (optional) - Specify the commit author's name
|
- `author_name` (optional) - Specify the commit author's name
|
||||||
- `commit_message` (required) - Commit message
|
- `commit_message` (required) - Commit message
|
||||||
|
- `last_commit_id` (optional) - Last known file commit id
|
||||||
|
|
|
@ -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
|
|
@ -0,0 +1,144 @@
|
||||||
|
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(: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, original_file_path).sha
|
||||||
|
end
|
||||||
|
|
||||||
|
let(:actions) do
|
||||||
|
[
|
||||||
|
{
|
||||||
|
action: action,
|
||||||
|
file_path: new_file_path,
|
||||||
|
previous_path: original_file_path,
|
||||||
|
content: 'New content',
|
||||||
|
last_commit_id: original_commit_id
|
||||||
|
}
|
||||||
|
]
|
||||||
|
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
|
||||||
|
|
||||||
|
describe 'Updating files' do
|
||||||
|
context 'when the file has been previously 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(new_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
|
||||||
|
|
||||||
|
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)
|
||||||
|
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
|
Loading…
Reference in New Issue