diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 0a77fbeba08..7bf553a0221 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -23,6 +23,7 @@ module Issuable include Sortable include CreatedAtFilterable include UpdatedAtFilterable + include IssuableStates # This object is used to gather issuable meta data for displaying # upvotes, downvotes, notes and closing merge requests count for issues and merge requests diff --git a/app/models/concerns/issuable_states.rb b/app/models/concerns/issuable_states.rb new file mode 100644 index 00000000000..7feaf7e8aac --- /dev/null +++ b/app/models/concerns/issuable_states.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +# == IssuableStates concern +# +# Defines statuses shared by issuables which are persisted on state column +# using the state machine. +# +# Used by EE::Epic, Issue and MergeRequest +# +module IssuableStates + extend ActiveSupport::Concern + + # Override this constant on model where different states are needed + # Check MergeRequest::AVAILABLE_STATES + AVAILABLE_STATES = { opened: 1, closed: 2 }.freeze + + included do + before_save :set_state_id + end + + class_methods do + def states + @states ||= OpenStruct.new(self::AVAILABLE_STATES) + end + end + + # 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 can be removed after the complete migration is done. + def set_state_id + return if state.nil? || state.empty? + + states_hash = self.class.states.to_h.with_indifferent_access + + self.state_id = states_hash[state] + end +end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 2035bffd829..67600383cf9 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -22,6 +22,8 @@ class MergeRequest < ActiveRecord::Base self.reactive_cache_lifetime = 10.minutes SORTING_PREFERENCE_FIELD = :merge_requests_sort + MERGE_REQUEST_STATES = + AVAILABLE_STATES = AVAILABLE_STATES.merge(merged: 3, locked: 4).freeze ignore_column :locked_at, :ref_fetched, diff --git a/db/migrate/20190211131150_add_state_id_to_issuables.rb b/db/migrate/20190211131150_add_state_id_to_issuables.rb new file mode 100644 index 00000000000..af02aa84afd --- /dev/null +++ b/db/migrate/20190211131150_add_state_id_to_issuables.rb @@ -0,0 +1,37 @@ +class AddStateIdToIssuables < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + MIGRATION = 'SyncIssuablesStateId'.freeze + + # TODO - find out how many issues and merge requests in production + # to adapt the batch size and delay interval + # Keep in mind that the migration will be scheduled for issues and merge requests. + BATCH_SIZE = 5000 + DELAY_INTERVAL = 5.minutes.to_i + + 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 + add_column :issues, :state_id, :integer, limit: 1 + add_column :merge_requests, :state_id, :integer, limit: 1 + + queue_background_migration_jobs_by_range_at_intervals(Issue.where(state_id: nil), MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) + queue_background_migration_jobs_by_range_at_intervals(MergeRequest.where(state_id: nil), MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) + end + + def down + remove_column :issues, :state_id + remove_column :merge_requests, :state_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 023eee5f33e..0fb31ce2ef2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20190204115450) do +ActiveRecord::Schema.define(version: 20190211131150) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1046,6 +1046,7 @@ ActiveRecord::Schema.define(version: 20190204115450) 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 @@ -1283,6 +1284,7 @@ ActiveRecord::Schema.define(version: 20190204115450) do t.string "rebase_commit_sha" t.boolean "squash", default: false, null: false 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 diff --git a/lib/gitlab/background_migration/sync_issuables_state_id.rb b/lib/gitlab/background_migration/sync_issuables_state_id.rb new file mode 100644 index 00000000000..1ac86b8acf2 --- /dev/null +++ b/lib/gitlab/background_migration/sync_issuables_state_id.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true +# rubocop:disable Style/Documentation + +module Gitlab + module BackgroundMigration + class SyncIssuablesStateId + def perform(start_id, end_id, model_class) + populate_new_state_id(start_id, end_id, model_class) + end + + def populate_new_state_id(start_id, end_id, model_class) + Rails.logger.info("#{model_class.model_name.human} - Populating state_id: #{start_id} - #{end_id}") + + ActiveRecord::Base.connection.execute <<~SQL + UPDATE #{model_class.table_name} + 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 diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 3abd0600e9d..20cbb9e096b 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -1033,7 +1033,7 @@ into similar problems in the future (e.g. when new tables are created). # `BackgroundMigrationWorker.bulk_perform_in` schedules all jobs for # the same time, which is not helpful in most cases where we wish to # spread the work over time. - BackgroundMigrationWorker.perform_in(delay_interval * index, job_class_name, [start_id, end_id]) + BackgroundMigrationWorker.perform_in(delay_interval * index, job_class_name, [start_id, end_id, model_class]) end end diff --git a/spec/migrations/add_state_id_to_issuables_spec.rb b/spec/migrations/add_state_id_to_issuables_spec.rb new file mode 100644 index 00000000000..4416f416c18 --- /dev/null +++ b/spec/migrations/add_state_id_to_issuables_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20190206144959_change_issuable_states_to_integer.rb') + +describe AddStateIdToIssuables, :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 } + + before do + @group = namespaces.create!(name: 'gitlab', path: 'gitlab') + @project = projects.create!(namespace_id: @group.id) + 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') + nil_state_issue = issues.create!(description: 'third', state: nil) + + migrate! + + issues.reset_column_information + expect(opened_issue.reload.state).to eq(Issue.states.opened) + expect(closed_issue.reload.state).to eq(Issue.states.closed) + expect(nil_state_issue.reload.state).to eq(nil) + 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') + nil_state_merge_request = merge_requests.create!(state: nil, target_project_id: @project.id, target_branch: 'feature5', source_branch: 'master') + + migrate! + + merge_requests.reset_column_information + expect(opened_merge_request.reload.state).to eq(MergeRequest.states.opened) + expect(closed_merge_request.reload.state).to eq(MergeRequest.states.closed) + expect(merged_merge_request.reload.state).to eq(MergeRequest.states.merged) + expect(locked_merge_request.reload.state).to eq(MergeRequest.states.locked) + expect(nil_state_merge_request.reload.state).to eq(nil) + end + end + end + + describe '#down' do + context 'issues' do + it 'migrates state column to string' do + opened_issue = issues.create!(description: 'first', state: 1) + closed_issue = issues.create!(description: 'second', state: 2) + nil_state_issue = issues.create!(description: 'third', state: nil) + + migration.down + + issues.reset_column_information + expect(opened_issue.reload.state).to eq('opened') + expect(closed_issue.reload.state).to eq('closed') + expect(nil_state_issue.reload.state).to eq(nil) + end + end + + context 'merge requests' do + it 'migrates state column to string' do + opened_merge_request = merge_requests.create!(state: 1, target_project_id: @project.id, target_branch: 'feature1', source_branch: 'master') + closed_merge_request = merge_requests.create!(state: 2, target_project_id: @project.id, target_branch: 'feature2', source_branch: 'master') + merged_merge_request = merge_requests.create!(state: 3, target_project_id: @project.id, target_branch: 'feature3', source_branch: 'master') + locked_merge_request = merge_requests.create!(state: 4, target_project_id: @project.id, target_branch: 'feature4', source_branch: 'master') + nil_state_merge_request = merge_requests.create!(state: nil, target_project_id: @project.id, target_branch: 'feature5', source_branch: 'master') + + migration.down + + merge_requests.reset_column_information + expect(opened_merge_request.reload.state).to eq('opened') + expect(closed_merge_request.reload.state).to eq('closed') + expect(merged_merge_request.reload.state).to eq('merged') + expect(locked_merge_request.reload.state).to eq('locked') + expect(nil_state_merge_request.reload.state).to eq(nil) + end + end + end +end