From 936e9e895000c9f9ae39713c969b325f6b59c0c3 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 14 Nov 2017 16:58:56 +0100 Subject: Clean up schema of the "merge_requests" table This adds various foreign keys and indexes to the "merge_requests" table as outlined in https://gitlab.com/gitlab-org/gitlab-ce/issues/31825. Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/31825 --- .../unreleased/merge-requests-schema-cleanup.yml | 5 +++ ...4150259_merge_requests_author_id_foreign_key.rb | 43 ++++++++++++++++++++ ...60005_merge_requests_assignee_id_foreign_key.rb | 39 ++++++++++++++++++ ...904_merge_requests_updated_by_id_foreign_key.rb | 46 ++++++++++++++++++++++ ...720_merge_requests_merge_user_id_foreign_key.rb | 46 ++++++++++++++++++++++ ...merge_requests_source_project_id_foreign_key.rb | 45 +++++++++++++++++++++ ...2227_merge_requests_milestone_id_foreign_key.rb | 39 ++++++++++++++++++ db/schema.rb | 12 +++++- lib/gitlab/import_export/merge_request_parser.rb | 2 +- .../import_export/project_tree_restorer_spec.rb | 2 +- spec/services/milestones/destroy_service_spec.rb | 2 +- 11 files changed, 276 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/merge-requests-schema-cleanup.yml create mode 100644 db/migrate/20171114150259_merge_requests_author_id_foreign_key.rb create mode 100644 db/migrate/20171114160005_merge_requests_assignee_id_foreign_key.rb create mode 100644 db/migrate/20171114160904_merge_requests_updated_by_id_foreign_key.rb create mode 100644 db/migrate/20171114161720_merge_requests_merge_user_id_foreign_key.rb create mode 100644 db/migrate/20171114161914_merge_requests_source_project_id_foreign_key.rb create mode 100644 db/migrate/20171114162227_merge_requests_milestone_id_foreign_key.rb diff --git a/changelogs/unreleased/merge-requests-schema-cleanup.yml b/changelogs/unreleased/merge-requests-schema-cleanup.yml new file mode 100644 index 00000000000..ccce9b1436c --- /dev/null +++ b/changelogs/unreleased/merge-requests-schema-cleanup.yml @@ -0,0 +1,5 @@ +--- +title: Clean up schema of the "merge_requests" table +merge_request: +author: +type: other diff --git a/db/migrate/20171114150259_merge_requests_author_id_foreign_key.rb b/db/migrate/20171114150259_merge_requests_author_id_foreign_key.rb new file mode 100644 index 00000000000..021eaa04a0c --- /dev/null +++ b/db/migrate/20171114150259_merge_requests_author_id_foreign_key.rb @@ -0,0 +1,43 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MergeRequestsAuthorIdForeignKey < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + class MergeRequest < ActiveRecord::Base + include EachBatch + + self.table_name = 'merge_requests' + + def self.with_orphaned_authors + where('NOT EXISTS (SELECT true FROM users WHERE merge_requests.author_id = users.id)') + .where('author_id IS NOT NULL') + end + end + + def up + # Replacing the ghost user ID logic would be too complex, hence we don't + # redefine the User model here. + ghost_id = User.select(:id).ghost.id + + MergeRequest.with_orphaned_authors.each_batch(of: 100) do |batch| + batch.update_all(author_id: ghost_id) + end + + add_concurrent_foreign_key( + :merge_requests, + :users, + column: :author_id, + on_delete: :nullify + ) + end + + def down + remove_foreign_key(:merge_requests, column: :author_id) + end +end diff --git a/db/migrate/20171114160005_merge_requests_assignee_id_foreign_key.rb b/db/migrate/20171114160005_merge_requests_assignee_id_foreign_key.rb new file mode 100644 index 00000000000..1a242f01051 --- /dev/null +++ b/db/migrate/20171114160005_merge_requests_assignee_id_foreign_key.rb @@ -0,0 +1,39 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MergeRequestsAssigneeIdForeignKey < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + class MergeRequest < ActiveRecord::Base + include EachBatch + + self.table_name = 'merge_requests' + + def self.with_orphaned_assignees + where('NOT EXISTS (SELECT true FROM users WHERE merge_requests.assignee_id = users.id)') + .where('assignee_id IS NOT NULL') + end + end + + def up + MergeRequest.with_orphaned_assignees.each_batch(of: 100) do |batch| + batch.update_all(assignee_id: nil) + end + + add_concurrent_foreign_key( + :merge_requests, + :users, + column: :assignee_id, + on_delete: :nullify + ) + end + + def down + remove_foreign_key(:merge_requests, column: :assignee_id) + end +end diff --git a/db/migrate/20171114160904_merge_requests_updated_by_id_foreign_key.rb b/db/migrate/20171114160904_merge_requests_updated_by_id_foreign_key.rb new file mode 100644 index 00000000000..eb3872e38da --- /dev/null +++ b/db/migrate/20171114160904_merge_requests_updated_by_id_foreign_key.rb @@ -0,0 +1,46 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MergeRequestsUpdatedByIdForeignKey < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + class MergeRequest < ActiveRecord::Base + include EachBatch + + self.table_name = 'merge_requests' + + def self.with_orphaned_updaters + where('NOT EXISTS (SELECT true FROM users WHERE merge_requests.updated_by_id = users.id)') + .where('updated_by_id IS NOT NULL') + end + end + + def up + MergeRequest.with_orphaned_updaters.each_batch(of: 100) do |batch| + batch.update_all(updated_by_id: nil) + end + + add_concurrent_index( + :merge_requests, + :updated_by_id, + where: 'updated_by_id IS NOT NULL' + ) + + add_concurrent_foreign_key( + :merge_requests, + :users, + column: :updated_by_id, + on_delete: :nullify + ) + end + + def down + remove_foreign_key_without_error(:merge_requests, column: :updated_by_id) + remove_concurrent_index(:merge_requests, :updated_by_id) + end +end diff --git a/db/migrate/20171114161720_merge_requests_merge_user_id_foreign_key.rb b/db/migrate/20171114161720_merge_requests_merge_user_id_foreign_key.rb new file mode 100644 index 00000000000..925b3e537d7 --- /dev/null +++ b/db/migrate/20171114161720_merge_requests_merge_user_id_foreign_key.rb @@ -0,0 +1,46 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MergeRequestsMergeUserIdForeignKey < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + class MergeRequest < ActiveRecord::Base + include EachBatch + + self.table_name = 'merge_requests' + + def self.with_orphaned_mergers + where('NOT EXISTS (SELECT true FROM users WHERE merge_requests.merge_user_id = users.id)') + .where('merge_user_id IS NOT NULL') + end + end + + def up + MergeRequest.with_orphaned_mergers.each_batch(of: 100) do |batch| + batch.update_all(merge_user_id: nil) + end + + add_concurrent_index( + :merge_requests, + :merge_user_id, + where: 'merge_user_id IS NOT NULL' + ) + + add_concurrent_foreign_key( + :merge_requests, + :users, + column: :merge_user_id, + on_delete: :nullify + ) + end + + def down + remove_foreign_key_without_error(:merge_requests, column: :merge_user_id) + remove_concurrent_index(:merge_requests, :merge_user_id) + end +end diff --git a/db/migrate/20171114161914_merge_requests_source_project_id_foreign_key.rb b/db/migrate/20171114161914_merge_requests_source_project_id_foreign_key.rb new file mode 100644 index 00000000000..2965e580c84 --- /dev/null +++ b/db/migrate/20171114161914_merge_requests_source_project_id_foreign_key.rb @@ -0,0 +1,45 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MergeRequestsSourceProjectIdForeignKey < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + class MergeRequest < ActiveRecord::Base + include EachBatch + + self.table_name = 'merge_requests' + + def self.with_orphaned_source_projects + where('NOT EXISTS (SELECT true FROM projects WHERE merge_requests.source_project_id = projects.id)') + .where('source_project_id IS NOT NULL') + end + end + + def up + MergeRequest.with_orphaned_source_projects.each_batch(of: 100) do |batch| + batch.update_all(source_project_id: nil) + end + + # We need to allow NULL values so we can nullify the column when the source + # project is removed. We _don't_ want to remove the merge request, instead + # the application will keep them but close them. + change_column_null(:merge_requests, :source_project_id, true) + + add_concurrent_foreign_key( + :merge_requests, + :projects, + column: :source_project_id, + on_delete: :nullify + ) + end + + def down + remove_foreign_key_without_error(:merge_requests, column: :source_project_id) + change_column_null(:merge_requests, :source_project_id, false) + end +end diff --git a/db/migrate/20171114162227_merge_requests_milestone_id_foreign_key.rb b/db/migrate/20171114162227_merge_requests_milestone_id_foreign_key.rb new file mode 100644 index 00000000000..c005cf7d173 --- /dev/null +++ b/db/migrate/20171114162227_merge_requests_milestone_id_foreign_key.rb @@ -0,0 +1,39 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MergeRequestsMilestoneIdForeignKey < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + class MergeRequest < ActiveRecord::Base + include EachBatch + + self.table_name = 'merge_requests' + + def self.with_orphaned_milestones + where('NOT EXISTS (SELECT true FROM milestones WHERE merge_requests.milestone_id = milestones.id)') + .where('milestone_id IS NOT NULL') + end + end + + def up + MergeRequest.with_orphaned_milestones.each_batch(of: 100) do |batch| + batch.update_all(milestone_id: nil) + end + + add_concurrent_foreign_key( + :merge_requests, + :milestones, + column: :milestone_id, + on_delete: :nullify + ) + end + + def down + remove_foreign_key_without_error(:merge_requests, column: :milestone_id) + end +end diff --git a/db/schema.rb b/db/schema.rb index 25b4fa8b888..7afab18df08 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20171114104051) do +ActiveRecord::Schema.define(version: 20171114162227) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1040,7 +1040,7 @@ ActiveRecord::Schema.define(version: 20171114104051) do create_table "merge_requests", force: :cascade do |t| t.string "target_branch", null: false t.string "source_branch", null: false - t.integer "source_project_id", null: false + t.integer "source_project_id" t.integer "author_id" t.integer "assignee_id" t.string "title" @@ -1080,6 +1080,7 @@ ActiveRecord::Schema.define(version: 20171114104051) do add_index "merge_requests", ["description"], name: "index_merge_requests_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"} add_index "merge_requests", ["head_pipeline_id"], name: "index_merge_requests_on_head_pipeline_id", using: :btree add_index "merge_requests", ["latest_merge_request_diff_id"], name: "index_merge_requests_on_latest_merge_request_diff_id", using: :btree + add_index "merge_requests", ["merge_user_id"], name: "index_merge_requests_on_merge_user_id", where: "(merge_user_id IS NOT NULL)", using: :btree add_index "merge_requests", ["milestone_id"], name: "index_merge_requests_on_milestone_id", using: :btree add_index "merge_requests", ["source_branch"], name: "index_merge_requests_on_source_branch", using: :btree add_index "merge_requests", ["source_project_id", "source_branch"], name: "index_merge_requests_on_source_project_id_and_source_branch", using: :btree @@ -1088,6 +1089,7 @@ ActiveRecord::Schema.define(version: 20171114104051) do add_index "merge_requests", ["target_project_id", "merge_commit_sha", "id"], name: "index_merge_requests_on_tp_id_and_merge_commit_sha_and_id", using: :btree add_index "merge_requests", ["title"], name: "index_merge_requests_on_title", using: :btree add_index "merge_requests", ["title"], name: "index_merge_requests_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"} + add_index "merge_requests", ["updated_by_id"], name: "index_merge_requests_on_updated_by_id", where: "(updated_by_id IS NOT NULL)", using: :btree create_table "merge_requests_closing_issues", force: :cascade do |t| t.integer "merge_request_id", null: false @@ -1965,7 +1967,13 @@ ActiveRecord::Schema.define(version: 20171114104051) do add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade add_foreign_key "merge_requests", "ci_pipelines", column: "head_pipeline_id", name: "fk_fd82eae0b9", on_delete: :nullify add_foreign_key "merge_requests", "merge_request_diffs", column: "latest_merge_request_diff_id", name: "fk_06067f5644", on_delete: :nullify + add_foreign_key "merge_requests", "milestones", name: "fk_6a5165a692", on_delete: :nullify + add_foreign_key "merge_requests", "projects", column: "source_project_id", name: "fk_3308fe130c", on_delete: :nullify add_foreign_key "merge_requests", "projects", column: "target_project_id", name: "fk_a6963e8447", on_delete: :cascade + add_foreign_key "merge_requests", "users", column: "assignee_id", name: "fk_6149611a04", on_delete: :nullify + add_foreign_key "merge_requests", "users", column: "author_id", name: "fk_e719a85f8a", on_delete: :nullify + add_foreign_key "merge_requests", "users", column: "merge_user_id", name: "fk_ad525e1f87", on_delete: :nullify + add_foreign_key "merge_requests", "users", column: "updated_by_id", name: "fk_641731faff", on_delete: :nullify add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade add_foreign_key "merge_requests_closing_issues", "merge_requests", on_delete: :cascade add_foreign_key "milestones", "namespaces", column: "group_id", name: "fk_95650a40d4", on_delete: :cascade diff --git a/lib/gitlab/import_export/merge_request_parser.rb b/lib/gitlab/import_export/merge_request_parser.rb index 61db4bd9ccc..f3d7407383c 100644 --- a/lib/gitlab/import_export/merge_request_parser.rb +++ b/lib/gitlab/import_export/merge_request_parser.rb @@ -1,7 +1,7 @@ module Gitlab module ImportExport class MergeRequestParser - FORKED_PROJECT_ID = -1 + FORKED_PROJECT_ID = nil def initialize(project, diff_head_sha, merge_request, relation_hash) @project = project diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index e4b4cf5ba85..c2bda6f8821 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -155,7 +155,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do end it 'has no source if source/target differ' do - expect(MergeRequest.find_by_title('MR2').source_project_id).to eq(-1) + expect(MergeRequest.find_by_title('MR2').source_project_id).to be_nil end end diff --git a/spec/services/milestones/destroy_service_spec.rb b/spec/services/milestones/destroy_service_spec.rb index 16e288b3148..af35e17bfa7 100644 --- a/spec/services/milestones/destroy_service_spec.rb +++ b/spec/services/milestones/destroy_service_spec.rb @@ -5,7 +5,7 @@ describe Milestones::DestroyService do let(:project) { create(:project) } let(:milestone) { create(:milestone, title: 'Milestone v1.0', project: project) } let!(:issue) { create(:issue, project: project, milestone: milestone) } - let(:merge_request) { create(:merge_request, source_project: project, milestone: milestone) } + let!(:merge_request) { create(:merge_request, source_project: project, milestone: milestone) } before do project.team << [user, :master] -- cgit v1.2.1