diff options
author | Robert Speicher <robert@gitlab.com> | 2017-03-02 23:14:02 +0000 |
---|---|---|
committer | Felipe Artur <felipefac@gmail.com> | 2017-03-03 18:40:06 -0300 |
commit | 4b692487f54da497d3159fe34373b6de14c53d15 (patch) | |
tree | 00279b83d41f1bf47de177c5ed2d7708a8f5b627 | |
parent | f56366f1b86fc2a2ce2050a284c1b4b2402964d9 (diff) | |
download | gitlab-ce-4b692487f54da497d3159fe34373b6de14c53d15.tar.gz |
Merge branch 'dm-fix-api-create-file-on-empty-repo' into 'master'
Fix creating a file in an empty repo using the API
Closes #28626
See merge request !9632
-rw-r--r-- | app/controllers/concerns/creates_commit.rb | 21 | ||||
-rw-r--r-- | app/models/repository.rb | 2 | ||||
-rw-r--r-- | app/services/files/base_service.rb | 16 | ||||
-rw-r--r-- | app/services/git_operation_service.rb | 37 | ||||
-rw-r--r-- | changelogs/unreleased/dm-fix-api-create-file-on-empty-repo.yml | 4 | ||||
-rw-r--r-- | spec/factories/projects.rb | 4 | ||||
-rw-r--r-- | spec/requests/api/files_spec.rb | 14 | ||||
-rw-r--r-- | spec/requests/api/v3/files_spec.rb | 285 |
8 files changed, 341 insertions, 42 deletions
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index 9ac8197e45a..d62130ce044 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -4,6 +4,8 @@ module CreatesCommit def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil) set_commit_variables + + start_branch = @mr_target_branch commit_params = @commit_params.merge( start_project: @mr_target_project, start_branch: @mr_target_branch, @@ -117,8 +119,23 @@ module CreatesCommit # Merge request to this project @mr_target_project = @project - @mr_target_branch ||= @ref || @target_branch - @mr_source_branch = @target_branch + @mr_target_branch = @ref || @target_branch + + @mr_source_branch = guess_mr_source_branch + end + + def guess_mr_source_branch + # XXX: Happens when viewing a commit without a branch. In this case, + # @target_branch would be the default branch for @mr_source_project, + # however we want a generated new branch here. Thus we can't use + # @target_branch, but should pass nil to indicate that we want a new + # branch instead of @target_branch. + return if + create_merge_request? && + # XXX: Don't understand why rubocop prefers this indention + @mr_source_project.repository.branch_exists?(@target_branch) + + @target_branch end end diff --git a/app/models/repository.rb b/app/models/repository.rb index 1a6418a4491..5085e5f436c 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1075,6 +1075,8 @@ class Repository end def with_repo_branch_commit(start_repository, start_branch_name) + return yield(nil) if start_repository.empty_repo? + branch_name_or_sha = if start_repository == self start_branch_name diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index 0a25f56d24c..0f31f4d7cbb 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -58,16 +58,12 @@ module Files raise_error("You are not allowed to push into this branch") end - unless project.empty_repo? - unless @start_project.repository.branch_exists?(@start_branch) - raise_error('You can only create or edit files when you are on a branch') - end - - if different_branch? - if repository.branch_exists?(@target_branch) - raise_error('Branch with such name already exists. You need to switch to this branch in order to make changes') - end - end + if !@start_project.empty_repo? && !@start_project.repository.branch_exists?(@start_branch) + raise ValidationError, 'You can only create or edit files when you are on a branch' + end + + if !project.empty_repo? && different_branch? && repository.branch_exists?(@branch_name) + raise ValidationError, "A branch called #{@branch_name} already exists. Switch to that branch in order to make changes" end end diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb index 27bcc047601..ed6ea638235 100644 --- a/app/services/git_operation_service.rb +++ b/app/services/git_operation_service.rb @@ -56,12 +56,16 @@ class GitOperationService start_project: repository.project, &block) - check_with_branch_arguments!( - branch_name, start_branch_name, start_project) + start_repository = start_project.repository + start_branch_name = nil if start_repository.empty_repo? + + if start_branch_name && !start_repository.branch_exists?(start_branch_name) + raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.path_with_namespace}" + end update_branch_with_hooks(branch_name) do repository.with_repo_branch_commit( - start_project.repository, + start_repository, start_branch_name || branch_name, &block) end @@ -149,31 +153,4 @@ class GitOperationService repository.raw_repository.autocrlf = :input end end - - def check_with_branch_arguments!( - branch_name, start_branch_name, start_project) - return if repository.branch_exists?(branch_name) - - if repository.project != start_project - unless start_branch_name - raise ArgumentError, - 'Should also pass :start_branch_name if' + - ' :start_project is different from current project' - end - - unless start_project.repository.branch_exists?(start_branch_name) - raise ArgumentError, - "Cannot find branch #{branch_name} nor" \ - " #{start_branch_name} from" \ - " #{start_project.path_with_namespace}" - end - elsif start_branch_name - unless repository.branch_exists?(start_branch_name) - raise ArgumentError, - "Cannot find branch #{branch_name} nor" \ - " #{start_branch_name} from" \ - " #{repository.project.path_with_namespace}" - end - end - end end diff --git a/changelogs/unreleased/dm-fix-api-create-file-on-empty-repo.yml b/changelogs/unreleased/dm-fix-api-create-file-on-empty-repo.yml new file mode 100644 index 00000000000..7ac25c0a83e --- /dev/null +++ b/changelogs/unreleased/dm-fix-api-create-file-on-empty-repo.yml @@ -0,0 +1,4 @@ +--- +title: Fix creating a file in an empty repo using the API +merge_request: 9632 +author: diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index c80b09e9b9d..3d8b7e879c7 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -39,6 +39,10 @@ FactoryGirl.define do trait :empty_repo do after(:create) do |project| project.create_repository + + # We delete hooks so that gitlab-shell will not try to authenticate with + # an API that isn't running + FileUtils.rm_r(File.join(project.repository_storage_path, "#{project.path_with_namespace}.git", 'hooks')) end end diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index 5e26e779366..07061548320 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -147,6 +147,20 @@ describe API::Files, api: true do expect(last_commit.author_name).to eq(author_name) end end + + context 'when the repo is empty' do + let!(:project) { create(:project_empty_repo, namespace: user.namespace ) } + + it "creates a new file in project repo" do + post api("/projects/#{project.id}/repository/files", user), valid_params + + expect(response).to have_http_status(201) + expect(json_response['file_path']).to eq('newfile.rb') + last_commit = project.repository.commit.raw + expect(last_commit.author_email).to eq(user.email) + expect(last_commit.author_name).to eq(user.name) + end + end end describe "PUT /projects/:id/repository/files" do diff --git a/spec/requests/api/v3/files_spec.rb b/spec/requests/api/v3/files_spec.rb new file mode 100644 index 00000000000..3b61139a2cd --- /dev/null +++ b/spec/requests/api/v3/files_spec.rb @@ -0,0 +1,285 @@ +require 'spec_helper' + +describe API::V3::Files, api: true do + include ApiHelpers + + # I have to remove periods from the end of the name + # This happened when the user's name had a suffix (i.e. "Sr.") + # This seems to be what git does under the hood. For example, this commit: + # + # $ git commit --author='Foo Sr. <foo@example.com>' -m 'Where's my trailing period?' + # + # results in this: + # + # $ git show --pretty + # ... + # Author: Foo Sr <foo@example.com> + # ... + + let(:user) { create(:user) } + let!(:project) { create(:project, :repository, namespace: user.namespace ) } + let(:guest) { create(:user) { |u| project.add_guest(u) } } + let(:file_path) { 'files/ruby/popen.rb' } + let(:params) do + { + file_path: file_path, + ref: 'master' + } + end + let(:author_email) { FFaker::Internet.email } + let(:author_name) { FFaker::Name.name.chomp("\.") } + + before { project.team << [user, :developer] } + + describe "GET /projects/:id/repository/files" do + let(:route) { "/projects/#{project.id}/repository/files" } + + shared_examples_for 'repository files' do + it "returns file info" do + get v3_api(route, current_user), params + + expect(response).to have_http_status(200) + expect(json_response['file_path']).to eq(file_path) + expect(json_response['file_name']).to eq('popen.rb') + expect(json_response['last_commit_id']).to eq('570e7b2abdd848b95f2f578043fc23bd6f6fd24d') + expect(Base64.decode64(json_response['content']).lines.first).to eq("require 'fileutils'\n") + end + + context 'when no params are given' do + it_behaves_like '400 response' do + let(:request) { get v3_api(route, current_user) } + end + end + + context 'when file_path does not exist' do + let(:params) do + { + file_path: 'app/models/application.rb', + ref: 'master', + } + end + + it_behaves_like '404 response' do + let(:request) { get v3_api(route, current_user), params } + let(:message) { '404 File Not Found' } + end + end + + context 'when repository is disabled' do + include_context 'disabled repository' + + it_behaves_like '403 response' do + let(:request) { get v3_api(route, current_user), params } + end + end + end + + context 'when unauthenticated', 'and project is public' do + it_behaves_like 'repository files' do + let(:project) { create(:project, :public) } + let(:current_user) { nil } + end + end + + context 'when unauthenticated', 'and project is private' do + it_behaves_like '404 response' do + let(:request) { get v3_api(route), params } + let(:message) { '404 Project Not Found' } + end + end + + context 'when authenticated', 'as a developer' do + it_behaves_like 'repository files' do + let(:current_user) { user } + end + end + + context 'when authenticated', 'as a guest' do + it_behaves_like '403 response' do + let(:request) { get v3_api(route, guest), params } + end + end + end + + describe "POST /projects/:id/repository/files" do + let(:valid_params) do + { + file_path: 'newfile.rb', + branch_name: 'master', + content: 'puts 8', + commit_message: 'Added newfile' + } + end + + it "creates a new file in project repo" do + post v3_api("/projects/#{project.id}/repository/files", user), valid_params + + expect(response).to have_http_status(201) + expect(json_response['file_path']).to eq('newfile.rb') + last_commit = project.repository.commit.raw + expect(last_commit.author_email).to eq(user.email) + expect(last_commit.author_name).to eq(user.name) + end + + it "returns a 400 bad request if no params given" do + post v3_api("/projects/#{project.id}/repository/files", user) + + expect(response).to have_http_status(400) + end + + it "returns a 400 if editor fails to create file" do + allow_any_instance_of(Repository).to receive(:create_file). + and_return(false) + + post v3_api("/projects/#{project.id}/repository/files", user), valid_params + + expect(response).to have_http_status(400) + end + + context "when specifying an author" do + it "creates a new file with the specified author" do + valid_params.merge!(author_email: author_email, author_name: author_name) + + post v3_api("/projects/#{project.id}/repository/files", user), valid_params + + expect(response).to have_http_status(201) + last_commit = project.repository.commit.raw + expect(last_commit.author_email).to eq(author_email) + expect(last_commit.author_name).to eq(author_name) + end + end + + context 'when the repo is empty' do + let!(:project) { create(:project_empty_repo, namespace: user.namespace ) } + + it "creates a new file in project repo" do + post v3_api("/projects/#{project.id}/repository/files", user), valid_params + + expect(response).to have_http_status(201) + expect(json_response['file_path']).to eq('newfile.rb') + last_commit = project.repository.commit.raw + expect(last_commit.author_email).to eq(user.email) + expect(last_commit.author_name).to eq(user.name) + end + end + end + + describe "PUT /projects/:id/repository/files" do + let(:valid_params) do + { + file_path: file_path, + branch_name: 'master', + content: 'puts 8', + commit_message: 'Changed file' + } + end + + it "updates existing file in project repo" do + put v3_api("/projects/#{project.id}/repository/files", user), valid_params + + expect(response).to have_http_status(200) + expect(json_response['file_path']).to eq(file_path) + last_commit = project.repository.commit.raw + expect(last_commit.author_email).to eq(user.email) + expect(last_commit.author_name).to eq(user.name) + end + + it "returns a 400 bad request if no params given" do + put v3_api("/projects/#{project.id}/repository/files", user) + + expect(response).to have_http_status(400) + end + + context "when specifying an author" do + it "updates a file with the specified author" do + valid_params.merge!(author_email: author_email, author_name: author_name, content: "New content") + + put v3_api("/projects/#{project.id}/repository/files", user), valid_params + + expect(response).to have_http_status(200) + last_commit = project.repository.commit.raw + expect(last_commit.author_email).to eq(author_email) + expect(last_commit.author_name).to eq(author_name) + end + end + end + + describe "DELETE /projects/:id/repository/files" do + let(:valid_params) do + { + file_path: file_path, + branch_name: 'master', + commit_message: 'Changed file' + } + end + + it "deletes existing file in project repo" do + delete v3_api("/projects/#{project.id}/repository/files", user), valid_params + + expect(response).to have_http_status(200) + expect(json_response['file_path']).to eq(file_path) + last_commit = project.repository.commit.raw + expect(last_commit.author_email).to eq(user.email) + expect(last_commit.author_name).to eq(user.name) + end + + it "returns a 400 bad request if no params given" do + delete v3_api("/projects/#{project.id}/repository/files", user) + + expect(response).to have_http_status(400) + end + + it "returns a 400 if fails to create file" do + allow_any_instance_of(Repository).to receive(:delete_file).and_return(false) + + delete v3_api("/projects/#{project.id}/repository/files", user), valid_params + + expect(response).to have_http_status(400) + end + + context "when specifying an author" do + it "removes a file with the specified author" do + valid_params.merge!(author_email: author_email, author_name: author_name) + + delete v3_api("/projects/#{project.id}/repository/files", user), valid_params + + expect(response).to have_http_status(200) + last_commit = project.repository.commit.raw + expect(last_commit.author_email).to eq(author_email) + expect(last_commit.author_name).to eq(author_name) + end + end + end + + describe "POST /projects/:id/repository/files with binary file" do + let(:file_path) { 'test.bin' } + let(:put_params) do + { + file_path: file_path, + branch_name: 'master', + content: 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAAA1BMVEUAAACnej3aAAAAAXRSTlMAQObYZgAAAApJREFUCNdjYAAAAAIAAeIhvDMAAAAASUVORK5CYII=', + commit_message: 'Binary file with a \n should not be touched', + encoding: 'base64' + } + end + let(:get_params) do + { + file_path: file_path, + ref: 'master', + } + end + + before do + post v3_api("/projects/#{project.id}/repository/files", user), put_params + end + + it "remains unchanged" do + get v3_api("/projects/#{project.id}/repository/files", user), get_params + + expect(response).to have_http_status(200) + expect(json_response['file_path']).to eq(file_path) + expect(json_response['file_name']).to eq(file_path) + expect(json_response['content']).to eq(put_params[:content]) + end + end +end |