Automatic merge of gitlab-org/gitlab-ce master

app/serializers/merge_request_noteable_entity.rb

 # frozen_string_literal: true
 
-class MergeRequestNoteableEntity < Grape::Entity
+class MergeRequestNoteableEntity < IssuableEntity
   include RequestAwareEntity
 
   # Currently this attr is exposed to be used in app/assets/javascripts/notes/stores/getters.js

changelogs/unreleased/ac-accelerate-wiki-attachments.yml

+---
+title: Preprocess wiki attachments with GitLab-Workhorse
+merge_request: 32663
+author:
+type: performance

changelogs/unreleased/id-autosave-for-new-mr.yml

+---
+title: Fix sharing localStorage with all MRs
+merge_request: 32699
+author:
+type: fixed

lib/api/validations/types/workhorse_file.rb

+# frozen_string_literal: true
+
+module API
+  module Validations
+    module Types
+      class WorkhorseFile < Virtus::Attribute
+        def coerce(input)
+          # Processing of multipart file objects
+          # is already taken care of by Gitlab::Middleware::Multipart.
+          # Nothing to do here.
+          input
+        end
+
+        def value_coerced?(value)
+          value.is_a?(::UploadedFile)
+        end
+      end
+    end
+  end
+end

lib/api/wikis.rb

@@ -4,11 +4,21 @@ module API
   class Wikis < Grape::API
     helpers do
       def commit_params(attrs)
-        {
-          file_name: attrs[:file][:filename],
-          file_content: attrs[:file][:tempfile].read,
-          branch_name: attrs[:branch]
-        }
+        # In order to avoid service disruption this can work with an old workhorse without the acceleration
+        # the first branch of this if must be removed when we drop support for non accelerated uploads
+        if attrs[:file].is_a?(Hash)
+          {
+            file_name: attrs[:file][:filename],
+            file_content: attrs[:file][:tempfile].read,
+            branch_name: attrs[:branch]
+          }
+        else
+          {
+            file_name: attrs[:file].original_filename,
+            file_content: attrs[:file].read,
+            branch_name: attrs[:branch]
+          }
+        end
       end
 
       params :common_wiki_page_params do
@@ -106,7 +116,7 @@ module API
         success Entities::WikiAttachment
       end
       params do
-        requires :file, type: ::API::Validations::Types::SafeFile, desc: 'The attachment file to be uploaded'
+        requires :file, types: [::API::Validations::Types::SafeFile, ::API::Validations::Types::WorkhorseFile], desc: 'The attachment file to be uploaded'
         optional :branch, type: String, desc: 'The name of the branch'
       end
       post ":id/wikis/attachments" do

lib/gitlab/middleware/multipart.rb

@@ -32,7 +32,7 @@ module Gitlab
 
       class Handler
         def initialize(env, message)
-          @request = ActionDispatch::Request.new(env)
+          @request = Rack::Request.new(env)
           @rewritten_fields = message['rewritten_fields']
           @open_files = []
         end
@@ -50,7 +50,7 @@ module Gitlab
               value = decorate_params_value(value, @request.params[key])
             end
 
-            @request.update_param(key, value)
+            update_param(key, value)
           end
 
           yield
@@ -92,6 +92,20 @@ module Gitlab
 
           ::UploadedFile.from_params(params, key, allowed_paths)
         end
+
+        # update_params ensures that both rails controllers and rack middleware can find
+        # workhorse accelerate files in the request
+        def update_param(key, value)
+          # we make sure we have key in POST otherwise update_params will add it in GET
+          @request.POST[key] ||= value
+
+          # this will force Rack::Request to properly update env keys
+          @request.update_param(key, value)
+
+          # ActionDispatch::Request is based on Rack::Request but it caches params
+          # inside other env keys, here we ensure everything is updated correctly
+          ActionDispatch::Request.new(@request.env).update_param(key, value)
+        end
       end
 
       def initialize(app)

spec/features/merge_request/user_posts_notes_spec.rb

@@ -5,7 +5,8 @@ require 'spec_helper'
 describe 'Merge request > User posts notes', :js do
   include NoteInteractionHelpers
 
-  let(:project) { create(:project, :repository) }
+  set(:project) { create(:project, :repository) }
+
   let(:user) { project.creator }
   let(:merge_request) do
     create(:merge_request, source_project: project, target_project: project)
@@ -33,17 +34,21 @@ describe 'Merge request > User posts notes', :js do
     end
 
     describe 'with text' do
+      let(:text) { 'This is awesome' }
+
       before do
         page.within('.js-main-target-form') do
-          fill_in 'note[note]', with: 'This is awesome'
+          fill_in 'note[note]', with: text
         end
       end
 
-      it 'has enable submit button and preview button' do
+      it 'has enable submit button, preview button and saves content to local storage' do
         page.within('.js-main-target-form') do
           expect(page).not_to have_css('.js-comment-button[disabled]')
           expect(page).to have_css('.js-md-preview-button', visible: true)
         end
