diff options
author | Oswaldo Ferreira <oswaldo@gitlab.com> | 2019-03-22 19:51:47 +0300 |
---|---|---|
committer | Oswaldo Ferreira <oswaldo@gitlab.com> | 2019-03-26 01:50:40 +0300 |
commit | 929ee4d18da886826e9fcc15c35b4d4024bc8237 (patch) | |
tree | 362d6627f738a968fea914e4b264789b1bb6b2a4 | |
parent | 09d25b04c34398f9f825b3cc95265af3097fa035 (diff) |
Add multiple assignees migration and table population
This will be further required for supporting multi-assignees MRs
10 files changed, 242 insertions, 1 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5f6d5095bcc..2c0692e1b45 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -67,6 +67,8 @@ class MergeRequest < ActiveRecord::Base has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue has_many :merge_request_pipelines, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline' + has_many :merge_request_assignees + # Will be deprecated at https://gitlab.com/gitlab-org/gitlab-ce/issues/59457 belongs_to :assignee, class_name: "User" serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize @@ -76,6 +78,10 @@ class MergeRequest < ActiveRecord::Base after_update :reload_diff_if_branch_changed after_save :ensure_metrics + # Required until the codebase starts using this relation for single or multiple assignees. + # TODO: Remove at gitlab-ee#2004 implementation. + after_save :refresh_merge_request_assignees, if: :assignee_id_changed? + # When this attribute is true some MR validation is ignored # It allows us to close or modify broken merge requests attr_accessor :allow_broken @@ -675,6 +681,15 @@ class MergeRequest < ActiveRecord::Base merge_request_diff || create_merge_request_diff end + def refresh_merge_request_assignees + transaction do + # Using it instead relation.delete_all in order to avoid adding a + # dependent: :delete_all (we already have foreign key cascade deletion). + MergeRequestAssignee.where(merge_request_id: self).delete_all + merge_request_assignees.create(user_id: assignee_id) if assignee_id + end + end + def create_merge_request_diff fetch_ref! diff --git a/app/models/merge_request_assignee.rb b/app/models/merge_request_assignee.rb new file mode 100644 index 00000000000..f0e6be51b7f --- /dev/null +++ b/app/models/merge_request_assignee.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +class MergeRequestAssignee < ApplicationRecord + belongs_to :merge_request + belongs_to :assignee, class_name: "User", foreign_key: :user_id +end diff --git a/db/migrate/20190315191339_create_merge_request_assignees_table.rb b/db/migrate/20190315191339_create_merge_request_assignees_table.rb new file mode 100644 index 00000000000..6fc4463f281 --- /dev/null +++ b/db/migrate/20190315191339_create_merge_request_assignees_table.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class CreateMergeRequestAssigneesTable < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + INDEX_NAME = 'index_merge_request_assignees_on_merge_request_id_and_user_id' + + def up + create_table :merge_request_assignees do |t| + t.references :user, foreign_key: { on_delete: :cascade }, index: true, null: false + t.references :merge_request, foreign_key: { on_delete: :cascade }, null: false + end + + add_index :merge_request_assignees, [:merge_request_id, :user_id], unique: true, name: INDEX_NAME + end + + def down + drop_table :merge_request_assignees + end +end diff --git a/db/post_migrate/20190322132835_schedule_populate_merge_request_assignees_table.rb b/db/post_migrate/20190322132835_schedule_populate_merge_request_assignees_table.rb new file mode 100644 index 00000000000..1ecb38e1a86 --- /dev/null +++ b/db/post_migrate/20190322132835_schedule_populate_merge_request_assignees_table.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class SchedulePopulateMergeRequestAssigneesTable < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + BATCH_SIZE = 10_000 + MIGRATION = 'PopulateMergeRequestAssigneesTable' + DELAY_INTERVAL = 8.minutes.to_i + + disable_ddl_transaction! + + def up + say 'Scheduling `PopulateMergeRequestAssigneesTable` jobs' + # We currently have ~4_500_000 merge request records on GitLab.com. + # This means it'll schedule ~450 jobs (10k MRs each) with a 8 minutes gap, + # so this should take ~60 hours for all background migrations to complete. + queue_background_migration_jobs_by_range_at_intervals(MergeRequest, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) + end +end diff --git a/db/schema.rb b/db/schema.rb index dda0445e3f2..0c997f3b8a2 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: 20190301182457) do +ActiveRecord::Schema.define(version: 20190322132835) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1201,6 +1201,14 @@ ActiveRecord::Schema.define(version: 20190301182457) do t.index ["user_id"], name: "index_members_on_user_id", using: :btree end + create_table "merge_request_assignees", force: :cascade do |t| + t.integer "user_id", null: false + t.integer "merge_request_id", null: false + t.index ["merge_request_id", "user_id"], name: "index_merge_request_assignees_on_merge_request_id_and_user_id", unique: true, using: :btree + t.index ["merge_request_id"], name: "index_merge_request_assignees_on_merge_request_id", using: :btree + t.index ["user_id"], name: "index_merge_request_assignees_on_user_id", using: :btree + end + create_table "merge_request_diff_commits", id: false, force: :cascade do |t| t.datetime_with_timezone "authored_date" t.datetime_with_timezone "committed_date" @@ -2454,6 +2462,8 @@ ActiveRecord::Schema.define(version: 20190301182457) do add_foreign_key "lists", "boards", name: "fk_0d3f677137", on_delete: :cascade add_foreign_key "lists", "labels", name: "fk_7a5553d60f", on_delete: :cascade add_foreign_key "members", "users", name: "fk_2e88fb7ce9", on_delete: :cascade + add_foreign_key "merge_request_assignees", "merge_requests", on_delete: :cascade + add_foreign_key "merge_request_assignees", "users", on_delete: :cascade add_foreign_key "merge_request_diff_commits", "merge_request_diffs", on_delete: :cascade add_foreign_key "merge_request_diff_files", "merge_request_diffs", on_delete: :cascade add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade diff --git a/lib/gitlab/background_migration/populate_merge_request_assignees_table.rb b/lib/gitlab/background_migration/populate_merge_request_assignees_table.rb new file mode 100644 index 00000000000..a4c6540c61b --- /dev/null +++ b/lib/gitlab/background_migration/populate_merge_request_assignees_table.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # This background migration creates records on merge_request_assignees according + # to the given merge request IDs range. A _single_ INSERT is issued for the given range. + # This is required for supporting multiple assignees on merge requests. + class PopulateMergeRequestAssigneesTable + def perform(from_id, to_id) + select_sql = + MergeRequest + .where(merge_request_assignees_not_exists_clause) + .where(id: from_id..to_id) + .where('assignee_id IS NOT NULL') + .select(:id, :assignee_id) + .to_sql + + execute("INSERT INTO merge_request_assignees (merge_request_id, user_id) #{select_sql}") + end + + private + + def merge_request_assignees_not_exists_clause + <<~SQL + NOT EXISTS (SELECT 1 FROM merge_request_assignees + WHERE merge_request_assignees.merge_request_id = merge_requests.id) + SQL + end + + def execute(sql) + @connection ||= ActiveRecord::Base.connection + @connection.execute(sql) + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/populate_merge_request_assignees_table_spec.rb b/spec/lib/gitlab/background_migration/populate_merge_request_assignees_table_spec.rb new file mode 100644 index 00000000000..4a81a37d341 --- /dev/null +++ b/spec/lib/gitlab/background_migration/populate_merge_request_assignees_table_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Gitlab::BackgroundMigration::PopulateMergeRequestAssigneesTable, :migration, schema: 20190315191339 do + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + let(:users) { table(:users) } + + let(:user) { users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') } + let(:user_2) { users.create!(email: 'test2@example.com', projects_limit: 100, username: 'test') } + let(:user_3) { users.create!(email: 'test3@example.com', projects_limit: 100, username: 'test') } + + let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') } + let(:project) { projects.create(namespace_id: namespace.id, name: 'foo') } + let(:merge_requests) { table(:merge_requests) } + let(:merge_request_assignees) { table(:merge_request_assignees) } + + def create_merge_request(id, params = {}) + params.merge!(id: id, + target_project_id: project.id, + target_branch: 'master', + source_project_id: project.id, + source_branch: 'mr name', + title: "mr name#{id}") + + merge_requests.create(params) + end + + describe '#perform' do + it 'creates merge_request_assignees rows according to merge_requests' do + create_merge_request(2, assignee_id: user.id) + create_merge_request(3, assignee_id: user_2.id) + create_merge_request(4, assignee_id: user_3.id) + # Test filtering already migrated row + merge_request_assignees.create!(merge_request_id: 2, user_id: user_3.id) + + subject.perform(1, 4) + + rows = merge_request_assignees.order(:id).map { |row| row.attributes.slice('merge_request_id', 'user_id') } + existing_rows = [ + { 'merge_request_id' => 2, 'user_id' => user_3.id } + ] + created_rows = [ + { 'merge_request_id' => 3, 'user_id' => user_2.id }, + { 'merge_request_id' => 4, 'user_id' => user_3.id } + ] + expected_rows = existing_rows + created_rows + + expect(rows.size).to eq(expected_rows.size) + expected_rows.each do |expected_row| + expect(rows).to include(expected_row) + end + end + end +end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 01da3ea7081..5299ab297f6 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -100,6 +100,7 @@ merge_requests: - head_pipeline - latest_merge_request_diff - merge_request_pipelines +- merge_request_assignees merge_request_diff: - merge_request - merge_request_diff_commits diff --git a/spec/migrations/schedule_populate_merge_request_assignees_table_spec.rb b/spec/migrations/schedule_populate_merge_request_assignees_table_spec.rb new file mode 100644 index 00000000000..e397fbb7138 --- /dev/null +++ b/spec/migrations/schedule_populate_merge_request_assignees_table_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20190322132835_schedule_populate_merge_request_assignees_table.rb') + +describe SchedulePopulateMergeRequestAssigneesTable, :migration, :sidekiq do + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') } + let(:project) { projects.create(namespace_id: namespace.id, name: 'foo') } + let(:merge_requests) { table(:merge_requests) } + + def create_merge_request(id) + params = { + id: id, + target_project_id: project.id, + target_branch: 'master', + source_project_id: project.id, + source_branch: 'mr name', + title: "mr name#{id}" + } + + merge_requests.create!(params) + end + + it 'correctly schedules background migrations' do + create_merge_request(1) + create_merge_request(2) + create_merge_request(3) + + stub_const("#{described_class.name}::BATCH_SIZE", 2) + + Sidekiq::Testing.fake! do + Timecop.freeze do + migrate! + + expect(described_class::MIGRATION) + .to be_scheduled_delayed_migration(8.minutes, 1, 2) + + expect(described_class::MIGRATION) + .to be_scheduled_delayed_migration(16.minutes, 3, 3) + + expect(BackgroundMigrationWorker.jobs.size).to eq(2) + end + end + end +end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 22998bc5b6a..ec39c174171 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -184,6 +184,31 @@ describe MergeRequest do expect(MergeRequest::Metrics.count).to eq(1) end end + + describe '#refresh_merge_request_assignees' do + set(:user) { create(:user) } + + it 'creates merge request assignees relation upon MR creation' do + merge_request = create(:merge_request, assignee: nil) + + expect(merge_request.merge_request_assignees).to be_empty + + expect { merge_request.update!(assignee: user) } + .to change { merge_request.reload.merge_request_assignees.count } + .from(0).to(1) + end + + it 'updates merge request assignees relation upon MR assignee change' do + another_user = create(:user) + merge_request = create(:merge_request, assignee: user) + + expect { merge_request.update!(assignee: another_user) } + .to change { merge_request.reload.merge_request_assignees.first.assignee } + .from(user).to(another_user) + + expect(merge_request.merge_request_assignees.count).to eq(1) + end + end end describe 'respond to' do |