Merge branch '32282-add-foreign-keys-to-todos' into 'master'

Add missing foreign key and NOT NULL constraints to todos table.

Closes #32282

See merge request gitlab-org/gitlab-ce!16849

11 files changed, 145 insertions, 6 deletions

diff --git a/app/models/note.rb b/app/models/note.rb
index 01a778a7424..cac60845a49 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -60,7 +60,7 @@ class Note < ActiveRecord::Base
   belongs_to :updated_by, class_name: "User"
   belongs_to :last_edited_by, class_name: 'User'
 
-  has_many :todos, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+  has_many :todos
   has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_one :system_note_metadata
 
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 7af54b2beb2..bb5965e20eb 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -28,6 +28,7 @@ class Todo < ActiveRecord::Base
   delegate :name, :email, to: :author, prefix: true, allow_nil: true
 
   validates :action, :project, :target_type, :user, presence: true
+  validates :author, presence: true
   validates :target_id, presence: true, unless: :for_commit?
   validates :commit_id, presence: true, if: :for_commit?
 
diff --git a/app/models/user.rb b/app/models/user.rb
index a57ba3740c9..4b44e9bb7eb 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -125,7 +125,7 @@ class User < ActiveRecord::Base
   has_many :spam_logs,                dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :builds,                   dependent: :nullify, class_name: 'Ci::Build' # rubocop:disable Cop/ActiveRecordDependent
   has_many :pipelines,                dependent: :nullify, class_name: 'Ci::Pipeline' # rubocop:disable Cop/ActiveRecordDependent
-  has_many :todos,                    dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+  has_many :todos
   has_many :notification_settings,    dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :award_emoji,              dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :triggers,                 dependent: :destroy, class_name: 'Ci::Trigger', foreign_key: :owner_id # rubocop:disable Cop/ActiveRecordDependent
diff --git a/changelogs/unreleased/32282-add-foreign-keys-to-todos.yml b/changelogs/unreleased/32282-add-foreign-keys-to-todos.yml
new file mode 100644
index 00000000000..e74c2a8b9ff
--- /dev/null
+++ b/changelogs/unreleased/32282-add-foreign-keys-to-todos.yml
@@ -0,0 +1,5 @@
+---
+title: Add foreign key and NOT NULL constraints to todos table.
+merge_request: 16849
+author:
+type: other
diff --git a/db/migrate/20180201110056_add_foreign_keys_to_todos.rb b/db/migrate/20180201110056_add_foreign_keys_to_todos.rb
new file mode 100644
index 00000000000..b7c40f8c01a
--- /dev/null
+++ b/db/migrate/20180201110056_add_foreign_keys_to_todos.rb
@@ -0,0 +1,38 @@
+class AddForeignKeysToTodos < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  class Todo < ActiveRecord::Base
+    self.table_name = 'todos'
+    include EachBatch
+  end
+
+  BATCH_SIZE = 1000
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    Todo.where('NOT EXISTS ( SELECT true FROM users WHERE id=todos.user_id )').each_batch(of: BATCH_SIZE) do |batch|
+      batch.delete_all
+    end
+
+    Todo.where('NOT EXISTS ( SELECT true FROM users WHERE id=todos.author_id )').each_batch(of: BATCH_SIZE) do |batch|
+      batch.delete_all
+    end
+
+    Todo.where('note_id IS NOT NULL AND NOT EXISTS ( SELECT true FROM notes WHERE id=todos.note_id )').each_batch(of: BATCH_SIZE) do |batch|
+      batch.delete_all
+    end
+
+    add_concurrent_foreign_key :todos, :users, column: :user_id, on_delete: :cascade
+    add_concurrent_foreign_key :todos, :users, column: :author_id, on_delete: :cascade
+    add_concurrent_foreign_key :todos, :notes, column: :note_id, on_delete: :cascade
+  end
+
+  def down
+    remove_foreign_key :todos, :users
+    remove_foreign_key :todos, column: :author_id
+    remove_foreign_key :todos, :notes
+  end
+end
diff --git a/db/post_migrate/20180204200836_change_author_id_to_not_null_in_todos.rb b/db/post_migrate/20180204200836_change_author_id_to_not_null_in_todos.rb
new file mode 100644
index 00000000000..92c32feebf7
--- /dev/null
+++ b/db/post_migrate/20180204200836_change_author_id_to_not_null_in_todos.rb
@@ -0,0 +1,26 @@
+class ChangeAuthorIdToNotNullInTodos < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  class Todo < ActiveRecord::Base
+    self.table_name = 'todos'
+    include EachBatch
+  end
+
+  BATCH_SIZE = 1000
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    Todo.where(author_id: nil).each_batch(of: BATCH_SIZE) do |batch|
+      batch.delete_all
+    end
+
+    change_column_null :todos, :author_id, false
+  end
+
+  def down
+    change_column_null :todos, :author_id, true
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 50a2ceaaeee..432eb095746 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: 20180202111106) do
+ActiveRecord::Schema.define(version: 20180204200836) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1707,7 +1707,7 @@ ActiveRecord::Schema.define(version: 20180202111106) do
     t.integer "project_id", null: false
     t.integer "target_id"
     t.string "target_type", null: false
