Merge branch 'fj-39265-update-snippet-repository-content' into 'master'

Update files when snippet is updated

See merge request gitlab-org/gitlab!23993

app/models/snippet.rb

@@ -281,11 +281,10 @@ class Snippet < ApplicationRecord
   end
 
   def create_repository
-    return if repository_exists?
+    return if repository_exists? && snippet_repository
 
     repository.create_if_not_exists
-
-    track_snippet_repository if repository_exists?
+    track_snippet_repository
   end
 
   def track_snippet_repository

app/services/snippets/update_service.rb

@@ -4,6 +4,9 @@ module Snippets
   class UpdateService < Snippets::BaseService
     include SpamCheckMethods
 
+    UpdateError = Class.new(StandardError)
+    CreateRepositoryError = Class.new(StandardError)
+
     def execute(snippet)
       # check that user is allowed to set specified visibility_level
       new_visibility = visibility_level
@@ -20,11 +23,7 @@ module Snippets
       snippet.assign_attributes(params)
       spam_check(snippet, current_user)
 
-      snippet_saved = snippet.with_transaction_returning_status do
-        snippet.save
-      end
-
-      if snippet_saved
+      if save_and_commit(snippet)
         Gitlab::UsageDataCounters::SnippetCounter.count(:update)
 
         ServiceResponse.success(payload: { snippet: snippet } )
@@ -32,5 +31,54 @@ module Snippets
         snippet_error_response(snippet, 400)
       end
     end
+
+    private
+
+    def save_and_commit(snippet)
+      snippet.with_transaction_returning_status do
+        snippet.save.tap do |saved|
+          break false unless saved
+
+          # In order to avoid non migrated snippets scenarios,
+          # if the snippet does not have a repository we created it
+          # We don't need to check if the repository exists
+          # because `create_repository` already handles it
+          if Feature.enabled?(:version_snippets, current_user)
+            create_repository_for(snippet)
+          end
+
+          # If the snippet repository exists we commit always
+          # the changes
+          create_commit(snippet) if snippet.repository_exists?
+        end
+      rescue
+        snippet.errors.add(:base, 'Error updating the snippet')
+
+        false
+      end
+    end
+
+    def create_repository_for(snippet)
+      snippet.create_repository
+
+      raise CreateRepositoryError, 'Repository could not be created' unless snippet.repository_exists?
+    end
+
+    def create_commit(snippet)
+      raise UpdateError unless snippet.snippet_repository
+
+      commit_attrs = {
+        branch_name: 'master',
+        message: 'Update snippet'
+      }
+
+      snippet.snippet_repository.multi_files_action(current_user, snippet_files(snippet), commit_attrs)
+    end
+
+    def snippet_files(snippet)
+      [{ previous_path: snippet.blobs.first&.path,
+         file_path: params[:file_name],
+         content: params[:content] }]
+    end
   end
 end

changelogs/unreleased/fj-39265-update-snippet-repository-content.yml

+---
+title: Update files when snippet is updated
+merge_request: 23993
+author:
+type: changed

spec/models/snippet_spec.rb

@@ -601,10 +601,23 @@ describe Snippet do
         expect(snippet.create_repository).to be_nil
       end
 
-      it 'does not track snippet repository' do
-        expect do
-          snippet.create_repository
-        end.not_to change(SnippetRepository, :count)
+      context 'when snippet_repository exists' do
+        it 'does not create a new snippet repository' do
+          expect do
+            snippet.create_repository
+          end.not_to change(SnippetRepository, :count)
+        end
+      end
+
+      context 'when snippet_repository does not exist' do
+        it 'creates a snippet_repository' do
+          snippet.snippet_repository.destroy
+          snippet.reload
+
+          expect do
+            snippet.create_repository
+          end.to change(SnippetRepository, :count).by(1)
+        end
       end
     end
   end

spec/requests/api/graphql/mutations/snippets/update_spec.rb

@@ -91,7 +91,7 @@ describe 'Updating a Snippet' do
 
   describe 'PersonalSnippet' do
     it_behaves_like 'graphql update actions' do
