Merge branch 'fj-40752-forks-api-not-using-services' into 'master'

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

Closes #40752

See merge request gitlab-org/gitlab-ce!15709

app/services/projects/fork_service.rb

 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

changelogs/unreleased/fj-40752-forks-api-not-using-services.yml

+---
+title: Using appropiate services in the API for managing forks
+merge_request: 15709
+author:
+type: fixed

db/post_migrate/20171205190711_reschedule_fork_network_creation_caller.rb

+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

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"

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

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