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