diff options
| -rw-r--r-- | app/services/projects/fork_service.rb | 36 | ||||
| -rw-r--r-- | changelogs/unreleased/fj-40752-forks-api-not-using-services.yml | 5 | ||||
| -rw-r--r-- | db/post_migrate/20171205190711_reschedule_fork_network_creation_caller.rb | 27 | ||||
| -rw-r--r-- | db/schema.rb | 2 | ||||
| -rw-r--r-- | lib/api/projects.rb | 21 | ||||
| -rw-r--r-- | spec/services/projects/fork_service_spec.rb | 353 |
6 files changed, 271 insertions, 173 deletions
diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb index eb5cce5ab98..03be7039b2a 100644 --- a/app/services/projects/fork_service.rb +++ b/app/services/projects/fork_service.rb @@ -1,6 +1,24 @@ module Projects class ForkService < BaseService - def execute + def execute(fork_to_project = nil) + if fork_to_project + link_existing_project(fork_to_project) + else + fork_new_project + end + end + + private + + def link_existing_project(fork_to_project) + return if fork_to_project.forked? + + link_fork_network(fork_to_project) + + fork_to_project + end + + def fork_new_project new_params = { forked_from_project_id: @project.id, visibility_level: allowed_visibility_level, @@ -21,15 +39,11 @@ module Projects builds_access_level = @project.project_feature.builds_access_level new_project.project_feature.update_attributes(builds_access_level: builds_access_level) - refresh_forks_count - link_fork_network(new_project) new_project end - private - def fork_network if @project.fork_network @project.fork_network @@ -43,9 +57,17 @@ module Projects end end - def link_fork_network(new_project) - fork_network.fork_network_members.create(project: new_project, + def link_fork_network(fork_to_project) + fork_network.fork_network_members.create(project: fork_to_project, forked_from_project: @project) + + # TODO: remove this when ForkedProjectLink model is removed + unless fork_to_project.forked_project_link + fork_to_project.create_forked_project_link(forked_to_project: fork_to_project, + forked_from_project: @project) + end + + refresh_forks_count end def refresh_forks_count diff --git a/changelogs/unreleased/fj-40752-forks-api-not-using-services.yml b/changelogs/unreleased/fj-40752-forks-api-not-using-services.yml new file mode 100644 index 00000000000..cd7b87596e6 --- /dev/null +++ b/changelogs/unreleased/fj-40752-forks-api-not-using-services.yml @@ -0,0 +1,5 @@ +--- +title: Using appropiate services in the API for managing forks +merge_request: 15709 +author: +type: fixed diff --git a/db/post_migrate/20171205190711_reschedule_fork_network_creation_caller.rb b/db/post_migrate/20171205190711_reschedule_fork_network_creation_caller.rb new file mode 100644 index 00000000000..30ff5173192 --- /dev/null +++ b/db/post_migrate/20171205190711_reschedule_fork_network_creation_caller.rb @@ -0,0 +1,27 @@ +class RescheduleForkNetworkCreationCaller < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + MIGRATION = 'PopulateForkNetworksRange'.freeze + BATCH_SIZE = 100 + DELAY_INTERVAL = 15.seconds + + disable_ddl_transaction! + + class ForkedProjectLink < ActiveRecord::Base + include EachBatch + + self.table_name = 'forked_project_links' + end + + def up + say 'Populating the `fork_networks` based on existing `forked_project_links`' + + queue_background_migration_jobs_by_range_at_intervals(ForkedProjectLink, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) + end + + def down + # nothing + end +end diff --git a/db/schema.rb b/db/schema.rb index 2be1b745342..6ea3ab54742 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20171124150326) do +ActiveRecord::Schema.define(version: 20171205190711) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 14a4fc6f025..fa222bf2b1c 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -367,15 +367,16 @@ module API post ":id/fork/:forked_from_id" do authenticated_as_admin! - forked_from_project = find_project!(params[:forked_from_id]) - not_found!("Source Project") unless forked_from_project + fork_from_project = find_project!(params[:forked_from_id]) - if user_project.forked_from_project.nil? - user_project.create_forked_project_link(forked_to_project_id: user_project.id, forked_from_project_id: forked_from_project.id) + not_found!("Source Project") unless fork_from_project - ::Projects::ForksCountService.new(forked_from_project).refresh_cache + result = ::Projects::ForkService.new(fork_from_project, current_user).execute(user_project) + + if result + present user_project.reload, with: Entities::Project else - render_api_error!("Project already forked", 409) + render_api_error!("Project already forked", 409) if user_project.forked? end end @@ -383,11 +384,11 @@ module API delete ":id/fork" do authorize! :remove_fork_project, user_project - if user_project.forked? - destroy_conditionally!(user_project.forked_project_link) - else - not_modified! + result = destroy_conditionally!(user_project) do + ::Projects::UnlinkForkService.new(user_project, current_user).execute end + + result ? status(204) : not_modified! end desc 'Share the project with a group' do diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 53862283a27..4057caca2ac 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -3,210 +3,253 @@ require 'spec_helper' describe Projects::ForkService do include ProjectForksHelper let(:gitlab_shell) { Gitlab::Shell.new } + context 'when forking a new project' do + describe 'fork by user' do + before do + @from_user = create(:user) + @from_namespace = @from_user.namespace + avatar = fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png") + @from_project = create(:project, + :repository, + creator_id: @from_user.id, + namespace: @from_namespace, + star_count: 107, + avatar: avatar, + description: 'wow such project') + @to_user = create(:user) + @to_namespace = @to_user.namespace + @from_project.add_user(@to_user, :developer) + end - describe 'fork by user' do - before do - @from_user = create(:user) - @from_namespace = @from_user.namespace - avatar = fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png") - @from_project = create(:project, - :repository, - creator_id: @from_user.id, - namespace: @from_namespace, - star_count: 107, - avatar: avatar, - description: 'wow such project') - @to_user = create(:user) - @to_namespace = @to_user.namespace - @from_project.add_user(@to_user, :developer) - end + context 'fork project' do + context 'when forker is a guest' do + before do + @guest = create(:user) + @from_project.add_user(@guest, :guest) + end + subject { fork_project(@from_project, @guest) } - context 'fork project' do - context 'when forker is a guest' do - before do - @guest = create(:user) - @from_project.add_user(@guest, :guest) + it { is_expected.not_to be_persisted } + it { expect(subject.errors[:forked_from_project_id]).to eq(['is forbidden']) } end - subject { fork_project(@from_project, @guest) } - it { is_expected.not_to be_persisted } - it { expect(subject.errors[:forked_from_project_id]).to eq(['is forbidden']) } - end + describe "successfully creates project in the user namespace" do + let(:to_project) { fork_project(@from_project, @to_user, namespace: @to_user.namespace) } - describe "successfully creates project in the user namespace" do - let(:to_project) { fork_project(@from_project, @to_user, namespace: @to_user.namespace) } - - it { expect(to_project).to be_persisted } - it { expect(to_project.errors).to be_empty } - it { expect(to_project.owner).to eq(@to_user) } - it { expect(to_project.namespace).to eq(@to_user.namespace) } - it { expect(to_project.star_count).to be_zero } - it { expect(to_project.description).to eq(@from_project.description) } - it { expect(to_project.avatar.file).to be_exists } - - # This test is here because we had a bug where the from-project lost its - # avatar after being forked. - # https://gitlab.com/gitlab-org/gitlab-ce/issues/26158 - it "after forking the from-project still has its avatar" do - # If we do not fork the project first we cannot detect the bug. - expect(to_project).to be_persisted - - expect(@from_project.avatar.file).to be_exists - end + it { expect(to_project).to be_persisted } + it { expect(to_project.errors).to be_empty } + it { expect(to_project.owner).to eq(@to_user) } + it { expect(to_project.namespace).to eq(@to_user.namespace) } + it { expect(to_project.star_count).to be_zero } + it { expect(to_project.description).to eq(@from_project.description) } + it { expect(to_project.avatar.file).to be_exists } - it 'flushes the forks count cache of the source project' do - expect(@from_project.forks_count).to be_zero + # This test is here because we had a bug where the from-project lost its + # avatar after being forked. + # https://gitlab.com/gitlab-org/gitlab-ce/issues/26158 + it "after forking the from-project still has its avatar" do + # If we do not fork the project first we cannot detect the bug. + expect(to_project).to be_persisted - fork_project(@from_project, @to_user) + expect(@from_project.avatar.file).to be_exists + end - expect(@from_project.forks_count).to eq(1) - end + it 'flushes the forks count cache of the source project' do + expect(@from_project.forks_count).to be_zero - it 'creates a fork network with the new project and the root project set' do - to_project - fork_network = @from_project.reload.fork_network + fork_project(@from_project, @to_user) - expect(fork_network).not_to be_nil - expect(fork_network.root_project).to eq(@from_project) - expect(fork_network.projects).to contain_exactly(@from_project, to_project) - end - end + expect(@from_project.forks_count).to eq(1) + end - context 'creating a fork of a fork' do - let(:from_forked_project) { fork_project(@from_project, @to_user) } - let(:other_namespace) do - group = create(:group) - group.add_owner(@to_user) - group - end - let(:to_project) { fork_project(from_forked_project, @to_user, namespace: other_namespace) } + it 'creates a fork network with the new project and the root project set' do + to_project + fork_network = @from_project.reload.fork_network - it 'sets the root of the network to the root project' do - expect(to_project.fork_network.root_project).to eq(@from_project) + expect(fork_network).not_to be_nil + expect(fork_network.root_project).to eq(@from_project) + expect(fork_network.projects).to contain_exactly(@from_project, to_project) + end end - it 'sets the forked_from_project on the membership' do - expect(to_project.fork_network_member.forked_from_project).to eq(from_forked_project) + context 'creating a fork of a fork' do + let(:from_forked_project) { fork_project(@from_project, @to_user) } + let(:other_namespace) do + group = create(:group) + group.add_owner(@to_user) + group + end + let(:to_project) { fork_project(from_forked_project, @to_user, namespace: other_namespace) } + + it 'sets the root of the network to the root project' do + expect(to_project.fork_network.root_project).to eq(@from_project) + end + + it 'sets the forked_from_project on the membership' do + expect(to_project.fork_network_member.forked_from_project).to eq(from_forked_project) + end end end - end - context 'project already exists' do - it "fails due to validation, not transaction failure" do - @existing_project = create(:project, :repository, creator_id: @to_user.id, name: @from_project.name, namespace: @to_namespace) - @to_project = fork_project(@from_project, @to_user, namespace: @to_namespace) - expect(@existing_project).to be_persisted + context 'project already exists' do + it "fails due to validation, not transaction failure" do + @existing_project = create(:project, :repository, creator_id: @to_user.id, name: @from_project.name, namespace: @to_namespace) + @to_project = fork_project(@from_project, @to_user, namespace: @to_namespace) + expect(@existing_project).to be_persisted - expect(@to_project).not_to be_persisted - expect(@to_project.errors[:name]).to eq(['has already been taken']) - expect(@to_project.errors[:path]).to eq(['has already been taken']) + expect(@to_project).not_to be_persisted + expect(@to_project.errors[:name]).to eq(['has already been taken']) + expect(@to_project.errors[:path]).to eq(['has already been taken']) + end end - end - context 'repository already exists' do - let(:repository_storage) { 'default' } - let(:repository_storage_path) { Gitlab.config.repositories.storages[repository_storage]['path'] } + context 'repository already exists' do + let(:repository_storage) { 'default' } + let(:repository_storage_path) { Gitlab.config.repositories.storages[repository_storage]['path'] } - before do - gitlab_shell.add_repository(repository_storage, "#{@to_user.namespace.full_path}/#{@from_project.path}") - end + before do + gitlab_shell.add_repository(repository_storage, "#{@to_user.namespace.full_path}/#{@from_project.path}") + end - after do - gitlab_shell.remove_repository(repository_storage_path, "#{@to_user.namespace.full_path}/#{@from_project.path}") - end + after do + gitlab_shell.remove_repository(repository_storage_path, "#{@to_user.namespace.full_path}/#{@from_project.path}") + end - it 'does not allow creation' do - to_project = fork_project(@from_project, @to_user, namespace: @to_user.namespace) + it 'does not allow creation' do + to_project = fork_project(@from_project, @to_user, namespace: @to_user.namespace) - expect(to_project).not_to be_persisted - expect(to_project.errors.messages).to have_key(:base) - expect(to_project.errors.messages[:base].first).to match('There is already a repository with that name on disk') + expect(to_project).not_to be_persisted + expect(to_project.errors.messages).to have_key(:base) + expect(to_project.errors.messages[:base].first).to match('There is already a repository with that name on disk') + end end - end - context 'GitLab CI is enabled' do - it "forks and enables CI for fork" do - @from_project.enable_ci - @to_project = fork_project(@from_project, @to_user) - expect(@to_project.builds_enabled?).to be_truthy + context 'GitLab CI is enabled' do + it "forks and enables CI for fork" do + @from_project.enable_ci + @to_project = fork_project(@from_project, @to_user) + expect(@to_project.builds_enabled?).to be_truthy + end end - end - context "when project has restricted visibility level" do - context "and only one visibility level is restricted" do - before do - @from_project.update_attributes(visibility_level: Gitlab::VisibilityLevel::INTERNAL) - stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL]) + context "when project has restricted visibility level" do + context "and only one visibility level is restricted" do + before do + @from_project.update_attributes(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL]) + end + + it "creates fork with highest allowed level" do + forked_project = fork_project(@from_project, @to_user) + + expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) + end end - it "creates fork with highest allowed level" do - forked_project = fork_project(@from_project, @to_user) + context "and all visibility levels are restricted" do + before do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PRIVATE]) + end + + it "creates fork with private visibility levels" do + forked_project = fork_project(@from_project, @to_user) - expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) + expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) + end end end + end - context "and all visibility levels are restricted" do - before do - stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PRIVATE]) + describe 'fork to namespace' do + before do + @group_owner = create(:user) + @developer = create(:user) + @project = create(:project, :repository, + creator_id: @group_owner.id, + star_count: 777, + description: 'Wow, such a cool project!') + @group = create(:group) + @group.add_user(@group_owner, GroupMember::OWNER) + @group.add_user(@developer, GroupMember::DEVELOPER) + @project.add_user(@developer, :developer) + @project.add_user(@group_owner, :developer) + @opts = { namespace: @group } + end + + context 'fork project for group' do + it 'group owner successfully forks project into the group' do + to_project = fork_project(@project, @group_owner, @opts) + + expect(to_project).to be_persisted + expect(to_project.errors).to be_empty + expect(to_project.owner).to eq(@group) + expect(to_project.namespace).to eq(@group) + expect(to_project.name).to eq(@project.name) + expect(to_project.path).to eq(@project.path) + expect(to_project.description).to eq(@project.description) + expect(to_project.star_count).to be_zero end + end - it "creates fork with private visibility levels" do - forked_project = fork_project(@from_project, @to_user) + context 'fork project for group when user not owner' do + it 'group developer fails to fork project into the group' do + to_project = fork_project(@project, @developer, @opts) + expect(to_project.errors[:namespace]).to eq(['is not valid']) + end + end - expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) + context 'project already exists in group' do + it 'fails due to validation, not transaction failure' do + existing_project = create(:project, :repository, + name: @project.name, + namespace: @group) + to_project = fork_project(@project, @group_owner, @opts) + expect(existing_project.persisted?).to be_truthy + expect(to_project.errors[:name]).to eq(['has already been taken']) + expect(to_project.errors[:path]).to eq(['has already been taken']) end end end end - describe 'fork to namespace' do - before do - @group_owner = create(:user) - @developer = create(:user) - @project = create(:project, :repository, - creator_id: @group_owner.id, - star_count: 777, - description: 'Wow, such a cool project!') - @group = create(:group) - @group.add_user(@group_owner, GroupMember::OWNER) - @group.add_user(@developer, GroupMember::DEVELOPER) - @project.add_user(@developer, :developer) - @project.add_user(@group_owner, :developer) - @opts = { namespace: @group } + context 'when linking fork to an existing project' do + let(:fork_from_project) { create(:project, :public) } + let(:fork_to_project) { create(:project, :public) } + let(:user) { create(:user) } + + subject { described_class.new(fork_from_project, user) } + + def forked_from_project(project) + project.fork_network_member&.forked_from_project end - context 'fork project for group' do - it 'group owner successfully forks project into the group' do - to_project = fork_project(@project, @group_owner, @opts) - - expect(to_project).to be_persisted - expect(to_project.errors).to be_empty - expect(to_project.owner).to eq(@group) - expect(to_project.namespace).to eq(@group) - expect(to_project.name).to eq(@project.name) - expect(to_project.path).to eq(@project.path) - expect(to_project.description).to eq(@project.description) - expect(to_project.star_count).to be_zero + context 'if project is already forked' do + it 'does not create fork relation' do + allow(fork_to_project).to receive(:forked?).and_return(true) + expect(forked_from_project(fork_to_project)).to be_nil + expect(subject.execute(fork_to_project)).to be_nil + expect(forked_from_project(fork_to_project)).to be_nil end end - context 'fork project for group when user not owner' do - it 'group developer fails to fork project into the group' do - to_project = fork_project(@project, @developer, @opts) - expect(to_project.errors[:namespace]).to eq(['is not valid']) + context 'if project is not forked' do + it 'creates fork relation' do + expect(fork_to_project.forked?).to be false + expect(forked_from_project(fork_to_project)).to be_nil + + subject.execute(fork_to_project) + + expect(fork_to_project.forked?).to be true + expect(forked_from_project(fork_to_project)).to eq fork_from_project + expect(fork_to_project.forked_from_project).to eq fork_from_project end - end - context 'project already exists in group' do - it 'fails due to validation, not transaction failure' do - existing_project = create(:project, :repository, - name: @project.name, - namespace: @group) - to_project = fork_project(@project, @group_owner, @opts) - expect(existing_project.persisted?).to be_truthy - expect(to_project.errors[:name]).to eq(['has already been taken']) - expect(to_project.errors[:path]).to eq(['has already been taken']) + it 'flushes the forks count cache of the source project' do + expect(fork_from_project.forks_count).to be_zero + + subject.execute(fork_to_project) + + expect(fork_from_project.forks_count).to eq(1) end end end |