Merge branch 'bvl-override-import-params' into 'master'

Allow passing params to import API to override project attributes

Closes gitlab-ee#4788

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

app/models/project.rb

@@ -637,7 +637,7 @@ class Project < ActiveRecord::Base
   end
 
   def create_or_update_import_data(data: nil, credentials: nil)
-    return unless import_url.present? && valid_import_url?
+    return if data.nil? && credentials.nil?
 
     project_import_data = import_data || build_import_data
     if data
@@ -1487,6 +1487,7 @@ class Project < ActiveRecord::Base
     remove_import_jid
     update_project_counter_caches
     after_create_default_branch
+    refresh_markdown_cache!
   end
 
   def update_project_counter_caches

app/services/projects/gitlab_projects_import_service.rb

@@ -5,8 +5,8 @@ module Projects
   class GitlabProjectsImportService
     attr_reader :current_user, :params
 
-    def initialize(user, params)
-      @current_user, @params = user, params.dup
+    def initialize(user, import_params, override_params = nil)
+      @current_user, @params, @override_params = user, import_params.dup, override_params
     end
 
     def execute
@@ -17,6 +17,7 @@ module Projects
 
       params[:import_type] = 'gitlab_project'
       params[:import_source] = import_upload_path
+      params[:import_data] = { data: { override_params: @override_params } } if @override_params
 
       ::Projects::CreateService.new(current_user, params).execute
     end

changelogs/unreleased/bvl-override-import-params.yml

+---
+title: Allow overriding params on project import through API
+merge_request: 18086
+author:
+type: added

doc/api/project_import_export.md

@@ -111,6 +111,9 @@ POST /projects/import
 | `namespace` | integer/string | no | The ID or path of the namespace that the project will be imported to. Defaults to the current user's namespace |
 | `file` | string | yes | The file to be uploaded |
 | `path` | string | yes | Name and path for new project |
+| `override_params` | Hash | no | Supports all fields defined in the [Project API](projects.md)] |
+
+The override params passed will take precendence over all values defined inside the export file.
 
 To upload a file from your filesystem, use the `--form` argument. This causes
 cURL to post data using the header `Content-Type: multipart/form-data`.

lib/api/helpers/projects_helpers.rb

+module API
+  module Helpers
+    module ProjectsHelpers
+      extend ActiveSupport::Concern
+
+      included do
+        helpers do
+          params :optional_project_params_ce do
+            optional :description, type: String, desc: 'The description of the project'
+            optional :ci_config_path, type: String, desc: 'The path to CI config file. Defaults to `.gitlab-ci.yml`'
+            optional :issues_enabled, type: Boolean, desc: 'Flag indication if the issue tracker is enabled'
+            optional :merge_requests_enabled, type: Boolean, desc: 'Flag indication if merge requests are enabled'
+            optional :wiki_enabled, type: Boolean, desc: 'Flag indication if the wiki is enabled'
+            optional :jobs_enabled, type: Boolean, desc: 'Flag indication if jobs are enabled'
+            optional :snippets_enabled, type: Boolean, desc: 'Flag indication if snippets are enabled'
+            optional :shared_runners_enabled, type: Boolean, desc: 'Flag indication if shared runners are enabled for that project'
+            optional :resolve_outdated_diff_discussions, type: Boolean, desc: 'Automatically resolve merge request diffs discussions on lines changed with a push'
+            optional :container_registry_enabled, type: Boolean, desc: 'Flag indication if the container registry is enabled for that project'
+            optional :lfs_enabled, type: Boolean, desc: 'Flag indication if Git LFS is enabled for that project'
+            optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The visibility of the project.'
+            optional :public_builds, type: Boolean, desc: 'Perform public builds'
+            optional :request_access_enabled, type: Boolean, desc: 'Allow users to request member access'
+            optional :only_allow_merge_if_pipeline_succeeds, type: Boolean, desc: 'Only allow to merge if builds succeed'
+            optional :only_allow_merge_if_all_discussions_are_resolved, type: Boolean, desc: 'Only allow to merge if all discussions are resolved'
+            optional :tag_list, type: Array[String], desc: 'The list of tags for a project'
+            optional :avatar, type: File, desc: 'Avatar image for project'
+            optional :printing_merge_request_link_enabled, type: Boolean, desc: 'Show link to create/view merge request when pushing from the command line'
+            optional :merge_method, type: String, values: %w(ff rebase_merge merge), desc: 'The merge method used when merging merge requests'
+          end
+
+          params :optional_project_params do
+            use :optional_project_params_ce
+          end
+        end
+      end
+    end
+  end
+end

