Handle snippet file path errors in services

Invalid snippet file names will raise an error in Gitaly,
this error should be relayed (in a formatted manner) to the
user so they may rectify the issue

app/controllers/concerns/snippets_actions.rb

@@ -65,11 +65,12 @@ module SnippetsActions
     params[:line_ending] == 'raw' ? content : content.gsub(/\r\n/, "\n")
   end
 
-  def check_repository_error
-    repository_errors = Array(snippet.errors.delete(:repository))
+  def handle_repository_error(action)
+    errors = Array(snippet.errors.delete(:repository))
 
-    flash.now[:alert] = repository_errors.first if repository_errors.present?
-    recaptcha_check_with_fallback(repository_errors.empty?) { render :edit }
+    flash.now[:alert] = errors.first if errors.present?
+
+    recaptcha_check_with_fallback(errors.empty?) { render action }
   end
 
   def redirect_if_binary

app/controllers/projects/snippets_controller.rb

@@ -52,15 +52,8 @@ class Projects::SnippetsController < Projects::ApplicationController
     create_params = snippet_params.merge(spammable_params)
     service_response = Snippets::CreateService.new(project, current_user, create_params).execute
     @snippet = service_response.payload[:snippet]
-    repository_operation_error = service_response.error? && !@snippet.persisted? && @snippet.valid?
 
-    if repository_operation_error
-      flash.now[:alert] = service_response.message
-
-      render :new
-    else
-      recaptcha_check_with_fallback { render :new }
-    end
+    handle_repository_error(:new)
   end
 
   def update
@@ -69,7 +62,7 @@ class Projects::SnippetsController < Projects::ApplicationController
     service_response = Snippets::UpdateService.new(project, current_user, update_params).execute(@snippet)
     @snippet = service_response.payload[:snippet]
 
-    check_repository_error
+    handle_repository_error(:edit)
   end
 
   def show

app/controllers/snippets_controller.rb

@@ -52,12 +52,9 @@ class SnippetsController < ApplicationController
     create_params = snippet_params.merge(spammable_params)
     service_response = Snippets::CreateService.new(nil, current_user, create_params).execute
     @snippet = service_response.payload[:snippet]
-    repository_operation_error = service_response.error? && !@snippet.persisted? && @snippet.valid?
 
-    if repository_operation_error
-      flash.now[:alert] = service_response.message
-
-      render :new
+    if service_response.error? && @snippet.errors[:repository].present?
+      handle_repository_error(:new)
     else
       move_temporary_files if @snippet.valid? && params[:files]
 
@@ -71,7 +68,7 @@ class SnippetsController < ApplicationController
     service_response = Snippets::UpdateService.new(nil, current_user, update_params).execute(@snippet)
     @snippet = service_response.payload[:snippet]
 
-    check_repository_error
+    handle_repository_error(:edit)
   end
 
   def show

app/models/snippet_repository.rb

@@ -44,6 +44,9 @@ class SnippetRepository < ApplicationRecord
          Gitlab::Git::PreReceiveError,
          Gitlab::Git::CommandError,
          ArgumentError => error
+
+    logger.error(message: "Snippet git error. Reason: #{error.message}", snippet: snippet.id)
+
     raise commit_error_exception(error)
   end
 
@@ -91,7 +94,7 @@ class SnippetRepository < ApplicationRecord
 
   def commit_error_exception(err)
     if invalid_path_error?(err)
-      InvalidPathError.new('Invalid Path') # To avoid returning the message with the path included
+      InvalidPathError.new('Invalid file name') # To avoid returning the message with the path included
     elsif invalid_signature_error?(err)
       InvalidSignatureError.new(err.message)
     else

app/services/snippets/base_service.rb

@@ -11,5 +11,22 @@ module Snippets
         payload: { snippet: snippet }
       )
     end
+
+    def add_snippet_repository_error(snippet:, error:)
+      message = repository_error_message(error)
+
+      snippet.errors.add(:repository, message)
+    end
+
+    def repository_error_message(error)
+      message = self.is_a?(Snippets::CreateService) ? _("Error creating the snippet") : _("Error updating the snippet")
+
+      # We only want to include additional error detail in the message
+      # if the error is not a CommitError because we cannot guarantee the message
+      # will be user-friendly
+      message += " - #{error.message}" unless error.instance_of?(SnippetRepository::CommitError)
+
+      message
+    end
   end
 end

app/services/snippets/create_service.rb

@@ -60,7 +60,7 @@ module Snippets
         @snippet = @snippet.dup
       end
 
-      @snippet.errors.add(:base, e.message)
+      add_snippet_repository_error(snippet: @snippet, error: e)
 
       false
     end

app/services/snippets/update_service.rb

@@ -54,7 +54,8 @@ module Snippets
         snippet.save
       end
 
-      snippet.errors.add(:repository, 'Error updating the snippet')
+      add_snippet_repository_error(snippet: snippet, error: e)
+
       log_error(e.message)
 
       # If the commit action failed we remove it because

changelogs/unreleased/vij-snippet-create-update-errors.yml

+---
+title: Modify Snippet git path errors to be more helpful
+merge_request: 31333
+author:
+type: changed

locale/gitlab.pot

@@ -8,8 +8,6 @@ msgid ""
 msgstr ""
 "Project-Id-Version: gitlab 1.0.0\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2020-04-24 17:33-0400\n"
-"PO-Revision-Date: 2020-04-24 17:33-0400\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"
 "Language: \n"
@@ -8449,6 +8447,9 @@ msgstr ""
 msgid "Error creating new iteration"
 msgstr ""
 
+msgid "Error creating the snippet"
+msgstr ""
+
 msgid "Error deleting %{issuableType}"
 msgstr ""
 
@@ -8569,6 +8570,9 @@ msgstr ""
 msgid "Error updating status of to-do item."
 msgstr ""
 
+msgid "Error updating the snippet"
+msgstr ""
+
 msgid "Error uploading file"
 msgstr ""
 

spec/features/projects/snippets/create_snippet_spec.rb

@@ -99,7 +99,7 @@ shared_examples_for 'snippet editor' do
     end
 
     context 'when the git operation fails' do
-      let(:error) { 'This is a git error' }
+      let(:error) { 'Error creating the snippet' }
 
       before do
         allow_next_instance_of(Snippets::CreateService) do |instance|

spec/features/projects/snippets/user_updates_snippet_spec.rb

@@ -42,7 +42,7 @@ describe 'Projects > Snippets > User updates a snippet', :js do
   context 'when the git operation fails' do
     before do
       allow_next_instance_of(Snippets::UpdateService) do |instance|
-        allow(instance).to receive(:create_commit).and_raise(StandardError)
+        allow(instance).to receive(:create_commit).and_raise(StandardError, 'Error Message')
       end
 
       fill_in('project_snippet_title', with: 'Snippet new title')
@@ -52,7 +52,7 @@ describe 'Projects > Snippets > User updates a snippet', :js do
     end
 
     it 'renders edit page and displays the error' do
-      expect(page.find('.flash-container span').text).to eq('Error updating the snippet')
+      expect(page.find('.flash-container span').text).to eq('Error updating the snippet - Error Message')
       expect(page).to have_content('Edit Snippet')
     end
   end

spec/features/snippets/user_creates_snippet_spec.rb

@@ -79,7 +79,7 @@ shared_examples_for 'snippet editor' do
   end
 
   context 'when the git operation fails' do
-    let(:error) { 'This is a git error' }
+    let(:error) { 'Error creating the snippet' }
 
     before do
       allow_next_instance_of(Snippets::CreateService) do |instance|

spec/features/snippets/user_edits_snippet_spec.rb

@@ -73,7 +73,7 @@ describe 'User edits snippet', :js do
   context 'when the git operation fails' do
     before do
       allow_next_instance_of(Snippets::UpdateService) do |instance|
-        allow(instance).to receive(:create_commit).and_raise(StandardError)
+        allow(instance).to receive(:create_commit).and_raise(StandardError, 'Error Message')
       end
 
       fill_in 'personal_snippet_title', with: 'New Snippet Title'
@@ -83,7 +83,7 @@ describe 'User edits snippet', :js do
     end
 
     it 'renders edit page and displays the error' do
-      expect(page.find('.flash-container span').text).to eq('Error updating the snippet')
+      expect(page.find('.flash-container span').text).to eq('Error updating the snippet - Error Message')
       expect(page).to have_content('Edit Snippet')
     end
   end

spec/services/snippets/create_service_spec.rb

@@ -169,8 +169,8 @@ describe Snippets::CreateService do
           expect { subject }.not_to change { Snippet.count }
         end
 
-        it 'returns the error' do
-          expect(snippet.errors.full_messages).to include('Repository could not be created')
+        it 'returns a generic creation error' do
+          expect(snippet.errors[:repository]).to eq ['Error creating the snippet - Repository could not be created']
         end
 
         it 'does not return a snippet with an id' do
@@ -178,6 +178,14 @@ describe Snippets::CreateService do
         end
       end
 
+      context 'when repository creation fails with invalid file name' do
+        let(:extra_opts) { { file_name: 'invalid://file/name/here' } }
+
+        it 'returns an appropriate error' do
+          expect(snippet.errors[:repository]).to eq ['Error creating the snippet - Invalid file name']
+        end
+      end
+
       context 'when the commit action fails' do
         before do
           allow_next_instance_of(SnippetRepository) do |instance|
@@ -209,11 +217,11 @@ describe Snippets::CreateService do
           subject
         end
 
-        it 'returns the error' do
+        it 'returns a generic error' do
           response = subject
 
           expect(response).to be_error
-          expect(response.payload[:snippet].errors.full_messages).to eq ['foobar']
+          expect(response.payload[:snippet].errors[:repository]).to eq ['Error creating the snippet']
         end
       end
 

spec/services/snippets/update_service_spec.rb

@@ -121,7 +121,7 @@ describe Snippets::UpdateService do
           response = subject
 
           expect(response).to be_error
-          expect(response.payload[:snippet].errors[:repository].to_sentence).to eq 'Error updating the snippet'
+          expect(response.payload[:snippet].errors[:repository].to_sentence).to eq 'Error updating the snippet - Repository could not be created'
         end
 
         it 'does not try to commit file' do