Merge branch '9-4-stable' into ce-to-ee-2017-07-08

* 9-4-stable: (41 commits)
  Also inject new route helpers into includers of GitlabRoutingHelper
  Remove many N+1 queries with merge requests API
  Change `sign_out` usage to `gitlab_sign_out` in one spec
  Simplify stage_id migration as we now use relations
  Do not schedule bg migration when it is not needed
  Schedule stage_id bg migrations in batches of 10
  Use new `each_batch` helper instead of custom one
  Revert recent changes in migration helpers
  Add some comments on new migrations helpers
  Schedule stage_id background migration in ranges
  Extract `execute_in_batches` migrations helper
  Add walk_table_in_batches and refactor migration helpers
  Reduce a delay between stage_id scheduled migrations
  Improve exception description in bg migrations
  Do not override original AR5 batching interface
  Sanitize id value passed to async background migration
  Improve code examples in background migrations docs
  Add description to exception in bg migrations worker
  Do not compare float with integer in migrations specs
  Improve readability of build stage id migration query
  ...

Conflicts:
	spec/features/boards/boards_spec.rb
	spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb
	spec/features/issues/user_uses_slash_commands_spec.rb
	spec/features/merge_requests/edit_mr_spec.rb
	spec/features/milestones/show_spec.rb
	spec/features/protected_tags_spec.rb

app/helpers/gitlab_routing_helper.rb

@@ -2,6 +2,10 @@
 module GitlabRoutingHelper
   extend ActiveSupport::Concern
 
+  included do
+    Gitlab::Routing.includes_helpers(self)
+  end
+
   # Project
   def project_tree_path(project, ref = nil, *args)
     namespace_project_tree_path(project.namespace, project, ref || @ref || project.repository.root_ref, *args) # rubocop:disable Cop/ProjectPathHelper

app/models/merge_request.rb

@@ -18,7 +18,7 @@ class MergeRequest < ActiveRecord::Base
 
   has_many :merge_request_diffs
   has_one :merge_request_diff,
-    -> { order('merge_request_diffs.id DESC') }
+    -> { order('merge_request_diffs.id DESC') }, inverse_of: :merge_request
 
   belongs_to :head_pipeline, foreign_key: "head_pipeline_id", class_name: "Ci::Pipeline"
 

app/workers/background_migration_worker.rb

@@ -2,18 +2,34 @@ class BackgroundMigrationWorker
   include Sidekiq::Worker
   include DedicatedSidekiqQueue
 
-  # Schedules a number of jobs in bulk
+  # Enqueues a number of jobs in bulk.
   #
   # The `jobs` argument should be an Array of Arrays, each sub-array must be in
   # the form:
   #
   #     [migration-class, [arg1, arg2, ...]]
-  def self.perform_bulk(*jobs)
+  def self.perform_bulk(jobs)
     Sidekiq::Client.push_bulk('class' => self,
                               'queue' => sidekiq_options['queue'],
                               'args' => jobs)
   end
 
+  # Schedules multiple jobs in bulk, with a delay.
+  #
+  def self.perform_bulk_in(delay, jobs)
+    now = Time.now.to_i
+    schedule = now + delay.to_i
+
+    if schedule <= now
+      raise ArgumentError, 'The schedule time must be in the future!'
+    end
+
+    Sidekiq::Client.push_bulk('class' => self,
+                              'queue' => sidekiq_options['queue'],
+                              'args' => jobs,
+                              'at' => schedule)
+  end
+
   # Performs the background migration.
   #
   # See Gitlab::BackgroundMigration.perform for more information.

db/post_migrate/20170628080858_migrate_stage_id_reference_in_background.rb

+class MigrateStageIdReferenceInBackground < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 10000
+  RANGE_SIZE = 1000
+  MIGRATION = 'MigrateBuildStageIdReference'.freeze
+
+  disable_ddl_transaction!
+
+  class Build < ActiveRecord::Base
+    self.table_name = 'ci_builds'
+    include ::EachBatch
+  end
+
+  ##
+  # It will take around 3 days to process 20M ci_builds.
+  #
+  def up
+    Build.where(stage_id: nil).each_batch(of: BATCH_SIZE) do |relation, index|
+      relation.each_batch(of: RANGE_SIZE) do |relation|
+        range = relation.pluck('MIN(id)', 'MAX(id)').first
+
+        BackgroundMigrationWorker
+          .perform_in(index * 2.minutes, MIGRATION, range)
+      end
+    end
+  end
+
+  def down
+    # noop
+  end
+end

doc/development/background_migrations.md

@@ -50,14 +50,13 @@ your migration:
 BackgroundMigrationWorker.perform_async('BackgroundMigrationClassName', [arg1, arg2, ...])
 ```
 
-Usually it's better to schedule jobs in bulk, for this you can use
+Usually it's better to enqueue jobs in bulk, for this you can use
 `BackgroundMigrationWorker.perform_bulk`:
 
 ```ruby
 BackgroundMigrationWorker.perform_bulk(
-  ['BackgroundMigrationClassName', [1]],
-  ['BackgroundMigrationClassName', [2]],
-  ...
+  [['BackgroundMigrationClassName', [1]],
+   ['BackgroundMigrationClassName', [2]]]
 )
 ```
 
@@ -68,6 +67,16 @@ consuming migrations it's best to schedule a background job using an
 updates. Removals in turn can be handled by simply defining foreign keys with
 cascading deletes.
 
+If you would like to schedule jobs in bulk with a delay, you can use
+`BackgroundMigrationWorker.perform_bulk_in`:
+
+```ruby
+jobs = [['BackgroundMigrationClassName', [1]],
+        ['BackgroundMigrationClassName', [2]]]
+
+BackgroundMigrationWorker.perform_bulk_in(5.minutes, jobs)
+```
+
 ## Cleaning Up
 
 Because background migrations can take a long time you can't immediately clean

lib/api/merge_requests.rb

@@ -47,7 +47,9 @@ module API
           args[:milestone_title] = args.delete(:milestone)
           args[:label_name] = args.delete(:labels)
 
-          merge_requests = MergeRequestsFinder.new(current_user, args).execute.inc_notes_with_associations
+          merge_requests = MergeRequestsFinder.new(current_user, args).execute
+                             .inc_notes_with_associations
+                             .preload(:target_project, :author, :assignee, :milestone, :merge_request_diff)
 
           merge_requests.reorder(args[:order_by] => args[:sort])
         end

lib/gitlab/background_migration/migrate_build_stage_id_reference.rb

+module Gitlab
+  module BackgroundMigration
+    class MigrateBuildStageIdReference
+      def perform(start_id, stop_id)
+        sql = <<-SQL.strip_heredoc
+          UPDATE ci_builds
+            SET stage_id =
+              (SELECT id FROM ci_stages
+                WHERE ci_stages.pipeline_id = ci_builds.commit_id
+                AND ci_stages.name = ci_builds.stage)
+          WHERE ci_builds.id BETWEEN #{start_id.to_i} AND #{stop_id.to_i}
+            AND ci_builds.stage_id IS NULL
+        SQL
+
+        ActiveRecord::Base.connection.execute(sql)
+      end
+    end
+  end
+end

lib/gitlab/routing.rb

@@ -6,21 +6,26 @@ module Gitlab
     self._includers = []
 
     included do
-      Gitlab::Routing._includers << self
+      Gitlab::Routing.includes_helpers(self)
+
       include Gitlab::Routing.url_helpers
     end
 
+    def self.includes_helpers(klass)
+      self._includers << klass
+    end
+
     def self.add_helpers(mod)
       url_helpers.include mod
       url_helpers.extend mod
 
+      GitlabRoutingHelper.include mod
+      GitlabRoutingHelper.extend mod
+
       app_url_helpers = Gitlab::Application.routes.named_routes.url_helpers_module
       app_url_helpers.include mod
       app_url_helpers.extend mod
 
-      GitlabRoutingHelper.include mod
-      GitlabRoutingHelper.extend mod
-
       _includers.each do |klass|
         klass.include mod
       end

spec/features/boards/boards_spec.rb

@@ -538,7 +538,11 @@ describe 'Issue Boards', feature: true, js: true do
   context 'signed out user' do
     before do
       sign_out(:user)
+<<<<<<< HEAD
       visit project_boards_path(project)
+=======
+      visit project_board_path(project, board)
+>>>>>>> 9-4-stable
       wait_for_requests
     end
 
@@ -562,7 +566,11 @@ describe 'Issue Boards', feature: true, js: true do
       project.team << [user_guest, :guest]
       sign_out(:user)
       sign_in(user_guest)
+<<<<<<< HEAD
       visit project_boards_path(project)
+=======
+      visit project_board_path(project, board)
+>>>>>>> 9-4-stable
       wait_for_requests
     end
 

spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb

@@ -67,9 +67,14 @@ feature 'Resolve an open discussion in a merge request by creating an issue', fe
     before do
       project.team << [user, :reporter]
       sign_in user
+<<<<<<< HEAD
       visit new_project_issue_path(project,
                                              merge_request_to_resolve_discussions_of: merge_request.iid,
                                              discussion_to_resolve: discussion.id)
+=======
+      visit new_project_issue_path(project, merge_request_to_resolve_discussions_of: merge_request.iid,
+                                            discussion_to_resolve: discussion.id)
+>>>>>>> 9-4-stable
     end
 
     it 'Shows a notice to ask someone else to resolve the discussions' do

spec/features/issues/user_uses_slash_commands_spec.rb

@@ -127,6 +127,7 @@ feature 'Issues > User uses quick actions', feature: true, js: true do
 
     describe 'toggling the WIP prefix from the title from note' do
       let(:issue) { create(:issue, project: project) }
+<<<<<<< HEAD
 
       it 'does not recognize the command nor create a note' do
         write_note("/wip")
@@ -203,6 +204,8 @@ feature 'Issues > User uses quick actions', feature: true, js: true do
 
           expect(page).to have_content '/clear_weight'
           expect(page).not_to have_content 'Commands applied'
+=======
+>>>>>>> 9-4-stable
 
           issue.reload
 

spec/features/merge_requests/edit_mr_spec.rb

@@ -9,7 +9,11 @@ feature 'Edit Merge Request', feature: true do
     before do
       project.team << [user, :master]
 
+<<<<<<< HEAD
       sign_in user
+=======
+    sign_in user
+>>>>>>> 9-4-stable
 
       visit_edit_mr_page
     end

spec/features/milestones/show_spec.rb

@@ -8,7 +8,11 @@ describe 'Milestone show', feature: true do
   let(:issue_params) { { project: project, assignees: [user], author: user, milestone: milestone, labels: labels } }
 
   before do
+<<<<<<< HEAD
     project.add_user(user, :developer)
+=======
+    project.add_user(user, :developer) 
+>>>>>>> 9-4-stable
     sign_in(user)
   end
 

spec/features/projects/pipeline_schedules_spec.rb

@@ -11,7 +11,7 @@ feature 'Pipeline Schedules', :feature, js: true do
 
   before do
     project.add_master(user)
-    gitlab_sign_in(user)
+    sign_in(user)
   end
 
   describe 'GET /projects/pipeline_schedules' do

spec/features/protected_tags_spec.rb

@@ -6,6 +6,7 @@ feature 'Projected Tags', feature: true, js: true do
 
   before do
     sign_in(user)
+<<<<<<< HEAD
   end
 
   def set_allowed_to(operation, option = 'Masters', form: '.new-protected-tag')
@@ -17,6 +18,8 @@ feature 'Projected Tags', feature: true, js: true do
 
       find(".js-allowed-to-#{operation}").click # needed to submit form in some cases
     end
+=======
+>>>>>>> 9-4-stable
   end
 
   def set_protected_tag_name(tag_name)

spec/migrations/migrate_stage_id_reference_in_background_spec.rb

+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20170628080858_migrate_stage_id_reference_in_background')
+
+describe MigrateStageIdReferenceInBackground, :migration, :sidekiq do
+  matcher :be_scheduled_migration do |delay, *expected|
+    match do |migration|
+      BackgroundMigrationWorker.jobs.any? do |job|
+        job['args'] == [migration, expected] &&
+          job['at'].to_i == (delay.to_i + Time.now.to_i)
+      end
+    end
+
+    failure_message do |migration|
+      "Migration `#{migration}` with args `#{expected.inspect}` not scheduled!"
+    end
+  end
+
+  let(:jobs) { table(:ci_builds) }
+  let(:stages) { table(:ci_stages) }
+  let(:pipelines) { table(:ci_pipelines) }
+  let(:projects) { table(:projects) }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 3)
+    stub_const("#{described_class.name}::RANGE_SIZE", 2)
+
+    projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1')
+    projects.create!(id: 345, name: 'gitlab2', path: 'gitlab2')
+
+    pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a')
+    pipelines.create!(id: 2, project_id: 345, ref: 'feature', sha: 'cdf43c3c')
+
+    jobs.create!(id: 1, commit_id: 1, project_id: 123, stage_idx: 2, stage: 'build')
+    jobs.create!(id: 2, commit_id: 1, project_id: 123, stage_idx: 2, stage: 'build')
+    jobs.create!(id: 3, commit_id: 1, project_id: 123, stage_idx: 1, stage: 'test')
+    jobs.create!(id: 4, commit_id: 1, project_id: 123, stage_idx: 3, stage: 'deploy')
+    jobs.create!(id: 5, commit_id: 2, project_id: 345, stage_idx: 1, stage: 'test')
+
+    stages.create(id: 101, pipeline_id: 1, project_id: 123, name: 'test')
+    stages.create(id: 102, pipeline_id: 1, project_id: 123, name: 'build')
+    stages.create(id: 103, pipeline_id: 1, project_id: 123, name: 'deploy')
+
+    jobs.create!(id: 6, commit_id: 2, project_id: 345, stage_id: 101, stage_idx: 1, stage: 'test')
+  end
+
+  it 'correctly schedules background migrations' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 1, 2)
+        expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 3, 3)
+        expect(described_class::MIGRATION).to be_scheduled_migration(4.minutes, 4, 5)
+        expect(BackgroundMigrationWorker.jobs.size).to eq 3
+      end
+    end
+  end
+
+  it 'schedules background migrations' do
+    Sidekiq::Testing.inline! do
+      expect(jobs.where(stage_id: nil).count).to eq 5
+
+      migrate!
+
+      expect(jobs.where(stage_id: nil).count).to eq 1
+    end
+  end
+end

