Merge branch 'fj-add-snippet-files-param-to-snippet-create-service' into 'master'

Refactor Snippets::CreateService to allow multiple files

See merge request gitlab-org/gitlab!32649

app/models/snippet_input_action.rb

+# frozen_string_literal: true
+
+class SnippetInputAction
+  include ActiveModel::Validations
+
+  ACTIONS = %w[create update delete move].freeze
+
+  ACTIONS.each do |action_const|
+    define_method "#{action_const}_action?" do
+      action == action_const
+    end
+  end
+
+  attr_reader :action, :previous_path, :file_path, :content
+
+  validates :action, inclusion: { in: ACTIONS, message: "%{value} is not a valid action" }
+  validates :previous_path, presence: true, if: :move_action?
+  validates :file_path, presence: true
+  validates :content, presence: true, if: :create_action?
+
+  def initialize(action: nil, previous_path: nil, file_path: nil, content: nil)
+    @action = action
+    @previous_path = previous_path
+    @file_path = file_path
+    @content = content
+  end
+
+  def to_commit_action
+    {
+      action: action&.to_sym,
+      previous_path: previous_path,
+      file_path: file_path,
+      content: content
+    }
+  end
+end

app/models/snippet_input_action_collection.rb

+# frozen_string_literal: true
+
+class SnippetInputActionCollection
+  include Gitlab::Utils::StrongMemoize
+
+  attr_reader :actions
+
+  delegate :empty?, to: :actions
+
+  def initialize(actions = [])
+    @actions = actions.map { |action| SnippetInputAction.new(action) }
+  end
+
+  def to_commit_actions
+    strong_memoize(:commit_actions) do
+      actions.map { |action| action.to_commit_action }
+    end
+  end
+
+  def valid?
+    strong_memoize(:valid) do
+      actions.all?(&:valid?)
+    end
+  end
+end

app/services/snippets/base_service.rb

@@ -6,12 +6,13 @@ module Snippets
 
     CreateRepositoryError = Class.new(StandardError)
 
-    attr_reader :uploaded_files
+    attr_reader :uploaded_assets, :snippet_files
 
     def initialize(project, user = nil, params = {})
       super
 
-      @uploaded_files = Array(@params.delete(:files).presence)
+      @uploaded_assets = Array(@params.delete(:files).presence)
+      @snippet_files = SnippetInputActionCollection.new(Array(@params.delete(:snippet_files).presence))
 
       filter_spam_check_params
     end
@@ -22,12 +23,30 @@ module Snippets
       Gitlab::VisibilityLevel.allowed_for?(current_user, visibility_level)
     end
 
-    def error_forbidden_visibility(snippet)
+    def forbidden_visibility_error(snippet)
       deny_visibility_level(snippet)
 
       snippet_error_response(snippet, 403)
     end
 
+    def valid_params?
+      return true if snippet_files.empty?
+
+      (params.keys & [:content, :file_name]).none? && snippet_files.valid?
+    end
+
+    def invalid_params_error(snippet)
+      if snippet_files.valid?
+        [:content, :file_name].each do |key|
+          snippet.errors.add(key, 'and snippet files cannot be used together') if params.key?(key)
+        end
+      else
+        snippet.errors.add(:snippet_files, 'have invalid data')
+      end
+
+      snippet_error_response(snippet, 403)
+    end
+
     def snippet_error_response(snippet, http_status)
       ServiceResponse.error(
         message: snippet.errors.full_messages.to_sentence,
@@ -52,5 +71,13 @@ module Snippets
 
       message
     end
+
+    def files_to_commit
+      snippet_files.to_commit_actions.presence || build_actions_from_params
+    end
+
+    def build_actions_from_params
+      raise NotImplementedError
+    end
   end
 end

app/services/snippets/create_service.rb

@@ -5,8 +5,10 @@ module Snippets
     def execute
       @snippet = build_from_params
 
+      return invalid_params_error(@snippet) unless valid_params?
+
       unless visibility_allowed?(@snippet, @snippet.visibility_level)
-        return error_forbidden_visibility(@snippet)
+        return forbidden_visibility_error(@snippet)
       end
 
       @snippet.author = current_user
@@ -29,10 +31,21 @@ module Snippets
 
     def build_from_params
       if project
-        project.snippets.build(params)
+        project.snippets.build(create_params)
       else
-        PersonalSnippet.new(params)
+        PersonalSnippet.new(create_params)
+      end
     end
+
+    # If the snippet_files param is present
+    # we need to fill content and file_name from
+    # the model
+    def create_params
+      return params if snippet_files.empty?
+
+      first_file = snippet_files.actions.first
+
+      params.merge(content: first_file.content, file_name: first_file.file_path)
     end
 
     def save_and_commit
@@ -75,19 +88,19 @@ module Snippets
         message: 'Initial commit'
       }
 
