Merge branch 'workhorse-helpers' into 'master'

Add workhorse controller and API helpers

Adds `send_git_blob` and `send_git_archive` controller and API helpers to reduce duplication and make Workhorse easier for a developer to work with.

See merge request !4486

14 files changed, 61 insertions, 34 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 0c712b445a4..fba25fca30c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -55,6 +55,7 @@ v 8.9.0 (unreleased)
   - Remove duplicated notification settings
   - Put project Files and Commits tabs under Code tab
   - Replace Colorize with Rainbow for coloring console output in Rake tasks.
+  - Add workhorse controller and API helpers
   - An indicator is now displayed at the top of the comment field for confidential issues.
   - RepositoryCheck::SingleRepositoryWorker public and private methods are now instrumented
   - Improve issuables APIs performance when accessing notes !4471
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 62f63701799..cd6ae507cf1 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -6,6 +6,7 @@ class ApplicationController < ActionController::Base
   include Gitlab::GonHelper
   include GitlabRoutingHelper
   include PageLayoutHelper
+  include WorkhorseHelper
 
   before_action :authenticate_user_from_token!
   before_action :authenticate_user!
diff --git a/app/controllers/projects/avatars_controller.rb b/app/controllers/projects/avatars_controller.rb
index 72921b3aa14..5962f74c39b 100644
--- a/app/controllers/projects/avatars_controller.rb
+++ b/app/controllers/projects/avatars_controller.rb
@@ -10,10 +10,7 @@ class Projects::AvatarsController < Projects::ApplicationController
 
       return if cached_blob?
 
-      headers.store(*Gitlab::Workhorse.send_git_blob(@repository, @blob))
-      headers['Content-Disposition'] = 'inline'
-      headers['Content-Type'] = safe_content_type(@blob)
-      head :ok # 'render nothing: true' messes up the Content-Type
+      send_git_blob @repository, @blob
     else
       render_404
     end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 06a114dcbe8..67e7187c10d 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -61,12 +61,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       format.json   { render json: @merge_request }
       format.patch  { render text: @merge_request.to_patch }
       format.diff do
-        headers.store(*Gitlab::Workhorse.send_git_diff(@project.repository,
-                                                        @merge_request.diff_base_commit.id,
-                                                        @merge_request.last_commit.id))
-        headers['Content-Disposition'] = 'inline'
+        return render_404 unless @merge_request.diff_refs
 
-        head :ok
+        send_git_diff @project.repository, @merge_request.diff_refs
       end
     end
   end
diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb
index 10de0e60530..10d24da16d7 100644
--- a/app/controllers/projects/raw_controller.rb
+++ b/app/controllers/projects/raw_controller.rb
@@ -18,10 +18,7 @@ class Projects::RawController < Projects::ApplicationController
       if @blob.lfs_pointer?
         send_lfs_object
       else
-        headers.store(*Gitlab::Workhorse.send_git_blob(@repository, @blob))
-        headers['Content-Disposition'] = 'inline'
-        headers['Content-Type'] = safe_content_type(@blob)
-        head :ok # 'render nothing: true' messes up the Content-Type
+        send_git_blob @repository, @blob
       end
     else
       render_404
diff --git a/app/controllers/projects/repositories_controller.rb b/app/controllers/projects/repositories_controller.rb
index bb7a6b6a5ab..d5af0341d18 100644
--- a/app/controllers/projects/repositories_controller.rb
+++ b/app/controllers/projects/repositories_controller.rb
@@ -11,8 +11,7 @@ class Projects::RepositoriesController < Projects::ApplicationController
   end
 
   def archive
-    headers.store(*Gitlab::Workhorse.send_git_archive(@project, params[:ref], params[:format]))
-    head :ok
+    send_git_archive @repository, ref: params[:ref], format: params[:format]
   rescue => ex
     logger.error("#{self.class.name}: #{ex}")
     return git_not_found!
diff --git a/app/helpers/workhorse_helper.rb b/app/helpers/workhorse_helper.rb
new file mode 100644
index 00000000000..2bd0dbfd095
--- /dev/null
+++ b/app/helpers/workhorse_helper.rb
@@ -0,0 +1,24 @@
+# Helpers to send Git blobs, diffs or archives through Workhorse.
+# Workhorse will also serve files when using `send_file`.
+module WorkhorseHelper
+  # Send a Git blob through Workhorse
+  def send_git_blob(repository, blob)
+    headers.store(*Gitlab::Workhorse.send_git_blob(repository, blob))
+    headers['Content-Disposition'] = 'inline'
+    headers['Content-Type'] = safe_content_type(blob)
+    head :ok # 'render nothing: true' messes up the Content-Type
+  end
+
+  # Send a Git diff through Workhorse
+  def send_git_diff(repository, diff_refs)
+    headers.store(*Gitlab::Workhorse.send_git_diff(repository, diff_refs))
+    headers['Content-Disposition'] = 'inline'
+    head :ok
+  end
+
+  # Archive a Git repository and send it through Workhorse
+  def send_git_archive(repository, ref:, format:)
+    headers.store(*Gitlab::Workhorse.send_git_archive(repository, ref: ref, format: format))
+    head :ok
+  end
+end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 2aaa0557ea3..e1d3bbcc02d 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -408,5 +408,15 @@ module API
       error!(errors[:access_level], 422) if errors[:access_level].any?
       not_found!(errors)
     end
+
+    def send_git_blob(repository, blob)
+      env['api.format'] = :txt
+      content_type 'text/plain'
+      header(*Gitlab::Workhorse.send_git_blob(repository, blob))
+    end
+
+    def send_git_archive(repository, ref:, format:)
+      header(*Gitlab::Workhorse.send_git_archive(repository, ref: ref, format: format))
+    end
   end
 end
diff --git a/lib/api/repositories.rb b/lib/api/repositories.rb
index 9cb14e95ebc..f55aceed92c 100644
--- a/lib/api/repositories.rb
+++ b/lib/api/repositories.rb
@@ -56,8 +56,7 @@ module API
         blob = Gitlab::Git::Blob.find(repo, commit.id, params[:filepath])
         not_found! "File" unless blob
 
-        content_type 'text/plain'
-        header(*Gitlab::Workhorse.send_git_blob(repo, blob))
+        send_git_blob repo, blob
       end
 
       # Get a raw blob contents by blob sha
@@ -80,10 +79,7 @@ module API
 
         not_found! 'Blob' unless blob
 
-        env['api.format'] = :txt
-
-        content_type blob.mime_type
-        header(*Gitlab::Workhorse.send_git_blob(repo, blob))
+        send_git_blob repo, blob
       end
 
       # Get a an archive of the repository
@@ -98,7 +94,7 @@ module API
         authorize! :download_code, user_project
 
         begin
-          header(*Gitlab::Workhorse.send_git_archive(user_project, params[:sha], params[:format]))
+          send_git_archive user_project.repository, ref: params[:sha], format: params[:format]
         rescue
           not_found!('File')
         end
diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb
index 56af739b1ef..388f84dbe0e 100644
--- a/lib/gitlab/workhorse.rb
+++ b/lib/gitlab/workhorse.rb
@@ -21,27 +21,29 @@ module Gitlab
 
         [
           SEND_DATA_HEADER,
-          "git-blob:#{encode(params)}",
+          "git-blob:#{encode(params)}"
         ]
       end
 
-      def send_git_archive(project, ref, format)
+      def send_git_archive(repository, ref:, format:)
         format ||= 'tar.gz'
         format.downcase!
-        params = project.repository.archive_metadata(ref, Gitlab.config.gitlab.repository_downloads_path, format)
+        params = repository.archive_metadata(ref, Gitlab.config.gitlab.repository_downloads_path, format)
         raise "Repository or ref not found" if params.empty?
 
         [
           SEND_DATA_HEADER,
-          "git-archive:#{encode(params)}",
+          "git-archive:#{encode(params)}"
         ]
       end
 
-      def send_git_diff(repository, from, to)
+      def send_git_diff(repository, diff_refs)
+        from, to = diff_refs
+
         params = {
-            'RepoPath'  => repository.path_to_repo,
-            'ShaFrom'   => from,
-            'ShaTo'     => to
+          'RepoPath'  => repository.path_to_repo,
+          'ShaFrom'   => from.sha,
+          'ShaTo'     => to.sha
         }
 
         [
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 1301574f489..4b408c03703 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -91,7 +91,7 @@ describe Projects::MergeRequestsController do
             id: merge_request.iid,
             format: :diff)
 
-        expect(response.headers['Gitlab-Workhorse-Send-Data']).to start_with("git-diff:")
+        expect(response.headers[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("git-diff:")
       end
     end
 
diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb
index fb29274c687..33c35161da3 100644
--- a/spec/controllers/projects/raw_controller_spec.rb
+++ b/spec/controllers/projects/raw_controller_spec.rb
@@ -17,6 +17,7 @@ describe Projects::RawController do
         expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
         expect(response.header['Content-Disposition']).
             to eq("inline")
+        expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("git-blob:")
       end
     end
 
@@ -31,6 +32,7 @@ describe Projects::RawController do
 
         expect(response.status).to eq(200)
         expect(response.header['Content-Type']).to eq('image/jpeg')
+        expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("git-blob:")
       end
     end
 
diff --git a/spec/controllers/projects/repositories_controller_spec.rb b/spec/controllers/projects/repositories_controller_spec.rb
index 0ddbec9eac2..aad62cf20e3 100644
--- a/spec/controllers/projects/repositories_controller_spec.rb
+++ b/spec/controllers/projects/repositories_controller_spec.rb
@@ -20,10 +20,11 @@ describe Projects::RepositoriesController do
         project.team << [user, :developer]
         sign_in(user)
       end
-      it "uses Gitlab::Workhorse" do
-        expect(Gitlab::Workhorse).to receive(:send_git_archive).with(project, "master", "zip")
 
+      it "uses Gitlab::Workhorse" do
         get :archive, namespace_id: project.namespace.path, project_id: project.path, ref: "master", format: "zip"
+
+        expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("git-archive:")
       end
 
       context "when the service raises an error" do
diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb
index d940bf05061..c5c1402e8fc 100644
--- a/spec/lib/gitlab/workhorse_spec.rb
+++ b/spec/lib/gitlab/workhorse_spec.rb
@@ -11,7 +11,7 @@ describe Gitlab::Workhorse, lib: true do
       end
 
       it "raises an error" do
-        expect { subject.send_git_archive(project, "master", "zip") }.to raise_error(RuntimeError)
+        expect { subject.send_git_archive(project.repository, ref: "master", format: "zip") }.to raise_error(RuntimeError)
       end
     end
   end