-      let_it_be(:snippet) do
+      let(:snippet) do
         create(:personal_snippet,
                :private,
                file_name: original_file_name,
@@ -104,7 +104,7 @@ describe 'Updating a Snippet' do
 
   describe 'ProjectSnippet' do
     let_it_be(:project) { create(:project, :private) }
-    let_it_be(:snippet) do
+    let(:snippet) do
       create(:project_snippet,
              :private,
              project: project,

spec/requests/api/project_snippets_spec.rb

@@ -278,13 +278,13 @@ describe API::ProjectSnippets do
 
   describe 'PUT /projects/:project_id/snippets/:id/' do
     let(:visibility_level) { Snippet::PUBLIC }
-    let(:snippet) { create(:project_snippet, author: admin, visibility_level: visibility_level, project: project) }
+    let(:snippet) { create(:project_snippet, :repository, author: admin, visibility_level: visibility_level, project: project) }
 
     it 'updates snippet' do
       new_content = 'New content'
       new_description = 'New description'
 
-      put api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin), params: { code: new_content, description: new_description, visibility: 'private' }
+      update_snippet(params: { code: new_content, description: new_description, visibility: 'private' })
 
       expect(response).to have_gitlab_http_status(:ok)
       snippet.reload
@@ -297,7 +297,7 @@ describe API::ProjectSnippets do
       new_content = 'New content'
       new_description = 'New description'
 
-      put api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin), params: { content: new_content, description: new_description }
+      update_snippet(params: { content: new_content, description: new_description })
 
       expect(response).to have_gitlab_http_status(:ok)
       snippet.reload
@@ -306,21 +306,21 @@ describe API::ProjectSnippets do
     end
 
     it 'returns 400 when both code and content parameters specified' do
-      put api("/projects/#{snippet.project.id}/snippets/1234", admin), params: { code: 'some content', content: 'other content' }
+      update_snippet(params: { code: 'some content', content: 'other content' })
 
       expect(response).to have_gitlab_http_status(:bad_request)
       expect(json_response['error']).to eq('code, content are mutually exclusive')
     end
 
     it 'returns 404 for invalid snippet id' do
-      put api("/projects/#{snippet.project.id}/snippets/1234", admin), params: { title: 'foo' }
+      update_snippet(snippet_id: '1234', params: { title: 'foo' })
 
       expect(response).to have_gitlab_http_status(:not_found)
       expect(json_response['message']).to eq('404 Snippet Not Found')
     end
 
     it 'returns 400 for missing parameters' do
-      put api("/projects/#{project.id}/snippets/1234", admin)
+      update_snippet
 
       expect(response).to have_gitlab_http_status(:bad_request)
     end
@@ -328,16 +328,16 @@ describe API::ProjectSnippets do
     it 'returns 400 for empty code field' do
       new_content = ''
 
-      put api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin), params: { code: new_content }
+      update_snippet(params: { code: new_content })
 
       expect(response).to have_gitlab_http_status(:bad_request)
     end
 
-    context 'when the snippet is spam' do
-      def update_snippet(snippet_params = {})
-        put api("/projects/#{snippet.project.id}/snippets/#{snippet.id}", admin), params: snippet_params
-      end
+    it_behaves_like 'update with repository actions' do
+      let(:snippet_without_repo) { create(:project_snippet, author: admin, project: project, visibility_level: visibility_level) }
+    end
 
+    context 'when the snippet is spam' do
       before do
         allow_next_instance_of(Spam::AkismetService) do |instance|
           allow(instance).to receive(:spam?).and_return(true)
@@ -348,7 +348,7 @@ describe API::ProjectSnippets do
         let(:visibility_level) { Snippet::PRIVATE }
 
         it 'creates the snippet' do
-          expect { update_snippet(title: 'Foo') }
+          expect { update_snippet(params: { title: 'Foo' }) }
             .to change { snippet.reload.title }.to('Foo')
         end
       end
@@ -357,12 +357,12 @@ describe API::ProjectSnippets do
         let(:visibility_level) { Snippet::PUBLIC }
 
         it 'rejects the snippet' do
