Add submodule update API endpoint

This new endpoint allow users to update a submodule's reference.

The MR involves adding a new operation RPC operation in gitaly-proto
(see gitlab-org/gitaly-proto!233) and change Gitaly to use this
new version (see gitlab-org/gitaly!936).

See gitlab-org/gitlab-ce!20949

Gemfile

@@ -416,7 +416,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.118.1', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.123.0', require: 'gitaly'
 gem 'grpc', '~> 1.15.0'
 
 gem 'google-protobuf', '~> 3.6'

Gemfile.lock

@@ -274,9 +274,8 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (0.118.1)
-      google-protobuf (~> 3.1)
-      grpc (~> 1.10)
+    gitaly-proto (0.123.0)
+      grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-markup (1.6.4)
     gitlab-sidekiq-fetcher (0.3.0)
@@ -1000,7 +999,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.118.1)
+  gitaly-proto (~> 0.123.0)
   github-markup (~> 1.7.0)
   gitlab-markup (~> 1.6.4)
   gitlab-sidekiq-fetcher
@@ -1158,4 +1157,4 @@ DEPENDENCIES
   wikicloth (= 0.8.1)
 
 BUNDLED WITH
-   1.16.6
+   1.17.1

Gemfile.rails5.lock

@@ -277,9 +277,8 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (0.118.1)
-      google-protobuf (~> 3.1)
-      grpc (~> 1.10)
+    gitaly-proto (0.123.0)
+      grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-markup (1.6.4)
     gitlab-sidekiq-fetcher (0.3.0)
@@ -1009,7 +1008,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.118.1)
+  gitaly-proto (~> 0.123.0)
   github-markup (~> 1.7.0)
   gitlab-markup (~> 1.6.4)
   gitlab-sidekiq-fetcher
@@ -1167,4 +1166,4 @@ DEPENDENCIES
   wikicloth (= 0.8.1)
 
 BUNDLED WITH
-   1.16.6
+   1.17.1

app/models/repository.rb

@@ -1014,6 +1014,18 @@ class Repository
                                        message: merge_request.title)
   end
 
+  def update_submodule(user, submodule, commit_sha, message:, branch:)
+    with_cache_hooks do
+      raw.update_submodule(
+        user: user,
+        submodule: submodule,
+        commit_sha: commit_sha,
+        branch: branch,
+        message: message
+      )
+    end
+  end
+
   def blob_data_at(sha, path)
     blob = blob_at(sha, path)
     return unless blob

app/services/submodules/update_service.rb

+# frozen_string_literal: true
+
+module Submodules
+  class UpdateService < Commits::CreateService
+    include Gitlab::Utils::StrongMemoize
+
+    def initialize(*args)
+      super
+
+      @start_branch = @branch_name
+      @commit_sha = params[:commit_sha].presence
+      @submodule = params[:submodule].presence
+      @commit_message = params[:commit_message].presence || "Update submodule #{@submodule} with oid #{@commit_sha}"
+    end
+
+    def validate!
+      super
+
+      raise ValidationError, 'The repository is empty' if repository.empty?
+    end
+
+    def execute
+      super
+    rescue StandardError => e
+      error(e.message)
+    end
+
+    def create_commit!
+      repository.update_submodule(current_user,
+                                  @submodule,
+                                  @commit_sha,
+                                  message: @commit_message,
+                                  branch: @branch_name)
+    rescue ArgumentError, TypeError
+      raise ValidationError, 'Invalid parameters'
+    end
+  end
+end

changelogs/unreleased/fj-41213-api-update-submodule-commit.yml

+---
+title: Add endpoint to update a git submodule reference
+merge_request: 20949
+author:
+type: added

doc/api/README.md

@@ -61,6 +61,7 @@ following locations:
 - [Protected Tags](protected_tags.md)
 - [Repositories](repositories.md)
 - [Repository Files](repository_files.md)
+- [Repository Submodules](repository_submodules.md)
 - [Runners](runners.md)
 - [Search](search.md)
 - [Services](services.md)
@@ -234,7 +235,7 @@ provided you are authenticated as an administrator with an OAuth or Personal Acc
 
 You need to pass the `sudo` parameter either via query string or a header with an ID/username of
 the user you want to perform the operation as. If passed as a header, the
-header name must be `Sudo`. 
+header name must be `Sudo`.
 
 NOTE: **Note:**
 Usernames are case insensitive.

doc/api/repository_submodules.md

