kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

The API isn't using the appropriate services for managing forks

Browse source
This commit is contained in:
Francisco Javier López 2017-12-07 08:44:55 +00:00 committed by Douwe Maan
parent af09fb858e
commit c9871e84e4
6 changed files with 292 additions and 194 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

36
app/services/projects/fork_service.rb
View file

@ -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

5
changelogs/unreleased/fj-40752-forks-api-not-using-services.yml Normal file
View file

@ -0,0 +1,5 @@
---
title: Using appropiate services in the API for managing forks
merge_request: 15709
author:
type: fixed

27
db/post_migrate/20171205190711_reschedule_fork_network_creation_caller.rb Normal file
View file

@ -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

2
db/schema.rb
View file

@ -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"

21
lib/api/projects.rb
View file

@ -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

395
spec/services/projects/fork_service_spec.rb
View file

@ -3,210 +3,253 @@ require 'spec_helper'
describe Projects::ForkService do
include ProjectForksHelper
let(:gitlab_shell) { Gitlab::Shell.new }
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) }
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) }
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 'flushes the forks count cache of the source project' do
expect(@from_project.forks_count).to be_zero
fork_project(@from_project, @to_user)
expect(@from_project.forks_count).to eq(1)
end
it 'creates a fork network with the new project and the root project set' do
to_project
fork_network = @from_project.reload.fork_network
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
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
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'])
end
end
context 'repository already exists' do
let(:repository_storage) { 'default' }
let(:repository_storage_path) { Gitlab.config.repositories.storages[repository_storage]['path'] }
context 'when forking a new project' do
describe 'fork by user' do
before do
gitlab_shell.add_repository(repository_storage, "#{@to_user.namespace.full_path}/#{@from_project.path}")
@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
after do
gitlab_shell.remove_repository(repository_storage_path, "#{@to_user.namespace.full_path}/#{@from_project.path}")
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) }
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) }
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 'flushes the forks count cache of the source project' do
expect(@from_project.forks_count).to be_zero
fork_project(@from_project, @to_user)
expect(@from_project.forks_count).to eq(1)
end
it 'creates a fork network with the new project and the root project set' do
to_project
fork_network = @from_project.reload.fork_network
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
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
it 'does not allow creation' do
to_project = fork_project(@from_project, @to_user, namespace: @to_user.namespace)
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.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[:name]).to eq(['has already been taken'])
expect(@to_project.errors[:path]).to eq(['has already been taken'])
end
end
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
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)
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
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
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
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::PRIVATE)
end
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
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
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])
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)
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)
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
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])
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
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::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'])
end
end
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
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'])
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
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