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

@@ -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

changelogs/unreleased/cleanup-issues-schema.yml

@@ -0,0 +1,5 @@
+---
+title: Clean up schema of the "issues" table
+merge_request:
+author:
+type: other

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

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

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

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

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

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

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

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

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

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

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)
 

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

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