-    t.integer "author_id"
+    t.integer "author_id", null: false
     t.integer "action", null: false
     t.string "state", null: false
     t.datetime "created_at"
@@ -2047,7 +2047,10 @@ ActiveRecord::Schema.define(version: 20180202111106) do
   add_foreign_key "system_note_metadata", "notes", name: "fk_d83a918cb1", on_delete: :cascade
   add_foreign_key "timelogs", "issues", name: "fk_timelogs_issues_issue_id", on_delete: :cascade
   add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade
+  add_foreign_key "todos", "notes", name: "fk_91d1f47b13", on_delete: :cascade
   add_foreign_key "todos", "projects", name: "fk_45054f9c45", on_delete: :cascade
+  add_foreign_key "todos", "users", column: "author_id", name: "fk_ccf0373936", on_delete: :cascade
+  add_foreign_key "todos", "users", name: "fk_d94154aa95", on_delete: :cascade
   add_foreign_key "trending_projects", "projects", on_delete: :cascade
   add_foreign_key "u2f_registrations", "users"
   add_foreign_key "user_callouts", "users", on_delete: :cascade
diff --git a/spec/migrations/add_foreign_keys_to_todos_spec.rb b/spec/migrations/add_foreign_keys_to_todos_spec.rb
new file mode 100644
index 00000000000..4a22bd6f342
--- /dev/null
+++ b/spec/migrations/add_foreign_keys_to_todos_spec.rb
@@ -0,0 +1,65 @@
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20180201110056_add_foreign_keys_to_todos.rb')
+
+describe AddForeignKeysToTodos, :migration do
+  let(:todos) { table(:todos) }
+
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+
+  context 'add foreign key on user_id' do
+    let!(:todo_with_user) { create_todo(user_id: user.id) }
+    let!(:todo_without_user) { create_todo(user_id: 4711) }
+
+    it 'removes orphaned todos without corresponding user' do
+      expect { migrate! }.to change { Todo.count }.from(2).to(1)
+    end
+
+    it 'does not remove entries with valid user_id' do
+      expect { migrate! }.not_to change { todo_with_user.reload }
+    end
+  end
+
+  context 'add foreign key on author_id' do
+    let!(:todo_with_author) { create_todo(author_id: user.id) }
+    let!(:todo_with_invalid_author) { create_todo(author_id: 4711) }
+
+    it 'removes orphaned todos by author_id' do
+      expect { migrate! }.to change { Todo.count }.from(2).to(1)
+    end
+
+    it 'does not touch author_id for valid entries' do
+      expect { migrate! }.not_to change { todo_with_author.reload }
+    end
+  end
+
+  context 'add foreign key on note_id' do
+    let(:note) { create(:note) }
+    let!(:todo_with_note) { create_todo(note_id: note.id) }
+    let!(:todo_with_invalid_note) { create_todo(note_id: 4711) }
+    let!(:todo_without_note) { create_todo(note_id: nil) }
+
+    it 'deletes todo if note_id is set but does not exist in notes table' do
+      expect { migrate! }.to change { Todo.count }.from(3).to(2)
+    end
+
+    it 'does not touch entry if note_id is nil' do
+      expect { migrate! }.not_to change { todo_without_note.reload }
+    end
+
+    it 'does not touch note_id for valid entries' do
+      expect { migrate! }.not_to change { todo_with_note.reload }
+    end
+  end
+
+  def create_todo(**opts)
+    todos.create!(
+      project_id: project.id,
+      user_id: user.id,
+      author_id: user.id,
+      target_type: '',
+      action: 0,
+      state: '', **opts
+    )
+  end
+end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 00dda7c9c60..c853f707e6d 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -8,7 +8,7 @@ describe Note do
     it { is_expected.to belong_to(:noteable).touch(false) }
     it { is_expected.to belong_to(:author).class_name('User') }
 
-    it { is_expected.to have_many(:todos).dependent(:destroy) }
+    it { is_expected.to have_many(:todos) }
   end
 
   describe 'modules' do
diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb
index 3e8f3848eca..bd498269798 100644
--- a/spec/models/todo_spec.rb
+++ b/spec/models/todo_spec.rb
@@ -20,6 +20,7 @@ describe Todo do
     it { is_expected.to validate_presence_of(:action) }
     it { is_expected.to validate_presence_of(:target_type) }
     it { is_expected.to validate_presence_of(:user) }
+    it { is_expected.to validate_presence_of(:author) }
 
     context 'for commits' do
       subject { described_class.new(target_type: 'Commit') }
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 18c91d4cffd..af79ea4c283 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -33,7 +33,7 @@ describe User do
     it { is_expected.to have_many(:merge_requests).dependent(:destroy) }
     it { is_expected.to have_many(:identities).dependent(:destroy) }
     it { is_expected.to have_many(:spam_logs).dependent(:destroy) }
-    it { is_expected.to have_many(:todos).dependent(:destroy) }
+    it { is_expected.to have_many(:todos) }
     it { is_expected.to have_many(:award_emoji).dependent(:destroy) }
     it { is_expected.to have_many(:triggers).dependent(:destroy) }
     it { is_expected.to have_many(:builds).dependent(:nullify) }