From d825c9cb5d89d18685a196789b477df83998fed2 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 6 Nov 2017 14:46:27 +0100 Subject: [PATCH] Clean up schema of the "issues" table This adds various foreign key constraints, indexes, missing NOT NULL constraints, and changes some column types from timestamp to timestamptz. Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/31811 --- app/serializers/issue_entity.rb | 1 - .../unreleased/cleanup-issues-schema.yml | 5 +++ ...1106132212_issues_confidential_not_null.rb | 23 ++++++++++ ...6135924_issues_milestone_id_foreign_key.rb | 38 ++++++++++++++++ ...150657_issues_updated_by_id_foreign_key.rb | 45 +++++++++++++++++++ ...06151218_issues_moved_to_id_foreign_key.rb | 44 ++++++++++++++++++ ...0171106154015_remove_issues_branch_name.rb | 13 ++++++ ..._issues_due_date_index_to_partial_index.rb | 37 +++++++++++++++ ...171453_add_timezone_to_issues_closed_at.rb | 19 ++++++++ ...leanup_add_timezone_to_issues_closed_at.rb | 19 ++++++++ db/schema.rb | 14 +++--- lib/gitlab/hook_data/issue_builder.rb | 1 - spec/features/issues/issue_detail_spec.rb | 7 +-- .../gitlab/hook_data/issue_builder_spec.rb | 1 - .../milestones/destroy_service_spec.rb | 2 +- 15 files changed, 257 insertions(+), 12 deletions(-) create mode 100644 changelogs/unreleased/cleanup-issues-schema.yml create mode 100644 db/migrate/20171106132212_issues_confidential_not_null.rb create mode 100644 db/migrate/20171106135924_issues_milestone_id_foreign_key.rb create mode 100644 db/migrate/20171106150657_issues_updated_by_id_foreign_key.rb create mode 100644 db/migrate/20171106151218_issues_moved_to_id_foreign_key.rb create mode 100644 db/migrate/20171106154015_remove_issues_branch_name.rb create mode 100644 db/migrate/20171106155656_turn_issues_due_date_index_to_partial_index.rb create mode 100644 db/migrate/20171106171453_add_timezone_to_issues_closed_at.rb create mode 100644 db/post_migrate/20171106180641_cleanup_add_timezone_to_issues_closed_at.rb diff --git a/app/serializers/issue_entity.rb b/app/serializers/issue_entity.rb index 5f47592e4ad..9d52b8d9752 100644 --- a/app/serializers/issue_entity.rb +++ b/app/serializers/issue_entity.rb @@ -3,7 +3,6 @@ class IssueEntity < IssuableEntity expose :state expose :deleted_at - expose :branch_name expose :confidential expose :discussion_locked expose :assignees, using: API::Entities::UserBasic diff --git a/changelogs/unreleased/cleanup-issues-schema.yml b/changelogs/unreleased/cleanup-issues-schema.yml new file mode 100644 index 00000000000..9f5fb0bdf82 --- /dev/null +++ b/changelogs/unreleased/cleanup-issues-schema.yml @@ -0,0 +1,5 @@ +--- +title: Clean up schema of the "issues" table +merge_request: +author: +type: other diff --git a/db/migrate/20171106132212_issues_confidential_not_null.rb b/db/migrate/20171106132212_issues_confidential_not_null.rb new file mode 100644 index 00000000000..c959d2dd938 --- /dev/null +++ b/db/migrate/20171106132212_issues_confidential_not_null.rb @@ -0,0 +1,23 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class IssuesConfidentialNotNull < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + class Issue < ActiveRecord::Base + self.table_name = 'issues' + end + + def up + Issue.where('confidential IS NULL').update_all(confidential: false) + + change_column_null :issues, :confidential, false + end + + def down + # There's no way / point to revert this. + end +end diff --git a/db/migrate/20171106135924_issues_milestone_id_foreign_key.rb b/db/migrate/20171106135924_issues_milestone_id_foreign_key.rb new file mode 100644 index 00000000000..e6a780d0964 --- /dev/null +++ b/db/migrate/20171106135924_issues_milestone_id_foreign_key.rb @@ -0,0 +1,38 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class IssuesMilestoneIdForeignKey < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + class Issue < ActiveRecord::Base + include EachBatch + + self.table_name = 'issues' + + def self.with_orphaned_milestones + where('NOT EXISTS (SELECT true FROM milestones WHERE milestones.id = issues.milestone_id)') + end + end + + def up + Issue.with_orphaned_milestones.each_batch(of: 100) do |batch| + batch.update_all(milestone_id: nil) + end + + add_concurrent_foreign_key( + :issues, + :milestones, + column: :milestone_id, + on_delete: :nullify + ) + end + + def down + remove_foreign_key_without_error(:issues, column: :milestone_id) + end +end diff --git a/db/migrate/20171106150657_issues_updated_by_id_foreign_key.rb b/db/migrate/20171106150657_issues_updated_by_id_foreign_key.rb new file mode 100644 index 00000000000..3b8844d7d9f --- /dev/null +++ b/db/migrate/20171106150657_issues_updated_by_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 IssuesUpdatedByIdForeignKey < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + class Issue < ActiveRecord::Base + include EachBatch + + self.table_name = 'issues' + + def self.with_orphaned_updaters + where('NOT EXISTS (SELECT true FROM users WHERE users.id = issues.updated_by_id)') + .where('updated_by_id IS NOT NULL') + end + end + + def up + Issue.with_orphaned_updaters.each_batch(of: 100) do |batch| + batch.update_all(updated_by_id: nil) + end + + # This index is only used for foreign keys, and those in turn will always + # specify a value. As such we can add a WHERE condition to make the index + # smaller. + add_concurrent_index(:issues, :updated_by_id, where: 'updated_by_id IS NOT NULL') + + add_concurrent_foreign_key( + :issues, + :users, + column: :updated_by_id, + on_delete: :nullify + ) + end + + def down + remove_foreign_key_without_error(:issues, column: :updated_by_id) + remove_concurrent_index(:issues, :updated_by_id) + end +end diff --git a/db/migrate/20171106151218_issues_moved_to_id_foreign_key.rb b/db/migrate/20171106151218_issues_moved_to_id_foreign_key.rb new file mode 100644 index 00000000000..8d2ceb8cc18 --- /dev/null +++ b/db/migrate/20171106151218_issues_moved_to_id_foreign_key.rb @@ -0,0 +1,44 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class IssuesMovedToIdForeignKey < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + class Issue < ActiveRecord::Base + include EachBatch + + self.table_name = 'issues' + + def self.with_orphaned_moved_to_issues + where('NOT EXISTS (SELECT true FROM issues WHERE issues.id = issues.moved_to_id)') + .where('moved_to_id IS NOT NULL') + end + end + + def up + Issue.with_orphaned_moved_to_issues.each_batch(of: 100) do |batch| + batch.update_all(moved_to_id: nil) + end + + add_concurrent_foreign_key( + :issues, + :issues, + column: :moved_to_id, + on_delete: :nullify + ) + + # We're using a partial index here so we only index the data we actually + # care about. + add_concurrent_index(:issues, :moved_to_id, where: 'moved_to_id IS NOT NULL') + end + + def down + remove_foreign_key_without_error(:issues, column: :moved_to_id) + remove_concurrent_index(:issues, :moved_to_id) + end +end diff --git a/db/migrate/20171106154015_remove_issues_branch_name.rb b/db/migrate/20171106154015_remove_issues_branch_name.rb new file mode 100644 index 00000000000..3d08225c96d --- /dev/null +++ b/db/migrate/20171106154015_remove_issues_branch_name.rb @@ -0,0 +1,13 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RemoveIssuesBranchName < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def change + remove_column :issues, :branch_name, :string + end +end diff --git a/db/migrate/20171106155656_turn_issues_due_date_index_to_partial_index.rb b/db/migrate/20171106155656_turn_issues_due_date_index_to_partial_index.rb new file mode 100644 index 00000000000..e4bed778695 --- /dev/null +++ b/db/migrate/20171106155656_turn_issues_due_date_index_to_partial_index.rb @@ -0,0 +1,37 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class TurnIssuesDueDateIndexToPartialIndex < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + NEW_INDEX_NAME = 'idx_issues_on_project_id_and_due_date_and_id_and_state_partial' + OLD_INDEX_NAME = 'index_issues_on_project_id_and_due_date_and_id_and_state' + + disable_ddl_transaction! + + def up + add_concurrent_index( + :issues, + [:project_id, :due_date, :id, :state], + where: 'due_date IS NOT NULL', + name: NEW_INDEX_NAME + ) + + # We set the column name to nil as otherwise Rails will ignore the custom + # index name and remove the wrong index. + remove_concurrent_index(:issues, nil, name: OLD_INDEX_NAME) + end + + def down + add_concurrent_index( + :issues, + [:project_id, :due_date, :id, :state], + name: OLD_INDEX_NAME + ) + + remove_concurrent_index(:issues, nil, name: NEW_INDEX_NAME) + end +end diff --git a/db/migrate/20171106171453_add_timezone_to_issues_closed_at.rb b/db/migrate/20171106171453_add_timezone_to_issues_closed_at.rb new file mode 100644 index 00000000000..ad540b1e509 --- /dev/null +++ b/db/migrate/20171106171453_add_timezone_to_issues_closed_at.rb @@ -0,0 +1,19 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddTimezoneToIssuesClosedAt < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + change_column_type_concurrently(:issues, :closed_at, :datetime_with_timezone) + end + + def down + cleanup_concurrent_column_type_change(:issues, :closed_at) + end +end diff --git a/db/post_migrate/20171106180641_cleanup_add_timezone_to_issues_closed_at.rb b/db/post_migrate/20171106180641_cleanup_add_timezone_to_issues_closed_at.rb new file mode 100644 index 00000000000..88dd8f89ba6 --- /dev/null +++ b/db/post_migrate/20171106180641_cleanup_add_timezone_to_issues_closed_at.rb @@ -0,0 +1,19 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CleanupAddTimezoneToIssuesClosedAt < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + cleanup_concurrent_column_type_change(:issues, :closed_at) + end + + # rubocop:disable Migration/Datetime + def down + change_column_type_concurrently(:issues, :closed_at, :datetime) + end +end diff --git a/db/schema.rb b/db/schema.rb index c60cb729b75..37e08d453c8 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: 20171106101200) do +ActiveRecord::Schema.define(version: 20171106180641) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -817,13 +817,12 @@ ActiveRecord::Schema.define(version: 20171106101200) do t.integer "project_id" t.datetime "created_at" t.datetime "updated_at" - t.string "branch_name" t.text "description" t.integer "milestone_id" t.string "state" t.integer "iid" t.integer "updated_by_id" - t.boolean "confidential", default: false + t.boolean "confidential", default: false, null: false t.datetime "deleted_at" t.date "due_date" t.integer "moved_to_id" @@ -832,11 +831,11 @@ ActiveRecord::Schema.define(version: 20171106101200) do t.text "description_html" t.integer "time_estimate" t.integer "relative_position" - t.datetime "closed_at" t.integer "cached_markdown_version" t.datetime "last_edited_at" t.integer "last_edited_by_id" t.boolean "discussion_locked" + t.datetime_with_timezone "closed_at" end add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree @@ -845,13 +844,15 @@ ActiveRecord::Schema.define(version: 20171106101200) do add_index "issues", ["deleted_at"], name: "index_issues_on_deleted_at", using: :btree add_index "issues", ["description"], name: "index_issues_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"} add_index "issues", ["milestone_id"], name: "index_issues_on_milestone_id", using: :btree + add_index "issues", ["moved_to_id"], name: "index_issues_on_moved_to_id", where: "(moved_to_id IS NOT NULL)", using: :btree add_index "issues", ["project_id", "created_at", "id", "state"], name: "index_issues_on_project_id_and_created_at_and_id_and_state", using: :btree - add_index "issues", ["project_id", "due_date", "id", "state"], name: "index_issues_on_project_id_and_due_date_and_id_and_state", using: :btree + add_index "issues", ["project_id", "due_date", "id", "state"], name: "idx_issues_on_project_id_and_due_date_and_id_and_state_partial", where: "(due_date IS NOT NULL)", using: :btree add_index "issues", ["project_id", "iid"], name: "index_issues_on_project_id_and_iid", unique: true, using: :btree add_index "issues", ["project_id", "updated_at", "id", "state"], name: "index_issues_on_project_id_and_updated_at_and_id_and_state", using: :btree add_index "issues", ["relative_position"], name: "index_issues_on_relative_position", using: :btree add_index "issues", ["state"], name: "index_issues_on_state", using: :btree add_index "issues", ["title"], name: "index_issues_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"} + add_index "issues", ["updated_by_id"], name: "index_issues_on_updated_by_id", where: "(updated_by_id IS NOT NULL)", using: :btree create_table "keys", force: :cascade do |t| t.integer "user_id" @@ -1937,8 +1938,11 @@ ActiveRecord::Schema.define(version: 20171106101200) do add_foreign_key "issue_assignees", "issues", name: "fk_b7d881734a", on_delete: :cascade add_foreign_key "issue_assignees", "users", name: "fk_5e0c8d9154", on_delete: :cascade add_foreign_key "issue_metrics", "issues", on_delete: :cascade + add_foreign_key "issues", "issues", column: "moved_to_id", name: "fk_a194299be1", on_delete: :nullify + add_foreign_key "issues", "milestones", name: "fk_96b1dd429c", on_delete: :nullify add_foreign_key "issues", "projects", name: "fk_899c8f3231", on_delete: :cascade add_foreign_key "issues", "users", column: "author_id", name: "fk_05f1e72feb", on_delete: :nullify + add_foreign_key "issues", "users", column: "updated_by_id", name: "fk_ffed080f01", on_delete: :nullify add_foreign_key "label_priorities", "labels", on_delete: :cascade add_foreign_key "label_priorities", "projects", on_delete: :cascade add_foreign_key "labels", "namespaces", column: "group_id", on_delete: :cascade diff --git a/lib/gitlab/hook_data/issue_builder.rb b/lib/gitlab/hook_data/issue_builder.rb index de9cab80a02..196f2b6b34c 100644 --- a/lib/gitlab/hook_data/issue_builder.rb +++ b/lib/gitlab/hook_data/issue_builder.rb @@ -4,7 +4,6 @@ module Gitlab SAFE_HOOK_ATTRIBUTES = %i[ assignee_id author_id - branch_name closed_at confidential created_at diff --git a/spec/features/issues/issue_detail_spec.rb b/spec/features/issues/issue_detail_spec.rb index 6fbee0ebcb5..4224a8fe5d4 100644 --- a/spec/features/issues/issue_detail_spec.rb +++ b/spec/features/issues/issue_detail_spec.rb @@ -1,9 +1,9 @@ require 'rails_helper' feature 'Issue Detail', :js do - let(:user) { create(:user) } - let(:project) { create(:project, :public) } - let(:issue) { create(:issue, project: project, author: user) } + let(:user) { create(:user) } + let(:project) { create(:project, :public) } + let(:issue) { create(:issue, project: project, author: user) } context 'when user displays the issue' do before do @@ -27,6 +27,7 @@ feature 'Issue Detail', :js do click_link 'Edit' fill_in 'issuable-title', with: 'issue title' click_button 'Save' + wait_for_requests Users::DestroyService.new(user).execute(user) diff --git a/spec/lib/gitlab/hook_data/issue_builder_spec.rb b/spec/lib/gitlab/hook_data/issue_builder_spec.rb index 6c529cdd051..aeacd577d18 100644 --- a/spec/lib/gitlab/hook_data/issue_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/issue_builder_spec.rb @@ -11,7 +11,6 @@ describe Gitlab::HookData::IssueBuilder do %w[ assignee_id author_id - branch_name closed_at confidential created_at diff --git a/spec/services/milestones/destroy_service_spec.rb b/spec/services/milestones/destroy_service_spec.rb index 5739386dd0d..16e288b3148 100644 --- a/spec/services/milestones/destroy_service_spec.rb +++ b/spec/services/milestones/destroy_service_spec.rb @@ -4,7 +4,7 @@ describe Milestones::DestroyService do let(:user) { create(:user) } let(:project) { create(:project) } let(:milestone) { create(:milestone, title: 'Milestone v1.0', project: project) } - let(:issue) { create(:issue, project: project, milestone: milestone) } + let!(:issue) { create(:issue, project: project, milestone: milestone) } let(:merge_request) { create(:merge_request, source_project: project, milestone: milestone) } before do