-          expect { update_snippet(title: 'Foo') }
+          expect { update_snippet(params: { title: 'Foo' }) }
             .not_to change { snippet.reload.title }
         end
 
         it 'creates a spam log' do
-          expect { update_snippet(title: 'Foo') }
+          expect { update_snippet(params: { title: 'Foo' }) }
             .to log_spam(title: 'Foo', user_id: admin.id, noteable_type: 'ProjectSnippet')
         end
       end
@@ -371,7 +371,7 @@ describe API::ProjectSnippets do
         let(:visibility_level) { Snippet::PRIVATE }
 
         it 'rejects the snippet' do
-          expect { update_snippet(title: 'Foo', visibility: 'public') }
+          expect { update_snippet(params: { title: 'Foo', visibility: 'public' }) }
             .not_to change { snippet.reload.title }
 
           expect(response).to have_gitlab_http_status(:bad_request)
@@ -379,7 +379,7 @@ describe API::ProjectSnippets do
         end
 
         it 'creates a spam log' do
-          expect { update_snippet(title: 'Foo', visibility: 'public') }
+          expect { update_snippet(params: { title: 'Foo', visibility: 'public' }) }
             .to log_spam(title: 'Foo', user_id: admin.id, noteable_type: 'ProjectSnippet')
         end
       end
@@ -390,6 +390,10 @@ describe API::ProjectSnippets do
         let(:request) { put api("/projects/#{project_no_snippets.id}/snippets/123", admin), params: { description: 'foo' } }
       end
     end
+
+    def update_snippet(snippet_id: snippet.id, params: {})
+      put api("/projects/#{snippet.project.id}/snippets/#{snippet_id}", admin), params: params
+    end
   end
 
   describe 'DELETE /projects/:project_id/snippets/:id/' do

spec/requests/api/snippets_spec.rb

@@ -301,7 +301,7 @@ describe API::Snippets do
     let(:visibility_level) { Snippet::PUBLIC }
     let(:other_user) { create(:user) }
     let(:snippet) do
-      create(:personal_snippet, author: user, visibility_level: visibility_level)
+      create(:personal_snippet, :repository, author: user, visibility_level: visibility_level)
     end
 
     shared_examples 'snippet updates' do
@@ -309,7 +309,7 @@ describe API::Snippets do
         new_content = 'New content'
         new_description = 'New description'
 
-        put api("/snippets/#{snippet.id}", user), params: { content: new_content, description: new_description, visibility: 'internal' }
+        update_snippet(params: { content: new_content, description: new_description, visibility: 'internal' })
 
         expect(response).to have_gitlab_http_status(:ok)
         snippet.reload
@@ -332,30 +332,30 @@ describe API::Snippets do
     it_behaves_like 'snippet updates'
 
     it 'returns 404 for invalid snippet id' do
-      put api("/snippets/1234", user), params: { title: 'foo' }
+      update_snippet(snippet_id: '1234', params: { title: 'Foo' })
 
       expect(response).to have_gitlab_http_status(:not_found)
       expect(json_response['message']).to eq('404 Snippet Not Found')
     end
 
     it "returns 404 for another user's snippet" do
-      put api("/snippets/#{snippet.id}", other_user), params: { title: 'fubar' }
+      update_snippet(requester: other_user, params: { title: 'foobar' })
 
       expect(response).to have_gitlab_http_status(:not_found)
       expect(json_response['message']).to eq('404 Snippet Not Found')
     end
 
     it 'returns 400 for missing parameters' do
-      put api("/snippets/1234", user)
+      update_snippet
 
       expect(response).to have_gitlab_http_status(:bad_request)
     end
 
-    context 'when the snippet is spam' do
-      def update_snippet(snippet_params = {})
-        put api("/snippets/#{snippet.id}", user), params: snippet_params
-      end
+    it_behaves_like 'update with repository actions' do
+      let(:snippet_without_repo) { create(:personal_snippet, author: user, visibility_level: visibility_level) }
+    end
 
+    context 'when the snippet is spam' do
       before do
         allow_next_instance_of(Spam::AkismetService) do |instance|
           allow(instance).to receive(:spam?).and_return(true)
