FEATURE: protect against accidental column or table drops

Often we need to amend our schema, it is tempting to use
drop_table, rename_column and drop_column to amned schema
trouble though is that existing code that is running in production
can depend on the existance of previous schema leading to application
breaking until new code base is deployed.

The commit enforces new rules to ensure we can never drop tables or
columns in migrations and instead use Migration::ColumnDropper and
Migration::TableDropper to defer drop the db objects
This commit is contained in:
Sam 2018-03-20 18:20:50 +11:00
parent 9f216ac182
commit 6a3c8fe69c
21 changed files with 462 additions and 166 deletions

View File

@ -1,4 +1,4 @@
require 'column_dropper'
require 'migration/column_dropper'
# fix any bust caches post initial migration
ActiveRecord::Base.send(:subclasses).each { |m| m.reset_column_information }
@ -26,7 +26,7 @@ if uncat_id == -1 || !Category.exists?(uncat_id)
VALUES ('uncategorized_category_id', 3, #{category_id}, now(), now())"
end
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'categories',
after_migration: 'AddSuppressFromLatestToCategories',
columns: ['logo_url', 'background_url', 'suppress_from_homepage'],

View File

@ -5,7 +5,7 @@ end
Group.where(name: 'everyone').update_all(visibility_level: Group.visibility_levels[:owners])
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'groups',
after_migration: 'SplitAliasLevels',
columns: %w[visible public alias_level],

View File

@ -33,7 +33,7 @@ UserOption.where(user_id: -1).update_all(
Group.user_trust_level_change!(-1, TrustLevel[4])
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'users',
after_migration: 'DropEmailFromUsers',
columns: %w[
@ -61,7 +61,7 @@ ColumnDropper.drop(
}
)
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'users',
after_migration: 'RenameBlockedSilence',
columns: %w[
@ -72,7 +72,7 @@ ColumnDropper.drop(
}
)
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'users',
after_migration: 'AddSilencedTillToUsers',
columns: %w[
@ -83,7 +83,7 @@ ColumnDropper.drop(
}
)
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'users',
after_migration: 'AddTrustLevelLocksToUsers',
columns: %w[

View File

@ -18,7 +18,7 @@ if !Theme.exists?
default_theme.set_default!
end
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'theme_fields',
after_migration: 'AddUploadIdToThemeFields',
columns: ['target'],

View File

@ -1,8 +1,8 @@
# Delayed migration steps
require 'table_migration_helper'
require 'migration/table_dropper'
TableMigrationHelper.delayed_drop(
Migration::TableDropper.delayed_drop(
old_name: 'topic_status_updates',
new_name: 'topic_timers',
after_migration: 'RenameTopicStatusUpdatesToTopicTimers',

View File

@ -64,7 +64,7 @@ end
# run this later, cause we need to make sure new application controller resilience is in place first
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'user_stats',
after_migration: 'DropUnreadTrackingColumns',
columns: %w{
@ -76,7 +76,7 @@ ColumnDropper.drop(
}
)
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'topics',
after_migration: 'DropUnreadTrackingColumns',
columns: %w{
@ -92,7 +92,7 @@ ColumnDropper.drop(
}
)
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'topics',
after_migration: 'RemoveAutoCloseColumnsFromTopics',
columns: %w{

View File

@ -1,4 +1,4 @@
require 'table_migration_helper'
require 'migration/table_dropper'
class CreateTopicStatusUpdatesAgain < ActiveRecord::Migration[4.2]
def up
@ -14,7 +14,7 @@ class CreateTopicStatusUpdatesAgain < ActiveRecord::Migration[4.2]
t.integer :category_id
end
TableMigrationHelper.read_only_table('topic_status_updates')
Migration::TableDropper.read_only_table('topic_status_updates')
end
def down

View File

@ -1,4 +1,4 @@
require_dependency 'column_dropper'
require 'migration/column_dropper'
class CreateUserEmails < ActiveRecord::Migration[4.2]
def up
@ -33,7 +33,7 @@ class CreateUserEmails < ActiveRecord::Migration[4.2]
SQL
change_column_null :users, :email, true
ColumnDropper.mark_readonly(:users, :email)
Migration::ColumnDropper.mark_readonly(:users, :email)
end
def down

View File

@ -1,75 +0,0 @@
class ColumnDropper
def self.drop(table:, after_migration:, columns:, delay: nil, on_drop: nil)
raise ArgumentError.new("Invalid table name passed to drop #{table}") if table =~ /[^a-z0-9_]/i
columns.each do |column|
raise ArgumentError.new("Invalid column name passed to drop #{column}") if column =~ /[^a-z0-9_]/i
end
# in production we need some extra delay to allow for slow migrations
delay ||= Rails.env.production? ? 3600 : 0
sql = <<~SQL
SELECT 1
FROM INFORMATION_SCHEMA.COLUMNS
WHERE table_schema = 'public' AND
table_name = :table AND
column_name IN (:columns) AND
EXISTS (
SELECT 1
FROM schema_migration_details
WHERE name = :after_migration AND
created_at <= (current_timestamp at time zone 'UTC' - interval :delay)
)
LIMIT 1
SQL
if ActiveRecord::Base.exec_sql(sql, table: table,
columns: columns,
delay: "#{delay.to_i || 0} seconds",
after_migration: after_migration).to_a.length > 0
on_drop&.call
columns.each do |column|
ActiveRecord::Base.exec_sql <<~SQL
DROP TRIGGER IF EXISTS #{readonly_trigger_name(table, column)} ON #{table};
DROP FUNCTION IF EXISTS #{readonly_function_name(table, column)} CASCADE;
SQL
# safe cause it is protected on method entry, can not be passed in params
ActiveRecord::Base.exec_sql("ALTER TABLE #{table} DROP COLUMN IF EXISTS #{column}")
end
Discourse.reset_active_record_cache
end
end
def self.mark_readonly(table_name, column_name)
ActiveRecord::Base.exec_sql <<-SQL
CREATE OR REPLACE FUNCTION #{readonly_function_name(table_name, column_name)} RETURNS trigger AS $rcr$
BEGIN
RAISE EXCEPTION 'Discourse: #{column_name} in #{table_name} is readonly';
END
$rcr$ LANGUAGE plpgsql;
SQL
ActiveRecord::Base.exec_sql <<-SQL
CREATE TRIGGER #{readonly_trigger_name(table_name, column_name)}
BEFORE INSERT OR UPDATE OF #{column_name}
ON #{table_name}
FOR EACH ROW
WHEN (NEW.#{column_name} IS NOT NULL)
EXECUTE PROCEDURE #{readonly_function_name(table_name, column_name)};
SQL
end
private
def self.readonly_function_name(table_name, column_name)
"raise_#{table_name}_#{column_name}_readonly()"
end
def self.readonly_trigger_name(table_name, column_name)
"#{table_name}_#{column_name}_readonly"
end
end

View File

@ -0,0 +1,3 @@
require_dependency 'migration/safe_migrate'
Migration::SafeMigrate.patch_active_record!

View File

@ -0,0 +1,72 @@
module Migration
class BaseDropper
def initialize(after_migration, delay, on_drop)
@after_migration = after_migration
@on_drop = on_drop
# in production we need some extra delay to allow for slow migrations
@delay = delay || (Rails.env.production? ? 3600 : 0)
end
def delayed_drop
if droppable?
@on_drop&.call
execute_drop!
Discourse.reset_active_record_cache
end
end
private
def droppable?
raise NotImplementedError
end
def execute_drop!
raise NotImplementedError
end
def previous_migration_done
<<~SQL
EXISTS(
SELECT 1
FROM schema_migration_details
WHERE name = :after_migration AND
created_at <= (current_timestamp AT TIME ZONE 'UTC' - INTERVAL :delay)
)
SQL
end
def self.create_readonly_function(table_name, column_name = nil)
message = column_name ?
"Discourse: #{column_name} in #{table_name} is readonly" :
"Discourse: #{table_name} is read only"
ActiveRecord::Base.exec_sql <<~SQL
CREATE OR REPLACE FUNCTION #{readonly_function_name(table_name, column_name)} RETURNS trigger AS $rcr$
BEGIN
RAISE EXCEPTION '#{message}';
END
$rcr$ LANGUAGE plpgsql;
SQL
end
private_class_method :create_readonly_function
def self.validate_table_name(table_name)
raise ArgumentError.new("Invalid table name passed: #{table_name}") if table_name =~ /[^a-z0-9_]/i
end
def self.validate_column_name(column_name)
raise ArgumentError.new("Invalid column name passed to drop #{column_name}") if column_name =~ /[^a-z0-9_]/i
end
def self.readonly_function_name(table_name, column_name = nil)
["raise", table_name, column_name, "readonly()"].compact.join("_")
end
def self.readonly_trigger_name(table_name, column_name = nil)
[table_name, column_name, "readonly"].compact.join("_")
end
end
end

View File

@ -0,0 +1,64 @@
require_dependency 'migration/base_dropper'
module Migration
class ColumnDropper < BaseDropper
def self.drop(table:, after_migration:, columns:, delay: nil, on_drop: nil)
validate_table_name(table)
columns.each { |column| validate_column_name(column) }
ColumnDropper.new(table, columns, after_migration, delay, on_drop).delayed_drop
end
def self.mark_readonly(table_name, column_name)
create_readonly_function(table_name, column_name)
ActiveRecord::Base.exec_sql <<~SQL
CREATE TRIGGER #{readonly_trigger_name(table_name, column_name)}
BEFORE INSERT OR UPDATE OF #{column_name}
ON #{table_name}
FOR EACH ROW
WHEN (NEW.#{column_name} IS NOT NULL)
EXECUTE PROCEDURE #{readonly_function_name(table_name, column_name)};
SQL
end
private
def initialize(table, columns, after_migration, delay, on_drop)
super(after_migration, delay, on_drop)
@table = table
@columns = columns
end
def droppable?
builder = SqlBuilder.new(<<~SQL)
SELECT 1
FROM INFORMATION_SCHEMA.COLUMNS
/*where*/
LIMIT 1
SQL
builder.where("table_schema = 'public'")
.where("table_name = :table")
.where("column_name IN (:columns)")
.where(previous_migration_done)
.exec(table: @table,
columns: @columns,
delay: "#{@delay} seconds",
after_migration: @after_migration).to_a.length > 0
end
def execute_drop!
@columns.each do |column|
ActiveRecord::Base.exec_sql <<~SQL
DROP TRIGGER IF EXISTS #{BaseDropper.readonly_trigger_name(@table, column)} ON #{@table};
DROP FUNCTION IF EXISTS #{BaseDropper.readonly_function_name(@table, column)} CASCADE;
SQL
# safe cause it is protected on method entry, can not be passed in params
ActiveRecord::Base.exec_sql("ALTER TABLE #{@table} DROP COLUMN IF EXISTS #{column}")
end
end
end
end

View File

@ -0,0 +1,118 @@
module Migration; end
class Discourse::InvalidMigration < StandardError; end
class Migration::SafeMigrate
module SafeMigration
UNSAFE_VERSION = 20180321015220
@@enable_safe = true
def self.enable_safe!
@@enable_safe = true
end
def self.disable_safe!
@@enable_safe = false
end
def migrate(direction)
if direction == :up && version && version > UNSAFE_VERSION && @@enable_safe != false
Migration::SafeMigrate.enable!
end
super
ensure
Migration::SafeMigrate.disable!
end
end
module NiceErrors
def migrate
super
rescue => e
if e.cause.is_a?(Discourse::InvalidMigration)
def e.cause; nil; end
def e.backtrace
super.reject do |frame|
frame =~ /safe_migrate\.rb/ || frame =~ /schema_migration_details\.rb/
end
end
raise e
else
raise e
end
end
end
def self.enable!
return if PG::Connection.method_defined?(:exec_migrator_unpatched)
PG::Connection.class_eval do
alias_method :exec_migrator_unpatched, :exec
alias_method :async_exec_migrator_unpatched, :async_exec
def exec(*args, &blk)
Migration::SafeMigrate.protect!(args[0])
exec_migrator_unpatched(*args, &blk)
end
def async_exec(*args, &blk)
Migration::SafeMigrate.protect!(args[0])
async_exec_migrator_unpatched(*args, &blk)
end
end
end
def self.disable!
return if !PG::Connection.method_defined?(:exec_migrator_unpatched)
PG::Connection.class_eval do
alias_method :exec, :exec_migrator_unpatched
alias_method :async_exec, :async_exec_migrator_unpatched
remove_method :exec_migrator_unpatched
remove_method :async_exec_migrator_unpatched
end
end
def self.patch_active_record!
ActiveSupport.on_load(:active_record) do
ActiveRecord::Migration.prepend(SafeMigration)
end
if defined?(ActiveRecord::Tasks::DatabaseTasks)
ActiveRecord::Tasks::DatabaseTasks.singleton_class.prepend(NiceErrors)
end
end
def self.protect!(sql)
if sql =~ /^\s*drop\s+table/i
$stdout.puts("", <<~STR)
WARNING
-------------------------------------------------------------------------------------
An attempt was made to drop a table in a migration
SQL used was: '#{sql}'
Please use the deferred pattrn using Migration::TableDropper in db/seeds to drop
the table.
This protection is in place to protect us against dropping tables that are currently
in use by live applications.
STR
raise Discourse::InvalidMigration, "Attempt was made to drop a table"
elsif sql =~ /^\s*alter\s+table.*(rename|drop)/i
$stdout.puts("", <<~STR)
WARNING
-------------------------------------------------------------------------------------
An attempt was made to drop or rename a column in a migration
SQL used was: '#{sql}'
Please use the deferred pattrn using Migration::ColumnDropper in db/seeds to drop
or rename columns.
Note, to minimize disruption use self.ignored_columns = ["column name"] on your
ActiveRecord model, this can be removed 6 months or so later.
This protection is in place to protect us against dropping columns that are currently
in use by live applications.
STR
raise Discourse::InvalidMigration, "Attempt was made to rename or delete column"
end
end
end

View File

@ -0,0 +1,69 @@
require_dependency 'migration/base_dropper'
module Migration
class Migration::TableDropper < BaseDropper
def self.delayed_drop(old_name:, new_name:, after_migration:, delay: nil, on_drop: nil)
validate_table_name(old_name)
validate_table_name(new_name)
TableDropper.new(old_name, new_name, after_migration, delay, on_drop).delayed_drop
end
def self.read_only_table(table_name)
create_readonly_function(table_name)
ActiveRecord::Base.exec_sql <<~SQL
CREATE TRIGGER #{readonly_trigger_name(table_name)}
BEFORE INSERT OR UPDATE OR DELETE OR TRUNCATE
ON #{table_name}
FOR EACH STATEMENT
EXECUTE PROCEDURE #{readonly_function_name(table_name)};
SQL
end
private
def initialize(old_name, new_name, after_migration, delay, on_drop)
super(after_migration, delay, on_drop)
@old_name = old_name
@new_name = new_name
end
def droppable?
builder = SqlBuilder.new(<<~SQL)
SELECT 1
FROM INFORMATION_SCHEMA.TABLES
/*where*/
LIMIT 1
SQL
builder.where("table_schema = 'public'")
.where(previous_migration_done)
.where(new_table_exists)
.exec(old_name: @old_name,
new_name: @new_name,
delay: "#{@delay} seconds",
after_migration: @after_migration).to_a.length > 0
end
def new_table_exists
<<~SQL
EXISTS(
SELECT 1
FROM INFORMATION_SCHEMA.TABLES
WHERE table_schema = 'public' AND
table_name = :new_name
)
SQL
end
def execute_drop!
ActiveRecord::Base.exec_sql("DROP TABLE IF EXISTS #{@old_name}")
ActiveRecord::Base.exec_sql <<~SQL
DROP FUNCTION IF EXISTS #{BaseDropper.readonly_function_name(@old_name)} CASCADE;
SQL
end
end
end

View File

@ -1,66 +0,0 @@
class TableMigrationHelper
def self.read_only_table(table_name)
ActiveRecord::Base.exec_sql <<-SQL
CREATE OR REPLACE FUNCTION #{readonly_function_name(table_name)} RETURNS trigger AS $rro$
BEGIN
RAISE EXCEPTION 'Discourse: Table is read only';
RETURN null;
END
$rro$ LANGUAGE plpgsql;
SQL
ActiveRecord::Base.exec_sql <<-SQL
CREATE TRIGGER #{readonly_trigger_name(table_name)}
BEFORE INSERT OR UPDATE OR DELETE OR TRUNCATE
ON #{table_name}
FOR EACH STATEMENT
EXECUTE PROCEDURE #{readonly_function_name(table_name)};
SQL
end
def self.delayed_drop(old_name:, new_name:, after_migration:, delay: nil, on_drop: nil)
delay ||= Rails.env.production? ? 3600 : 0
sql = <<~SQL
SELECT 1
FROM INFORMATION_SCHEMA.TABLES
WHERE table_schema = 'public' AND
EXISTS (
SELECT 1
FROM schema_migration_details
WHERE name = :after_migration AND
created_at <= (current_timestamp at time zone 'UTC' - interval :delay)
)
AND EXISTS (
SELECT 1
FROM INFORMATION_SCHEMA.TABLES
WHERE table_schema = 'public' AND
table_name = :new_name
)
LIMIT 1
SQL
if ActiveRecord::Base.exec_sql(sql, old_name: old_name,
new_name: new_name,
delay: "#{delay.to_i || 0} seconds",
after_migration: after_migration).to_a.length > 0
on_drop&.call
ActiveRecord::Base.exec_sql("DROP TABLE IF EXISTS #{old_name}")
ActiveRecord::Base.exec_sql <<~SQL
DROP FUNCTION IF EXISTS #{readonly_function_name(old_name)} CASCADE;
SQL
end
end
private
def self.readonly_function_name(table_name)
"public.raise_#{table_name}_read_only()"
end
def self.readonly_trigger_name(table_name)
"#{table_name}_read_only"
end
end

View File

@ -1,7 +1,7 @@
require 'rails_helper'
require 'column_dropper'
require_dependency 'migration/column_dropper'
RSpec.describe ColumnDropper do
RSpec.describe Migration::ColumnDropper do
def has_column?(table, column)
Topic.exec_sql("SELECT 1 FROM INFORMATION_SCHEMA.COLUMNS
@ -25,7 +25,7 @@ RSpec.describe ColumnDropper do
dropped_proc_called = false
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'topics',
after_migration: name,
columns: ['junk'],
@ -36,7 +36,7 @@ RSpec.describe ColumnDropper do
expect(has_column?('topics', 'junk')).to eq(true)
expect(dropped_proc_called).to eq(false)
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: 'topics',
after_migration: name,
columns: ['junk'],
@ -60,7 +60,7 @@ RSpec.describe ColumnDropper do
VALUES (1, 'something@email.com');
SQL
ColumnDropper.mark_readonly(table_name, 'email')
Migration::ColumnDropper.mark_readonly(table_name, 'email')
end
after do
@ -78,7 +78,7 @@ RSpec.describe ColumnDropper do
.getvalue(0, 0)
dropped_proc_called = false
ColumnDropper.drop(
Migration::ColumnDropper.drop(
table: table_name,
after_migration: name,
columns: ['email'],

View File

@ -0,0 +1,84 @@
require 'rails_helper'
require_dependency 'migration/safe_migrate'
describe Migration::SafeMigrate do
before do
Migration::SafeMigrate::SafeMigration.disable_safe!
end
after do
Migration::SafeMigrate.disable!
Migration::SafeMigrate::SafeMigration.enable_safe!
end
def capture_stdout
old_stdout = $stdout
io = StringIO.new
$stdout = io
yield
io.string
ensure
$stdout = old_stdout
end
it "bans all table removal" do
Migration::SafeMigrate.enable!
path = File.expand_path "#{Rails.root}/spec/fixtures/migrate/drop_table"
output = capture_stdout do
expect(lambda do
ActiveRecord::Migrator.up([path])
end).to raise_error(StandardError)
end
expect(output).to include("TableDropper")
expect(User.first).not_to eq(nil)
end
it "bans all column removal" do
Migration::SafeMigrate.enable!
path = File.expand_path "#{Rails.root}/spec/fixtures/migrate/remove_column"
output = capture_stdout do
expect(lambda do
ActiveRecord::Migrator.up([path])
end).to raise_error(StandardError)
end
expect(output).to include("ColumnDropper")
expect(User.first).not_to eq(nil)
end
it "bans all column renames" do
Migration::SafeMigrate.enable!
path = File.expand_path "#{Rails.root}/spec/fixtures/migrate/rename_column"
output = capture_stdout do
expect(lambda do
ActiveRecord::Migrator.up([path])
end).to raise_error(StandardError)
end
expect(output).to include("ColumnDropper")
expect(User.first).not_to eq(nil)
end
it "supports being disabled" do
Migration::SafeMigrate.enable!
Migration::SafeMigrate.disable!
path = File.expand_path "#{Rails.root}/spec/fixtures/migrate/drop_table"
output = capture_stdout do
ActiveRecord::Migrator.up([path])
end
expect(output).to include("drop_table(:users)")
end
end

View File

@ -1,7 +1,7 @@
require 'rails_helper'
require 'table_migration_helper'
require_dependency 'migration/table_dropper'
describe TableMigrationHelper do
describe Migration::TableDropper do
def table_exists?(table_name)
sql = <<-SQL

View File

@ -0,0 +1,9 @@
class DropTable < ActiveRecord::Migration[5.1]
def up
drop_table :users
end
def down
raise "not tested"
end
end

View File

@ -0,0 +1,9 @@
class RemoveColumn < ActiveRecord::Migration[5.1]
def up
remove_column :users, :username
end
def down
raise "not tested"
end
end

View File

@ -0,0 +1,9 @@
class RenameColumn < ActiveRecord::Migration[5.1]
def up
rename_column :users, :username, :username1
end
def down
raise "not tested"
end
end