From e3c9bd690d7eb94a6fe48d1835f61f4c4be490c7 Mon Sep 17 00:00:00 2001 From: Ryan Krage Date: Fri, 14 Jun 2024 13:34:31 -0500 Subject: [PATCH] Introduce raw methods for direct dispatch (#98) Co-authored-by: James Coleman --- README.md | 17 ++- lib/pg_ha_migrations.rb | 2 +- lib/pg_ha_migrations/unsafe_statements.rb | 75 ++++++---- spec/dependent_object_checks_spec.rb | 103 +++++++++---- spec/pg_ha_migrations_spec.rb | 30 ++-- spec/safe_statements_spec.rb | 175 +++++++++++++++++++++- 6 files changed, 319 insertions(+), 83 deletions(-) diff --git a/README.md b/README.md index 0b1d0e3..3204944 100644 --- a/README.md +++ b/README.md @@ -42,19 +42,20 @@ There are two major classes of concerns we try to handle in the API: We rename migration methods with prefixes to explicitly denote their safety level: - `safe_*`: These methods check for both application and database safety concerns, prefer concurrent operations where available, set low lock timeouts where appropriate, and decompose operations into multiple safe steps. -- `unsafe_*`: These methods are generally a direct dispatch to the native ActiveRecord migration method. +- `unsafe_*`: Using these methods is a signal that the DDL operation is not necessarily safe for a running application. They include basic safety features like safe lock acquisition and dependent object checking, but otherwise dispatch directly to the native ActiveRecord migration method. +- `raw_*`: These methods are a direct dispatch to the native ActiveRecord migration method. Calling the original migration methods without a prefix will raise an error. The API is designed to be explicit yet remain flexible. There may be situations where invoking the `unsafe_*` method is preferred (or the only option available for definitionally unsafe operations). -While `unsafe_*` methods were historically (through 1.0) pure wrappers for invoking the native ActiveRecord migration method, there is a class of problems that we can't handle easily without breaking that design rule a bit. For example, dropping a column is unsafe from an application perspective, so we make the application safety concerns explicit by using an `unsafe_` prefix. Using `unsafe_remove_column` calls out the need to audit the application to confirm the migration won't break the application. Because there are no safe alternatives we don't define a `safe_remove_column` analogue. However there are still conditions we'd like to assert before dropping a column. For example, dropping an unused column that's used in one or more indexes may be safe from an application perspective, but the cascading drop of the index won't use a `CONCURRENT` operation to drop the dependent indexes and is therefore unsafe from a database perspective. +While `unsafe_*` methods were historically (before 2.0) pure wrappers for invoking the native ActiveRecord migration method, there is a class of problems that we can't handle easily without breaking that design rule a bit. For example, dropping a column is unsafe from an application perspective, so we make the application safety concerns explicit by using an `unsafe_` prefix. Using `unsafe_remove_column` calls out the need to audit the application to confirm the migration won't break the application. Because there are no safe alternatives we don't define a `safe_remove_column` analogue. However there are still conditions we'd like to assert before dropping a column. For example, dropping an unused column that's used in one or more indexes may be safe from an application perspective, but the cascading drop of the index won't use a `CONCURRENT` operation to drop the dependent indexes and is therefore unsafe from a database perspective. -When `unsafe_*` migration methods support checks of this type you can bypass the checks by passing an `:allow_dependent_objects` key in the method's `options` hash containing an array of dependent object types you'd like to allow. Until 2.0 none of these checks will run by default, but you can opt-in by setting `config.check_for_dependent_objects = true` [in your configuration initializer](#configuration).a +For `unsafe_*` migration methods which support checks of this type you can bypass the checks by passing an `:allow_dependent_objects` key in the method's `options` hash containing an array of dependent object types you'd like to allow. These checks will run by default, but you can opt-out by setting `config.check_for_dependent_objects = false` [in your configuration initializer](#configuration). ### Migration Method Arguments -We believe the `force: true` option to ActiveRecord's `create_table` method is always unsafe because it's not possible to denote exactly how the current state will change. Therefore we disallow using `force: true` even when calling `unsafe_create_table`. This option won't be enabled by default until 2.0, but you can opt-in by setting `config.allow_force_create_table = false` [in your configuration initializer](#configuration). +We believe the `force: true` option to ActiveRecord's `create_table` method is always unsafe because it's not possible to denote exactly how the current state will change. Therefore we disallow using `force: true` even when calling `unsafe_create_table`. This option is enabled by default, but you can opt-out by setting `config.allow_force_create_table = true` [in your configuration initializer](#configuration). ### Rollback @@ -184,7 +185,7 @@ safe_change_column_default :table, :column, -> { "NOW()" } safe_change_column_default :table, :column, -> { "'NOW()'" } ``` -Note: On Postgres 11+ adding a column with a constant default value does not rewrite or scan the table (under a lock or otherwise). In that case a migration adding a column with a default should do so in a single operation rather than the two-step `safe_add_column` followed by `safe_change_column_default`. We enforce this best practice with the error `PgHaMigrations::BestPracticeError`, but if your prefer otherwise (or are running in a mixed Postgres version environment), you may opt out by setting `config.prefer_single_step_column_addition_with_default = true` [in your configuration initializer](#configuration). +Note: On Postgres 11+ adding a column with a constant default value does not rewrite or scan the table (under a lock or otherwise). In that case a migration adding a column with a default should do so in a single operation rather than the two-step `safe_add_column` followed by `safe_change_column_default`. We enforce this best practice with the error `PgHaMigrations::BestPracticeError`, but if your prefer otherwise (or are running in a mixed Postgres version environment), you may opt out by setting `config.prefer_single_step_column_addition_with_default = false` [in your configuration initializer](#configuration). #### safe\_make\_column\_nullable @@ -544,9 +545,9 @@ end #### Available options - `disable_default_migration_methods`: If true, the default implementations of DDL changes in `ActiveRecord::Migration` and the PostgreSQL adapter will be overridden by implementations that raise a `PgHaMigrations::UnsafeMigrationError`. Default: `true` -- `check_for_dependent_objects`: If true, some `unsafe_*` migration methods will raise a `PgHaMigrations::UnsafeMigrationError` if any dependent objects exist. Default: `false` -- `prefer_single_step_column_addition_with_default`: If true, raise an error when adding a column and separately setting a constant default value for that column in the same migration. Default: `false` -- `allow_force_create_table`: If false, the `force: true` option to ActiveRecord's `create_table` method is disallowed. Default: `true` +- `check_for_dependent_objects`: If true, some `unsafe_*` migration methods will raise a `PgHaMigrations::UnsafeMigrationError` if any dependent objects exist. Default: `true` +- `prefer_single_step_column_addition_with_default`: If true, raise an error when adding a column and separately setting a constant default value for that column in the same migration. Default: `true` +- `allow_force_create_table`: If false, the `force: true` option to ActiveRecord's `create_table` method is disallowed. Default: `false` - `infer_primary_key_on_partitioned_tables`: If true, the primary key for partitioned tables will be inferred on PostgreSQL 11+ databases (identifier column + partition key columns). Default: `true` ### Rake Tasks diff --git a/lib/pg_ha_migrations.rb b/lib/pg_ha_migrations.rb index 3e5b48c..edb512b 100644 --- a/lib/pg_ha_migrations.rb +++ b/lib/pg_ha_migrations.rb @@ -19,9 +19,9 @@ module PgHaMigrations def self.config @config ||= Config.new( true, - false, true, false, + true, true ) end diff --git a/lib/pg_ha_migrations/unsafe_statements.rb b/lib/pg_ha_migrations/unsafe_statements.rb index d0a2ee9..ec5c9d7 100644 --- a/lib/pg_ha_migrations/unsafe_statements.rb +++ b/lib/pg_ha_migrations/unsafe_statements.rb @@ -1,10 +1,6 @@ module PgHaMigrations::UnsafeStatements def self.disable_or_delegate_default_method(method_name, error_message, allow_reentry_from_compatibility_module: false) define_method(method_name) do |*args, &block| - if PgHaMigrations.config.check_for_dependent_objects - disallow_migration_method_if_dependent_objects!(method_name, arguments: args) - end - if PgHaMigrations.config.disable_default_migration_methods # Most migration methods are only ever called by a migration and # therefore aren't re-entrant or callable from another migration @@ -12,7 +8,7 @@ def self.disable_or_delegate_default_method(method_name, error_message, allow_re # implementations in `ActiveRecord::Migration::Compatibility` so # we have to explicitly handle that case by allowing execution of # the original implementation by its original name. - unless allow_reentry_from_compatibility_module && caller[0] =~ /lib\/active_record\/migration\/compatibility.rb/ + unless allow_reentry_from_compatibility_module && caller[0] =~ /lib\/active_record\/migration\/compatibility.rb/ raise PgHaMigrations::UnsafeMigrationError, error_message end end @@ -33,38 +29,67 @@ def self.delegate_unsafe_method_to_migration_base_class(method_name) ruby2_keywords "unsafe_#{method_name}" end + def self.delegate_raw_method_to_migration_base_class(method_name) + define_method("raw_#{method_name}") do |*args, &block| + execute_ancestor_statement(method_name, *args, &block) + end + ruby2_keywords "raw_#{method_name}" + end + + # Direct dispatch to underlying Rails method as unsafe_ with dependent object check + delegate_unsafe_method_to_migration_base_class :add_check_constraint delegate_unsafe_method_to_migration_base_class :add_column - delegate_unsafe_method_to_migration_base_class :change_table - delegate_unsafe_method_to_migration_base_class :drop_table - delegate_unsafe_method_to_migration_base_class :rename_table - delegate_unsafe_method_to_migration_base_class :rename_column + delegate_unsafe_method_to_migration_base_class :add_foreign_key delegate_unsafe_method_to_migration_base_class :change_column delegate_unsafe_method_to_migration_base_class :change_column_default - delegate_unsafe_method_to_migration_base_class :remove_column + delegate_unsafe_method_to_migration_base_class :change_table + delegate_unsafe_method_to_migration_base_class :drop_table delegate_unsafe_method_to_migration_base_class :execute - delegate_unsafe_method_to_migration_base_class :remove_index - delegate_unsafe_method_to_migration_base_class :add_foreign_key - delegate_unsafe_method_to_migration_base_class :remove_foreign_key - delegate_unsafe_method_to_migration_base_class :add_check_constraint delegate_unsafe_method_to_migration_base_class :remove_check_constraint + delegate_unsafe_method_to_migration_base_class :remove_column + delegate_unsafe_method_to_migration_base_class :remove_foreign_key + delegate_unsafe_method_to_migration_base_class :remove_index + delegate_unsafe_method_to_migration_base_class :rename_column + delegate_unsafe_method_to_migration_base_class :rename_table - disable_or_delegate_default_method :create_table, ":create_table is NOT SAFE! Use safe_create_table instead" + # Direct dispatch to underlying Rails method as raw_ without dependent object check + delegate_raw_method_to_migration_base_class :add_check_constraint + delegate_raw_method_to_migration_base_class :add_column + delegate_raw_method_to_migration_base_class :add_foreign_key + delegate_raw_method_to_migration_base_class :add_index + delegate_raw_method_to_migration_base_class :change_column + delegate_raw_method_to_migration_base_class :change_column_default + delegate_raw_method_to_migration_base_class :change_column_null + delegate_raw_method_to_migration_base_class :change_table + delegate_raw_method_to_migration_base_class :create_table + delegate_raw_method_to_migration_base_class :drop_table + delegate_raw_method_to_migration_base_class :execute + delegate_raw_method_to_migration_base_class :remove_check_constraint + delegate_raw_method_to_migration_base_class :remove_column + delegate_raw_method_to_migration_base_class :remove_foreign_key + delegate_raw_method_to_migration_base_class :remove_index + delegate_raw_method_to_migration_base_class :rename_column + delegate_raw_method_to_migration_base_class :rename_table + + # Raises error if disable_default_migration_methods is true + # Otherwise, direct dispatch to underlying Rails method without dependent object check + disable_or_delegate_default_method :add_check_constraint, ":add_check_constraint is NOT SAFE! Use :safe_add_unvalidated_check_constraint and then :safe_validate_check_constraint instead" disable_or_delegate_default_method :add_column, ":add_column is NOT SAFE! Use safe_add_column instead" - disable_or_delegate_default_method :change_table, ":change_table is NOT SAFE! Use a combination of safe and explicit unsafe migration methods instead" - disable_or_delegate_default_method :drop_table, ":drop_table is NOT SAFE! Explicitly call :unsafe_drop_table to proceed" - disable_or_delegate_default_method :rename_table, ":rename_table is NOT SAFE! Explicitly call :unsafe_rename_table to proceed" - disable_or_delegate_default_method :rename_column, ":rename_column is NOT SAFE! Explicitly call :unsafe_rename_column to proceed" + disable_or_delegate_default_method :add_foreign_key, ":add_foreign_key is NOT SAFE! Explicitly call :unsafe_add_foreign_key" + disable_or_delegate_default_method :add_index, ":add_index is NOT SAFE! Use safe_add_concurrent_index instead" disable_or_delegate_default_method :change_column, ":change_column is NOT SAFE! Use a combination of safe and explicit unsafe migration methods instead" disable_or_delegate_default_method :change_column_default, ":change_column_default is NOT SAFE! Use safe_change_column_default instead" disable_or_delegate_default_method :change_column_null, ":change_column_null is NOT (guaranteed to be) SAFE! Either use :safe_make_column_nullable or explicitly call :unsafe_make_column_not_nullable to proceed" - disable_or_delegate_default_method :remove_column, ":remove_column is NOT SAFE! Explicitly call :unsafe_remove_column to proceed" - disable_or_delegate_default_method :add_index, ":add_index is NOT SAFE! Use safe_add_concurrent_index instead" + disable_or_delegate_default_method :change_table, ":change_table is NOT SAFE! Use a combination of safe and explicit unsafe migration methods instead" + disable_or_delegate_default_method :create_table, ":create_table is NOT SAFE! Use safe_create_table instead" + disable_or_delegate_default_method :drop_table, ":drop_table is NOT SAFE! Explicitly call :unsafe_drop_table to proceed" disable_or_delegate_default_method :execute, ":execute is NOT SAFE! Explicitly call :unsafe_execute to proceed", allow_reentry_from_compatibility_module: true - disable_or_delegate_default_method :remove_index, ":remove_index is NOT SAFE! Use safe_remove_concurrent_index instead for Postgres 9.6 databases; Explicitly call :unsafe_remove_index to proceed on Postgres 9.1" - disable_or_delegate_default_method :add_foreign_key, ":add_foreign_key is NOT SAFE! Explicitly call :unsafe_add_foreign_key" - disable_or_delegate_default_method :remove_foreign_key, ":remove_foreign_key is NOT SAFE! Explicitly call :unsafe_remove_foreign_key" - disable_or_delegate_default_method :add_check_constraint, ":add_check_constraint is NOT SAFE! Use :safe_add_unvalidated_check_constraint and then :safe_validate_check_constraint instead" disable_or_delegate_default_method :remove_check_constraint, ":remove_check_constraint is NOT SAFE! Explicitly call :unsafe_remove_check_constraint to proceed" + disable_or_delegate_default_method :remove_column, ":remove_column is NOT SAFE! Explicitly call :unsafe_remove_column to proceed" + disable_or_delegate_default_method :remove_foreign_key, ":remove_foreign_key is NOT SAFE! Explicitly call :unsafe_remove_foreign_key" + disable_or_delegate_default_method :remove_index, ":remove_index is NOT SAFE! Use safe_remove_concurrent_index instead for Postgres 9.6 databases; Explicitly call :unsafe_remove_index to proceed on Postgres 9.1" + disable_or_delegate_default_method :rename_column, ":rename_column is NOT SAFE! Explicitly call :unsafe_rename_column to proceed" + disable_or_delegate_default_method :rename_table, ":rename_table is NOT SAFE! Explicitly call :unsafe_rename_table to proceed" def unsafe_create_table(table, options={}, &block) if options[:force] && !PgHaMigrations.config.allow_force_create_table diff --git a/spec/dependent_object_checks_spec.rb b/spec/dependent_object_checks_spec.rb index a172376..9830f33 100644 --- a/spec/dependent_object_checks_spec.rb +++ b/spec/dependent_object_checks_spec.rb @@ -44,17 +44,11 @@ def up end end - [true, false].each do |disable_default_migration_methods| - describe "disable_default_migration_methods = #{disable_default_migration_methods}" do - method_prefix = disable_default_migration_methods ? "unsafe_" : "" - + ["", "raw_"].each do |method_prefix| + describe "#{method_prefix.empty? ? "standard rails" : "raw"} methods" do before(:each) do allow(PgHaMigrations.config).to receive(:disable_default_migration_methods) - .and_return(disable_default_migration_methods) - - # The default behavior until 2.0 is to disable this feature, but it's a lot easier - # to write most of these tests if we default to enabled here instead. - allow(PgHaMigrations.config).to receive(:check_for_dependent_objects).and_return(true) + .and_return(false) end describe "##{method_prefix}remove_column" do @@ -77,48 +71,101 @@ def up setup_migration.suppress_messages { setup_migration.migrate(:up) } end - it "raises when dependent indexes exist" do + it "does not raise and drops dependent indexes" do migration = Class.new(migration_klass) do def up public_send(remove_column_method_name, :foos3, :text_column) end end + indexes = ActiveRecord::Base.connection.indexes("foos3") + expect(indexes.size).to eq(1) + expect(indexes).to all(have_attributes(:table => "foos3", :name => "index_foos3_on_text_column", :columns => ["text_column"])) + expect do migration.suppress_messages { migration.migrate(:up) } - end.to raise_error(PgHaMigrations::UnsafeMigrationError, /index 'index_foos3_on_text_column' depends on column 'text_column'/) + end.to_not raise_error - # We can't drop the index _before_ raising the error :D + expect(ActiveRecord::Base.connection.columns("foos3").map(&:name)).not_to include("text_column") indexes = ActiveRecord::Base.connection.indexes("foos3") - expect(indexes.size).to eq(1) - expect(indexes).to all(have_attributes(:table => "foos3", :name => "index_foos3_on_text_column", :columns => ["text_column"])) + expect(indexes).to be_empty end + end + end + end - it "ignores dependent indexes when explicitly allowed" do - migration = Class.new(migration_klass) do - def up - public_send(remove_column_method_name, :foos3, :text_column, :allow_dependent_objects => [:indexes]) + describe "unsafe methods" do + describe "#unsafe_remove_column" do + before(:each) do + setup_migration = Class.new(migration_klass) do + def up + safe_create_table :foos3 do |t| + t.timestamps :null => false + t.text :text_column end + safe_add_concurrent_index :foos3, :text_column end + end - migration.suppress_messages { migration.migrate(:up) } + setup_migration.suppress_messages { setup_migration.migrate(:up) } + end - expect(ActiveRecord::Base.connection.columns("foos3").map(&:name)).not_to include("text_column") + it "raises when dependent indexes exist" do + migration = Class.new(migration_klass) do + def up + unsafe_remove_column :foos3, :text_column + end end - it "ignores dependent indexes when dependent object checks are disabled" do - allow(PgHaMigrations.config).to receive(:check_for_dependent_objects).and_return(false) + indexes = ActiveRecord::Base.connection.indexes("foos3") + expect(indexes.size).to eq(1) + expect(indexes).to all(have_attributes(:table => "foos3", :name => "index_foos3_on_text_column", :columns => ["text_column"])) - migration = Class.new(migration_klass) do - def up - public_send(remove_column_method_name, :foos3, :text_column) - end + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to raise_error(PgHaMigrations::UnsafeMigrationError, /index 'index_foos3_on_text_column' depends on column 'text_column'/) + + indexes = ActiveRecord::Base.connection.indexes("foos3") + expect(indexes.size).to eq(1) + expect(indexes).to all(have_attributes(:table => "foos3", :name => "index_foos3_on_text_column", :columns => ["text_column"])) + end + + it "ignores dependent indexes when explicitly allowed" do + migration = Class.new(migration_klass) do + def up + unsafe_remove_column :foos3, :text_column, :allow_dependent_objects => [:indexes] end + end - migration.suppress_messages { migration.migrate(:up) } + indexes = ActiveRecord::Base.connection.indexes("foos3") + expect(indexes.size).to eq(1) + expect(indexes).to all(have_attributes(:table => "foos3", :name => "index_foos3_on_text_column", :columns => ["text_column"])) - expect(ActiveRecord::Base.connection.columns("foos3").map(&:name)).not_to include("text_column") + migration.suppress_messages { migration.migrate(:up) } + + expect(ActiveRecord::Base.connection.columns("foos3").map(&:name)).not_to include("text_column") + indexes = ActiveRecord::Base.connection.indexes("foos3") + expect(indexes).to be_empty + end + + it "ignores dependent indexes when dependent object checks are disabled" do + allow(PgHaMigrations.config).to receive(:check_for_dependent_objects).and_return(false) + + migration = Class.new(migration_klass) do + def up + unsafe_remove_column :foos3, :text_column + end end + + indexes = ActiveRecord::Base.connection.indexes("foos3") + expect(indexes.size).to eq(1) + expect(indexes).to all(have_attributes(:table => "foos3", :name => "index_foos3_on_text_column", :columns => ["text_column"])) + + migration.suppress_messages { migration.migrate(:up) } + + expect(ActiveRecord::Base.connection.columns("foos3").map(&:name)).not_to include("text_column") + indexes = ActiveRecord::Base.connection.indexes("foos3") + expect(indexes).to be_empty end end end diff --git a/spec/pg_ha_migrations_spec.rb b/spec/pg_ha_migrations_spec.rb index 95f6c4d..6b2cf38 100644 --- a/spec/pg_ha_migrations_spec.rb +++ b/spec/pg_ha_migrations_spec.rb @@ -29,44 +29,44 @@ end context "check_for_dependent_objects" do - it "is set to false by default" do - expect(PgHaMigrations.config.check_for_dependent_objects).to be(false) + it "is set to true by default" do + expect(PgHaMigrations.config.check_for_dependent_objects).to be(true) end - it "can be overriden to true" do + it "can be overriden to false" do PgHaMigrations.configure do |config| - config.check_for_dependent_objects = true + config.check_for_dependent_objects = false end - expect(PgHaMigrations.config.check_for_dependent_objects).to be(true) + expect(PgHaMigrations.config.check_for_dependent_objects).to be(false) end end context "allow_force_create_table" do - it "is set to true by default" do - expect(PgHaMigrations.config.allow_force_create_table).to be(true) + it "is set to false by default" do + expect(PgHaMigrations.config.allow_force_create_table).to be(false) end - it "can be overriden to false" do + it "can be overriden to true" do PgHaMigrations.configure do |config| - config.allow_force_create_table = false + config.allow_force_create_table = true end - expect(PgHaMigrations.config.allow_force_create_table).to be(false) + expect(PgHaMigrations.config.allow_force_create_table).to be(true) end end context "prefer_single_step_column_addition_with_default" do - it "is set to false by default" do - expect(PgHaMigrations.config.prefer_single_step_column_addition_with_default).to be(false) + it "is set to true by default" do + expect(PgHaMigrations.config.prefer_single_step_column_addition_with_default).to be(true) end - it "can be overriden to true" do + it "can be overriden to false" do PgHaMigrations.configure do |config| - config.prefer_single_step_column_addition_with_default = true + config.prefer_single_step_column_addition_with_default = false end - expect(PgHaMigrations.config.prefer_single_step_column_addition_with_default).to be(true) + expect(PgHaMigrations.config.prefer_single_step_column_addition_with_default).to be(false) end end diff --git a/spec/safe_statements_spec.rb b/spec/safe_statements_spec.rb index 5500bd4..3ef972a 100644 --- a/spec/safe_statements_spec.rb +++ b/spec/safe_statements_spec.rb @@ -538,6 +538,169 @@ def up end end + context "raw methods" do + it "does not raise when using raw_create_table method" do + migration = Class.new(migration_klass) do + def up + raw_create_table :foos + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + + it "does not raise when using raw_add_column method" do + migration = Class.new(migration_klass) do + def up + safe_create_table :foos + raw_add_column :foos, :bar, :text + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + + it "does not raise when using raw_change_table method" do + migration = Class.new(migration_klass) do + def up + safe_create_table :foos + raw_change_table(:foos) { } + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + + it "does not raise when using raw_drop_table method" do + migration = Class.new(migration_klass) do + def up + safe_create_table :foos + raw_drop_table :foos + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + + it "does not raise when using raw_rename_table method" do + migration = Class.new(migration_klass) do + def up + safe_create_table :foos + raw_rename_table :foos, :bars + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + + it "does not raise when using raw_rename_column method" do + migration = Class.new(migration_klass) do + def up + safe_create_table :foos + safe_add_column :foos, :bar, :text + raw_rename_column :foos, :bar, :baz + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + + it "does not raise when using raw_change_column method" do + migration = Class.new(migration_klass) do + def up + safe_create_table :foos + safe_add_column :foos, :bar, :text + raw_change_column :foos, :bar, :string + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + + it "does not raise when using raw_change_column_null method" do + migration = Class.new(migration_klass) do + def up + safe_create_table :foos + safe_add_column :foos, :bar, :text + raw_change_column_null :foos, :bar, false + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + + it "does not raise when using raw_remove_column method" do + migration = Class.new(migration_klass) do + def up + safe_create_table :foos + safe_add_column :foos, :bar, :text + raw_remove_column :foos, :bar, :text + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + + it "does not raise when using raw_add_index method" do + migration = Class.new(migration_klass) do + def up + safe_create_table :foos + safe_add_column :foos, :bar, :text + raw_add_index :foos, :bar + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + + it "does not raise when using raw_add_foreign_key method" do + migration = Class.new(migration_klass) do + def up + safe_create_table :foos + safe_create_table :bars + safe_add_column :foos, :bar_id, :integer + raw_add_foreign_key :foos, :bars, :foreign_key => :bar_id + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + + it "does not raise when using raw_execute method" do + migration = Class.new(migration_klass) do + def up + raw_execute "SELECT current_date" + end + end + + expect do + migration.suppress_messages { migration.migrate(:up) } + end.to_not raise_error + end + end + describe "disabling `force: true`" do it "is allowed when config.allow_force_create_table = true" do allow(PgHaMigrations.config) @@ -1078,6 +1241,12 @@ def up end describe "when not configured to disallow two-step new column and adding default" do + before(:each) do + allow(PgHaMigrations.config) + .to receive(:prefer_single_step_column_addition_with_default) + .and_return(false) + end + it "allows setting a constant default value when the column was added in the same migration" do migration = Class.new(migration_klass) do define_method(:up) do @@ -1094,12 +1263,6 @@ def up end describe "when configured to disallow two-step new column and adding default" do - before(:each) do - allow(PgHaMigrations.config) - .to receive(:prefer_single_step_column_addition_with_default) - .and_return(true) - end - it "disallows setting a constant default value when the column was added in the same migration" do migration = Class.new(migration_klass) do define_method(:up) do