@@ -366,7 +366,7 @@ describe API::Snippets do
         let(:visibility_level) { Snippet::PRIVATE }
 
         it 'updates the snippet' do
-          expect { update_snippet(title: 'Foo') }
+          expect { update_snippet(params: { title: 'Foo' }) }
             .to change { snippet.reload.title }.to('Foo')
         end
       end
@@ -375,7 +375,7 @@ describe API::Snippets do
         let(:visibility_level) { Snippet::PUBLIC }
 
         it 'rejects the shippet' do
-          expect { update_snippet(title: 'Foo') }
+          expect { update_snippet(params: { title: 'Foo' }) }
             .not_to change { snippet.reload.title }
 
           expect(response).to have_gitlab_http_status(:bad_request)
@@ -383,7 +383,7 @@ describe API::Snippets do
         end
 
         it 'creates a spam log' do
-          expect { update_snippet(title: 'Foo') }.to log_spam(title: 'Foo', user_id: user.id, noteable_type: 'PersonalSnippet')
+          expect { update_snippet(params: { title: 'Foo' }) }.to log_spam(title: 'Foo', user_id: user.id, noteable_type: 'PersonalSnippet')
         end
       end
 
@@ -391,16 +391,20 @@ describe API::Snippets do
         let(:visibility_level) { Snippet::PRIVATE }
 
         it 'rejects the snippet' do
-          expect { update_snippet(title: 'Foo', visibility: 'public') }
+          expect { update_snippet(params: { title: 'Foo', visibility: 'public' }) }
             .not_to change { snippet.reload.title }
         end
 
         it 'creates a spam log' do
-          expect { update_snippet(title: 'Foo', visibility: 'public') }
+          expect { update_snippet(params: { title: 'Foo', visibility: 'public' }) }
             .to log_spam(title: 'Foo', user_id: user.id, noteable_type: 'PersonalSnippet')
         end
       end
     end
+
+    def update_snippet(snippet_id: snippet.id, params: {}, requester: user)
+      put api("/snippets/#{snippet_id}", requester), params: params
+    end
   end
 
   describe 'DELETE /snippets/:id' do

spec/services/snippets/update_service_spec.rb

@@ -16,14 +16,9 @@ describe Snippets::UpdateService do
       }
     end
     let(:updater) { user }
+    let(:service) { Snippets::UpdateService.new(project, updater, options) }
 
-    subject do
-      described_class.new(
-        project,
-        updater,
-        options
-      ).execute(snippet)
-    end
+    subject { service.execute(snippet) }
 
     shared_examples 'a service that updates a snippet' do
       it 'updates a snippet with the provided attributes' do
@@ -98,9 +93,109 @@ describe Snippets::UpdateService do
       end
     end
 