-      @snippet.snippet_repository.multi_files_action(current_user, snippet_files, commit_attrs)
-    end
-
-    def snippet_files
-      [{ file_path: params[:file_name], content: params[:content] }]
+      @snippet.snippet_repository.multi_files_action(current_user, files_to_commit, commit_attrs)
     end
 
     def move_temporary_files
       return unless @snippet.is_a?(PersonalSnippet)
 
-      uploaded_files.each do |file|
+      uploaded_assets.each do |file|
         FileMover.new(file, from_model: current_user, to_model: @snippet).execute
       end
     end
+
+    def build_actions_from_params
+      [{ file_path: params[:file_name], content: params[:content] }]
+    end
   end
 end

app/services/snippets/update_service.rb

@@ -8,7 +8,7 @@ module Snippets
 
     def execute(snippet)
       if visibility_changed?(snippet) && !visibility_allowed?(snippet, visibility_level)
-        return error_forbidden_visibility(snippet)
+        return forbidden_visibility_error(snippet)
       end
 
       snippet.assign_attributes(params)

changelogs/unreleased/fj-add-snippet-files-param-to-snippet-create-service.yml

+---
+title: Allow the snippet create service to accept an array of files
+merge_request: 32649
+author:
+type: changed

spec/models/snippet_input_action_collection_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe SnippetInputActionCollection do
+  let(:action_name)    { 'create' }
+  let(:action)         { { action: action_name, file_path: 'foo', content: 'bar', previous_path: 'foobar' } }
+  let(:data)           { [action, action] }
+
+  it { is_expected.to delegate_method(:empty?).to(:actions) }
+
+  describe '#to_commit_actions' do
+    subject { described_class.new(data).to_commit_actions}
+
+    it 'translates all actions to commit actions' do
+      transformed_action = action.merge(action: action_name.to_sym)
+
+      expect(subject).to eq [transformed_action, transformed_action]
+    end
+  end
+
+  describe '#valid?' do
+    subject { described_class.new(data).valid?}
+
+    it 'returns true' do
+      expect(subject).to be true
+    end
+
+    context 'when any of the actions is invalid' do
+      let(:data) { [action, { action: 'foo' }, action]}
+
+      it 'returns false' do
+        expect(subject).to be false
+      end
+    end
+  end
+end

spec/models/snippet_input_action_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe SnippetInputAction do
+  describe 'validations' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:action, :file_path, :content, :previous_path, :is_valid) do
+      'create' | 'foobar'  | 'foobar' | 'foobar' | true
+      'move'   | 'foobar'  | 'foobar' | 'foobar' | true
+      'delete' | 'foobar'  | 'foobar' | 'foobar' | true
+      'update' | 'foobar'  | 'foobar' | 'foobar' | true
+      'foo'    | 'foobar'  | 'foobar' | 'foobar' | false
+      nil      | 'foobar'  | 'foobar' | 'foobar' | false
+      ''       | 'foobar'  | 'foobar' | 'foobar' | false
+      'move'   | 'foobar'  | 'foobar' | nil      | false
+      'move'   | 'foobar'  | 'foobar' | ''       | false
+      'create' | 'foobar'  | nil      | 'foobar' | false
+      'create' | 'foobar'  | ''       | 'foobar' | false
+      'create' | nil       | 'foobar' | 'foobar' | false
+      'create' | ''        | 'foobar' | 'foobar' | false
+    end
+
+    with_them do
+      subject { described_class.new(action: action, file_path: file_path, content: content, previous_path: previous_path).valid? }
+
+      specify { is_expected.to be is_valid}
+    end
+  end
+
+  describe '#to_commit_action' do
+    let(:action)        { 'create' }
+    let(:file_path)     { 'foo' }
+    let(:content)       { 'bar' }
+    let(:previous_path) { 'previous_path' }
+    let(:options)       { { action: action, file_path: file_path, content: content, previous_path: previous_path } }
+
+    subject { described_class.new(options).to_commit_action }
+
+    it 'transforms attributes to commit action' do
+      expect(subject).to eq(options.merge(action: action.to_sym))
+    end
+  end
+end

