Commit cf9e2967 authored by Yorick Peterse's avatar Yorick Peterse

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
parents df176fe7 24a11c95
...@@ -60,7 +60,7 @@ class Note < ActiveRecord::Base ...@@ -60,7 +60,7 @@ class Note < ActiveRecord::Base
belongs_to :updated_by, class_name: "User" belongs_to :updated_by, class_name: "User"
belongs_to :last_edited_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_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_one :system_note_metadata has_one :system_note_metadata
......
...@@ -28,6 +28,7 @@ class Todo < ActiveRecord::Base ...@@ -28,6 +28,7 @@ class Todo < ActiveRecord::Base
delegate :name, :email, to: :author, prefix: true, allow_nil: true delegate :name, :email, to: :author, prefix: true, allow_nil: true
validates :action, :project, :target_type, :user, presence: true validates :action, :project, :target_type, :user, presence: true
validates :author, presence: true
validates :target_id, presence: true, unless: :for_commit? validates :target_id, presence: true, unless: :for_commit?
validates :commit_id, presence: true, if: :for_commit? validates :commit_id, presence: true, if: :for_commit?
......
...@@ -125,7 +125,7 @@ class User < ActiveRecord::Base ...@@ -125,7 +125,7 @@ class User < ActiveRecord::Base
has_many :spam_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent 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 :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 :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 :notification_settings, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_many :award_emoji, 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 has_many :triggers, dependent: :destroy, class_name: 'Ci::Trigger', foreign_key: :owner_id # rubocop:disable Cop/ActiveRecordDependent
......
---
title: Add foreign key and NOT NULL constraints to todos table.
merge_request: 16849
author:
type: other
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
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
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # 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 # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
...@@ -1707,7 +1707,7 @@ ActiveRecord::Schema.define(version: 20180202111106) do ...@@ -1707,7 +1707,7 @@ ActiveRecord::Schema.define(version: 20180202111106) do
t.integer "project_id", null: false t.integer "project_id", null: false
t.integer "target_id" t.integer "target_id"
t.string "target_type", null: false t.string "target_type", null: false
t.integer "author_id" t.integer "author_id", null: false
t.integer "action", null: false t.integer "action", null: false
t.string "state", null: false t.string "state", null: false
t.datetime "created_at" t.datetime "created_at"
...@@ -2047,7 +2047,10 @@ ActiveRecord::Schema.define(version: 20180202111106) do ...@@ -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 "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", "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 "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", "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 "trending_projects", "projects", on_delete: :cascade
add_foreign_key "u2f_registrations", "users" add_foreign_key "u2f_registrations", "users"
add_foreign_key "user_callouts", "users", on_delete: :cascade add_foreign_key "user_callouts", "users", on_delete: :cascade
......
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
...@@ -8,7 +8,7 @@ describe Note do ...@@ -8,7 +8,7 @@ describe Note do
it { is_expected.to belong_to(:noteable).touch(false) } it { is_expected.to belong_to(:noteable).touch(false) }
it { is_expected.to belong_to(:author).class_name('User') } 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 end
describe 'modules' do describe 'modules' do
......
...@@ -20,6 +20,7 @@ describe Todo do ...@@ -20,6 +20,7 @@ describe Todo do
it { is_expected.to validate_presence_of(:action) } it { is_expected.to validate_presence_of(:action) }
it { is_expected.to validate_presence_of(:target_type) } it { is_expected.to validate_presence_of(:target_type) }
it { is_expected.to validate_presence_of(:user) } it { is_expected.to validate_presence_of(:user) }
it { is_expected.to validate_presence_of(:author) }
context 'for commits' do context 'for commits' do
subject { described_class.new(target_type: 'Commit') } subject { described_class.new(target_type: 'Commit') }
......
...@@ -33,7 +33,7 @@ describe User do ...@@ -33,7 +33,7 @@ describe User do
it { is_expected.to have_many(:merge_requests).dependent(:destroy) } 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(:identities).dependent(:destroy) }
it { is_expected.to have_many(:spam_logs).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(:award_emoji).dependent(:destroy) }
it { is_expected.to have_many(:triggers).dependent(:destroy) } it { is_expected.to have_many(:triggers).dependent(:destroy) }
it { is_expected.to have_many(:builds).dependent(:nullify) } it { is_expected.to have_many(:builds).dependent(:nullify) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment