Automatic merge of gitlab-org/gitlab-ce master

app/models/concerns/issuable.rb

@@ -23,6 +23,7 @@ module Issuable
   include Sortable
   include CreatedAtFilterable
   include UpdatedAtFilterable
+  include IssuableStates
   include ClosedAtFilterable
 
   # This object is used to gather issuable meta data for displaying
@@ -145,6 +146,15 @@ module Issuable
       fuzzy_search(query, [:title])
     end
 
+    # Available state values persisted in state_id column using state machine
+    #
+    # Override this on subclasses if different states are needed
+    #
+    # Check MergeRequest.available_states for example
+    def available_states
+      @available_states ||= { opened: 1, closed: 2 }.with_indifferent_access
+    end
+
     # Searches for records with a matching title or description.
     #
     # This method uses ILIKE on PostgreSQL and LIKE on MySQL.

app/models/concerns/issuable_states.rb

+# frozen_string_literal: true
+
+module IssuableStates
+  extend ActiveSupport::Concern
+
+  # The state:string column is being migrated to state_id:integer column
+  # This is a temporary hook to populate state_id column with new values
+  # and should be removed after the state column is removed.
+  # Check https://gitlab.com/gitlab-org/gitlab-ce/issues/51789 for more information
+  included do
+    before_save :set_state_id
+  end
+
+  def set_state_id
+    return if state.nil? || state.empty?
+
+    # Needed to prevent breaking some migration specs that
+    # rollback database to a point where state_id does not exist.
+    # We can use this guard clause for now since this file will
+    # be removed in the next release.
+    return unless self.has_attribute?(:state_id)
+
+    self.state_id = self.class.available_states[state]
+  end
+end

app/models/merge_request.rb

@@ -212,6 +212,10 @@ class MergeRequest < ApplicationRecord
     '!'
   end
 
+  def self.available_states
+    @available_states ||= super.merge(merged: 3, locked: 4)
+  end
+
   # Returns the top 100 target branches
   #
   # The returned value is a Array containing branch names

db/migrate/20190211131150_add_state_id_to_issuables.rb

+# frozen_string_literal: true
+
+class AddStateIdToIssuables < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    add_column :issues, :state_id, :integer, limit: 2
+    add_column :merge_requests, :state_id, :integer, limit: 2
+  end
+
+  def down
+    remove_column :issues, :state_id
+    remove_column :merge_requests, :state_id
+  end
+end

db/post_migrate/20190214112022_schedule_sync_issuables_state_id.rb

+class ScheduleSyncIssuablesStateId < ActiveRecord::Migration[5.0]
+  # This migration schedules the sync of state_id for issues and merge requests
+  # which are converting the state column from string to integer.
+  # For more information check: https://gitlab.com/gitlab-org/gitlab-ce/issues/51789
+
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  # 2019-02-12 gitlab.com issuable numbers
+  # issues count: 13587305
+  # merge requests count: 18925274
+  #
+  # Using 5000 as batch size and 115 seconds interval will give:
+  # 2718 jobs for issues - taking ~86 hours
+  # 3786 jobs for merge requests - taking ~120 hours
+  #
+  BATCH_SIZE = 5000
+  DELAY_INTERVAL = 120.seconds.to_i
+  ISSUES_MIGRATION = 'SyncIssuesStateId'.freeze
+  MERGE_REQUESTS_MIGRATION = 'SyncMergeRequestsStateId'.freeze
+
+  disable_ddl_transaction!
+
+  class Issue < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'issues'
+  end
+
+  class MergeRequest < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'merge_requests'
+  end
+
+  def up
+    queue_background_migration_jobs_by_range_at_intervals(
+      Issue.all,
+      ISSUES_MIGRATION,
+      DELAY_INTERVAL,
+      batch_size: BATCH_SIZE
+    )
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      MergeRequest.all,
+      MERGE_REQUESTS_MIGRATION,
+      DELAY_INTERVAL,
+      batch_size: BATCH_SIZE
+    )
+  end
+
+  def down
+    # No op
+  end
+end

db/schema.rb