lib/api/project_import.rb

 module API
   class ProjectImport < Grape::API
     include PaginationParams
+    include Helpers::ProjectsHelpers
 
     helpers do
       def import_params
@@ -25,6 +26,11 @@ module API
         requires :path, type: String, desc: 'The new project path and name'
         requires :file, type: File, desc: 'The project export file to be imported'
         optional :namespace, type: String, desc: "The ID or name of the namespace that the project will be imported into. Defaults to the current user's namespace."
+        optional :override_params,
+                 type: Hash,
+                 desc: 'New project params to override values in the export' do
+          use :optional_project_params
+        end
       end
       desc 'Create a new project import' do
         detail 'This feature was introduced in GitLab 10.6.'
@@ -47,7 +53,11 @@ module API
             file: import_params[:file]['tempfile']
         }
 
-        project = ::Projects::GitlabProjectsImportService.new(current_user, project_params).execute
+        override_params = import_params.delete(:override_params)
+
+        project = ::Projects::GitlabProjectsImportService.new(
+          current_user, project_params, override_params
+        ).execute
 
         render_api_error!(project.errors.full_messages&.first, 400) unless project.saved?
 

lib/api/projects.rb

@@ -4,37 +4,11 @@ module API
   class Projects < Grape::API
     include PaginationParams
     include Helpers::CustomAttributes
+    include Helpers::ProjectsHelpers
 
     before { authenticate_non_get! }
 
     helpers do
-      params :optional_params_ce do
-        optional :description, type: String, desc: 'The description of the project'
-        optional :ci_config_path, type: String, desc: 'The path to CI config file. Defaults to `.gitlab-ci.yml`'
-        optional :issues_enabled, type: Boolean, desc: 'Flag indication if the issue tracker is enabled'
-        optional :merge_requests_enabled, type: Boolean, desc: 'Flag indication if merge requests are enabled'
-        optional :wiki_enabled, type: Boolean, desc: 'Flag indication if the wiki is enabled'
-        optional :jobs_enabled, type: Boolean, desc: 'Flag indication if jobs are enabled'
-        optional :snippets_enabled, type: Boolean, desc: 'Flag indication if snippets are enabled'
-        optional :shared_runners_enabled, type: Boolean, desc: 'Flag indication if shared runners are enabled for that project'
-        optional :resolve_outdated_diff_discussions, type: Boolean, desc: 'Automatically resolve merge request diffs discussions on lines changed with a push'
-        optional :container_registry_enabled, type: Boolean, desc: 'Flag indication if the container registry is enabled for that project'
-        optional :lfs_enabled, type: Boolean, desc: 'Flag indication if Git LFS is enabled for that project'
-        optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The visibility of the project.'
-        optional :public_builds, type: Boolean, desc: 'Perform public builds'
-        optional :request_access_enabled, type: Boolean, desc: 'Allow users to request member access'
-        optional :only_allow_merge_if_pipeline_succeeds, type: Boolean, desc: 'Only allow to merge if builds succeed'
-        optional :only_allow_merge_if_all_discussions_are_resolved, type: Boolean, desc: 'Only allow to merge if all discussions are resolved'
-        optional :tag_list, type: Array[String], desc: 'The list of tags for a project'
-        optional :avatar, type: File, desc: 'Avatar image for project'
-        optional :printing_merge_request_link_enabled, type: Boolean, desc: 'Show link to create/view merge request when pushing from the command line'
-        optional :merge_method, type: String, values: %w(ff rebase_merge merge), desc: 'The merge method used when merging merge requests'
-      end
-
-      params :optional_params do
-        use :optional_params_ce
-      end
-
       params :statistics_params do
         optional :statistics, type: Boolean, default: false, desc: 'Include project statistics'
       end
@@ -144,7 +118,7 @@ module API
         optional :name, type: String, desc: 'The name of the project'
         optional :path, type: String, desc: 'The path of the repository'
         at_least_one_of :name, :path
-        use :optional_params
+        use :optional_project_params
         use :create_params
       end
       post do
@@ -172,7 +146,7 @@ module API
         requires :user_id, type: Integer, desc: 'The ID of a user'
         optional :path, type: String, desc: 'The path of the repository'
         optional :default_branch, type: String, desc: 'The default branch of the project'
-        use :optional_params
+        use :optional_project_params
         use :create_params
       end
       post "user/:user_id" do
@@ -293,7 +267,7 @@ module API
         optional :default_branch, type: String, desc: 'The default branch of the project'
         optional :path, type: String, desc: 'The path of the repository'
 
-        use :optional_params
+        use :optional_project_params
         at_least_one_of(*at_least_one_of_ce)
       end
       put ':id' do

lib/gitlab/import_export/import_export.yml

@@ -105,6 +105,7 @@ excluded_attributes:
     - :last_repository_updated_at
     - :last_repository_check_at
     - :storage_version
+    - :description_html
   snippets:
     - :expired_at
   merge_request_diff:
@@ -144,8 +145,6 @@ methods:
     - :diff_head_sha
     - :source_branch_sha
     - :target_branch_sha
-  project:
-    - :description_html
   events:
     - :action
   push_event_payload:

lib/gitlab/import_export/project_tree_restorer.rb

@@ -77,24 +77,31 @@ module Gitlab
       end
 
       def default_relation_list
-        Gitlab::ImportExport::Reader.new(shared: @shared).tree.reject do |model|
+        reader.tree.reject do |model|
           model.is_a?(Hash) && model[:project_members]
         end
       end
 
       def restore_project
-        params = project_params
+        @project.update_columns(project_params)
+        @project
+      end
 
-        if params[:description].present?
-          params[:description_html] = nil
+      def project_params
+        @project_params ||= json_params.merge(override_params)
       end
 
-        @project.update_columns(params)
-        @project
+      def override_params
+        return {} unless params = @project.import_data&.data&.fetch('override_params')
+
+        @override_params ||= params.select do |key, _value|
+          Project.column_names.include?(key.to_s) &&
+            !reader.project_tree[:except].include?(key.to_sym)
+        end
       end
 
-      def project_params
-        @tree_hash.reject do |key, value|
+      def json_params
+        @json_params ||= @tree_hash.reject do |key, value|
           # return params that are not 1 to many or 1 to 1 relations
           value.respond_to?(:each) && !Project.column_names.include?(key)
         end
@@ -181,6 +188,10 @@ module Gitlab
 
         relation_hash.merge(params)
       end
+
+      def reader
+        @reader ||= Gitlab::ImportExport::Reader.new(shared: @shared)
+      end
     end
   end
 end

spec/lib/gitlab/import_export/project.json

@@ -2,7 +2,6 @@
   "description": "Nisi et repellendus ut enim quo accusamus vel magnam.",
   "visibility_level": 10,
   "archived": false,