+# Repository submodules API
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/41213) in GitLab 11.5
+
+## Update existing submodule reference in repository
+
+In some workflows, especially automated ones, it can be useful to update a
+submodule's reference to keep up to date other projects that use it.
+This endpoint allows you to update a [Git submodule](https://git-scm.com/book/en/v2/Git-Tools-Submodules) reference in a
+specific branch.
+
+```
+PUT /projects/:id/repository/submodules/:submodule
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
+| `submodule` | string | yes | URL encoded full path to the submodule. For example, `lib%2Fclass%2Erb` |
+| `branch` | string | yes | Name of the branch to commit into |
+| `commit_sha` | string | yes | Full commit SHA to update the submodule to |
+| `commit_message` | string | no | Commit message. If no message is provided, a default one will be set |
+
+```sh
+curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/5/repositories/submodules/lib%2Fmodules%2Fexample"
+--data "branch=master&commit_sha=3ddec28ea23acc5caa5d8331a6ecb2a65fc03e88&commit_message=Update submodule reference"
+```
+
+Example response:
+
+```json
+{
+  "id": "ed899a2f4b50b4370feeea94676502b42383c746",
+  "short_id": "ed899a2f4b5",
+  "title": "Updated submodule example_submodule with oid 3ddec28ea23acc5caa5d8331a6ecb2a65fc03e88",
+  "author_name": "Dmitriy Zaporozhets",
+  "author_email": "dzaporozhets@sphereconsultinginc.com",
+  "committer_name": "Dmitriy Zaporozhets",
+  "committer_email": "dzaporozhets@sphereconsultinginc.com",
+  "created_at": "2018-09-20T09:26:24.000-07:00",
+  "message": "Updated submodule example_submodule with oid 3ddec28ea23acc5caa5d8331a6ecb2a65fc03e88",
+  "parent_ids": [
+    "ae1d9fb46aa2b07ee9836d49862ec4e2c46fbbba"
+  ],
+  "committed_date": "2018-09-20T09:26:24.000-07:00",
+  "authored_date": "2018-09-20T09:26:24.000-07:00",  
+  "status": null
+}
+```

lib/api/api.rb

@@ -143,6 +143,7 @@ module API
     mount ::API::Settings
     mount ::API::SidekiqMetrics
     mount ::API::Snippets
+    mount ::API::Submodules
     mount ::API::Subscriptions
     mount ::API::SystemHooks
     mount ::API::Tags

lib/api/submodules.rb

+# frozen_string_literal: true
+
+module API
+  class Submodules < Grape::API
+    before { authenticate! }
+
+    helpers do
+      def commit_params(attrs)
+        {
+          submodule: attrs[:submodule],
+          commit_sha: attrs[:commit_sha],
+          branch_name: attrs[:branch],
+          commit_message: attrs[:commit_message]
+        }
+      end
+    end
+
+    params do
+      requires :id, type: String, desc: 'The project ID'
+    end
+    resource :projects, requirements: Files::FILE_ENDPOINT_REQUIREMENTS do
+      desc 'Update existing submodule reference in repository' do
+        success Entities::Commit
+      end
+      params do
+        requires :submodule, type: String, desc: 'Url encoded full path to submodule.'
+        requires :commit_sha, type: String, desc: 'Commit sha to update the submodule to.'
+        requires :branch, type: String, desc: 'Name of the branch to commit into.'
+        optional :commit_message, type: String, desc: 'Commit message. If no message is provided a default one will be set.'
+      end
+      put ":id/repository/submodules/:submodule", requirements: Files::FILE_ENDPOINT_REQUIREMENTS do
+        authorize! :push_code, user_project
+
+        submodule_params = declared_params(include_missing: false)
+
+        result = ::Submodules::UpdateService.new(user_project, current_user, commit_params(submodule_params)).execute
+
+        if result[:status] == :success
+          commit_detail = user_project.repository.commit(result[:result])
+          present commit_detail, with: Entities::CommitDetail
+        else
+          render_api_error!(result[:message], result[:http_status] || 400)
+        end
+      end
+    end
+  end
+end

lib/gitlab/git/repository.rb

@@ -571,6 +571,20 @@ module Gitlab
         end
       end
 
+      def update_submodule(user:, submodule:, commit_sha:, message:, branch:)
+        args = {
+          user: user,
+          submodule: submodule,
+          commit_sha: commit_sha,
+          branch: branch,
+          message: message
+        }
+
+        wrapped_gitaly_errors do
+          gitaly_operation_client.user_update_submodule(args)
+        end
+      end
+
       # Delete the specified branch from the repository
       def delete_branch(branch_name)
         wrapped_gitaly_errors do

lib/gitlab/gitaly_client/operation_service.rb

@@ -230,6 +230,32 @@ module Gitlab
         response.squash_sha
       end
 
+      def user_update_submodule(user:, submodule:, commit_sha:, branch:, message:)
+        request = Gitaly::UserUpdateSubmoduleRequest.new(
+          repository: @gitaly_repo,
+          user: Gitlab::Git::User.from_gitlab(user).to_gitaly,
+          commit_sha: commit_sha,
+          branch: encode_binary(branch),
+          submodule: encode_binary(submodule),
+          commit_message: encode_binary(message)
+        )
+
+        response = GitalyClient.call(
+          @repository.storage,
+          :operation_service,
+          :user_update_submodule,
+          request
+        )
+
+        if response.pre_receive_error.present?
+          raise Gitlab::Git::PreReceiveError, response.pre_receive_error
+        elsif response.commit_error.present?
+          raise Gitlab::Git::CommitError, response.commit_error
+        else
+          Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
+        end
+      end
+
       def user_commit_files(
         user, branch_name, commit_message, actions, author_email, author_name,
         start_branch_name, start_repository)

spec/requests/api/submodules_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Submodules do
+  let(:user) { create(:user) }
+  let!(:project) { create(:project, :repository, namespace: user.namespace ) }
+  let(:guest) { create(:user) { |u| project.add_guest(u) } }
+  let(:submodule) { 'six' }
+  let(:commit_sha) { 'e25eda1fece24ac7a03624ed1320f82396f35bd8' }
+  let(:branch) { 'master' }
+  let(:commit_message) { 'whatever' }
+
+  let(:params) do
+    {
+      submodule: submodule,
+      commit_sha: commit_sha,
+      branch: branch,
+      commit_message: commit_message
+    }
+  end
+
+  before do
+    project.add_developer(user)
+  end
+
+  def route(submodule = nil)
+    "/projects/#{project.id}/repository/submodules/#{submodule}"
+  end
+
+  describe "PUT /projects/:id/repository/submodule/:submodule" do
+    context 'when unauthenticated' do
+      it 'returns 401' do
+        put api(route(submodule)), params
+
+        expect(response).to have_gitlab_http_status(401)
+      end
+    end
+
+    context 'when authenticated', 'as a guest' do
+      it 'returns 403' do
+        put api(route(submodule), guest), params
+
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+
+    context 'when authenticated', 'as a developer' do
+      it 'returns 400 if params is missing' do
+        put api(route(submodule), user)
+
+        expect(response).to have_gitlab_http_status(400)
+      end
+
+      it 'returns 400 if branch is missing' do
+        put api(route(submodule), user), params.except(:branch)
+
+        expect(response).to have_gitlab_http_status(400)
+      end
+
+      it 'returns 400 if commit_sha is missing' do
+        put api(route(submodule), user), params.except(:commit_sha)
+
+        expect(response).to have_gitlab_http_status(400)
+      end
+
+      it 'returns the commmit' do
+        head_commit = project.repository.commit.id
+
+        put api(route(submodule), user), params
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['message']).to eq commit_message
+        expect(json_response['author_name']).to eq user.name
+        expect(json_response['committer_name']).to eq user.name
+        expect(json_response['parent_ids'].first).to eq head_commit
+      end
+
+      context 'when the submodule name is urlencoded' do
+        let(:submodule) { 'test_inside_folder/another_folder/six' }
+        let(:branch) { 'submodule_inside_folder' }
+        let(:encoded_submodule) { CGI.escape(submodule) }
+
+        it 'returns the commmit' do
+          expect(Submodules::UpdateService)
+            .to receive(:new)
+                  .with(any_args, hash_including(submodule: submodule))
+                  .and_call_original
+
+          put api(route(encoded_submodule), user), params
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['id']).to eq project.repository.commit(branch).id
+          expect(project.repository.blob_at(branch, submodule).id).to eq commit_sha
+        end
+      end
+    end
+  end
+end

spec/services/submodules/update_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Submodules::UpdateService do
+  let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
+  let(:user) { create(:user, :commit_email) }
+  let(:branch_name) { project.default_branch }
+  let(:submodule) { 'six' }
+  let(:commit_sha) { 'e25eda1fece24ac7a03624ed1320f82396f35bd8' }
+  let(:commit_message) { 'whatever' }
+  let(:current_sha) { repository.blob_at('HEAD', submodule).id }
+  let(:commit_params) do
+    {
+      submodule: submodule,
+      commit_message: commit_message,
+      commit_sha: commit_sha,
+      branch_name: branch_name
+    }
+  end
+
+  subject { described_class.new(project, user, commit_params) }
+
+  describe "#execute" do
+    shared_examples 'returns error result' do
+      it do
+        result = subject.execute
+
+        expect(result[:status]).to eq :error
+        expect(result[:message]).to eq error_message
+      end
+    end
+
+    context 'when the user is not authorized' do
+      it_behaves_like 'returns error result' do
+        let(:error_message) { 'You are not allowed to push into this branch' }
+      end
+    end
+
+    context 'when the user is authorized' do
+      before do
+        project.add_maintainer(user)
+      end
+
+      context 'when the branch is protected' do
+        before do
+          create(:protected_branch, :no_one_can_push, project: project, name: branch_name)
+        end
+
+        it_behaves_like 'returns error result' do
+          let(:error_message) { 'You are not allowed to push into this branch' }
+        end
+      end
+
+      context 'validations' do
+        context 'when submodule' do
+          context 'is empty' do
+            let(:submodule) { '' }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { 'Invalid parameters' }
+            end
+          end
+
+          context 'is not present' do
+            let(:submodule) { nil }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { 'Invalid parameters' }
+            end
+          end
+
+          context 'is invalid' do
+            let(:submodule) { 'VERSION' }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { 'Invalid submodule path' }
+            end
+          end
+
+          context 'does not exist' do
+            let(:submodule) { 'non-existent-submodule' }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { 'Invalid submodule path' }
+            end
+          end
+
+          context 'has traversal path' do
+            let(:submodule) { '../six' }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { 'Invalid parameters' }
+            end
+          end
+        end
+
+        context 'commit_sha' do
+          context 'is empty' do
+            let(:commit_sha) { '' }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { 'Invalid parameters' }
+            end
+          end
+
+          context 'is not present' do
+            let(:commit_sha) { nil }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { 'Invalid parameters' }
+            end
+          end
+
+          context 'is invalid' do
+            let(:commit_sha) { '1' }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { 'Invalid parameters' }
+            end
+          end
+
+          context 'is the same as the current ref' do
+            let(:commit_sha) { current_sha }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { "The submodule #{submodule} is already at #{commit_sha}" }
+            end
+          end
+        end
+
+        context 'branch_name' do
+          context 'is empty' do
+            let(:branch_name) { '' }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { 'You can only create or edit files when you are on a branch' }
+            end
+          end
+
+          context 'is not present' do
+            let(:branch_name) { nil }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { 'You can only create or edit files when you are on a branch' }
+            end
+          end
+
+          context 'does not exist' do
+            let(:branch_name) { 'non/existent-branch' }
+
+            it_behaves_like 'returns error result' do
+              let(:error_message) { 'You can only create or edit files when you are on a branch' }
+            end
+          end
+
+          context 'when commit message is empty' do
+            let(:commit_message) { '' }
+
+            it 'a default commit message is set' do
+              message = "Update submodule #{submodule} with oid #{commit_sha}"
+
+              expect(repository).to receive(:update_submodule).with(any_args, hash_including(message: message))
+
+              subject.execute
+            end
+          end
+        end
+      end
+
+      context 'when there is an unexpected error' do
+        before do
+          allow(repository).to receive(:update_submodule).and_raise(StandardError, 'error message')
+        end
+
+        it_behaves_like 'returns error result' do
+          let(:error_message) { 'error message' }
+        end
+      end
+
+      it 'updates the submodule reference' do
+        result = subject.execute
+
+        expect(result[:status]).to eq :success
+        expect(result[:result]).to eq repository.head_commit.id
+        expect(repository.blob_at('HEAD', submodule).id).to eq commit_sha
+      end
+
+      context 'when submodule is inside a directory' do
+        let(:submodule) { 'test_inside_folder/another_folder/six' }
+        let(:branch_name) { 'submodule_inside_folder' }
+
+        it 'updates the submodule reference' do
+          expect(repository.blob_at(branch_name, submodule).id).not_to eq commit_sha
+
+          subject.execute
+
+          expect(repository.blob_at(branch_name, submodule).id).to eq commit_sha
+        end
+      end
+
+      context 'when repository is empty' do
+        let(:project) { create(:project, :empty_repo) }
+        let(:branch_name) { 'master' }
+
+        it_behaves_like 'returns error result' do
+          let(:error_message) { 'The repository is empty' }
+        end
+      end
+    end
+  end
+end

spec/support/helpers/test_env.rb

@@ -58,7 +58,8 @@ module TestEnv
     'before-create-delete-modify-move'   => '845009f',
     'between-create-delete-modify-move'  => '3f5f443',
     'after-create-delete-modify-move'    => 'ba3faa7',
-    'with-codeowners'                    => '219560e'
+    'with-codeowners'                    => '219560e',
+    'submodule_inside_folder'            => 'b491b92'
   }.freeze
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily