Merge branch 'ce-to-ee-2018-06-13' into 'master'

CE upstream - 2018-06-13 09:24 UTC

Closes gitlab-qa#271 et gitaly#771

See merge request gitlab-org/gitlab-ee!6110

app/models/project_services/chat_message/base_message.rb

@@ -26,13 +26,18 @@ module ChatMessage
       end
     end
 
-    def pretext
+    def summary
       return message if markdown
 
       format(message)
     end
 
+    def pretext
+      summary
+    end
+
     def fallback
+      format(message)
     end
 
     def attachments

app/models/project_services/chat_message/pipeline_message.rb

@@ -23,10 +23,6 @@ module ChatMessage
       ''
     end
 
-    def fallback
-      format(message)
-    end
-
     def attachments
       return message if markdown
 

app/models/project_services/microsoft_teams_service.rb

@@ -44,7 +44,7 @@ class MicrosoftTeamsService < ChatNotificationService
   def notify(message, opts)
     MicrosoftTeams::Notifier.new(webhook).ping(
       title: message.project_name,
-      pretext: message.pretext,
+      summary: message.summary,
       activity: message.activity,
       attachments: message.attachments
     )

app/models/timelog.rb

@@ -19,4 +19,9 @@ class Timelog < ActiveRecord::Base
       errors.add(:base, 'Issue or Merge Request ID is required')
     end
   end
+
+  # Rails5 defaults to :touch_later, overwrite for normal touch
+  def belongs_to_touch_method
+    :touch
+  end
 end

app/uploaders/file_uploader.rb

@@ -65,10 +65,10 @@ class FileUploader < GitlabUploader
     SecureRandom.hex
   end
 
-  def upload_paths(filename)
+  def upload_paths(identifier)
     [
-      File.join(secret, filename),
-      File.join(base_dir(Store::REMOTE), secret, filename)
+      File.join(secret, identifier),
+      File.join(base_dir(Store::REMOTE), secret, identifier)
     ]
   end
 

app/uploaders/object_storage.rb

@@ -10,6 +10,17 @@ module ObjectStorage
   UnknownStoreError = Class.new(StandardError)
   ObjectStorageUnavailable = Class.new(StandardError)
 
+  class ExclusiveLeaseTaken < StandardError
+    def initialize(lease_key)
+      @lease_key = lease_key
+    end
+
+    def message
+      *lease_key_group, _ = *@lease_key.split(":")
+      "Exclusive lease for #{lease_key_group.join(':')} is already taken."
+    end
+  end
+
   TMP_UPLOAD_PATH = 'tmp/uploads'.freeze
 
   module Store
@@ -29,7 +40,7 @@ module ObjectStorage
       end
 
       def retrieve_from_store!(identifier)
-        paths = store_dirs.map { |store, path| File.join(path, identifier) }
+        paths = upload_paths(identifier)
 
         unless current_upload_satisfies?(paths, model)
           # the upload we already have isn't right, find the correct one
@@ -261,7 +272,7 @@ module ObjectStorage
     end
 
     def delete_migrated_file(migrated_file)
-      migrated_file.delete if exists?
+      migrated_file.delete
     end
 
     def exists?
@@ -279,6 +290,13 @@ module ObjectStorage
       }
     end
 
+    # Returns all the possible paths for an upload.
+    # the `upload.path` is a lookup parameter, and it may change
+    # depending on the `store` param.
+    def upload_paths(identifier)
+      store_dirs.map { |store, path| File.join(path, identifier) }
+    end
+
     def cache!(new_file = sanitized_file)
       # We intercept ::UploadedFile which might be stored on remote storage
       # We use that for "accelerated" uploads, where we store result on remote storage
@@ -369,12 +387,13 @@ module ObjectStorage
     end
 
     def with_exclusive_lease
-      uuid = Gitlab::ExclusiveLease.new(exclusive_lease_key, timeout: 1.hour.to_i).try_obtain
-      raise 'exclusive lease already taken' unless uuid
+      lease_key = exclusive_lease_key
+      uuid = Gitlab::ExclusiveLease.new(lease_key, timeout: 1.hour.to_i).try_obtain
+      raise ExclusiveLeaseTaken.new(lease_key) unless uuid
 
       yield uuid
     ensure
-      Gitlab::ExclusiveLease.cancel(exclusive_lease_key, uuid)
+      Gitlab::ExclusiveLease.cancel(lease_key, uuid)
     end
 
     #

app/uploaders/records_uploads.rb

@@ -22,7 +22,7 @@ module RecordsUploads
 
       Upload.transaction do
         uploads.where(path: upload_path).delete_all
-        upload.destroy! if upload
+        upload.delete if upload
 
         self.upload = build_upload.tap(&:save!)
       end

app/views/admin/application_settings/_signup.html.haml

@@ -24,7 +24,7 @@
           Enable domain blacklist for sign ups
     .form-group
       .form-check
-        = radio_button_tag :blacklist_type, :file, false, class: "form-check-input"
+        = radio_button_tag :blacklist_type, :file, false, class: 'form-check-input'
         = label_tag :blacklist_type_file, class: 'form-check-label' do
           .option-title
             Upload blacklist file

app/views/shared/empty_states/_wikis.html.haml

@@ -8,7 +8,7 @@
     %h4
       = s_('WikiEmpty|The wiki lets you write documentation for your project')
     %p.text-left
-      = s_("WikiEmpty|A wiki is where you can store all the details about your project. This can include why you've created it, it's principles, how to use it, and so on.")
+      = s_("WikiEmpty|A wiki is where you can store all the details about your project. This can include why you've created it, its principles, how to use it, and so on.")
     = create_link
 
 - elsif can?(current_user, :read_issue, @project)

changelogs/unreleased/42342-teams-pipeline-notifications.yml

+---
+title: Fixes Microsoft Teams notifications for pipeline events
+merge_request: 19632
+author: Jeff Brown
+type: fixed

changelogs/unreleased/44674-use-one-column-form-layout-on-admin-area-settings-page.yml

+---
+title: Use one column form layout on Admin Area Settings page
+merge_request:
+author:
+type: changed

changelogs/unreleased/47408-migrateuploadsworker-is-doing-n-1-queries-on-migration.yml

+---
+title: Optimize the upload migration proces
+merge_request: 15947
+author:
+type: fixed

changelogs/unreleased/commits_api_with_stats.yml

+---
+title: Added with_statsoption for GET /projects/:id/repository/commits
+merge_request:
+author:
+type: added

changelogs/unreleased/fj-restore-users-v3-endpoint.yml

+---
+title: Restore API v3 user endpoint
+merge_request:
+author:
+type: changed

changelogs/unreleased/rails5-fix-47366.yml

+---
+title: Rails5 fix expected `issuable.reload.updated_at` to have changed
+merge_request: 19733
+author: Jasper Maes
+type: fixed

config/application.rb

@@ -14,6 +14,7 @@ module Gitlab
     require_dependency Rails.root.join('lib/gitlab/redis/shared_state')
     require_dependency Rails.root.join('lib/gitlab/request_context')
     require_dependency Rails.root.join('lib/gitlab/current_settings')
+    require_dependency Rails.root.join('lib/gitlab/middleware/read_only')
 
     # Settings in config/environments/* take precedence over those specified here.
     # Application configuration should go into files in config/initializers
@@ -203,7 +204,7 @@ module Gitlab
     ENV['GIT_TERMINAL_PROMPT'] = '0'
 
     # Gitlab Read-only middleware support
-    config.middleware.insert_after ActionDispatch::Flash, '::Gitlab::Middleware::ReadOnly'
+    config.middleware.insert_after ActionDispatch::Flash, ::Gitlab::Middleware::ReadOnly
 
     config.generators do |g|
       g.factory_bot false

config/environments/test.rb

 Rails.application.configure do
   # Make sure the middleware is inserted first in middleware chain
-  config.middleware.insert_before('ActionDispatch::Static', 'Gitlab::Testing::RequestBlockerMiddleware')
-  config.middleware.insert_before('ActionDispatch::Static', 'Gitlab::Testing::RequestInspectorMiddleware')
+  config.middleware.insert_before(ActionDispatch::Static, Gitlab::Testing::RequestBlockerMiddleware)
+  config.middleware.insert_before(ActionDispatch::Static, Gitlab::Testing::RequestInspectorMiddleware)
 
   # Settings specified here will take precedence over those in config/application.rb
 

doc/api/commits.md

@@ -16,6 +16,7 @@ GET /projects/:id/repository/commits
 | `until` | string | no | Only commits before or on this date will be returned in ISO 8601 format YYYY-MM-DDTHH:MM:SSZ |
 | `path` | string | no | The file path |
 | `all` | boolean | no | Retrieve every commit from the repository |
+| `with_stats` | boolean | no | Stats about each commit will be added to the response |
 
 
 ```bash

ee/spec/services/merge_requests/build_service_spec.rb

@@ -28,7 +28,7 @@ describe MergeRequests::BuildService do
 
   context 'project default template configured' do
     let(:template) { "I am the template, you fill me in" }
-    let(:project) { create(:project, merge_requests_template: template) }
+    let(:project) { create(:project, :repository, merge_requests_template: template) }
 
     context 'issuable default templates feature not available' do
       before do

lib/api/commits.rb

@@ -15,19 +15,21 @@ module API
       end
       params do
         optional :ref_name, type: String, desc: 'The name of a repository branch or tag, if not given the default branch is used'
-        optional :since,    type: DateTime, desc: 'Only commits after or on this date will be returned'
-        optional :until,    type: DateTime, desc: 'Only commits before or on this date will be returned'
-        optional :path,     type: String, desc: 'The file path'
-        optional :all,      type: Boolean, desc: 'Every commit will be returned'
+        optional :since, type: DateTime, desc: 'Only commits after or on this date will be returned'
+        optional :until, type: DateTime, desc: 'Only commits before or on this date will be returned'
+        optional :path, type: String, desc: 'The file path'
+        optional :all, type: Boolean, desc: 'Every commit will be returned'
+        optional :with_stats, type: Boolean, desc: 'Stats about each commit will be added to the response'
         use :pagination
       end
       get ':id/repository/commits' do
-        path   = params[:path]
+        path = params[:path]
         before = params[:until]
-        after  = params[:since]
-        ref    = params[:ref_name] || user_project.try(:default_branch) || 'master' unless params[:all]
+        after = params[:since]
+        ref = params[:ref_name] || user_project.try(:default_branch) || 'master' unless params[:all]
         offset = (params[:page] - 1) * params[:per_page]
-        all    = params[:all]
+        all = params[:all]
+        with_stats = params[:with_stats]
 
         commits = user_project.repository.commits(ref,
                                                   path: path,
@@ -47,7 +49,9 @@ module API
 
         paginated_commits = Kaminari.paginate_array(commits, total_count: commit_count)
 
-        present paginate(paginated_commits), with: Entities::Commit
+        serializer = with_stats ? Entities::CommitWithStats : Entities::Commit
+
+        present paginate(paginated_commits), with: serializer
       end
 
       desc 'Commit multiple file changes as one commit' do

lib/api/entities.rb

@@ -308,6 +308,10 @@ module API
       expose :additions, :deletions, :total
     end
 
+    class CommitWithStats < Commit
+      expose :stats, using: Entities::CommitStats
+    end
+
     class CommitDetail < Commit
       expose :stats, using: Entities::CommitStats, if: :stats
       expose :status

lib/api/users.rb

@@ -537,7 +537,7 @@ module API
         authenticate!
       end
 
-      # Enabling /users/:id endpoint for the v3 version to allow oauth
+      # Enabling /user endpoint for the v3 version to allow oauth
       # authentication through this endpoint.
       version %w(v3 v4), using: :path do
         desc 'Get the currently authenticated user' do

lib/gitlab/git/repository.rb

@@ -1185,18 +1185,18 @@ module Gitlab
       end
 
       def compare_source_branch(target_branch_name, source_repository, source_branch_name, straight:)
-        Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-          with_repo_branch_commit(source_repository, source_branch_name) do |commit|
-            break unless commit
+        tmp_ref = "refs/tmp/#{SecureRandom.hex}"
 
-            Gitlab::Git::Compare.new(
-              self,
-              target_branch_name,
-              commit.sha,
-              straight: straight
-            )
-          end
-        end
+        return unless fetch_source_branch!(source_repository, source_branch_name, tmp_ref)
+
+        Gitlab::Git::Compare.new(
+          self,
+          target_branch_name,
+          tmp_ref,
+          straight: straight
+        )
+      ensure
+        delete_refs(tmp_ref)
       end
 
       def write_ref(ref_path, ref, old_ref: nil, shell: true)

lib/microsoft_teams/notifier.rb

@@ -30,7 +30,7 @@ module MicrosoftTeams
       result = { 'sections' => [] }
 
       result['title'] = options[:title]
-      result['summary'] = options[:pretext]
+      result['summary'] = options[:summary]
       result['sections'] << MicrosoftTeams::Activity.new(options[:activity]).prepare
 
       attachments = options[:attachments]

locale/gitlab.pot

@@ -5859,7 +5859,7 @@ msgstr ""
 msgid "WikiEmptyIssueMessage|issue tracker"
 msgstr ""
 
-msgid "WikiEmpty|A wiki is where you can store all the details about your project. This can include why you've created it, it's principles, how to use it, and so on."
+msgid "WikiEmpty|A wiki is where you can store all the details about your project. This can include why you've created it, its principles, how to use it, and so on."
 msgstr ""
 
 msgid "WikiEmpty|Create your first page"

spec/fixtures/api/schemas/public_api/v4/commit/with_stats.json

+{
+  "type": "object",
+  "allOf": [
+    { "$ref": "basic.json" },
+    {
+      "required" : [
+        "stats"
+      ],
+      "properties": {
+        "stats": { "$ref": "../commit_stats.json" }
+      }
+    }
+  ]
+}

spec/fixtures/api/schemas/public_api/v4/commits_with_stats.json

+{
+  "type": "array",
+  "items": { "$ref": "commit/with_stats.json" }
+}

spec/lib/microsoft_teams/notifier_spec.rb

@@ -8,7 +8,7 @@ describe MicrosoftTeams::Notifier do
   let(:options) do
     {
       title: 'JohnDoe4/project2',
-      pretext: '[[JohnDoe4/project2](http://localhost/namespace2/gitlabhq)] Issue [#1 Awesome issue](http://localhost/namespace2/gitlabhq/issues/1) opened by user6',
+      summary: '[[JohnDoe4/project2](http://localhost/namespace2/gitlabhq)] Issue [#1 Awesome issue](http://localhost/namespace2/gitlabhq/issues/1) opened by user6',
       activity: {
         title: 'Issue opened by user6',
         subtitle: 'in [JohnDoe4/project2](http://localhost/namespace2/gitlabhq)',

spec/models/project_services/microsoft_teams_service_spec.rb

@@ -225,10 +225,15 @@ describe MicrosoftTeamsService do
 
       it 'calls Microsoft Teams API for pipeline events' do
         data = Gitlab::DataBuilder::Pipeline.build(pipeline)
+        data[:markdown] = true
 
         chat_service.execute(data)
 
-        expect(WebMock).to have_requested(:post, webhook_url).once
+        message = ChatMessage::PipelineMessage.new(data)
+
+        expect(WebMock).to have_requested(:post, webhook_url)
+          .with(body: hash_including({ summary: message.summary }))
+          .once
       end
     end
 

spec/requests/api/commits_spec.rb

@@ -18,14 +18,14 @@ describe API::Commits do
   describe 'GET /projects/:id/repository/commits' do
     let(:route) { "/projects/#{project_id}/repository/commits" }
 
-    shared_examples_for 'project commits' do
+    shared_examples_for 'project commits' do |schema: 'public_api/v4/commits'|
       it "returns project commits" do
         commit = project.repository.commit
 
         get api(route, current_user)
 
         expect(response).to have_gitlab_http_status(200)
-        expect(response).to match_response_schema('public_api/v4/commits')
+        expect(response).to match_response_schema(schema)
         expect(json_response.first['id']).to eq(commit.id)
         expect(json_response.first['committer_name']).to eq(commit.committer_name)
         expect(json_response.first['committer_email']).to eq(commit.committer_email)
@@ -161,6 +161,23 @@ describe API::Commits do
         end
       end
 
+      context 'with_stats optional parameter' do
+        let(:project) { create(:project, :public, :repository) }
+
+        it_behaves_like 'project commits', schema: 'public_api/v4/commits_with_stats' do
+          let(:route) { "/projects/#{project_id}/repository/commits?with_stats=true" }
+
+          it 'include commits details' do
+            commit = project.repository.commit
+            get api(route, current_user)
+
+            expect(json_response.first['stats']['additions']).to eq(commit.stats.additions)
+            expect(json_response.first['stats']['deletions']).to eq(commit.stats.deletions)
+            expect(json_response.first['stats']['total']).to eq(commit.stats.total)
+          end
+        end
+      end
+
       context 'with pagination params' do
         let(:page) { 1 }
         let(:per_page) { 5 }

spec/support/shared_examples/uploaders/object_storage_shared_examples.rb

@@ -85,13 +85,13 @@ shared_examples "migrates" do |to_store:, from_store: nil|
     it 'does not execute migrate!' do
       expect(subject).not_to receive(:unsafe_migrate!)
 
-      expect { migrate(to) }.to raise_error('exclusive lease already taken')
+      expect { migrate(to) }.to raise_error(ObjectStorage::ExclusiveLeaseTaken)
     end
 
     it 'does not execute use_file' do
       expect(subject).not_to receive(:unsafe_use_file)
 
-      expect { subject.use_file }.to raise_error('exclusive lease already taken')
+      expect { subject.use_file }.to raise_error(ObjectStorage::ExclusiveLeaseTaken)
     end
 
     after do

spec/uploaders/object_storage_spec.rb

@@ -333,7 +333,7 @@ describe ObjectStorage do
       when_file_is_in_use do
         expect(uploader).not_to receive(:unsafe_migrate!)
 
-        expect { uploader.migrate!(described_class::Store::REMOTE) }.to raise_error('exclusive lease already taken')
+        expect { uploader.migrate!(described_class::Store::REMOTE) }.to raise_error(ObjectStorage::ExclusiveLeaseTaken)
       end
     end
 
@@ -341,7 +341,7 @@ describe ObjectStorage do
       when_file_is_in_use do
         expect(uploader).not_to receive(:unsafe_use_file)
 
-        expect { uploader.use_file }.to raise_error('exclusive lease already taken')
+        expect { uploader.use_file }.to raise_error(ObjectStorage::ExclusiveLeaseTaken)
       end
     end
   end

spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb

@@ -11,6 +11,12 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do
   let(:uploads) { Upload.all }
   let(:to_store) { ObjectStorage::Store::REMOTE }
 
+  def perform(uploads)
+    described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store)
+  rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures
+    # swallow
+  end
+
   shared_examples "uploads migration worker" do
     describe '.enqueue!' do
       def enqueue!
@@ -69,12 +75,6 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do
     end
 
     describe '#perform' do
-      def perform
-        described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store)
-      rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures
-        # swallow
-      end
-
       shared_examples 'outputs correctly' do |success: 0, failures: 0|
         total = success + failures
 
@@ -82,7 +82,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do
           it 'outputs the reports' do
             expect(Rails.logger).to receive(:info).with(%r{Migrated #{success}/#{total} files})
 
-            perform
+            perform(uploads)
           end
         end
 
@@ -90,7 +90,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do
           it 'outputs upload failures' do
             expect(Rails.logger).to receive(:warn).with(/Error .* I am a teapot/)
 
-            perform
+            perform(uploads)
           end
         end
       end
@@ -98,7 +98,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do
       it_behaves_like 'outputs correctly', success: 10
 
       it 'migrates files' do
-        perform
+        perform(uploads)
 
         expect(Upload.where(store: ObjectStorage::Store::LOCAL).count).to eq(0)
       end
@@ -123,6 +123,17 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do
     end
 
     it_behaves_like "uploads migration worker"
+
+    describe "limits N+1 queries" do
+      it "to N*5" do
+        query_count = ActiveRecord::QueryRecorder.new { perform(uploads) }
+
+        more_projects = create_list(:project, 3, :with_avatar)
+
+        expected_queries_per_migration = 5 * more_projects.count
+        expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration)
+      end
+    end
   end
 
   context "for FileUploader" do
@@ -130,15 +141,29 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do
     let(:secret) { SecureRandom.hex }
     let(:mounted_as) { nil }
 
+    def upload_file(project)
+      uploader = FileUploader.new(project)
+      uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt'))
+    end
+
     before do
       stub_uploads_object_storage(FileUploader)
 
-      projects.map do |project|
-        uploader = FileUploader.new(project)
-        uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt'))
-      end
+      projects.map(&method(:upload_file))
     end
 
     it_behaves_like "uploads migration worker"
+
+    describe "limits N+1 queries" do
+      it "to N*5" do
+        query_count = ActiveRecord::QueryRecorder.new { perform(uploads) }
+
+        more_projects = create_list(:project, 3)
+        more_projects.map(&method(:upload_file))
+
+        expected_queries_per_migration = 5 * more_projects.count
+        expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration)
+      end
+    end
   end
 end