-  "description_html": "description",
   "labels": [
     {
       "id": 2,

spec/lib/gitlab/import_export/project_tree_restorer_spec.rb

@@ -46,10 +46,6 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
         expect(Project.find_by_path('project').description).to eq('Nisi et repellendus ut enim quo accusamus vel magnam.')
       end
 
-      it 'has the project html description' do
-        expect(Project.find_by_path('project').description_html).to eq('description')
-      end
-
       it 'has the same label associated to two issues' do
         expect(ProjectLabel.find_by_title('test2').issues.count).to eq(2)
       end
@@ -317,6 +313,24 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
       end
     end
 
+    context 'when the project has overriden params in import data' do
+      it 'overwrites the params stored in the JSON' do
+        project.create_import_data(data: { override_params: { description: "Overridden" } })
+
+        restored_project_json
+
+        expect(project.description).to eq("Overridden")
+      end
+
+      it 'does not allow setting params that are excluded from import_export settings' do
+        project.create_import_data(data: { override_params: { lfs_enabled: true } })
+
+        restored_project_json
+
+        expect(project.lfs_enabled).to be_nil
+      end
+    end
+
     context 'with a project that has a group' do
       let!(:project) do
         create(:project,

spec/lib/gitlab/import_export/project_tree_saver_spec.rb

@@ -245,10 +245,6 @@ describe Gitlab::ImportExport::ProjectTreeSaver do
       end
 
       context 'project attributes' do
-        it 'contains the html description' do
-          expect(saved_project_json).to include("description_html" => 'description')
-        end
-
         it 'does not contain the runners token' do
           expect(saved_project_json).not_to include("runners_token" => 'token')
         end
@@ -274,7 +270,6 @@ describe Gitlab::ImportExport::ProjectTreeSaver do
                      releases: [release],
                      group: group
                     )
-    project.update_column(:description_html, 'description')
     project_label = create(:label, project: project)
     group_label = create(:group_label, group: group)
     create(:label_link, label: project_label, target: issue)

spec/models/project_spec.rb

@@ -3243,6 +3243,7 @@ describe Project do
       expect(project).to receive(:update_project_counter_caches)
       expect(project).to receive(:remove_import_jid)
       expect(project).to receive(:after_create_default_branch)
+      expect(project).to receive(:refresh_markdown_cache!)
 
       project.after_import
     end

spec/requests/api/project_import_spec.rb

@@ -40,7 +40,7 @@ describe API::ProjectImport do
       expect(response).to have_gitlab_http_status(201)
     end
 
-    it 'schedules an import at the user namespace level' do
+    it 'does not shedule an import for a nampespace that does not exist' do
       expect_any_instance_of(Project).not_to receive(:import_schedule)
       expect(::Projects::CreateService).not_to receive(:new)
 
@@ -71,6 +71,49 @@ describe API::ProjectImport do
       expect(json_response['error']).to eq('file is invalid')
     end
 
+    it 'stores params that can be overridden' do
+      stub_import(namespace)
+      override_params = { 'description' => 'Hello world' }
+
+      post api('/projects/import', user),
+           path: 'test-import',
+           file: fixture_file_upload(file),
+           namespace: namespace.id,
+           override_params: override_params
+      import_project = Project.find(json_response['id'])
+
+      expect(import_project.import_data.data['override_params']).to eq(override_params)
+    end
+
+    it 'does not store params that are not allowed' do
+      stub_import(namespace)
+      override_params = { 'not_allowed' => 'Hello world' }
+
+      post api('/projects/import', user),
+           path: 'test-import',
+           file: fixture_file_upload(file),
+           namespace: namespace.id,
+           override_params: override_params
+      import_project = Project.find(json_response['id'])
+
+      expect(import_project.import_data.data['override_params']).to be_empty
+    end
+
+    it 'correctly overrides params during the import' do
+      override_params = { 'description' => 'Hello world' }
+
+      Sidekiq::Testing.inline! do
+        post api('/projects/import', user),
+             path: 'test-import',
+             file: fixture_file_upload(file),
+             namespace: namespace.id,
+             override_params: override_params
+      end
+      import_project = Project.find(json_response['id'])
+
+      expect(import_project.description).to eq('Hello world')
+    end
+
     def stub_import(namespace)
       expect_any_instance_of(Project).to receive(:import_schedule)
       expect(::Projects::CreateService).to receive(:new).with(user, hash_including(namespace_id: namespace.id)).and_call_original

spec/services/projects/gitlab_projects_import_service_spec.rb

@@ -2,8 +2,10 @@ require 'spec_helper'
 
 describe Projects::GitlabProjectsImportService do
   set(:namespace) { create(:namespace) }
+  let(:path) { 'test-path' }
   let(:file) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }
-  subject { described_class.new(namespace.owner, { namespace_id: namespace.id, path: path, file: file }) }
+  let(:import_params) { { namespace_id: namespace.id, path: path, file: file } }
+  subject { described_class.new(namespace.owner, import_params) }
 
   describe '#execute' do
     context 'with an invalid path' do
@@ -18,8 +20,6 @@ describe Projects::GitlabProjectsImportService do
     end
 
     context 'with a valid path' do
-      let(:path) { 'test-path' }
-
       it 'creates a project' do
         project = subject.execute
 
@@ -27,5 +27,15 @@ describe Projects::GitlabProjectsImportService do
         expect(project).to be_valid
       end
     end
+
+    context 'override params' do
+      it 'stores them as import data when passed' do
+        project = described_class
+                    .new(namespace.owner, import_params, description: 'Hello')
+                    .execute
+
+        expect(project.import_data.data['override_params']['description']).to eq('Hello')
+      end
+    end
   end
 end