spec/services/snippets/create_service_spec.rb

@@ -109,7 +109,7 @@ describe Snippets::CreateService do
         expect(snippet.repository.exists?).to be_truthy
       end
 
-      it 'commit the files to the repository' do
+      it 'commits the files to the repository' do
         subject
 
         blob = snippet.repository.blob_at('master', base_opts[:file_name])
@@ -230,6 +230,61 @@ describe Snippets::CreateService do
       end
     end
 
+    shared_examples 'when snippet_files param is present' do
+      let(:file_path) { 'snippet_file_path.rb' }
+      let(:content) { 'snippet_content' }
+      let(:snippet_files) { [{ action: 'create', file_path: file_path, content: content }] }
+      let(:base_opts) do
+        {
+          title: 'Test snippet',
+          visibility_level: Gitlab::VisibilityLevel::PRIVATE,
+          snippet_files: snippet_files
+        }
+      end
+
+      it 'creates a snippet with the provided attributes' do
+        expect(snippet.title).to eq(opts[:title])
+        expect(snippet.visibility_level).to eq(opts[:visibility_level])
+        expect(snippet.file_name).to eq(file_path)
+        expect(snippet.content).to eq(content)
+      end
+
+      it 'commit the files to the repository' do
+        subject
+
+        blob = snippet.repository.blob_at('master', file_path)
+
+        expect(blob.data).to eq content
+      end
+
+      context 'when content or file_name params are present' do
+        let(:extra_opts) { { content: 'foo', file_name: 'path' } }
+
+        it 'a validation error is raised' do
+          response = subject
+          snippet = response.payload[:snippet]
+
+          expect(response).to be_error
+          expect(snippet.errors.full_messages_for(:content)).to eq ['Content and snippet files cannot be used together']
+          expect(snippet.errors.full_messages_for(:file_name)).to eq ['File name and snippet files cannot be used together']
+          expect(snippet.repository.exists?).to be_falsey
+        end
+      end
+
+      context 'when snippet_files param is invalid' do
+        let(:snippet_files) { [{ action: 'invalid_action', file_path: 'snippet_file_path.rb', content: 'snippet_content' }] }
+
+        it 'a validation error is raised' do
+          response = subject
+          snippet = response.payload[:snippet]
+
+          expect(response).to be_error
+          expect(snippet.errors.full_messages_for(:snippet_files)).to eq ['Snippet files have invalid data']
+          expect(snippet.repository.exists?).to be_falsey
+        end
+      end
+    end
+
     context 'when ProjectSnippet' do
       let_it_be(:project) { create(:project) }
 
@@ -244,6 +299,7 @@ describe Snippets::CreateService do
       it_behaves_like 'an error service response when save fails'
       it_behaves_like 'creates repository and files'
       it_behaves_like 'after_save callback to store_mentions', ProjectSnippet
+      it_behaves_like 'when snippet_files param is present'
 
       context 'when uploaded files are passed to the service' do
         let(:extra_opts) { { files: ['foo'] } }
@@ -270,6 +326,7 @@ describe Snippets::CreateService do
       it_behaves_like 'an error service response when save fails'
       it_behaves_like 'creates repository and files'
       it_behaves_like 'after_save callback to store_mentions', PersonalSnippet
+      it_behaves_like 'when snippet_files param is present'
 
       context 'when the snippet description contains files' do
         include FileMoverHelpers