spec/support/sidekiq.rb

@@ -3,3 +3,9 @@ require 'sidekiq/testing/inline'
 Sidekiq::Testing.server_middleware do |chain|
   chain.add Gitlab::SidekiqStatus::ServerMiddleware
 end
+
+RSpec.configure do |config|
+  config.after(:each, :sidekiq) do
+    Sidekiq::Worker.clear_all
+  end
+end

spec/workers/background_migration_worker_spec.rb

 require 'spec_helper'
 
-describe BackgroundMigrationWorker do
+describe BackgroundMigrationWorker, :sidekiq do
   describe '.perform' do
     it 'performs a background migration' do
       expect(Gitlab::BackgroundMigration)
@@ -10,4 +10,35 @@ describe BackgroundMigrationWorker do
       described_class.new.perform('Foo', [10, 20])
     end
   end
+
+  describe '.perform_bulk' do
+    it 'enqueues background migrations in bulk' do
+      Sidekiq::Testing.fake! do
+        described_class.perform_bulk([['Foo', [1]], ['Foo', [2]]])
+
+        expect(described_class.jobs.count).to eq 2
+        expect(described_class.jobs).to all(include('enqueued_at'))
+      end
+    end
+  end
+
+  describe '.perform_bulk_in' do
+    context 'when delay is valid' do
+      it 'correctly schedules background migrations' do
+        Sidekiq::Testing.fake! do
+          described_class.perform_bulk_in(1.minute, [['Foo', [1]], ['Foo', [2]]])
+
+          expect(described_class.jobs.count).to eq 2
+          expect(described_class.jobs).to all(include('at'))
+        end
+      end
+    end
+
+    context 'when delay is invalid' do
+      it 'raises an ArgumentError exception' do
+        expect { described_class.perform_bulk_in(-60, [['Foo']]) }
+          .to raise_error(ArgumentError)
+      end
+    end
+  end
 end