From 40fa96777ddad4698df20d6b8a61afcf87743ed7 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 8 Oct 2018 15:47:38 +0800 Subject: [PATCH] FEATURE: Post deployment migrations. (#6406) This moves us away from the delayed drops pattern which was problematic on two counts. First, it uses a hardcoded "delay for" duration which may be too short for certain deployment strategies. Second, delayed drop doesn't ensure that it only runs after the latest application code has been deployed. If the migration runs and the application code fails to deploy, running the migration after "delay for" has been met will cause the application to blow up. The new strategy allows post deployment migrations to be skipped if the env `SKIP_POST_DEPLOYMENT_MIGRATIONS` is provided. ``` SKIP_POST_DEPLOYMENT_MIGRATIONS=1 rake db:migrate -> deploy app servers SKIP_POST_DEPLOYMENT_MIGRATIONS=0 rake db:migrate ``` To aid with the generation of a post deployment migration, a generator has been added. Simply run `rails generate post_migration`. --- config/initializers/000-post_migration.rb | 5 + db/fixtures/000_delayed_drops.rb | 226 ------------------ ...70524182846_add_unread_tracking_columns.rb | 9 - ...0526125321_drop_unread_tracking_columns.rb | 8 - ...0170728054115_remove_public_from_groups.rb | 9 - .../20171107020512_drop_email_from_users.rb | 9 - .../20180227161818_drop_unused_tables.rb | 9 - ...20180504000531_remove_legacy_auth_token.rb | 5 - ...8_drop_vote_count_from_topics_and_posts.rb | 9 - ...180917024729_remove_superfluous_columns.rb | 82 +++++++ ...0180917034056_remove_superfluous_tables.rb | 17 ++ lib/discourse.rb | 1 + .../rails/post_migration_generator.rb | 13 + lib/migration/base_dropper.rb | 54 ----- lib/migration/column_dropper.rb | 56 +---- lib/migration/safe_migrate.rb | 27 ++- lib/migration/table_dropper.rb | 75 +----- .../migration/column_dropper_spec.rb | 124 ++-------- .../components/migration/safe_migrate_spec.rb | 34 ++- .../migration/table_dropper_spec.rb | 208 +++------------- .../drop_table/20990309014014_drop_table.rb | 0 .../20990309014014_remove_column.rb | 0 .../20990309014014_rename_column.rb | 0 .../20990309014014_rename_table.rb | 0 .../20990309014013_drop_users_table.rb | 10 + spec/rails_helper.rb | 8 + 26 files changed, 260 insertions(+), 738 deletions(-) create mode 100644 config/initializers/000-post_migration.rb delete mode 100644 db/fixtures/000_delayed_drops.rb delete mode 100644 db/migrate/20170524182846_add_unread_tracking_columns.rb delete mode 100644 db/migrate/20170526125321_drop_unread_tracking_columns.rb delete mode 100644 db/migrate/20170728054115_remove_public_from_groups.rb delete mode 100644 db/migrate/20171107020512_drop_email_from_users.rb delete mode 100644 db/migrate/20180227161818_drop_unused_tables.rb delete mode 100644 db/migrate/20180504000531_remove_legacy_auth_token.rb delete mode 100644 db/migrate/20180619091608_drop_vote_count_from_topics_and_posts.rb create mode 100644 db/post_migrate/20180917024729_remove_superfluous_columns.rb create mode 100644 db/post_migrate/20180917034056_remove_superfluous_tables.rb create mode 100644 lib/generators/rails/post_migration_generator.rb rename spec/fixtures/{ => db}/migrate/drop_table/20990309014014_drop_table.rb (100%) rename spec/fixtures/{ => db}/migrate/remove_column/20990309014014_remove_column.rb (100%) rename spec/fixtures/{ => db}/migrate/rename_column/20990309014014_rename_column.rb (100%) rename spec/fixtures/{ => db}/migrate/rename_table/20990309014014_rename_table.rb (100%) create mode 100644 spec/fixtures/db/post_migrate/drop_table/20990309014013_drop_users_table.rb diff --git a/config/initializers/000-post_migration.rb b/config/initializers/000-post_migration.rb new file mode 100644 index 00000000000..1afb441cc20 --- /dev/null +++ b/config/initializers/000-post_migration.rb @@ -0,0 +1,5 @@ +unless ['1', 'true'].include?(ENV["SKIP_POST_DEPLOYMENT_MIGRATIONS"]&.to_s) + Rails.application.config.paths['db/migrate'] << Rails.root.join( + Discourse::DB_POST_MIGRATE_PATH + ).to_s +end diff --git a/db/fixtures/000_delayed_drops.rb b/db/fixtures/000_delayed_drops.rb deleted file mode 100644 index a46bfed42ea..00000000000 --- a/db/fixtures/000_delayed_drops.rb +++ /dev/null @@ -1,226 +0,0 @@ -# Delayed migration steps - -require 'migration/table_dropper' -require 'migration/column_dropper' -require 'badge_posts_view_manager' - -Migration::ColumnDropper.drop( - table: 'user_profiles', - after_migration: 'DropUserCardBadgeColumns', - columns: ['card_image_badge_id'], - on_drop: ->() { - STDERR.puts "Removing user_profiles column card_image_badge_id" - }, - delay: 3600 -) - -Migration::ColumnDropper.drop( - table: 'categories', - after_migration: 'AddSuppressFromLatestToCategories', - columns: ['logo_url', 'background_url', 'suppress_from_homepage'], - on_drop: ->() { - STDERR.puts 'Removing superflous categories columns!' - } -) - -Migration::ColumnDropper.drop( - table: 'groups', - after_migration: 'SplitAliasLevels', - columns: %w[visible public alias_level], - on_drop: ->() { - STDERR.puts 'Removing superflous visible group column!' - } -) - -Migration::ColumnDropper.drop( - table: 'theme_fields', - after_migration: 'AddUploadIdToThemeFields', - columns: ['target'], - on_drop: ->() { - STDERR.puts 'Removing superflous theme_fields target column!' - } -) - -Migration::ColumnDropper.drop( - table: 'user_stats', - after_migration: 'DropUnreadTrackingColumns', - columns: %w{ - first_topic_unread_at - }, - on_drop: ->() { - STDERR.puts "Removing superflous user stats columns!" - DB.exec "DROP FUNCTION IF EXISTS first_unread_topic_for(int)" - } -) - -Migration::ColumnDropper.drop( - table: 'topics', - after_migration: 'DropVoteCountFromTopicsAndPosts', - columns: %w{ - auto_close_at - auto_close_user_id - auto_close_started_at - auto_close_based_on_last_post - auto_close_hours - inappropriate_count - bookmark_count - off_topic_count - illegal_count - notify_user_count - last_unread_at - vote_count - }, - on_drop: ->() { - STDERR.puts "Removing superflous topic columns!" - } -) - -Migration::ColumnDropper.drop( - table: 'posts', - after_migration: 'DropVoteCountFromTopicsAndPosts', - columns: %w{ - vote_count - }, - on_drop: ->() { - STDERR.puts "Removing superflous post columns!" - BadgePostsViewManager.drop! - }, - after_drop: -> () { - BadgePostsViewManager.create! - } -) - -Migration::ColumnDropper.drop( - table: 'users', - after_migration: 'DropEmailFromUsers', - columns: %w[ - email - email_always - mailing_list_mode - email_digests - email_direct - email_private_messages - external_links_in_new_tab - enable_quoting - dynamic_favicon - disable_jump_reply - edit_history_public - automatically_unpin_topics - digest_after_days - auto_track_topics_after_msecs - new_topic_duration_minutes - last_redirected_to_top_at - auth_token - auth_token_updated_at - ], - on_drop: ->() { - STDERR.puts 'Removing superflous users columns!' - } -) - -Migration::ColumnDropper.drop( - table: 'users', - after_migration: 'RenameBlockedSilence', - columns: %w[ - blocked - ], - on_drop: ->() { - STDERR.puts 'Removing user blocked column!' - } -) - -Migration::ColumnDropper.drop( - table: 'users', - after_migration: 'AddSilencedTillToUsers', - columns: %w[ - silenced - ], - on_drop: ->() { - STDERR.puts 'Removing user silenced column!' - } -) - -Migration::ColumnDropper.drop( - table: 'users', - after_migration: 'AddTrustLevelLocksToUsers', - columns: %w[ - trust_level_locked - ], - on_drop: ->() { - STDERR.puts 'Removing user trust_level_locked!' - } -) - -Migration::ColumnDropper.drop( - table: 'user_auth_tokens', - after_migration: 'RemoveLegacyAuthToken', - columns: %w[ - legacy - ], - on_drop: ->() { - STDERR.puts 'Removing user_auth_token legacy column!' - } -) - -Migration::TableDropper.delayed_rename( - old_name: 'topic_status_updates', - new_name: 'topic_timers', - after_migration: 'RenameTopicStatusUpdatesToTopicTimers', - on_drop: ->() { - STDERR.puts "Dropping topic_status_updates. It was moved to topic_timers." - } -) - -Migration::TableDropper.delayed_drop( - table_name: 'category_featured_users', - after_migration: 'DropUnusedTables', - on_drop: ->() { - STDERR.puts "Dropping category_featured_users. It isn't used anymore." - } -) - -Migration::TableDropper.delayed_drop( - table_name: 'versions', - after_migration: 'DropUnusedTables', - on_drop: ->() { - STDERR.puts "Dropping versions. It isn't used anymore." - } -) - -Migration::ColumnDropper.drop( - table: 'user_options', - after_migration: 'DropKeyColumnFromThemes', - columns: %w[ - theme_key - ], - on_drop: ->() { - STDERR.puts 'Removing theme_key column from user_options table!' - } -) - -Migration::ColumnDropper.drop( - table: 'themes', - after_migration: 'DropKeyColumnFromThemes', - columns: %w[ - key - ], - on_drop: ->() { - STDERR.puts 'Removing key column from themes table!' - } -) - -Migration::ColumnDropper.drop( - table: 'email_logs', - after_migration: 'DropReplyKeySkippedSkippedReasonFromEmailLogs', - columns: %w{ - topic_id - reply_key - skipped - skipped_reason - }, - on_drop: ->() { - STDERR.puts "Removing superflous email_logs columns!" - } -) - -Discourse.reset_active_record_cache diff --git a/db/migrate/20170524182846_add_unread_tracking_columns.rb b/db/migrate/20170524182846_add_unread_tracking_columns.rb deleted file mode 100644 index 45b94c43194..00000000000 --- a/db/migrate/20170524182846_add_unread_tracking_columns.rb +++ /dev/null @@ -1,9 +0,0 @@ -class AddUnreadTrackingColumns < ActiveRecord::Migration[4.2] - def up - # no op, no need to create all data, next migration will delete it - end - - def down - raise ActiveRecord::IrreversibleMigration - end -end diff --git a/db/migrate/20170526125321_drop_unread_tracking_columns.rb b/db/migrate/20170526125321_drop_unread_tracking_columns.rb deleted file mode 100644 index 8e609f8c32d..00000000000 --- a/db/migrate/20170526125321_drop_unread_tracking_columns.rb +++ /dev/null @@ -1,8 +0,0 @@ -class DropUnreadTrackingColumns < ActiveRecord::Migration[4.2] - def up - end - - def down - raise ActiveRecord::IrreversibleMigration - end -end diff --git a/db/migrate/20170728054115_remove_public_from_groups.rb b/db/migrate/20170728054115_remove_public_from_groups.rb deleted file mode 100644 index ec16a39d52b..00000000000 --- a/db/migrate/20170728054115_remove_public_from_groups.rb +++ /dev/null @@ -1,9 +0,0 @@ -class RemovePublicFromGroups < ActiveRecord::Migration[4.2] - def up - # Defer dropping of the columns until the new application code has been deployed. - end - - def down - raise ActiveRecord::IrreversibleMigration - end -end diff --git a/db/migrate/20171107020512_drop_email_from_users.rb b/db/migrate/20171107020512_drop_email_from_users.rb deleted file mode 100644 index f5697d37187..00000000000 --- a/db/migrate/20171107020512_drop_email_from_users.rb +++ /dev/null @@ -1,9 +0,0 @@ -class DropEmailFromUsers < ActiveRecord::Migration[5.1] - def up - # Defer dropping of the columns until the new application code has been deployed. - end - - def down - raise ActiveRecord::IrreversibleMigration - end -end diff --git a/db/migrate/20180227161818_drop_unused_tables.rb b/db/migrate/20180227161818_drop_unused_tables.rb deleted file mode 100644 index f0c735f0218..00000000000 --- a/db/migrate/20180227161818_drop_unused_tables.rb +++ /dev/null @@ -1,9 +0,0 @@ -class DropUnusedTables < ActiveRecord::Migration[5.1] - def up - # Delayed drop of tables "category_featured_users" and "versions" - end - - def down - raise ActiveRecord::IrreversibleMigration - end -end diff --git a/db/migrate/20180504000531_remove_legacy_auth_token.rb b/db/migrate/20180504000531_remove_legacy_auth_token.rb deleted file mode 100644 index 19c785fb437..00000000000 --- a/db/migrate/20180504000531_remove_legacy_auth_token.rb +++ /dev/null @@ -1,5 +0,0 @@ -class RemoveLegacyAuthToken < ActiveRecord::Migration[5.1] - def change - # placeholder so we can drop column in 009_users.rb - end -end diff --git a/db/migrate/20180619091608_drop_vote_count_from_topics_and_posts.rb b/db/migrate/20180619091608_drop_vote_count_from_topics_and_posts.rb deleted file mode 100644 index 1e061449d62..00000000000 --- a/db/migrate/20180619091608_drop_vote_count_from_topics_and_posts.rb +++ /dev/null @@ -1,9 +0,0 @@ -class DropVoteCountFromTopicsAndPosts < ActiveRecord::Migration[5.2] - def up - # Delayed drop - end - - def down - raise ActiveRecord::IrreversibleMigration - end -end diff --git a/db/post_migrate/20180917024729_remove_superfluous_columns.rb b/db/post_migrate/20180917024729_remove_superfluous_columns.rb new file mode 100644 index 00000000000..3b7d915e19d --- /dev/null +++ b/db/post_migrate/20180917024729_remove_superfluous_columns.rb @@ -0,0 +1,82 @@ +require 'migration/column_dropper' +require 'badge_posts_view_manager' + +class RemoveSuperfluousColumns < ActiveRecord::Migration[5.2] + def up + { + user_profiles: %i{ + card_image_badge_id + }, + categories: %i{ + logo_url + background_url + suppress_from_homepage + }, + groups: %i{ + visible + public + alias_level + }, + theme_fields: %i{target}, + user_stats: %i{first_topic_unread_at}, + topics: %i{ + auto_close_at + auto_close_user_id + auto_close_started_at + auto_close_based_on_last_post + auto_close_hours + inappropriate_count + bookmark_count + off_topic_count + illegal_count + notify_user_count + last_unread_at + vote_count + }, + users: %i{ + email + email_always + mailing_list_mode + email_digests + email_direct + email_private_messages + external_links_in_new_tab + enable_quoting + dynamic_favicon + disable_jump_reply + edit_history_public + automatically_unpin_topics + digest_after_days + auto_track_topics_after_msecs + new_topic_duration_minutes + last_redirected_to_top_at + auth_token + auth_token_updated_at + blocked + silenced + trust_level_locked + }, + user_auth_tokens: %i{legacy}, + user_options: %i{theme_key}, + themes: %i{key}, + email_logs: %i{ + topic_id + reply_key + skipped + skipped_reason + }, + }.each do |table, columns| + Migration::ColumnDropper.execute_drop(table, columns) + end + + DB.exec "DROP FUNCTION IF EXISTS first_unread_topic_for(int)" + + BadgePostsViewManager.drop! + Migration::ColumnDropper.execute_drop(:posts, %i{vote_count}) + BadgePostsViewManager.create! + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/post_migrate/20180917034056_remove_superfluous_tables.rb b/db/post_migrate/20180917034056_remove_superfluous_tables.rb new file mode 100644 index 00000000000..0c713b3a5c8 --- /dev/null +++ b/db/post_migrate/20180917034056_remove_superfluous_tables.rb @@ -0,0 +1,17 @@ +require 'migration/table_dropper' + +class RemoveSuperfluousTables < ActiveRecord::Migration[5.2] + def up + %i{ + category_featured_users + versions + topic_status_updates + }.each do |table| + Migration::TableDropper.execute_drop(table) + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/discourse.rb b/lib/discourse.rb index 90c5faff19a..ee09328ad80 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -13,6 +13,7 @@ if Rails.env.development? end module Discourse + DB_POST_MIGRATE_PATH ||= "db/post_migrate" require 'sidekiq/exception_handler' class SidekiqExceptionHandler diff --git a/lib/generators/rails/post_migration_generator.rb b/lib/generators/rails/post_migration_generator.rb new file mode 100644 index 00000000000..70d809323c9 --- /dev/null +++ b/lib/generators/rails/post_migration_generator.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require "rails/generators/active_record/migration/migration_generator" + +class Rails::PostMigrationGenerator < ActiveRecord::Generators::MigrationGenerator + source_root "#{Gem.loaded_specs["activerecord"].full_gem_path}/lib/rails/generators/active_record/migration/templates" + + private + + def db_migrate_path + Discourse::DB_POST_MIGRATE_PATH + end +end diff --git a/lib/migration/base_dropper.rb b/lib/migration/base_dropper.rb index 9215c559df2..0616257f443 100644 --- a/lib/migration/base_dropper.rb +++ b/lib/migration/base_dropper.rb @@ -2,51 +2,6 @@ module Migration class BaseDropper FUNCTION_SCHEMA_NAME = "discourse_functions".freeze - def initialize(after_migration, delay, on_drop, after_drop) - @after_migration = after_migration - @on_drop = on_drop - @after_drop = after_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! - @after_drop&.call - - 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) OR - (SELECT created_at - FROM schema_migration_details - ORDER BY id ASC - LIMIT 1) > (current_timestamp AT TIME ZONE 'UTC' - INTERVAL '10 minutes') - ) - ) - SQL - end - def self.create_readonly_function(table_name, column_name = nil) DB.exec <<~SQL CREATE SCHEMA IF NOT EXISTS #{FUNCTION_SCHEMA_NAME}; @@ -64,15 +19,6 @@ module Migration $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) function_name = [ diff --git a/lib/migration/column_dropper.rb b/lib/migration/column_dropper.rb index 77f3d118813..a2c421ab52f 100644 --- a/lib/migration/column_dropper.rb +++ b/lib/migration/column_dropper.rb @@ -1,69 +1,35 @@ require_dependency 'migration/base_dropper' module Migration - class ColumnDropper < BaseDropper - def self.drop(table:, after_migration:, columns:, delay: nil, on_drop: nil, after_drop: nil) - validate_table_name(table) - columns.each { |column| validate_column_name(column) } - - ColumnDropper.new( - table, columns, after_migration, delay, on_drop, after_drop - ).delayed_drop - end - + class ColumnDropper def self.mark_readonly(table_name, column_name) - create_readonly_function(table_name, column_name) + BaseDropper.create_readonly_function(table_name, column_name) DB.exec <<~SQL - CREATE TRIGGER #{readonly_trigger_name(table_name, column_name)} + CREATE TRIGGER #{BaseDropper.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)}; + EXECUTE PROCEDURE #{BaseDropper.readonly_function_name(table_name, column_name)}; SQL end - private + def self.execute_drop(table, columns) + table = table.to_s - def initialize(table, columns, after_migration, delay, on_drop, after_drop) - super(after_migration, delay, on_drop, after_drop) + columns.each do |column| + column = column.to_s - @table = table - @columns = columns - end - - def droppable? - builder = DB.build(<<~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) > 0 - end - - def execute_drop! - @columns.each do |column| DB.exec <<~SQL - DROP TRIGGER IF EXISTS #{BaseDropper.readonly_trigger_name(@table, column)} ON #{@table}; - DROP FUNCTION IF EXISTS #{BaseDropper.readonly_function_name(@table, column)} CASCADE; + DROP FUNCTION IF EXISTS #{BaseDropper.readonly_function_name(table, column)} CASCADE; -- Backward compatibility for old functions created in the public -- schema - DROP FUNCTION IF EXISTS #{BaseDropper.old_readonly_function_name(@table, column)} CASCADE; + DROP FUNCTION IF EXISTS #{BaseDropper.old_readonly_function_name(table, column)} CASCADE; SQL # safe cause it is protected on method entry, can not be passed in params - DB.exec("ALTER TABLE #{@table} DROP COLUMN IF EXISTS #{column}") + DB.exec("ALTER TABLE #{table} DROP COLUMN IF EXISTS #{column}") end end end diff --git a/lib/migration/safe_migrate.rb b/lib/migration/safe_migrate.rb index 18e1e14d308..d777f095bd0 100644 --- a/lib/migration/safe_migrate.rb +++ b/lib/migration/safe_migrate.rb @@ -16,13 +16,33 @@ class Migration::SafeMigrate end def migrate(direction) - if direction == :up && version && version > UNSAFE_VERSION && @@enable_safe != false + if direction == :up && + version && version > UNSAFE_VERSION && + @@enable_safe != false && + !is_post_deploy_migration? + Migration::SafeMigrate.enable! end + super ensure Migration::SafeMigrate.disable! end + + private + + def is_post_deploy_migration? + method = + if self.respond_to?(:up) + :up + elsif self.respond_to?(:change) + :change + end + + self.method(method).source_location.first.include?( + Discourse::DB_POST_MIGRATE_PATH + ) + end end module NiceErrors @@ -90,7 +110,7 @@ class Migration::SafeMigrate ------------------------------------------------------------------------------------- An attempt was made to drop or rename a table in a migration SQL used was: '#{sql}' - Please use the deferred pattern using Migration::TableDropper in db/seeds to drop + Please generate a post deployment migration using `rails g post_migration` to drop or rename the table. This protection is in place to protect us against dropping tables that are currently @@ -103,7 +123,8 @@ class Migration::SafeMigrate ------------------------------------------------------------------------------------- An attempt was made to drop or rename a column in a migration SQL used was: '#{sql}' - Please use the deferred pattern using Migration::ColumnDropper in db/seeds to drop + + Please generate a post deployment migration using `rails g post_migration` to drop or rename columns. Note, to minimize disruption use self.ignored_columns = ["column name"] on your diff --git a/lib/migration/table_dropper.rb b/lib/migration/table_dropper.rb index 5718ac0ecb7..23568d0c35a 100644 --- a/lib/migration/table_dropper.rb +++ b/lib/migration/table_dropper.rb @@ -1,84 +1,21 @@ require_dependency 'migration/base_dropper' module Migration - class Migration::TableDropper < BaseDropper - def self.delayed_drop(table_name:, after_migration:, delay: nil, on_drop: nil, after_drop: nil) - validate_table_name(table_name) - - TableDropper.new( - table_name, nil, after_migration, delay, on_drop, after_drop - ).delayed_drop - end - - def self.delayed_rename(old_name:, new_name:, after_migration:, delay: nil, on_drop: nil, after_drop: nil) - validate_table_name(old_name) - validate_table_name(new_name) - - TableDropper.new( - old_name, new_name, after_migration, delay, on_drop, after_drop - ).delayed_drop - end - + class Migration::TableDropper def self.read_only_table(table_name) - create_readonly_function(table_name) + BaseDropper.create_readonly_function(table_name) DB.exec <<~SQL - CREATE TRIGGER #{readonly_trigger_name(table_name)} + CREATE TRIGGER #{BaseDropper.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)}; + EXECUTE PROCEDURE #{BaseDropper.readonly_function_name(table_name)}; SQL end - private - - def initialize(old_name, new_name, after_migration, delay, on_drop, after_drop) - super(after_migration, delay, on_drop, after_drop) - - @old_name = old_name - @new_name = new_name - end - - def droppable? - builder = DB.build(<<~SQL) - SELECT 1 - FROM INFORMATION_SCHEMA.TABLES - /*where*/ - LIMIT 1 - SQL - - builder.where(table_exists(":new_name")) if @new_name.present? - - builder.where("table_schema = 'public'") - .where(table_exists(":old_name")) - .where(previous_migration_done) - .exec(old_name: @old_name, - new_name: @new_name, - delay: "#{@delay} seconds", - after_migration: @after_migration) > 0 - end - - def table_exists(table_name_placeholder) - <<~SQL - EXISTS( - SELECT 1 - FROM INFORMATION_SCHEMA.TABLES - WHERE table_schema = 'public' AND - table_name = #{table_name_placeholder} - ) - SQL - end - - def execute_drop! - DB.exec("DROP TABLE IF EXISTS #{@old_name}") - - DB.exec <<~SQL - DROP FUNCTION IF EXISTS #{BaseDropper.readonly_function_name(@old_name)} CASCADE; - -- Backward compatibility for old functions created in the public - -- schema - DROP FUNCTION IF EXISTS #{BaseDropper.old_readonly_function_name(@old_name)} CASCADE; - SQL + def self.execute_drop(table_name) + DB.exec("DROP TABLE IF EXISTS #{table_name}") end end end diff --git a/spec/components/migration/column_dropper_spec.rb b/spec/components/migration/column_dropper_spec.rb index 4dc80556f87..c255606f734 100644 --- a/spec/components/migration/column_dropper_spec.rb +++ b/spec/components/migration/column_dropper_spec.rb @@ -2,7 +2,6 @@ require 'rails_helper' require_dependency 'migration/column_dropper' RSpec.describe Migration::ColumnDropper do - def has_column?(table, column) DB.exec(<<~SQL, table: table, column: column) == 1 SELECT 1 @@ -14,105 +13,27 @@ RSpec.describe Migration::ColumnDropper do SQL end - def update_first_migration_date(created_at) - DB.exec(<<~SQL, created_at: created_at) - UPDATE schema_migration_details - SET created_at = :created_at - WHERE id = (SELECT MIN(id) - FROM schema_migration_details) - SQL - end - - describe ".drop" do - let(:migration_name) do - DB.query_single("SELECT name FROM schema_migration_details ORDER BY id DESC LIMIT 1").first - end + describe ".execute_drop" do + let(:columns) { %w{junk junk2} } before do - DB.exec "ALTER TABLE topics ADD COLUMN junk int" - - DB.exec(<<~SQL, name: migration_name, created_at: 15.minutes.ago) - UPDATE schema_migration_details - SET created_at = :created_at - WHERE name = :name - SQL + columns.each do |column| + DB.exec("ALTER TABLE topics ADD COLUMN #{column} int") + end end - it "can correctly drop columns after correct delay" do - dropped_proc_called = false - after_dropped_proc_called = false - update_first_migration_date(2.years.ago) - - Migration::ColumnDropper.drop( - table: 'topics', - after_migration: migration_name, - columns: ['junk'], - delay: 20.minutes, - on_drop: ->() { dropped_proc_called = true }, - after_drop: ->() { after_dropped_proc_called = true } - ) - - expect(has_column?('topics', 'junk')).to eq(true) - expect(dropped_proc_called).to eq(false) - expect(dropped_proc_called).to eq(false) - - Migration::ColumnDropper.drop( - table: 'topics', - after_migration: migration_name, - columns: ['junk'], - delay: 10.minutes, - on_drop: ->() { dropped_proc_called = true }, - after_drop: ->() { after_dropped_proc_called = true } - ) - - expect(has_column?('topics', 'junk')).to eq(false) - expect(dropped_proc_called).to eq(true) - expect(after_dropped_proc_called).to eq(true) - - dropped_proc_called = false - after_dropped_proc_called = false - - Migration::ColumnDropper.drop( - table: 'topics', - after_migration: migration_name, - columns: ['junk'], - delay: 10.minutes, - on_drop: ->() { dropped_proc_called = true }, - after_drop: ->() { after_dropped_proc_called = true } - ) - - # it should call "on_drop" only when there are columns to drop - expect(dropped_proc_called).to eq(false) - expect(after_dropped_proc_called).to eq(false) + after do + columns.each do |column| + DB.exec("ALTER TABLE topics DROP COLUMN IF EXISTS #{column}") + end end - it "drops the columns immediately if the first migration was less than 10 minutes ago" do - dropped_proc_called = false - update_first_migration_date(11.minutes.ago) + it "drops the columns" do + Migration::ColumnDropper.execute_drop("topics", columns) - Migration::ColumnDropper.drop( - table: 'topics', - after_migration: migration_name, - columns: ['junk'], - delay: 30.minutes, - on_drop: ->() { dropped_proc_called = true } - ) - - expect(has_column?('topics', 'junk')).to eq(true) - expect(dropped_proc_called).to eq(false) - - update_first_migration_date(9.minutes.ago) - - Migration::ColumnDropper.drop( - table: 'topics', - after_migration: migration_name, - columns: ['junk'], - delay: 30.minutes, - on_drop: ->() { dropped_proc_called = true } - ) - - expect(has_column?('topics', 'junk')).to eq(false) - expect(dropped_proc_called).to eq(true) + columns.each do |column| + expect(has_column?('topics', column)).to eq(false) + end end end @@ -135,23 +56,18 @@ RSpec.describe Migration::ColumnDropper do DB.exec <<~SQL DROP TABLE IF EXISTS #{table_name}; - DROP TRIGGER IF EXISTS #{table_name}_email_readonly ON #{table_name}; + DROP FUNCTION IF EXISTS #{Migration::BaseDropper.readonly_function_name(table_name, 'email')} CASCADE; SQL end it 'should be droppable' do - name = DB.query_single("SELECT name FROM schema_migration_details LIMIT 1").first + Migration::ColumnDropper.execute_drop(table_name, ['email']) - dropped_proc_called = false - Migration::ColumnDropper.drop( - table: table_name, - after_migration: name, - columns: ['email'], - delay: 0.minutes, - on_drop: ->() { dropped_proc_called = true } - ) + expect(has_trigger?(Migration::BaseDropper.readonly_trigger_name( + table_name, 'email' + ))).to eq(false) - expect(dropped_proc_called).to eq(true) + expect(has_column?(table_name, 'email')).to eq(false) end it 'should prevent updates to the readonly column' do diff --git a/spec/components/migration/safe_migrate_spec.rb b/spec/components/migration/safe_migrate_spec.rb index 15659e4b900..315cedd646d 100644 --- a/spec/components/migration/safe_migrate_spec.rb +++ b/spec/components/migration/safe_migrate_spec.rb @@ -29,7 +29,7 @@ describe Migration::SafeMigrate do it "bans all table removal" do Migration::SafeMigrate.enable! - path = File.expand_path "#{Rails.root}/spec/fixtures/migrate/drop_table" + path = File.expand_path "#{Rails.root}/spec/fixtures/db/migrate/drop_table" output = capture_stdout do expect(lambda do @@ -37,7 +37,7 @@ describe Migration::SafeMigrate do end).to raise_error(StandardError) end - expect(output).to include("TableDropper") + expect(output).to include("rails g post_migration") expect { User.first }.not_to raise_error expect(User.first).not_to eq(nil) @@ -46,7 +46,7 @@ describe Migration::SafeMigrate do it "bans all table renames" do Migration::SafeMigrate.enable! - path = File.expand_path "#{Rails.root}/spec/fixtures/migrate/rename_table" + path = File.expand_path "#{Rails.root}/spec/fixtures/db/migrate/rename_table" output = capture_stdout do expect(lambda do @@ -57,13 +57,13 @@ describe Migration::SafeMigrate do expect { User.first }.not_to raise_error expect(User.first).not_to eq(nil) - expect(output).to include("TableDropper") + expect(output).to include("rails g post_migration") end it "bans all column removal" do Migration::SafeMigrate.enable! - path = File.expand_path "#{Rails.root}/spec/fixtures/migrate/remove_column" + path = File.expand_path "#{Rails.root}/spec/fixtures/db/migrate/remove_column" output = capture_stdout do expect(lambda do @@ -71,7 +71,7 @@ describe Migration::SafeMigrate do end).to raise_error(StandardError) end - expect(output).to include("ColumnDropper") + expect(output).to include("rails g post_migration") expect(User.first).not_to eq(nil) expect { User.first.username }.not_to raise_error @@ -80,7 +80,7 @@ describe Migration::SafeMigrate do it "bans all column renames" do Migration::SafeMigrate.enable! - path = File.expand_path "#{Rails.root}/spec/fixtures/migrate/rename_column" + path = File.expand_path "#{Rails.root}/spec/fixtures/db/migrate/rename_column" output = capture_stdout do expect(lambda do @@ -88,7 +88,7 @@ describe Migration::SafeMigrate do end).to raise_error(StandardError) end - expect(output).to include("ColumnDropper") + expect(output).to include("rails g post_migration") expect(User.first).not_to eq(nil) expect { User.first.username }.not_to raise_error @@ -98,7 +98,7 @@ describe Migration::SafeMigrate do Migration::SafeMigrate.enable! Migration::SafeMigrate.disable! - path = File.expand_path "#{Rails.root}/spec/fixtures/migrate/drop_table" + path = File.expand_path "#{Rails.root}/spec/fixtures/db/migrate/drop_table" output = capture_stdout do migrate_up(path) @@ -106,4 +106,20 @@ describe Migration::SafeMigrate do expect(output).to include("drop_table(:users)") end + + describe 'for a post deployment migration' do + it 'should not ban unsafe migrations' do + user = Fabricate(:user) + Migration::SafeMigrate::SafeMigration.enable_safe! + + path = File.expand_path "#{Rails.root}/spec/fixtures/db/post_migrate/drop_table" + + output = capture_stdout do + migrate_up(path) + end + + expect(output).to include("drop_table(:users)") + expect(user.reload).to eq(user) + end + end end diff --git a/spec/components/migration/table_dropper_spec.rb b/spec/components/migration/table_dropper_spec.rb index 7db3c64f1de..9b9ac0cdf54 100644 --- a/spec/components/migration/table_dropper_spec.rb +++ b/spec/components/migration/table_dropper_spec.rb @@ -4,200 +4,68 @@ require_dependency 'migration/table_dropper' describe Migration::TableDropper do def table_exists?(table_name) - sql = <<~SQL + DB.exec(<<~SQL) > 0 SELECT 1 FROM INFORMATION_SCHEMA.TABLES WHERE table_schema = 'public' AND table_name = '#{table_name}' SQL - - DB.exec(sql) > 0 end - def update_first_migration_date(created_at) - DB.exec(<<~SQL, created_at: created_at) - UPDATE schema_migration_details - SET created_at = :created_at - WHERE id = (SELECT MIN(id) - FROM schema_migration_details) - SQL - end - - def create_new_table - DB.exec "CREATE TABLE table_with_new_name (topic_id INTEGER)" - end - - let(:migration_name) do - DB.query_single("SELECT name FROM schema_migration_details ORDER BY id DESC LIMIT 1").first - end + let(:table_name) { 'table_with_old_name' } before do - DB.exec "CREATE TABLE table_with_old_name (topic_id INTEGER)" + DB.exec "CREATE TABLE #{table_name} (topic_id INTEGER)" - DB.exec(<<~SQL, name: migration_name, created_at: 15.minutes.ago) - UPDATE schema_migration_details - SET created_at = :created_at - WHERE name = :name + DB.exec <<~SQL + INSERT INTO #{table_name} (topic_id) VALUES (1) SQL end - context "first migration was a long time ago" do - before do - update_first_migration_date(2.years.ago) - end + describe ".execute_drop" do + it "should drop the table" do + Migration::TableDropper.execute_drop(table_name) - describe ".delayed_rename" do - it "can drop a table after correct delay and when new table exists" do - dropped_proc_called = false - - described_class.delayed_rename( - old_name: 'table_with_old_name', - new_name: 'table_with_new_name', - after_migration: migration_name, - delay: 20.minutes, - on_drop: ->() { dropped_proc_called = true } - ) - - expect(table_exists?('table_with_old_name')).to eq(true) - expect(dropped_proc_called).to eq(false) - - described_class.delayed_rename( - old_name: 'table_with_old_name', - new_name: 'table_with_new_name', - after_migration: migration_name, - delay: 10.minutes, - on_drop: ->() { dropped_proc_called = true } - ) - - expect(table_exists?('table_with_old_name')).to eq(true) - expect(dropped_proc_called).to eq(false) - - create_new_table - - described_class.delayed_rename( - old_name: 'table_with_old_name', - new_name: 'table_with_new_name', - after_migration: migration_name, - delay: 10.minutes, - on_drop: ->() { dropped_proc_called = true } - ) - - expect(table_exists?('table_with_old_name')).to eq(false) - expect(dropped_proc_called).to eq(true) - - dropped_proc_called = false - - described_class.delayed_rename( - old_name: 'table_with_old_name', - new_name: 'table_with_new_name', - after_migration: migration_name, - delay: 10.minutes, - on_drop: ->() { dropped_proc_called = true } - ) - - # it should call "on_drop" only when there is a table to drop - expect(dropped_proc_called).to eq(false) - end - end - - describe ".delayed_drop" do - it "can drop a table after correct delay" do - dropped_proc_called = false - - described_class.delayed_drop( - table_name: 'table_with_old_name', - after_migration: migration_name, - delay: 20.minutes, - on_drop: ->() { dropped_proc_called = true } - ) - - expect(table_exists?('table_with_old_name')).to eq(true) - expect(dropped_proc_called).to eq(false) - - described_class.delayed_drop( - table_name: 'table_with_old_name', - after_migration: migration_name, - delay: 10.minutes, - on_drop: ->() { dropped_proc_called = true } - ) - - expect(table_exists?('table_with_old_name')).to eq(false) - expect(dropped_proc_called).to eq(true) - - dropped_proc_called = false - - described_class.delayed_drop( - table_name: 'table_with_old_name', - after_migration: migration_name, - delay: 10.minutes, - on_drop: ->() { dropped_proc_called = true } - ) - - # it should call "on_drop" only when there is a table to drop - expect(dropped_proc_called).to eq(false) - end + expect(table_exists?(table_name)).to eq(false) end end - context "first migration was a less than 10 minutes ago" do - describe ".delayed_rename" do - it "can drop a table after correct delay and when new table exists" do - dropped_proc_called = false - update_first_migration_date(11.minutes.ago) - create_new_table - - described_class.delayed_rename( - old_name: 'table_with_old_name', - new_name: 'table_with_new_name', - after_migration: migration_name, - delay: 30.minutes, - on_drop: ->() { dropped_proc_called = true } - ) - - expect(table_exists?('table_with_old_name')).to eq(true) - expect(dropped_proc_called).to eq(false) - - update_first_migration_date(9.minutes.ago) - - described_class.delayed_rename( - old_name: 'table_with_old_name', - new_name: 'table_with_new_name', - after_migration: migration_name, - delay: 30.minutes, - on_drop: ->() { dropped_proc_called = true } - ) - - expect(table_exists?('table_with_old_name')).to eq(false) - expect(dropped_proc_called).to eq(true) - end + describe ".readonly_only_table" do + before do + Migration::TableDropper.read_only_table(table_name) end - describe ".delayed_drop" do - it "immediately drops the table" do - dropped_proc_called = false - update_first_migration_date(11.minutes.ago) + after do + ActiveRecord::Base.connection.reset! - described_class.delayed_drop( - table_name: 'table_with_old_name', - after_migration: migration_name, - delay: 30.minutes, - on_drop: ->() { dropped_proc_called = true } - ) + DB.exec(<<~SQL) + DROP TABLE IF EXISTS #{table_name}; + DROP FUNCTION IF EXISTS #{Migration::BaseDropper.readonly_function_name(table_name)} CASCADE; + SQL + end - expect(table_exists?('table_with_old_name')).to eq(true) - expect(dropped_proc_called).to eq(false) + it 'should be droppable' do + Migration::TableDropper.execute_drop(table_name) - update_first_migration_date(9.minutes.ago) + expect(has_trigger?(Migration::BaseDropper.readonly_trigger_name( + table_name + ))).to eq(false) - described_class.delayed_drop( - table_name: 'table_with_old_name', - after_migration: migration_name, - delay: 30.minutes, - on_drop: ->() { dropped_proc_called = true } - ) + expect(table_exists?(table_name)).to eq(false) + end - expect(table_exists?('table_with_old_name')).to eq(false) - expect(dropped_proc_called).to eq(true) + it 'should prevent insertions to the table' do + begin + DB.exec <<~SQL + INSERT INTO #{table_name} (topic_id) VALUES (2) + SQL + rescue PG::RaiseException => e + [ + "Discourse: #{table_name} is read only", + 'discourse_functions.raise_table_with_old_name_readonly()' + ].each do |message| + expect(e.message).to include(message) + end end end end diff --git a/spec/fixtures/migrate/drop_table/20990309014014_drop_table.rb b/spec/fixtures/db/migrate/drop_table/20990309014014_drop_table.rb similarity index 100% rename from spec/fixtures/migrate/drop_table/20990309014014_drop_table.rb rename to spec/fixtures/db/migrate/drop_table/20990309014014_drop_table.rb diff --git a/spec/fixtures/migrate/remove_column/20990309014014_remove_column.rb b/spec/fixtures/db/migrate/remove_column/20990309014014_remove_column.rb similarity index 100% rename from spec/fixtures/migrate/remove_column/20990309014014_remove_column.rb rename to spec/fixtures/db/migrate/remove_column/20990309014014_remove_column.rb diff --git a/spec/fixtures/migrate/rename_column/20990309014014_rename_column.rb b/spec/fixtures/db/migrate/rename_column/20990309014014_rename_column.rb similarity index 100% rename from spec/fixtures/migrate/rename_column/20990309014014_rename_column.rb rename to spec/fixtures/db/migrate/rename_column/20990309014014_rename_column.rb diff --git a/spec/fixtures/migrate/rename_table/20990309014014_rename_table.rb b/spec/fixtures/db/migrate/rename_table/20990309014014_rename_table.rb similarity index 100% rename from spec/fixtures/migrate/rename_table/20990309014014_rename_table.rb rename to spec/fixtures/db/migrate/rename_table/20990309014014_rename_table.rb diff --git a/spec/fixtures/db/post_migrate/drop_table/20990309014013_drop_users_table.rb b/spec/fixtures/db/post_migrate/drop_table/20990309014013_drop_users_table.rb new file mode 100644 index 00000000000..7dbcf227afb --- /dev/null +++ b/spec/fixtures/db/post_migrate/drop_table/20990309014013_drop_users_table.rb @@ -0,0 +1,10 @@ +class DropUsersTable < ActiveRecord::Migration[5.2] + def up + drop_table :users + raise ActiveRecord::Rollback + end + + def down + raise "not tested" + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index c1ac4760774..2a78230fbed 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -279,3 +279,11 @@ def file_from_fixtures(filename, directory = "images") FileUtils.cp("#{Rails.root}/spec/fixtures/#{directory}/#{filename}", "#{Rails.root}/tmp/spec/#{filename}") File.new("#{Rails.root}/tmp/spec/#{filename}") end + +def has_trigger?(trigger_name) + DB.exec(<<~SQL) != 0 + SELECT 1 + FROM INFORMATION_SCHEMA.TRIGGERS + WHERE trigger_name = '#{trigger_name}' + SQL +end