@@ -1597,6 +1597,7 @@ ActiveRecord::Schema.define(version: 20190325165127) do
     t.boolean "discussion_locked"
     t.datetime_with_timezone "closed_at"
     t.integer "closed_by_id"
+    t.integer "state_id", limit: 2
     t.index ["author_id"], name: "index_issues_on_author_id", using: :btree
     t.index ["closed_by_id"], name: "index_issues_on_closed_by_id", using: :btree
     t.index ["confidential"], name: "index_issues_on_confidential", using: :btree
@@ -1876,6 +1877,7 @@ ActiveRecord::Schema.define(version: 20190325165127) do
     t.boolean "discussion_locked"
     t.integer "latest_merge_request_diff_id"
     t.boolean "allow_maintainer_to_push"
+    t.integer "state_id", limit: 2
     t.index ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
     t.index ["author_id"], name: "index_merge_requests_on_author_id", using: :btree
     t.index ["created_at"], name: "index_merge_requests_on_created_at", using: :btree

lib/gitlab/background_migration/sync_issues_state_id.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class SyncIssuesStateId
+      def perform(start_id, end_id)
+        ActiveRecord::Base.connection.execute <<~SQL
+          UPDATE issues
+          SET state_id =
+            CASE state
+            WHEN 'opened' THEN 1
+            WHEN 'closed' THEN 2
+            END
+          WHERE state_id IS NULL
+          AND id BETWEEN #{start_id} AND #{end_id}
+        SQL
+      end
+    end
+  end
+end

lib/gitlab/background_migration/sync_merge_requests_state_id.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class SyncMergeRequestsStateId
+      def perform(start_id, end_id)
+        ActiveRecord::Base.connection.execute <<~SQL
+          UPDATE merge_requests
+          SET state_id =
+            CASE state
+            WHEN 'opened' THEN 1
+            WHEN 'closed' THEN 2
+            WHEN 'merged' THEN 3
+            WHEN 'locked' THEN 4
+            END
+          WHERE state_id IS NULL
+          AND id BETWEEN #{start_id} AND #{end_id}
+        SQL
+      end
+    end
+  end
+end

spec/db/schema_spec.rb

@@ -29,12 +29,12 @@ describe 'Database schema' do
     events: %w[target_id],
     forked_project_links: %w[forked_from_project_id],
     identities: %w[user_id],
-    issues: %w[last_edited_by_id],
+    issues: %w[last_edited_by_id state_id],
     keys: %w[user_id],
     label_links: %w[target_id],
     lfs_objects_projects: %w[lfs_object_id project_id],
     members: %w[source_id created_by_id],
-    merge_requests: %w[last_edited_by_id],
+    merge_requests: %w[last_edited_by_id state_id],
     namespaces: %w[owner_id parent_id],
     notes: %w[author_id commit_id noteable_id updated_by_id resolved_by_id discussion_id],
     notification_settings: %w[source_id],

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -11,6 +11,7 @@ Issue:
 - branch_name
 - description
 - state
+- state_id
 - iid
 - updated_by_id
 - confidential
@@ -161,6 +162,7 @@ MergeRequest:
 - created_at
 - updated_at
 - state
+- state_id
 - merge_status
 - target_project_id
 - iid