+
+        expect(page.evaluate_script("localStorage['autosave/Note/MergeRequest/#{merge_request.id}']")).to eq(text)
       end
     end
   end

spec/fixtures/api/schemas/entities/merge_request_noteable.json

 {
   "type": "object",
   "properties" : {
+    "id": { "type": "integer" },
+    "iid": { "type": "integer" },
+    "title": { "type": "string" },
+    "description": { "type": "string" },
     "merge_params": { "type": ["object", "null"] },
     "state": { "type": "string" },
     "source_branch": { "type": "string" },

spec/requests/api/wikis_spec.rb

@@ -11,6 +11,8 @@ require 'spec_helper'
 # because they are 3 edge cases of using wiki pages.
 
 describe API::Wikis do
+  include WorkhorseHelpers
+
   let(:user) { create(:user) }
   let(:group) { create(:group).tap { |g| g.add_owner(user) } }
   let(:project_wiki) { create(:project_wiki, project: project, user: user) }
@@ -155,7 +157,7 @@ describe API::Wikis do
     it 'pushes attachment to the wiki repository' do
       allow(SecureRandom).to receive(:hex).and_return('fixed_hex')
 
-      post(api(url, user), params: payload)
+      workhorse_post_with_file(api(url, user), file_key: :file, params: payload)
 
       expect(response).to have_gitlab_http_status(201)
       expect(json_response).to eq result_hash.deep_stringify_keys
@@ -180,6 +182,15 @@ describe API::Wikis do
       expect(json_response.size).to eq(1)
       expect(json_response['error']).to eq('file is invalid')
     end
+
+    it 'is backward compatible with regular multipart uploads' do
+      allow(SecureRandom).to receive(:hex).and_return('fixed_hex')
+
+      post(api(url, user), params: payload)
+
+      expect(response).to have_gitlab_http_status(201)
+      expect(json_response).to eq result_hash.deep_stringify_keys
+    end
   end
 
   describe 'GET /projects/:id/wikis' do

spec/requests/projects/uploads_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'File uploads' do
+  include WorkhorseHelpers
+
+  let(:project) { create(:project, :public, :repository) }
+  let(:user) { create(:user) }
+
+  describe 'POST /:namespace/:project/create/:branch' do
+    let(:branch) { 'master' }
+    let(:create_url) { project_blob_path(project, branch) }
+    let(:blob_url) { project_blob_path(project, "#{branch}/dk.png") }
+    let(:params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        id: branch,
+        branch_name: branch,
+        file: fixture_file_upload('spec/fixtures/dk.png'),
+        commit_message: 'Add an image'
+      }
+    end
+
+    before do
+      project.add_maintainer(user)
+
+      login_as(user)
+    end
+
+    it 'redirects to blob' do
+      workhorse_post_with_file(create_url, file_key: :file, params: params)
+
+      expect(response).to redirect_to(blob_url)
+    end
+  end
+end

spec/serializers/merge_request_serializer_spec.rb

 require 'spec_helper'
 
 describe MergeRequestSerializer do
-  let(:user) { create(:user) }
-  let(:resource) { create(:merge_request) }
+  set(:user) { create(:user) }
+  set(:resource) { create(:merge_request, description: "Description") }
+
   let(:json_entity) do
     described_class.new(current_user: user)
       .represent(resource, serializer: serializer)

spec/support/helpers/workhorse_helpers.rb

@@ -17,7 +17,36 @@ module WorkhorseHelpers
   end
 
   def workhorse_internal_api_request_header
-    jwt_token = JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256')
     { 'HTTP_' + Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER.upcase.tr('-', '_') => jwt_token }
   end
+
+  # workhorse_post_with_file will transform file_key inside params as if it was disk accelerated by workhorse
+  def workhorse_post_with_file(url, file_key:, params:)
+    workhorse_params = params.dup
+    file = workhorse_params.delete(file_key)
+
+    workhorse_params.merge!(workhorse_disk_accelerated_file_params(file_key, file))
+
+    post(url,
+         params: workhorse_params,
+         headers: workhorse_rewritten_fields_header('file' => file.path)
+        )
+  end
+
+  private
+
+  def jwt_token(data = {})
+    JWT.encode({ 'iss' => 'gitlab-workhorse' }.merge(data), Gitlab::Workhorse.secret, 'HS256')
+  end
+
+  def workhorse_rewritten_fields_header(fields)
+    { Gitlab::Middleware::Multipart::RACK_ENV_KEY => jwt_token('rewritten_fields' => fields) }
+  end
+
+  def workhorse_disk_accelerated_file_params(key, file)
+    {
+      "#{key}.name" => file.original_filename,
+      "#{key}.path" => file.path
+    }
+  end
 end