+    shared_examples 'creates repository and creates file' do
+      it 'creates repository' do
+        expect(snippet.repository).not_to exist
+
+        subject
+
+        expect(snippet.repository).to exist
+      end
+
+      it 'commits the files to the repository' do
+        subject
+
+        expect(snippet.blobs.count).to eq 1
+
+        blob = snippet.repository.blob_at('master', options[:file_name])
+
+        expect(blob.data).to eq options[:content]
+      end
+
+      context 'when the repository does not exist' do
+        it 'does not try to commit file' do
+          allow(snippet).to receive(:repository_exists?).and_return(false)
+
+          expect(service).not_to receive(:create_commit)
+
+          subject
+        end
+      end
+
+      context 'when feature flag is disabled' do
+        before do
+          stub_feature_flags(version_snippets: false)
+        end
+
+        it 'does not create repository' do
+          subject
+
+          expect(snippet.repository).not_to exist
+        end
+
+        it 'does not try to commit file' do
+          expect(service).not_to receive(:create_commit)
+
+          subject
+        end
+      end
+
+      it 'returns error when the commit action fails' do
+        allow_next_instance_of(SnippetRepository) do |instance|
+          allow(instance).to receive(:multi_files_action).and_raise(SnippetRepository::CommitError)
+        end
+
+        response = subject
+
+        expect(response).to be_error
+        expect(response.payload[:snippet].errors.full_messages).to eq ['Error updating the snippet']
+      end
+    end
+
+    shared_examples 'updates repository content' do
+      it 'commit the files to the repository' do
+        blob = snippet.blobs.first
+        options[:file_name] = blob.path + '_new'
+
+        expect(blob.data).not_to eq(options[:content])
+
+        subject
+
+        blob = snippet.blobs.first
+
+        expect(blob.path).to eq(options[:file_name])
+        expect(blob.data).to eq(options[:content])
+      end
+
+      it 'returns error when the commit action fails' do
+        allow(snippet.snippet_repository).to receive(:multi_files_action).and_raise(SnippetRepository::CommitError)
+
+        response = subject
+
+        expect(response).to be_error
+        expect(response.payload[:snippet].errors.full_messages).to eq ['Error updating the snippet']
+      end
+
+      it 'returns error if snippet does not have a snippet_repository' do
+        allow(snippet).to receive(:snippet_repository).and_return(nil)
+
+        expect(subject).to be_error
+      end
+
+      context 'when the repository does not exist' do
+        it 'does not try to commit file' do
+          allow(snippet).to receive(:repository_exists?).and_return(false)
+
+          expect(service).not_to receive(:create_commit)
+
+          subject
+        end
+      end
+    end
+
     context 'when Project Snippet' do
       let_it_be(:project) { create(:project) }
-      let!(:snippet) { create(:project_snippet, author: user, project: project) }
+      let!(:snippet) { create(:project_snippet, :repository, author: user, project: project) }
 
       before do
         project.add_developer(user)
@@ -109,15 +204,29 @@ describe Snippets::UpdateService do
       it_behaves_like 'a service that updates a snippet'
       it_behaves_like 'public visibility level restrictions apply'
       it_behaves_like 'snippet update data is tracked'
+      it_behaves_like 'updates repository content'
+
+      context 'when snippet does not have a repository' do
+        let!(:snippet) { create(:project_snippet, author: user, project: project) }
+
+        it_behaves_like 'creates repository and creates file'
+      end
     end
 
     context 'when PersonalSnippet' do
       let(:project) { nil }
-      let!(:snippet) { create(:personal_snippet, author: user) }
+      let!(:snippet) { create(:personal_snippet, :repository, author: user) }
 
       it_behaves_like 'a service that updates a snippet'
       it_behaves_like 'public visibility level restrictions apply'
       it_behaves_like 'snippet update data is tracked'
+      it_behaves_like 'updates repository content'
+
+      context 'when snippet does not have a repository' do
+        let!(:snippet) { create(:personal_snippet, author: user, project: project) }
+
+        it_behaves_like 'creates repository and creates file'
+      end
     end
   end
 end

spec/support/shared_examples/requests/snippet_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'update with repository actions' do
+  context 'when the repository exists' do
+    it 'commits the changes to the repository' do
+      existing_blob = snippet.blobs.first
+      new_file_name = existing_blob.path + '_new'
+      new_content = 'New content'
+
+      update_snippet(params: { content: new_content, file_name: new_file_name })
+
+      aggregate_failures do
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(snippet.repository.blob_at('master', existing_blob.path)).to be_nil
+
+        blob = snippet.repository.blob_at('master', new_file_name)
+        expect(blob).not_to be_nil
+        expect(blob.data).to eq(new_content)
+      end
+    end
+  end
+
+  context 'when the repository does not exist' do
+    let(:snippet) { snippet_without_repo }
+
+    it 'creates the repository' do
+      update_snippet(snippet_id: snippet.id, params: { title: 'foo' })
+
+      expect(snippet.repository).to exist
+    end
+
+    it 'commits the file to the repository' do
+      content = 'New Content'
+      file_name = 'file_name.rb'
+
+      update_snippet(snippet_id: snippet.id, params: { content: content, file_name: file_name })
+
+      blob = snippet.repository.blob_at('master', file_name)
+      expect(blob).not_to be_nil
+      expect(blob.data).to eq content
+    end
+  end
+end