spec/migrations/schedule_sync_issuables_state_id_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20190214112022_schedule_sync_issuables_state_id.rb')
+
+describe ScheduleSyncIssuablesStateId, :migration do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:merge_requests) { table(:merge_requests) }
+  let(:issues) { table(:issues) }
+  let(:migration) { described_class.new }
+  let(:group) { namespaces.create!(name: 'gitlab', path: 'gitlab') }
+  let(:project) { projects.create!(namespace_id: group.id) }
+
+  shared_examples 'scheduling migrations' do
+    before do
+      Sidekiq::Worker.clear_all
+      stub_const("#{described_class.name}::BATCH_SIZE", 2)
+    end
+
+    it 'correctly schedules issuable sync background migration' do
+      Sidekiq::Testing.fake! do
+        Timecop.freeze do
+          migrate!
+
+          expect(migration).to be_scheduled_delayed_migration(120.seconds, resource_1.id, resource_2.id)
+          expect(migration).to be_scheduled_delayed_migration(240.seconds, resource_3.id, resource_4.id)
+          expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+        end
+      end
+    end
+  end
+
+  describe '#up' do
+    context 'issues' do
+      it 'migrates state column to integer' do
+        opened_issue = issues.create!(description: 'first', state: 'opened')
+        closed_issue = issues.create!(description: 'second', state: 'closed')
+        invalid_state_issue = issues.create!(description: 'fourth', state: 'not valid')
+
+        migrate!
+
+        expect(opened_issue.reload.state_id).to eq(Issue.available_states[:opened])
+        expect(closed_issue.reload.state_id).to eq(Issue.available_states[:closed])
+        expect(invalid_state_issue.reload.state_id).to be_nil
+      end
+
+      it_behaves_like 'scheduling migrations' do
+        let(:migration) { described_class::ISSUES_MIGRATION }
+        let!(:resource_1) { issues.create!(description: 'first', state: 'opened') }
+        let!(:resource_2) { issues.create!(description: 'second', state: 'closed') }
+        let!(:resource_3) { issues.create!(description: 'third', state: 'closed') }
+        let!(:resource_4) { issues.create!(description: 'fourth', state: 'closed') }
+      end
+    end
+
+    context 'merge requests' do
+      it 'migrates state column to integer' do
+        opened_merge_request = merge_requests.create!(state: 'opened', target_project_id: project.id, target_branch: 'feature1', source_branch: 'master')
+        closed_merge_request = merge_requests.create!(state: 'closed', target_project_id: project.id, target_branch: 'feature2', source_branch: 'master')
+        merged_merge_request = merge_requests.create!(state: 'merged', target_project_id: project.id, target_branch: 'feature3', source_branch: 'master')
+        locked_merge_request = merge_requests.create!(state: 'locked', target_project_id: project.id, target_branch: 'feature4', source_branch: 'master')
+
+        migrate!
+
+        expect(opened_merge_request.reload.state_id).to eq(MergeRequest.available_states[:opened])
+        expect(closed_merge_request.reload.state_id).to eq(MergeRequest.available_states[:closed])
+        expect(merged_merge_request.reload.state_id).to eq(MergeRequest.available_states[:merged])
+        expect(locked_merge_request.reload.state_id).to eq(MergeRequest.available_states[:locked])
+      end
+
+      it_behaves_like 'scheduling migrations' do
+        let(:migration) { described_class::MERGE_REQUESTS_MIGRATION }
+        let!(:resource_1) { merge_requests.create!(state: 'opened', target_project_id: project.id, target_branch: 'feature1', source_branch: 'master') }
+        let!(:resource_2) { merge_requests.create!(state: 'closed', target_project_id: project.id, target_branch: 'feature2', source_branch: 'master') }
+        let!(:resource_3) { merge_requests.create!(state: 'merged', target_project_id: project.id, target_branch: 'feature3', source_branch: 'master') }
+        let!(:resource_4) { merge_requests.create!(state: 'locked', target_project_id: project.id, target_branch: 'feature4', source_branch: 'master') }
+      end
+    end
+  end
+end

spec/models/concerns/issuable_states_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# This spec checks if state_id column of issues and merge requests
+# are being synced on every save.
+# It can be removed in the next release. Check https://gitlab.com/gitlab-org/gitlab-ce/issues/51789  for more information.
+describe IssuableStates do
+  [Issue, MergeRequest].each do |klass|
+    it "syncs state_id column when #{klass.model_name.human} gets created" do
+      klass.available_states.each do |state, state_id|
+        issuable = build(klass.model_name.param_key, state: state.to_s)
+
+        issuable.save!
+
+        expect(issuable.state_id).to eq(state_id)
+      end
+    end
+
+    it "syncs state_id column when #{klass.model_name.human} gets updated" do
+      klass.available_states.each do |state, state_id|
+        issuable = create(klass.model_name.param_key, state: state.to_s)
+
+        issuable.update(state: state)
+
+        expect(issuable.state_id).to eq(state_id)
+      end
+    end
+  end
+end