From 5f5efdd70cfd7606f1adc2a31cbc1b9a7878a5e3 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Wed, 22 Nov 2023 01:06:39 +0200 Subject: [PATCH] Drop support for ruby < 2.7 and rails < 6.1 --- .github/workflows/test.yml | 12 +- .rubocop.yml | 2 +- CHANGELOG.md | 1 + Gemfile.lock | 2 +- README.md | 6 +- gemfiles/activerecord_42.gemfile | 6 - gemfiles/activerecord_50.gemfile | 5 - gemfiles/activerecord_51.gemfile | 5 - gemfiles/activerecord_52.gemfile | 5 - gemfiles/activerecord_60.gemfile | 5 - .../online_migrations/install_generator.rb | 10 +- lib/online_migrations.rb | 5 - .../background_migrations/advisory_lock.rb | 2 +- .../background_migrations/backfill_column.rb | 2 +- .../background_migrations/copy_column.rb | 26 +- .../delete_orphaned_records.rb | 22 +- .../background_migrations/migration.rb | 14 +- .../migration_helpers.rb | 4 - .../background_migrations/migration_job.rb | 14 +- .../background_migrations/reset_counters.rb | 12 +- .../change_column_type_helpers.rb | 43 ++- lib/online_migrations/command_checker.rb | 40 +-- lib/online_migrations/config.rb | 10 +- lib/online_migrations/copy_trigger.rb | 6 +- lib/online_migrations/error_messages.rb | 22 +- .../foreign_key_definition.rb | 17 -- .../foreign_keys_collector.rb | 4 +- lib/online_migrations/indexes_collector.rb | 6 +- lib/online_migrations/schema_cache.rb | 6 - lib/online_migrations/schema_statements.rb | 282 +++--------------- lib/online_migrations/utils.rb | 52 +--- lib/online_migrations/verbose_sql_logs.rb | 4 +- online_migrations.gemspec | 4 +- .../delete_orphaned_records_test.rb | 14 +- .../migration_runner_test.rb | 2 +- test/background_migrations/migration_test.rb | 2 +- .../reset_counters_test.rb | 6 +- test/command_checker/add_column_test.rb | 28 +- test/command_checker/add_reference_test.rb | 28 +- test/command_checker/add_timestamps_test.rb | 4 +- .../change_column_default_test.rb | 16 +- .../change_column_null_test.rb | 24 +- test/command_checker/change_column_test.rb | 19 +- .../command_checker/check_constraints_test.rb | 6 +- test/command_checker/foreign_keys_test.rb | 42 +-- test/command_checker/indexes_test.rb | 13 +- .../inheritance_column_test.rb | 76 ++--- test/command_checker/misc_test.rb | 61 ++-- test/command_checker/removing_columns_test.rb | 74 ++--- test/command_checker/removing_tables_test.rb | 8 +- test/command_recorder_test.rb | 2 +- test/config_test.rb | 26 +- .../add_column_with_default_test.rb | 18 +- .../add_reference_concurrently_test.rb | 8 +- .../changing_column_type_test.rb | 40 +-- test/schema_statements/foreign_keys_test.rb | 99 +----- test/schema_statements/indexes_test.rb | 5 +- test/schema_statements/misc_test.rb | 2 - .../renaming_columns_test.rb | 11 +- test/schema_statements/renaming_table_test.rb | 12 +- .../update_column_in_batches_test.rb | 4 +- test/support/minitest_helpers.rb | 23 +- test/test_helper.rb | 4 +- 63 files changed, 308 insertions(+), 1025 deletions(-) delete mode 100644 gemfiles/activerecord_42.gemfile delete mode 100644 gemfiles/activerecord_50.gemfile delete mode 100644 gemfiles/activerecord_51.gemfile delete mode 100644 gemfiles/activerecord_52.gemfile delete mode 100644 gemfiles/activerecord_60.gemfile delete mode 100644 lib/online_migrations/foreign_key_definition.rb diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 176bbb9..6f598f4 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -33,17 +33,7 @@ jobs: strategy: matrix: include: - - ruby-version: 2.4 - gemfile: activerecord_42.gemfile - - ruby-version: 2.4 - gemfile: activerecord_50.gemfile - - ruby-version: 2.4 - gemfile: activerecord_51.gemfile - - ruby-version: 2.4 - gemfile: activerecord_52.gemfile - - ruby-version: 2.5 - gemfile: activerecord_60.gemfile - - ruby-version: 2.5 + - ruby-version: 2.7 gemfile: activerecord_61.gemfile - ruby-version: 2.7 gemfile: activerecord_70.gemfile diff --git a/.rubocop.yml b/.rubocop.yml index 8030f6d..1913c3f 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -3,7 +3,7 @@ require: - rubocop-disable_syntax AllCops: - TargetRubyVersion: 2.3 + TargetRubyVersion: 2.7 NewCops: enable SuggestExtensions: false diff --git a/CHANGELOG.md b/CHANGELOG.md index aed5ae7..5299987 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## master (unreleased) +- Drop support for Ruby < 2.7 and Rails < 6.1 - Fix `backfill_column_for_type_change_in_background` for cast expressions - Fix copying indexes with long names when changing column type - Enhance error messages with the link to the detailed description diff --git a/Gemfile.lock b/Gemfile.lock index c006cfd..2219eca 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -2,7 +2,7 @@ PATH remote: . specs: online_migrations (0.9.2) - activerecord (>= 4.2) + activerecord (>= 6.1) GEM remote: https://rubygems.org/ diff --git a/README.md b/README.md index fccc5c1..1ccc811 100644 --- a/README.md +++ b/README.md @@ -16,10 +16,12 @@ See [comparison to `strong_migrations`](#comparison-to-strong_migrations) ## Requirements -- Ruby 2.1+ -- Rails 4.2+ +- Ruby 2.7+ +- Rails 6.1+ - PostgreSQL 9.6+ +For older Ruby and Rails versions you can use '< 0.10' version of this gem. + **Note**: Since some migration helpers use database `VIEW`s to implement their logic, it is recommended to use `structure.sql` schema format, or otherwise add some gem (like [scenic](https://github.com/scenic-views/scenic)) to be able to dump them into the `schema.rb`. ## Installation diff --git a/gemfiles/activerecord_42.gemfile b/gemfiles/activerecord_42.gemfile deleted file mode 100644 index 3a60313..0000000 --- a/gemfiles/activerecord_42.gemfile +++ /dev/null @@ -1,6 +0,0 @@ -# frozen_string_literal: true - -@ar_gem_requirement = "~> 4.2.0" -@pg_gem_requirement = "< 1" - -eval_gemfile "../Gemfile" diff --git a/gemfiles/activerecord_50.gemfile b/gemfiles/activerecord_50.gemfile deleted file mode 100644 index 77c70e5..0000000 --- a/gemfiles/activerecord_50.gemfile +++ /dev/null @@ -1,5 +0,0 @@ -# frozen_string_literal: true - -@ar_gem_requirement = "~> 5.0.0" - -eval_gemfile "../Gemfile" diff --git a/gemfiles/activerecord_51.gemfile b/gemfiles/activerecord_51.gemfile deleted file mode 100644 index 0cbb161..0000000 --- a/gemfiles/activerecord_51.gemfile +++ /dev/null @@ -1,5 +0,0 @@ -# frozen_string_literal: true - -@ar_gem_requirement = "~> 5.1.0" - -eval_gemfile "../Gemfile" diff --git a/gemfiles/activerecord_52.gemfile b/gemfiles/activerecord_52.gemfile deleted file mode 100644 index 7701e99..0000000 --- a/gemfiles/activerecord_52.gemfile +++ /dev/null @@ -1,5 +0,0 @@ -# frozen_string_literal: true - -@ar_gem_requirement = "~> 5.2.0" - -eval_gemfile "../Gemfile" diff --git a/gemfiles/activerecord_60.gemfile b/gemfiles/activerecord_60.gemfile deleted file mode 100644 index bb878c8..0000000 --- a/gemfiles/activerecord_60.gemfile +++ /dev/null @@ -1,5 +0,0 @@ -# frozen_string_literal: true - -@ar_gem_requirement = "~> 6.0.0" - -eval_gemfile "../Gemfile" diff --git a/lib/generators/online_migrations/install_generator.rb b/lib/generators/online_migrations/install_generator.rb index 0bcf826..f6c3f22 100644 --- a/lib/generators/online_migrations/install_generator.rb +++ b/lib/generators/online_migrations/install_generator.rb @@ -15,20 +15,16 @@ def copy_initializer_file end def create_migration_file - migration_template("migration.rb", File.join(migrations_dir, "install_online_migrations.rb")) + migration_template("migration.rb", File.join(db_migrate_path, "install_online_migrations.rb")) end private def migration_parent - Utils.migration_parent_string + "ActiveRecord::Migration[#{Utils.ar_version}]" end def start_after - self.class.current_migration_number(migrations_dir) - end - - def migrations_dir - Utils.ar_version >= 5.1 ? db_migrate_path : "db/migrate" + self.class.current_migration_number(db_migrate_path) end end end diff --git a/lib/online_migrations.rb b/lib/online_migrations.rb index 25801fb..ff0c8f8 100644 --- a/lib/online_migrations.rb +++ b/lib/online_migrations.rb @@ -17,7 +17,6 @@ class UnsafeMigration < Error; end autoload :Migration autoload :Migrator autoload :DatabaseTasks - autoload :ForeignKeyDefinition autoload :ForeignKeysCollector autoload :IndexDefinition autoload :IndexesCollector @@ -90,10 +89,6 @@ def load else ActiveRecord::ConnectionAdapters::SchemaCache.prepend(OnlineMigrations::SchemaCache) end - - if OnlineMigrations::Utils.ar_version <= 5.1 - ActiveRecord::ConnectionAdapters::ForeignKeyDefinition.prepend(OnlineMigrations::ForeignKeyDefinition) - end end end end diff --git a/lib/online_migrations/background_migrations/advisory_lock.rb b/lib/online_migrations/background_migrations/advisory_lock.rb index afbaba0..25c5a8a 100644 --- a/lib/online_migrations/background_migrations/advisory_lock.rb +++ b/lib/online_migrations/background_migrations/advisory_lock.rb @@ -37,7 +37,7 @@ def active? objid = lock_key & 0xffffffff classid = (lock_key & (0xffffffff << 32)) >> 32 - active = connection.select_value(<<-SQL.strip_heredoc) + active = connection.select_value(<<~SQL) SELECT granted FROM pg_locks WHERE locktype = 'advisory' diff --git a/lib/online_migrations/background_migrations/backfill_column.rb b/lib/online_migrations/background_migrations/backfill_column.rb index 60411c7..298f0a9 100644 --- a/lib/online_migrations/background_migrations/backfill_column.rb +++ b/lib/online_migrations/background_migrations/backfill_column.rb @@ -22,7 +22,7 @@ def relation quoted_column = connection.quote_column_name(column) model.unscoped.where("#{quoted_column} != ? OR #{quoted_column} IS NULL", value) else - Utils.ar_where_not_multiple_conditions(model.unscoped, updates) + model.unscoped.where.not(updates) end end diff --git a/lib/online_migrations/background_migrations/copy_column.rb b/lib/online_migrations/background_migrations/copy_column.rb index ad75fac..7b332f1 100644 --- a/lib/online_migrations/background_migrations/copy_column.rb +++ b/lib/online_migrations/background_migrations/copy_column.rb @@ -25,14 +25,10 @@ def initialize(table_name, copy_from, copy_to, model_name = nil, type_cast_funct end def relation - relation = model + model .unscoped - .where(copy_to.map { |to_column| [to_column, nil] }.to_h) - - Utils.ar_where_not_multiple_conditions( - relation, - copy_from.map { |from_column| [from_column, nil] }.to_h - ) + .where(copy_to.index_with(nil)) + .where.not(copy_from.index_with(nil)) end def process_batch(relation) @@ -43,13 +39,8 @@ def process_batch(relation) old_value = arel_table[from_column] if (type_cast_function = type_cast_functions[from_column]) old_value = - if type_cast_function =~ /\A\w+\z/ - if Utils.ar_version <= 5.2 - # Active Record <= 5.2 does not support quoting of Arel::Nodes::NamedFunction - Arel.sql("#{type_cast_function}(#{connection.quote_column_name(from_column)})") - else - Arel::Nodes::NamedFunction.new(type_cast_function, [old_value]) - end + if type_cast_function.match?(/\A\w+\z/) + Arel::Nodes::NamedFunction.new(type_cast_function, [old_value]) else # We got a cast expression. Arel.sql(type_cast_function) @@ -58,12 +49,7 @@ def process_batch(relation) old_value end - if Utils.ar_version <= 4.2 - stmt = Arel::UpdateManager.new(arel.engine) - else - stmt = Arel::UpdateManager.new - end - + stmt = Arel::UpdateManager.new stmt.table(arel_table) stmt.wheres = arel.constraints diff --git a/lib/online_migrations/background_migrations/delete_orphaned_records.rb b/lib/online_migrations/background_migrations/delete_orphaned_records.rb index 665fbe8..2019acb 100644 --- a/lib/online_migrations/background_migrations/delete_orphaned_records.rb +++ b/lib/online_migrations/background_migrations/delete_orphaned_records.rb @@ -12,29 +12,11 @@ def initialize(model_name, associations, _options = {}) end def relation - # For Active Record 6.1+ we can use `where.missing` - # https://github.com/rails/rails/pull/34727 - associations.inject(model.unscoped) do |relation, association| - reflection = model.reflect_on_association(association) - if reflection.nil? - raise ArgumentError, "'#{model.name}' has no association called '#{association}'" - end - - # left_joins was added in Active Record 5.0 - https://github.com/rails/rails/pull/12071 - relation - .left_joins(association) - .where(reflection.table_name => { reflection.association_primary_key => nil }) - end + model.unscoped.where.missing(*associations) end def process_batch(relation) - if Utils.ar_version > 5.0 - relation.delete_all - else - # Older Active Record generates incorrect query when running delete_all - primary_key = model.primary_key - model.unscoped.where(primary_key => relation.select(primary_key)).delete_all - end + relation.delete_all end def count diff --git a/lib/online_migrations/background_migrations/migration.rb b/lib/online_migrations/background_migrations/migration.rb index c214ccc..f6ec7d4 100644 --- a/lib/online_migrations/background_migrations/migration.rb +++ b/lib/online_migrations/background_migrations/migration.rb @@ -21,7 +21,7 @@ class Migration < ActiveRecord::Base for_migration_name(migration_name).where("arguments = ?", arguments.to_json) end - enum status: STATUSES.map { |status| [status, status.to_s] }.to_h + enum status: STATUSES.index_with(&:to_s) has_many :migration_jobs @@ -138,16 +138,10 @@ def next_batch_range # rubocop:disable Lint/UnreachableLoop iterator.each_batch(of: batch_size, column: batch_column_name, start: next_min_value) do |relation| - if Utils.ar_version <= 4.2 - # Active Record <= 4.2 does not support pluck with Arel nodes - quoted_column = self.class.connection.quote_column_name(batch_column_name) - batch_range = relation.pluck("MIN(#{quoted_column}), MAX(#{quoted_column})").first - else - min = relation.arel_table[batch_column_name].minimum - max = relation.arel_table[batch_column_name].maximum + min = relation.arel_table[batch_column_name].minimum + max = relation.arel_table[batch_column_name].maximum + batch_range = relation.pick(min, max) - batch_range = relation.pluck(min, max).first - end break end # rubocop:enable Lint/UnreachableLoop diff --git a/lib/online_migrations/background_migrations/migration_helpers.rb b/lib/online_migrations/background_migrations/migration_helpers.rb index f702c65..05782f2 100644 --- a/lib/online_migrations/background_migrations/migration_helpers.rb +++ b/lib/online_migrations/background_migrations/migration_helpers.rb @@ -223,10 +223,6 @@ def reset_counters_in_background(model_name, *counters, touch: nil, **options) # For smaller tables it is probably better and easier to directly find and delete orpahed records. # def delete_orphaned_records_in_background(model_name, *associations, **options) - if Utils.ar_version <= 4.2 - raise "#{__method__} does not support Active Record <= 4.2 yet" - end - model_name = model_name.name if model_name.is_a?(Class) enqueue_background_migration( diff --git a/lib/online_migrations/background_migrations/migration_job.rb b/lib/online_migrations/background_migrations/migration_job.rb index 1b200db..c699648 100644 --- a/lib/online_migrations/background_migrations/migration_job.rb +++ b/lib/online_migrations/background_migrations/migration_job.rb @@ -12,9 +12,8 @@ class MigrationJob < ActiveRecord::Base self.table_name = :background_migration_jobs - # For Active Record <= 4.2 needs to fully specify enum values - scope :active, -> { where(status: [statuses[:enqueued], statuses[:running]]) } - scope :completed, -> { where(status: [statuses[:failed], statuses[:succeeded]]) } + scope :active, -> { where(status: [:enqueued, :running]) } + scope :completed, -> { where(status: [:failed, :succeeded]) } scope :stuck, -> do timeout = ::OnlineMigrations.config.background_migrations.stuck_jobs_timeout active.where("updated_at <= ?", timeout.ago) @@ -26,7 +25,7 @@ class MigrationJob < ActiveRecord::Base stuck_sql = connection.unprepared_statement { stuck.to_sql } failed_retriable_sql = connection.unprepared_statement { failed_retriable.to_sql } - from(Arel.sql(<<-SQL.strip_heredoc)) + from(Arel.sql(<<~SQL)) ( (#{failed_retriable_sql}) UNION @@ -35,19 +34,16 @@ class MigrationJob < ActiveRecord::Base SQL end - scope :except_succeeded, -> { where("status != ?", statuses[:succeeded]) } + scope :except_succeeded, -> { where.not(status: :succeeded) } scope :attempts_exceeded, -> { where("attempts >= max_attempts") } - enum status: STATUSES.map { |status| [status, status.to_s] }.to_h + enum status: STATUSES.index_with(&:to_s) delegate :migration_class, :migration_object, :migration_relation, :batch_column_name, :arguments, :batch_pause, to: :migration belongs_to :migration - # For Active Record 5.0+ this is validated by default from belongs_to - validates :migration, presence: true - validates :min_value, :max_value, presence: true, numericality: { greater_than: 0 } validate :values_in_migration_range, if: :min_value? validate :validate_values_order, if: :min_value? diff --git a/lib/online_migrations/background_migrations/reset_counters.rb b/lib/online_migrations/background_migrations/reset_counters.rb index 17929b4..e4ebc5c 100644 --- a/lib/online_migrations/background_migrations/reset_counters.rb +++ b/lib/online_migrations/background_migrations/reset_counters.rb @@ -26,7 +26,7 @@ def process_batch(relation) counter_name = reflection.counter_cache_column quoted_association_table = connection.quote_table_name(has_many_association.table_name) - count_subquery = <<-SQL.strip_heredoc + count_subquery = <<~SQL SELECT COUNT(*) FROM #{quoted_association_table} WHERE #{quoted_association_table}.#{connection.quote_column_name(foreign_key)} = @@ -41,8 +41,7 @@ def process_batch(relation) names = Array.wrap(names) options = names.extract_options! touch_updates = touch_attributes_with_time(*names, **options) - # In Active Record 4.2 sanitize_sql_for_assignment is protected - updates << model.send(:sanitize_sql_for_assignment, touch_updates) + updates << model.sanitize_sql_for_assignment(touch_updates) end relation.update_all(updates.join(", ")) @@ -64,11 +63,6 @@ def has_many_association(counter_association) # rubocop:disable Naming/Predicate has_many_association = has_many.find do |association| counter_cache_column = association.counter_cache_column - - # Active Record <= 4.2 is able to return only explicitly provided `counter_cache` column. - if !counter_cache_column && Utils.ar_version <= 4.2 - counter_cache_column = "#{association.name}_count" - end counter_cache_column && counter_cache_column.to_sym == counter_association.to_sym end @@ -86,7 +80,7 @@ def has_many_association(counter_association) # rubocop:disable Naming/Predicate def touch_attributes_with_time(*names, time: nil) attribute_names = timestamp_attributes_for_update & model.column_names attribute_names |= names.map(&:to_s) - attribute_names.map { |attribute_name| [attribute_name, time || Time.current] }.to_h + attribute_names.index_with(time || Time.current) end def timestamp_attributes_for_update diff --git a/lib/online_migrations/change_column_type_helpers.rb b/lib/online_migrations/change_column_type_helpers.rb index 95ee25f..a9c37b0 100644 --- a/lib/online_migrations/change_column_type_helpers.rb +++ b/lib/online_migrations/change_column_type_helpers.rb @@ -91,13 +91,13 @@ def initialize_column_type_change(table_name, column_name, new_type, **options) # @see #initialize_column_type_change # def initialize_columns_type_change(table_name, columns_and_types, **options) - if !columns_and_types.is_a?(Array) || !columns_and_types.all? { |e| e.is_a?(Array) } + if !columns_and_types.is_a?(Array) || !columns_and_types.all?(Array) raise ArgumentError, "columns_and_types must be an array of arrays" end - conversions = columns_and_types.map do |(column_name, _new_type)| + conversions = columns_and_types.to_h do |(column_name, _new_type)| [column_name, __change_type_column(column_name)] - end.to_h + end if (extra_keys = (options.keys - conversions.keys)).any? raise ArgumentError, "Options has unknown keys: #{extra_keys.map(&:inspect).join(', ')}. " \ @@ -106,7 +106,7 @@ def initialize_columns_type_change(table_name, columns_and_types, **options) transaction do columns_and_types.each do |(column_name, new_type)| - old_col = __column_for(table_name, column_name) + old_col = column_for(table_name, column_name) old_col_options = __options_from_column(old_col, [:collation, :comment]) column_options = options[column_name] || {} tmp_column_name = conversions[column_name] @@ -239,13 +239,13 @@ def finalize_column_type_change(table_name, column_name) def finalize_columns_type_change(table_name, *column_names) __ensure_not_in_transaction! - conversions = column_names.map do |column_name| + conversions = column_names.to_h do |column_name| [column_name.to_s, __change_type_column(column_name)] - end.to_h + end conversions.each do |column_name, tmp_column_name| - old_column = __column_for(table_name, column_name) - column = __column_for(table_name, tmp_column_name) + old_column = column_for(table_name, column_name) + column = column_for(table_name, tmp_column_name) # We already set default and NOT NULL for to-be-PK columns # for PG >= 11, so can skip this case @@ -302,9 +302,9 @@ def revert_finalize_column_type_change(table_name, column_name) def revert_finalize_columns_type_change(table_name, *column_names) __ensure_not_in_transaction! - conversions = column_names.map do |column_name| + conversions = column_names.to_h do |column_name| [column_name.to_s, __change_type_column(column_name)] - end.to_h + end transaction do conversions @@ -362,9 +362,9 @@ def cleanup_column_type_change(table_name, column_name) # @see #cleanup_column_type_change # def cleanup_columns_type_change(table_name, *column_names) - conversions = column_names.map do |column_name| - [column_name, __change_type_column(column_name)] - end.to_h + conversions = column_names.index_with do |column_name| + __change_type_column(column_name) + end transaction do __remove_copy_triggers(table_name, conversions.keys, conversions.values) @@ -419,8 +419,6 @@ def __copy_indexes(table_name, from_column, to_column) name = index.name.gsub(from_column, to_column) end - # Generate a shorter name if needed. - max_identifier_length = 63 # could use just `max_identifier_length` method for ActiveRecord >= 5.0. name = index_name(table_name, new_columns) if !name || name.length > max_identifier_length options = { @@ -433,8 +431,7 @@ def __copy_indexes(table_name, from_column, to_column) options[:using] = index.using if index.using options[:where] = index.where if index.where - # Opclasses were added in 5.2 - if Utils.ar_version >= 5.2 && !index.opclasses.blank? + if index.opclasses.present? opclasses = index.opclasses.dup # Copy the operator classes for the old column (if any) to the new column. @@ -491,7 +488,7 @@ def __set_not_null(table_name, column_name) # For PG >= 12 we can "promote" CHECK constraint to NOT NULL constraint: # https://github.com/postgres/postgres/commit/bbb96c3704c041d139181c6601e5bc770e045d26 if raw_connection.server_version >= 12_00_00 - execute(<<-SQL.strip_heredoc) + execute(<<~SQL) ALTER TABLE #{quote_table_name(table_name)} ALTER #{quote_column_name(column_name)} SET NOT NULL @@ -501,7 +498,7 @@ def __set_not_null(table_name, column_name) # through PG internal tables. # In-depth analysis of implications of this was made, so this approach # is considered safe - https://habr.com/ru/company/haulmont/blog/493954/ (in russian). - execute(<<-SQL.strip_heredoc) + execute(<<~SQL) UPDATE pg_catalog.pg_attribute SET attnotnull = true WHERE attrelid = #{quote(table_name)}::regclass @@ -517,7 +514,7 @@ def __check_constraints_for(table_name, column_name) def __check_constraints(table_name) schema = __schema_for_table(table_name) - check_sql = <<-SQL.strip_heredoc + select_all(<<~SQL) SELECT ccu.column_name as column_name, con.conname as constraint_name, @@ -535,12 +532,10 @@ def __check_constraints(table_name) AND con.contype = 'c' AND nsp.nspname = #{schema} SQL - - select_all(check_sql) end def __rename_constraint(table_name, old_name, new_name) - execute(<<-SQL.strip_heredoc) + execute(<<~SQL) ALTER TABLE #{quote_table_name(table_name)} RENAME CONSTRAINT #{quote_column_name(old_name)} TO #{quote_column_name(new_name)} SQL @@ -612,7 +607,7 @@ def __replace_referencing_foreign_keys(table_name, from_column, to_column) def __referencing_table_names(table_name) schema = __schema_for_table(table_name) - select_values(<<-SQL.strip_heredoc) + select_values(<<~SQL) SELECT DISTINCT con.conrelid::regclass::text AS conrelname FROM pg_catalog.pg_constraint con INNER JOIN pg_catalog.pg_namespace nsp diff --git a/lib/online_migrations/command_checker.rb b/lib/online_migrations/command_checker.rb index da9e7ad..33cfcae 100644 --- a/lib/online_migrations/command_checker.rb +++ b/lib/online_migrations/command_checker.rb @@ -154,13 +154,7 @@ def do_check(command, *args, **options, &block) check_columns_removal(command, *args, **options) else if respond_to?(command, true) - if options.any? - # Workaround for Active Record < 5 change_column_default - # not accepting options. - send(command, *args, **options, &block) - else - send(command, *args, &block) - end + send(command, *args, **options, &block) else # assume it is safe true @@ -362,9 +356,7 @@ def change_column(table_name, column_name, type, **options) precision = options[:precision] || options[:limit] || 6 existing_precision = existing_column.precision || existing_column.limit || 6 - # PostgreSQL interval data type was added in https://github.com/rails/rails/pull/16919 - (existing_type == :interval || (Utils.ar_version < 6.1 && existing_column.sql_type.start_with?("interval"))) && - precision >= existing_precision + existing_type == :interval && precision >= existing_precision when :inet existing_type == :cidr else @@ -487,7 +479,7 @@ def add_timestamps(table_name, **options) def add_reference(table_name, ref_name, **options) # Always added by default in 5.0+ - index = options.fetch(:index) { Utils.ar_version >= 5.0 } + index = options.fetch(:index, true) if index.is_a?(Hash) && index[:using].to_s == "hash" && postgresql_version < Gem::Version.new("10") raise_error :add_hash_index @@ -515,7 +507,7 @@ def add_reference(table_name, ref_name, **options) end if !options[:polymorphic] - type = (options[:type] || (Utils.ar_version >= 5.1 ? :bigint : :integer)).to_sym + type = (options[:type] || :bigint).to_sym column_name = "#{ref_name}_id" foreign_key_options = foreign_key.is_a?(Hash) ? foreign_key : {} @@ -568,9 +560,9 @@ def remove_index(table_name, column_name = nil, **options) end if index_def - existing_options = [:name, :columns, :unique, :where, :type, :using, :opclasses].map do |option| + existing_options = [:name, :columns, :unique, :where, :type, :using, :opclasses].filter_map do |option| [option, index_def.public_send(option)] if index_def.respond_to?(option) - end.compact.to_h + end.to_h @removed_indexes << IndexDefinition.new(table: table_name, **existing_options) end @@ -733,20 +725,10 @@ def raise_error(message_key, header: nil, **vars) template = OnlineMigrations.config.error_messages.fetch(message_key) vars[:migration_name] = @migration.name - vars[:migration_parent] = Utils.migration_parent_string - vars[:model_parent] = Utils.model_parent_string + vars[:migration_parent] = "ActiveRecord::Migration[#{Utils.ar_version}]" vars[:ar_version] = Utils.ar_version - if RUBY_VERSION >= "2.6" - message = ERB.new(template, trim_mode: "<>").result_with_hash(vars) - else - # `result_with_hash` was added in ruby 2.5 - b = TOPLEVEL_BINDING.dup - vars.each_pair do |key, value| - b.local_variable_set(key, value) - end - message = ERB.new(template, nil, "<>").result(b) - end + message = ERB.new(template, trim_mode: "<>").result_with_hash(vars) if (link = ERROR_MESSAGE_TO_LINK[message_key]) message += "\nFor more details, see https://github.com/fatkodima/online_migrations##{link}" @@ -785,7 +767,7 @@ def command_str(command, *args) end def crud_blocked? - locks_query = <<-SQL.strip_heredoc + locks_query = <<~SQL SELECT relation::regclass::text FROM pg_locks WHERE mode IN ('ShareLock', 'ShareRowExclusiveLock', 'ExclusiveLock', 'AccessExclusiveLock') @@ -803,7 +785,7 @@ def check_constraint_name(table_name, expression) end def check_constraints(table_name) - constraints_query = <<-SQL.strip_heredoc + constraints_query = <<~SQL SELECT pg_get_constraintdef(oid) AS def FROM pg_constraint WHERE contype = 'c' @@ -824,7 +806,7 @@ def check_inheritance_column(table_name, column_name, default) def check_mismatched_foreign_key_type(table_name, column_name, type, **options) column_name = column_name.to_s - ref_name = column_name.sub(/_id\z/, "") + ref_name = column_name.delete_suffix("_id") if like_foreign_key?(column_name, type) foreign_table_name = Utils.foreign_table_name(ref_name, options) diff --git a/lib/online_migrations/config.rb b/lib/online_migrations/config.rb index 924996d..8d904fb 100644 --- a/lib/online_migrations/config.rb +++ b/lib/online_migrations/config.rb @@ -16,7 +16,6 @@ class Config # def start_after=(value) if value.is_a?(Hash) - ensure_supports_multiple_dbs @start_after = value.stringify_keys else @start_after = value @@ -49,7 +48,6 @@ def start_after # def target_version=(value) if value.is_a?(Hash) - ensure_supports_multiple_dbs @target_version = value.stringify_keys else @target_version = value @@ -184,7 +182,7 @@ def initialize @target_version = nil @small_tables = [] @check_down = false - @enabled_checks = @error_messages.keys.map { |k| [k, {}] }.to_h + @enabled_checks = @error_messages.keys.index_with({}) @verbose_sql_logs = defined?(Rails.env) && Rails.env.production? end @@ -260,12 +258,6 @@ def add_check(start_after: nil, &block) end private - def ensure_supports_multiple_dbs - if !Utils.supports_multiple_dbs? - raise "OnlineMigrations does not support multiple databases for Active Record < 6.1" - end - end - def db_config_name connection = OnlineMigrations.current_migration.connection connection.pool.db_config.name diff --git a/lib/online_migrations/copy_trigger.rb b/lib/online_migrations/copy_trigger.rb index 5d1b335..d3db1b5 100644 --- a/lib/online_migrations/copy_trigger.rb +++ b/lib/online_migrations/copy_trigger.rb @@ -23,7 +23,7 @@ def create(from_columns, to_columns) trigger_name = name(from_columns, to_columns) assignment_clauses = assignment_clauses_for_columns(from_columns, to_columns) - connection.execute(<<-SQL.strip_heredoc) + connection.execute(<<~SQL) CREATE OR REPLACE FUNCTION #{trigger_name}() RETURNS TRIGGER AS $$ BEGIN #{assignment_clauses}; @@ -32,11 +32,11 @@ def create(from_columns, to_columns) $$ LANGUAGE plpgsql; SQL - connection.execute(<<-SQL.strip_heredoc) + connection.execute(<<~SQL) DROP TRIGGER IF EXISTS #{trigger_name} ON #{quoted_table_name} SQL - connection.execute(<<-SQL.strip_heredoc) + connection.execute(<<~SQL) CREATE TRIGGER #{trigger_name} BEFORE INSERT OR UPDATE ON #{quoted_table_name} diff --git a/lib/online_migrations/error_messages.rb b/lib/online_migrations/error_messages.rb index 55f8805..c8d6f20 100644 --- a/lib/online_migrations/error_messages.rb +++ b/lib/online_migrations/error_messages.rb @@ -109,14 +109,8 @@ def change 1. ignore the column: - class <%= model %> < <%= model_parent %> -<% if ar_version >= 5 %> + class <%= model %> < ApplicationRecord self.ignored_columns = [\"<%= column_name %>\"] -<% else %> - def self.columns - super.reject { |c| c.name == \"<%= column_name %>\" } - end -<% end %> end 2. deploy @@ -157,13 +151,7 @@ def change <% if enumerate_columns_in_select_statements %> 5. Ignore old column -<% if ar_version >= 5 %> self.ignored_columns = [:<%= column_name %>] -<% else %> - def self.columns - super.reject { |c| c.name == \"<%= column_name %>\" } - end -<% end %> 6. Deploy 7. Remove the column rename config from step 1 @@ -303,14 +291,8 @@ def change 1. Ignore the column(s): - class <%= model %> < <%= model_parent %> -<% if ar_version >= 5 %> + class <%= model %> < ApplicationRecord self.ignored_columns = <%= columns %> -<% else %> - def self.columns - super.reject { |c| <%= columns %>.include?(c.name) } - end -<% end %> end 2. Deploy diff --git a/lib/online_migrations/foreign_key_definition.rb b/lib/online_migrations/foreign_key_definition.rb deleted file mode 100644 index 7c1f01d..0000000 --- a/lib/online_migrations/foreign_key_definition.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -module OnlineMigrations - # @private - module ForeignKeyDefinition - if Utils.ar_version <= 4.2 - def defined_for?(to_table: nil, **options) - (to_table.nil? || to_table.to_s == self.to_table) && - options.all? { |k, v| self.options[k].to_s == v.to_s } - end - elsif Utils.ar_version <= 5.1 - def defined_for?(*args, **options) - super(*args, **options.except(:validate)) - end - end - end -end diff --git a/lib/online_migrations/foreign_keys_collector.rb b/lib/online_migrations/foreign_keys_collector.rb index 9287e72..6992be5 100644 --- a/lib/online_migrations/foreign_keys_collector.rb +++ b/lib/online_migrations/foreign_keys_collector.rb @@ -9,8 +9,8 @@ def initialize @referenced_tables = Set.new end - def collect(&table_definition) - table_definition.call(self) + def collect + yield self end def foreign_key(to_table, **_options) diff --git a/lib/online_migrations/indexes_collector.rb b/lib/online_migrations/indexes_collector.rb index becf49f..5259206 100644 --- a/lib/online_migrations/indexes_collector.rb +++ b/lib/online_migrations/indexes_collector.rb @@ -12,8 +12,8 @@ def initialize @indexes = [] end - def collect(&table_definition) - table_definition.call(self) + def collect + yield self end def index(_column_name, **options) @@ -21,7 +21,7 @@ def index(_column_name, **options) end def references(*_ref_names, **options) - index = options.fetch(:index) { Utils.ar_version >= 5.0 } + index = options.fetch(:index, true) if index using = index.is_a?(Hash) ? index[:using].to_s : nil diff --git a/lib/online_migrations/schema_cache.rb b/lib/online_migrations/schema_cache.rb index eb3b58e..54089c1 100644 --- a/lib/online_migrations/schema_cache.rb +++ b/lib/online_migrations/schema_cache.rb @@ -28,9 +28,6 @@ def columns(table_name) end def indexes(table_name) - # Available only in Active Record 6.0+ - return if !defined?(super) - if (renamed_table = renamed_table?(table_name)) super(renamed_table) elsif renamed_column?(table_name) @@ -109,9 +106,6 @@ def columns(connection, table_name) end def indexes(connection, table_name) - # Available only in Active Record 6.0+ - return if !defined?(super) - if (renamed_table = renamed_table?(connection, table_name)) super(connection, renamed_table) elsif renamed_column?(connection, table_name) diff --git a/lib/online_migrations/schema_statements.rb b/lib/online_migrations/schema_statements.rb index 8b49e69..5d9fd35 100644 --- a/lib/online_migrations/schema_statements.rb +++ b/lib/online_migrations/schema_statements.rb @@ -69,7 +69,7 @@ def update_columns_in_batches(table_name, columns_and_values, batch_size: 1000, batch_column_name: primary_key(table_name), progress: false, pause_ms: 50) __ensure_not_in_transaction! - if !columns_and_values.is_a?(Array) || !columns_and_values.all? { |e| e.is_a?(Array) } + if !columns_and_values.is_a?(Array) || !columns_and_values.all?(Array) raise ArgumentError, "columns_and_values must be an array of arrays" end @@ -83,11 +83,11 @@ def update_columns_in_batches(table_name, columns_and_values, model = Utils.define_model(table_name, self) - conditions = columns_and_values.map do |(column_name, value)| + conditions = columns_and_values.filter_map do |(column_name, value)| value = Arel.sql(value.call.to_s) if value.is_a?(Proc) # Ignore subqueries in conditions - if !value.is_a?(Arel::Nodes::SqlLiteral) || value.to_s !~ /select\s+/i + if !value.is_a?(Arel::Nodes::SqlLiteral) || !value.to_s.match?(/select\s+/i) arel_column = model.arel_table[column_name] if value.nil? arel_column.not_eq(nil) @@ -95,7 +95,7 @@ def update_columns_in_batches(table_name, columns_and_values, arel_column.not_eq(value).or(arel_column.eq(nil)) end end - end.compact + end batch_relation = model.where(conditions.inject(:or)) batch_relation = yield batch_relation if block_given? @@ -103,30 +103,9 @@ def update_columns_in_batches(table_name, columns_and_values, iterator = BatchIterator.new(batch_relation) iterator.each_batch(of: batch_size, column: batch_column_name) do |relation| updates = - if Utils.ar_version <= 5.2 - columns_and_values.map do |(column_name, value)| - rhs = - # Active Record <= 5.2 can't quote these - we need to handle these cases manually - case value - when Arel::Attributes::Attribute - quote_column_name(value.name) - when Arel::Nodes::SqlLiteral - value - when Arel::Nodes::NamedFunction - "#{value.name}(#{quote_column_name(value.expressions.first.name)})" - when Proc - value.call - else - quote(value) - end - - "#{quote_column_name(column_name)} = #{rhs}" - end.join(", ") - else - columns_and_values.map do |(column, value)| - value = Arel.sql(value.call.to_s) if value.is_a?(Proc) - [column, value] - end.to_h + columns_and_values.to_h do |(column, value)| + value = Arel.sql(value.call.to_s) if value.is_a?(Proc) + [column, value] end relation.update_all(updates) @@ -454,9 +433,6 @@ def swap_column_names(table_name, column1, column2) # def add_column_with_default(table_name, column_name, type, **options) default = options.fetch(:default) - if default.is_a?(Proc) && Utils.ar_version < 5.0 # https://github.com/rails/rails/pull/20005 - raise ArgumentError, "Expressions as default are not supported" - end if raw_connection.server_version >= 11_00_00 && !Utils.volatile_default?(self, type, default) add_column(table_name, column_name, type, **options) @@ -507,7 +483,8 @@ def add_column_with_default(table_name, column_name, type, **options) # add_not_null_constraint(:users, :email, validate: false) # def add_not_null_constraint(table_name, column_name, name: nil, validate: true) - if __column_not_nullable?(table_name, column_name) || + column = column_for(table_name, column_name) + if column.null == false || __not_null_constraint_exists?(table_name, column_name, name: name) Utils.say("NOT NULL constraint was not created: column #{table_name}.#{column_name} is already defined as `NOT NULL`") else @@ -577,7 +554,7 @@ def remove_not_null_constraint(table_name, column_name, name: nil) # @note This helper must be used only with text columns # def add_text_limit_constraint(table_name, column_name, limit, name: nil, validate: true) - column = __column_for(table_name, column_name) + column = column_for(table_name, column_name) if column.type != :text raise "add_text_limit_constraint must be used only with :text columns" end @@ -666,13 +643,13 @@ def add_reference_concurrently(table_name, ref_name, **options) column_name = "#{ref_name}_id" if !column_exists?(table_name, column_name) - type = options[:type] || (Utils.ar_version >= 5.1 ? :bigint : :integer) + type = options[:type] || :bigint allow_null = options.fetch(:null, true) add_column(table_name, column_name, type, null: allow_null) end # Always added by default in 5.0+ - index = options.fetch(:index) { Utils.ar_version >= 5.0 } + index = options.fetch(:index, true) if index index = {} if index == true @@ -708,7 +685,7 @@ def add_index(table_name, column_name, options = {}) __ensure_not_in_transaction! if algorithm == :concurrently - column_names = __index_column_names(column_name || options[:column]) + column_names = index_column_names(column_name || options[:column]) index_name = options[:name] index_name ||= index_name(table_name, column_names) @@ -734,7 +711,7 @@ def add_index(table_name, column_name, options = {}) end end - # Extends default method to be idempotent and accept `:algorithm` option for Active Record <= 4.2. + # Extends default method to be idempotent. # # @see https://edgeapi.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-remove_index # @@ -743,33 +720,15 @@ def remove_index(table_name, column_name = nil, **options) __ensure_not_in_transaction! if algorithm == :concurrently - column_names = __index_column_names(column_name || options[:column]) - index_name = options[:name] - index_name ||= index_name(table_name, column_names) - - index_exists = - if Utils.ar_version <= 5.0 - # Older Active Record is unable to handle blank columns correctly in `index_exists?`, - # so we need to use `index_name_exists?`. - index_name_exists?(table_name, index_name, nil) - elsif Utils.ar_version <= 6.0 - index_name_exists?(table_name, index_name) - else - index_exists?(table_name, column_names, **options) - end + column_names = index_column_names(column_name || options[:column]) - if index_exists + if index_exists?(table_name, column_names, **options) disable_statement_timeout do # "DROP INDEX CONCURRENTLY" requires a "SHARE UPDATE EXCLUSIVE" lock. # It only conflicts with constraint validations, other creating/removing indexes, # and some "ALTER TABLE"s. - # Active Record <= 4.2 does not support removing indexes concurrently - if Utils.ar_version <= 4.2 && algorithm == :concurrently - execute("DROP INDEX CONCURRENTLY #{quote_table_name(index_name)}") - else - super(table_name, **options.merge(column: column_names)) - end + super(table_name, **options.merge(column: column_names)) end else Utils.say("Index was not removed because it does not exist (this may be due to an aborted migration " \ @@ -793,132 +752,70 @@ def index_name(table_name, options) end end - # Extends default method to be idempotent and accept `:validate` option for Active Record < 5.2. + # Extends default method to be idempotent. # # @see https://edgeapi.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_foreign_key # - def add_foreign_key(from_table, to_table, validate: true, **options) + def add_foreign_key(from_table, to_table, **options) if foreign_key_exists?(from_table, to_table, **options) - message = "Foreign key was not created because it already exists " \ - "(this can be due to an aborted migration or similar): from_table: #{from_table}, to_table: #{to_table}".dup + message = +"Foreign key was not created because it already exists " \ + "(this can be due to an aborted migration or similar): from_table: #{from_table}, to_table: #{to_table}" message << ", #{options.inspect}" if options.any? Utils.say(message) else - # Active Record >= 5.2 supports adding non-validated foreign keys natively - options = options.dup - options[:column] ||= "#{to_table.to_s.singularize}_id" - options[:primary_key] ||= "id" - options[:name] ||= __foreign_key_name(to_table, options[:column]) - - query = <<-SQL.strip_heredoc.dup - ALTER TABLE #{quote_table_name(from_table)} - ADD CONSTRAINT #{quote_column_name(options[:name])} - FOREIGN KEY (#{quote_column_name(options[:column])}) - REFERENCES #{quote_table_name(to_table)} (#{quote_column_name(options[:primary_key])}) - SQL - query << "#{__action_sql('DELETE', options[:on_delete])}\n" if options[:on_delete].present? - query << "#{__action_sql('UPDATE', options[:on_update])}\n" if options[:on_update].present? - query << "NOT VALID\n" if !validate - if Utils.ar_version >= 7.0 && options[:deferrable] - query << " DEFERRABLE" - query << " INITIALLY #{options[:deferrable].to_s.upcase}\n" if options[:deferrable] != true - end - - execute(query.squish) + super end end # Extends default method with disabled statement timeout while validation is run # # @see https://edgeapi.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/PostgreSQL/SchemaStatements.html#method-i-validate_foreign_key - # @note This method was added in Active Record 5.2 # def validate_foreign_key(from_table, to_table = nil, **options) - fk_name_to_validate = __foreign_key_for!(from_table, to_table: to_table, **options).name + foreign_key = foreign_key_for!(from_table, to_table: to_table, **options) # Skip costly operation if already validated. - return if __constraint_validated?(from_table, fk_name_to_validate, type: :foreign_key) + return if foreign_key.validated? disable_statement_timeout do # "VALIDATE CONSTRAINT" requires a "SHARE UPDATE EXCLUSIVE" lock. # It only conflicts with other validations, creating/removing indexes, # and some other "ALTER TABLE"s. - execute("ALTER TABLE #{quote_table_name(from_table)} VALIDATE CONSTRAINT #{quote_column_name(fk_name_to_validate)}") + super end end - def foreign_key_exists?(from_table, to_table = nil, **options) - foreign_keys(from_table).any? { |fk| fk.defined_for?(to_table: to_table, **options) } - end - # Extends default method to be idempotent # # @see https://edgeapi.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_check_constraint - # @note This method was added in Active Record 6.1 # - def add_check_constraint(table_name, expression, validate: true, **options) - constraint_name = __check_constraint_name(table_name, expression: expression, **options) + def add_check_constraint(table_name, expression, **options) + constraint_name = check_constraint_name(table_name, expression: expression, **options) if __check_constraint_exists?(table_name, constraint_name) Utils.say("Check constraint was not created because it already exists (this may be due to an aborted migration " \ "or similar) table_name: #{table_name}, expression: #{expression}, constraint name: #{constraint_name}") else - query = <<-SQL.squish - ALTER TABLE #{quote_table_name(table_name)} - ADD CONSTRAINT #{quote_column_name(constraint_name)} CHECK (#{expression}) - SQL - query += " NOT VALID" if !validate - - execute(query) + super end end # Extends default method with disabled statement timeout while validation is run # # @see https://edgeapi.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/PostgreSQL/SchemaStatements.html#method-i-validate_check_constraint - # @note This method was added in Active Record 6.1 # def validate_check_constraint(table_name, **options) - constraint_name = __check_constraint_name!(table_name, **options) + check_constraint = check_constraint_for!(table_name, **options) # Skip costly operation if already validated. - return if __constraint_validated?(table_name, constraint_name, type: :check) + return if check_constraint.validated? disable_statement_timeout do # "VALIDATE CONSTRAINT" requires a "SHARE UPDATE EXCLUSIVE" lock. # It only conflicts with other validations, creating/removing indexes, # and some other "ALTER TABLE"s. - execute(<<-SQL.squish) - ALTER TABLE #{quote_table_name(table_name)} - VALIDATE CONSTRAINT #{quote_column_name(constraint_name)} - SQL - end - end - - if Utils.ar_version < 6.1 - # @see https://edgeapi.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-remove_check_constraint - # @note This method was added in Active Record 6.1 - # - def remove_check_constraint(table_name, expression = nil, **options) - constraint_name = __check_constraint_name!(table_name, expression: expression, **options) - execute(<<-SQL.squish) - ALTER TABLE #{quote_table_name(table_name)} - DROP CONSTRAINT #{quote_column_name(constraint_name)} - SQL - end - end - - if Utils.ar_version <= 4.2 - # @private - def views - select_values(<<-SQL, "SCHEMA") - SELECT c.relname - FROM pg_class c - LEFT JOIN pg_namespace n ON n.oid = c.relnamespace - WHERE c.relkind IN ('v','m') -- (v)iew, (m)aterialized view - AND n.nspname = ANY (current_schemas(false)) - SQL + super end end @@ -986,7 +883,7 @@ def with_lock_retries(&block) # Active Record methods def __ensure_not_in_transaction!(method_name = caller[0]) if transaction_open? - raise <<-MSG.strip_heredoc + raise <<~MSG `#{method_name}` cannot run inside a transaction block. You can remove transaction block by calling `disable_ddl_transaction!` in the body of @@ -995,31 +892,17 @@ def __ensure_not_in_transaction!(method_name = caller[0]) end end - def __column_not_nullable?(table_name, column_name) - schema = __schema_for_table(table_name) - - query = <<-SQL.strip_heredoc - SELECT is_nullable - FROM information_schema.columns - WHERE table_schema = #{schema} - AND table_name = #{quote(table_name)} - AND column_name = #{quote(column_name)} - SQL - - select_value(query) == "NO" - end - def __not_null_constraint_exists?(table_name, column_name, name: nil) name ||= __not_null_constraint_name(table_name, column_name) __check_constraint_exists?(table_name, name) end def __not_null_constraint_name(table_name, column_name) - __check_constraint_name(table_name, expression: "#{column_name}_not_null") + check_constraint_name(table_name, expression: "#{column_name}_not_null") end def __text_limit_constraint_name(table_name, column_name) - __check_constraint_name(table_name, expression: "#{column_name}_max_length") + check_constraint_name(table_name, expression: "#{column_name}_max_length") end def __text_limit_constraint_exists?(table_name, column_name, name: nil) @@ -1027,17 +910,9 @@ def __text_limit_constraint_exists?(table_name, column_name, name: nil) __check_constraint_exists?(table_name, name) end - def __index_column_names(column_names) - if column_names.is_a?(String) && /\W/.match(column_names) - column_names - elsif column_names.present? - Array(column_names) - end - end - + # Can use index validity attribute for Active Record >= 7.1. def __index_valid?(index_name, schema:) - # Active Record <= 4.2 returns a string, instead of automatically casting to boolean - valid = select_value <<-SQL.strip_heredoc + select_value(<<~SQL) SELECT indisvalid FROM pg_index i JOIN pg_class c @@ -1047,28 +922,6 @@ def __index_valid?(index_name, schema:) WHERE n.nspname = #{schema} AND c.relname = #{quote(index_name)} SQL - - Utils.to_bool(valid) - end - - def __column_for(table_name, column_name) - column_name = column_name.to_s - - columns(table_name).find { |c| c.name == column_name } || - raise("No such column: #{table_name}.#{column_name}") - end - - def __action_sql(action, dependency) - case dependency - when :nullify then "ON #{action} SET NULL" - when :cascade then "ON #{action} CASCADE" - when :restrict then "ON #{action} RESTRICT" - else - raise ArgumentError, <<-MSG.strip_heredoc - '#{dependency}' is not supported for :on_update or :on_delete. - Supported values are: :nullify, :cascade, :restrict - MSG - end end def __copy_foreign_key(fk, to_column, **options) @@ -1087,71 +940,16 @@ def __copy_foreign_key(fk, to_column, **options) **fkey_options ) - if !fk.respond_to?(:validated?) || fk.validated? + if fk.validated? validate_foreign_key(fk.from_table, fk.to_table, column: to_column, **options) end end - def __foreign_key_name(table_name, column_name) - identifier = "#{table_name}_#{column_name}_fk" - hashed_identifier = Digest::SHA256.hexdigest(identifier).first(10) - - "fk_rails_#{hashed_identifier}" - end - - if Utils.ar_version <= 4.2 - def foreign_key_for(from_table, **options) - foreign_keys(from_table).detect { |fk| fk.defined_for?(**options) } - end - end - - def __foreign_key_for!(from_table, **options) - foreign_key_for(from_table, **options) || - raise(ArgumentError, "Table '#{from_table}' has no foreign key for #{options[:to_table] || options}") - end - - def __constraint_validated?(table_name, name, type:) - schema = __schema_for_table(table_name) - contype = type == :check ? "c" : "f" - - validated = select_value(<<-SQL.strip_heredoc) - SELECT convalidated - FROM pg_catalog.pg_constraint con - INNER JOIN pg_catalog.pg_namespace nsp - ON nsp.oid = con.connamespace - WHERE con.conrelid = #{quote(table_name)}::regclass - AND con.conname = #{quote(name)} - AND con.contype = '#{contype}' - AND nsp.nspname = #{schema} - SQL - - Utils.to_bool(validated) - end - - def __check_constraint_name!(table_name, expression: nil, **options) - constraint_name = __check_constraint_name(table_name, expression: expression, **options) - - if __check_constraint_exists?(table_name, constraint_name) - constraint_name - else - raise(ArgumentError, "Table '#{table_name}' has no check constraint for #{expression || options}") - end - end - - def __check_constraint_name(table_name, **options) - options.fetch(:name) do - expression = options.fetch(:expression) - identifier = "#{table_name}_#{expression}_chk" - hashed_identifier = Digest::SHA256.hexdigest(identifier).first(10) - - "chk_rails_#{hashed_identifier}" - end - end - + # Can be replaced by native method in Active Record >= 7.1. def __check_constraint_exists?(table_name, constraint_name) schema = __schema_for_table(table_name) - check_sql = <<-SQL.strip_heredoc + check_sql = <<~SQL SELECT COUNT(*) FROM pg_catalog.pg_constraint con INNER JOIN pg_catalog.pg_class cl @@ -1179,7 +977,7 @@ def rename_table_create_view(table_name, old_new_column_hash) "#{quote_column_name(column_name)} AS #{quote_column_name(new_column_name)}" end.join(", ") - execute(<<-SQL.squish) + execute(<<~SQL.squish) CREATE VIEW #{quote_table_name(table_name)} AS SELECT *, #{column_mapping} FROM #{quote_table_name(tmp_table)} diff --git a/lib/online_migrations/utils.rb b/lib/online_migrations/utils.rb index 429b40a..64c401f 100644 --- a/lib/online_migrations/utils.rb +++ b/lib/online_migrations/utils.rb @@ -27,36 +27,6 @@ def warn(message) Kernel.warn("[online_migrations] #{message}") end - def supports_multiple_dbs? - # Technically, Active Record 6.0+ supports multiple databases, - # but we can not get the database spec name for this version. - ar_version >= 6.1 - end - - def migration_parent - if ar_version <= 4.2 - ActiveRecord::Migration - else - ActiveRecord::Migration[ar_version] - end - end - - def migration_parent_string - if ar_version <= 4.2 - "ActiveRecord::Migration" - else - "ActiveRecord::Migration[#{ar_version}]" - end - end - - def model_parent_string - if ar_version >= 5.0 - "ApplicationRecord" - else - "ActiveRecord::Base" - end - end - def define_model(table_name, connection = ActiveRecord::Base.connection) Class.new(ActiveRecord::Base) do self.table_name = table_name @@ -71,7 +41,7 @@ def self.connection end def to_bool(value) - !value.to_s.match(/^(true|t|yes|y|1|on)$/i).nil? + value.to_s.match?(/^true|t|yes|y|1|on$/i) end def foreign_table_name(ref_name, options) @@ -81,7 +51,7 @@ def foreign_table_name(ref_name, options) end # Implementation is from ActiveRecord. - # This is not needed for ActiveRecord < 7.1 (https://github.com/rails/rails/pull/47753). + # This is not needed for ActiveRecord >= 7.1 (https://github.com/rails/rails/pull/47753). def index_name(table_name, column_name) max_index_name_size = 62 name = "index_#{table_name}_on_#{Array(column_name) * '_and_'}" @@ -122,7 +92,7 @@ def ar_enumerate_columns_in_select_statements def estimated_count(connection, table_name) quoted_table = connection.quote(table_name) - count = connection.select_value(<<-SQL.strip_heredoc) + count = connection.select_value(<<~SQL) SELECT (reltuples / COALESCE(NULLIF(relpages, 0), 1)) * (pg_relation_size(#{quoted_table}) / (current_setting('block_size')::integer)) @@ -140,20 +110,6 @@ def estimated_count(connection, table_name) end end - def ar_where_not_multiple_conditions(relation, conditions) - if Utils.ar_version >= 6.1 - relation.where.not(conditions) - else - # In Active Record < 6.1, NOT with multiple conditions behaves as NOR, - # which should really behave as NAND. - # https://www.bigbinary.com/blog/rails-6-deprecates-where-not-working-as-nor-and-will-change-to-nand-in-rails-6-1 - arel_table = relation.arel_table - conditions = conditions.map { |column, value| arel_table[column].not_eq(value) } - conditions = conditions.inject(:or) - relation.where(conditions) - end - end - FUNCTION_CALL_RE = /(\w+)\s*\(/ private_constant :FUNCTION_CALL_RE @@ -167,7 +123,7 @@ def volatile_default?(connection, type, value) end def volatile_function?(connection, function_name) - query = <<-SQL.strip_heredoc + query = <<~SQL SELECT provolatile FROM pg_catalog.pg_proc WHERE proname = #{connection.quote(function_name)} diff --git a/lib/online_migrations/verbose_sql_logs.rb b/lib/online_migrations/verbose_sql_logs.rb index 3722dc9..e385e8b 100644 --- a/lib/online_migrations/verbose_sql_logs.rb +++ b/lib/online_migrations/verbose_sql_logs.rb @@ -34,7 +34,7 @@ def disable def verbose_query_logs if Utils.ar_version >= 7.0 ActiveRecord.verbose_query_logs - elsif Utils.ar_version >= 5.2 + else ActiveRecord::Base.verbose_query_logs end end @@ -42,7 +42,7 @@ def verbose_query_logs def set_verbose_query_logs(value) # rubocop:disable Naming/AccessorMethodName if Utils.ar_version >= 7.0 ActiveRecord.verbose_query_logs = value - elsif Utils.ar_version >= 5.2 + else ActiveRecord::Base.verbose_query_logs = value end end diff --git a/online_migrations.gemspec b/online_migrations.gemspec index 5209e2c..2d87b16 100644 --- a/online_migrations.gemspec +++ b/online_migrations.gemspec @@ -11,7 +11,7 @@ Gem::Specification.new do |spec| spec.summary = "Catch unsafe PostgreSQL migrations in development and run them easier in production" spec.homepage = "https://github.com/fatkodima/online_migrations" spec.license = "MIT" - spec.required_ruby_version = Gem::Requirement.new(">= 2.1.0") + spec.required_ruby_version = Gem::Requirement.new(">= 2.7") spec.metadata["homepage_uri"] = spec.homepage spec.metadata["source_code_uri"] = spec.homepage @@ -20,5 +20,5 @@ Gem::Specification.new do |spec| spec.files = Dir["**/*.{md,txt}", "{lib}/**/*"] spec.require_paths = ["lib"] - spec.add_dependency "activerecord", ">= 4.2" + spec.add_dependency "activerecord", ">= 6.1" end diff --git a/test/background_migrations/delete_orphaned_records_test.rb b/test/background_migrations/delete_orphaned_records_test.rb index 1ee7298..fda5e61 100644 --- a/test/background_migrations/delete_orphaned_records_test.rb +++ b/test/background_migrations/delete_orphaned_records_test.rb @@ -12,9 +12,7 @@ class User < ActiveRecord::Base class Post < ActiveRecord::Base default_scope { where(archived: false) } - optional_setting = OnlineMigrations::Utils.ar_version > 4.2 ? { optional: true } : { required: false } - - belongs_to :author, class_name: User.name, **optional_setting + belongs_to :author, class_name: User.name, optional: true has_many :comments end @@ -53,8 +51,6 @@ def teardown end def test_relation - skip if ar_version <= 4.2 - migration = OnlineMigrations::BackgroundMigrations::DeleteOrphanedRecords.new(Post.name, [:author]) assert_kind_of ActiveRecord::Relation, migration.relation @@ -62,8 +58,6 @@ def test_relation end def test_process_batch - skip if ar_version <= 4.2 - migration = OnlineMigrations::BackgroundMigrations::DeleteOrphanedRecords.new(Post.name, [:author]) migration.process_batch(migration.relation) @@ -75,14 +69,12 @@ def test_process_batch def test_raises_for_unknown_association migration = OnlineMigrations::BackgroundMigrations::DeleteOrphanedRecords.new(Post.name, [:non_existent]) - assert_raises_with_message(ArgumentError, "'#{Post.name}' has no association called 'non_existent'") do + assert_raises_with_message(ArgumentError, "An association named `:non_existent` does not exist on the model `#{Post.name}`") do migration.process_batch(migration.relation) end end def test_multiple_associations - skip if ar_version <= 4.2 - @post1.comments.create! migration = OnlineMigrations::BackgroundMigrations::DeleteOrphanedRecords.new(Post.name, [:author, :comments]) @@ -94,8 +86,6 @@ def test_multiple_associations end def test_count - skip if ar_version <= 4.2 - migration = OnlineMigrations::BackgroundMigrations::DeleteOrphanedRecords.new(Post.name, [:author]) assert_kind_of Integer, migration.count end diff --git a/test/background_migrations/migration_runner_test.rb b/test/background_migrations/migration_runner_test.rb index 8616549..f944547 100644 --- a/test/background_migrations/migration_runner_test.rb +++ b/test/background_migrations/migration_runner_test.rb @@ -220,7 +220,7 @@ def migration_runner(migration) end def assert_no_admins - refute User.exists?(admin: [nil, true]) + assert_not User.exists?(admin: [nil, true]) end end end diff --git a/test/background_migrations/migration_test.rb b/test/background_migrations/migration_test.rb index 98ad47d..aaab67a 100644 --- a/test/background_migrations/migration_test.rb +++ b/test/background_migrations/migration_test.rb @@ -73,7 +73,7 @@ def test_migration_relation_with_order_clause m.valid? errors = m.errors.full_messages - assert(errors.any? { |error| error =~ /relation cannot use ORDER BY or LIMIT/ }) + assert(errors.any? { |error| error.include?("relation cannot use ORDER BY or LIMIT") }) end def test_status_transitions diff --git a/test/background_migrations/reset_counters_test.rb b/test/background_migrations/reset_counters_test.rb index bcf921f..75a9c5a 100644 --- a/test/background_migrations/reset_counters_test.rb +++ b/test/background_migrations/reset_counters_test.rb @@ -118,7 +118,7 @@ def test_touches_parent migration = OnlineMigrations::BackgroundMigrations::ResetCounters.new(User.name, [:projects], { touch: true }) migration.process_batch(migration.relation) - refute_equal prev_updated_at, @user1.reload.updated_at + assert_not_equal prev_updated_at, @user1.reload.updated_at end def test_touches_specific_parent_column @@ -130,8 +130,8 @@ def test_touches_specific_parent_column @user1.reload - refute_equal prev_timestamp, @user1.updated_at - refute_equal prev_timestamp, @user1.touched_at + assert_not_equal prev_timestamp, @user1.updated_at + assert_not_equal prev_timestamp, @user1.touched_at end def test_touches_with_concrete_time diff --git a/test/command_checker/add_column_test.rb b/test/command_checker/add_column_test.rb index f00ddbe..dad3483 100644 --- a/test/command_checker/add_column_test.rb +++ b/test/command_checker/add_column_test.rb @@ -29,7 +29,7 @@ def test_add_column_default def test_add_column_default_before_11 with_target_version(10) do - assert_unsafe AddColumnDefault, <<-MSG.strip_heredoc + assert_unsafe AddColumnDefault, <<~MSG Adding a column with a non-null default blocks reads and writes while the entire table is rewritten. A safer approach is to: @@ -39,7 +39,7 @@ def test_add_column_default_before_11 add_column_with_default takes care of all this steps: - class CommandChecker::AddColumnTest::AddColumnDefault < #{migration_parent_string} + class CommandChecker::AddColumnTest::AddColumnDefault < #{migration_parent} disable_ddl_transaction! def change @@ -58,11 +58,11 @@ def change def test_add_column_default_null_before_11 with_target_version(10) do - assert_unsafe AddColumnDefaultNull, <<-MSG.strip_heredoc + assert_unsafe AddColumnDefaultNull, <<~MSG Adding a column with a null default blocks reads and writes while the entire table is rewritten. Instead, add the column without a default value. - class CommandChecker::AddColumnTest::AddColumnDefaultNull < #{migration_parent_string} + class CommandChecker::AddColumnTest::AddColumnDefaultNull < #{migration_parent} def change add_column :users, :admin, :boolean end @@ -156,15 +156,9 @@ def test_add_column_no_default end class AddColumnDefaultSafe < TestMigration - def up + def change add_column :users, :admin, :boolean - # Active Record <= 4.2 does not support reversible change_column_default, - # so needs to split to up/down methods - change_column_default :users, :admin, false - end - - def down - remove_column :users, :admin + change_column_default :users, :admin, from: nil, to: false end end @@ -190,11 +184,11 @@ def change end def test_add_column_json - assert_unsafe AddColumnJson, <<-MSG.strip_heredoc + assert_unsafe AddColumnJson, <<~MSG There's no equality operator for the json column type, which can cause errors for existing SELECT DISTINCT queries in your application. Use jsonb instead. - class CommandChecker::AddColumnTest::AddColumnJson < #{migration_parent_string} + class CommandChecker::AddColumnTest::AddColumnJson < #{migration_parent} def change add_column :projects, :settings, :jsonb end @@ -219,11 +213,11 @@ def change end def test_add_column_with_default_json - assert_unsafe AddColumnWithDefaultJson, <<-MSG.strip_heredoc + assert_unsafe AddColumnWithDefaultJson, <<~MSG There's no equality operator for the json column type, which can cause errors for existing SELECT DISTINCT queries in your application. Use jsonb instead. - class CommandChecker::AddColumnTest::AddColumnWithDefaultJson < #{migration_parent_string} + class CommandChecker::AddColumnTest::AddColumnWithDefaultJson < #{migration_parent} def change add_column_with_default :projects, :settings, :jsonb, default: {} end @@ -248,7 +242,7 @@ def change end def test_generated_stored - assert_unsafe AddColumnGeneratedStored, <<-MSG.strip_heredoc + assert_unsafe AddColumnGeneratedStored, <<~MSG Adding a stored generated column blocks reads and writes while the entire table is rewritten. Add a non-generated column and use callbacks or triggers instead. MSG diff --git a/test/command_checker/add_reference_test.rb b/test/command_checker/add_reference_test.rb index 292d3c3..c0d2597 100644 --- a/test/command_checker/add_reference_test.rb +++ b/test/command_checker/add_reference_test.rb @@ -22,22 +22,18 @@ def change end def test_add_reference_default - if ar_version >= 5.0 - assert_unsafe AddReferenceDefault, <<-MSG.strip_heredoc - Adding an index non-concurrently blocks writes. - Instead, use add_reference_concurrently helper. It will create a reference and take care of safely adding index. + assert_unsafe AddReferenceDefault, <<~MSG + Adding an index non-concurrently blocks writes. + Instead, use add_reference_concurrently helper. It will create a reference and take care of safely adding index. - class CommandChecker::AddReferenceTest::AddReferenceDefault < #{migration_parent_string} - disable_ddl_transaction! + class CommandChecker::AddReferenceTest::AddReferenceDefault < #{migration_parent} + disable_ddl_transaction! - def change - add_reference_concurrently :projects, :user - end + def change + add_reference_concurrently :projects, :user end - MSG - else - assert_safe AddReferenceDefault - end + end + MSG end class AddReferenceIndex < TestMigration @@ -79,11 +75,11 @@ def change end def test_add_reference_foreign_key - assert_unsafe AddReferenceForeignKey, <<-MSG.strip_heredoc + assert_unsafe AddReferenceForeignKey, <<~MSG Adding a foreign key blocks writes on both tables. Instead, use add_reference_concurrently helper. It will create a reference and take care of safely adding a foreign key. - class CommandChecker::AddReferenceTest::AddReferenceForeignKey < #{migration_parent_string} + class CommandChecker::AddReferenceTest::AddReferenceForeignKey < #{migration_parent} disable_ddl_transaction! def change @@ -120,7 +116,7 @@ def change end def test_add_reference_index_and_foreign_key - assert_unsafe AddReferenceIndexAndForeignKey, <<-MSG.strip_heredoc + assert_unsafe AddReferenceIndexAndForeignKey, <<~MSG Adding a foreign key blocks writes on both tables. Adding an index non-concurrently blocks writes. Instead, use add_reference_concurrently helper. It will create a reference and take care of safely adding a foreign key and index. diff --git a/test/command_checker/add_timestamps_test.rb b/test/command_checker/add_timestamps_test.rb index 7b9bbe6..17819ba 100644 --- a/test/command_checker/add_timestamps_test.rb +++ b/test/command_checker/add_timestamps_test.rb @@ -27,7 +27,7 @@ def test_add_timestamps_default def test_add_timestamps_default_before_11 with_target_version(10) do - assert_unsafe AddTimestampsDefault, <<-MSG.strip_heredoc + assert_unsafe AddTimestampsDefault, <<~MSG Adding timestamps columns with non-null defaults blocks reads and writes while the entire table is rewritten. A safer approach is to, for both timestamps columns: @@ -38,7 +38,7 @@ def test_add_timestamps_default_before_11 add_column_with_default takes care of all this steps: - class CommandChecker::AddTimestampsTest::AddTimestampsDefault < #{migration_parent_string} + class CommandChecker::AddTimestampsTest::AddTimestampsDefault < #{migration_parent} disable_ddl_transaction! def change diff --git a/test/command_checker/change_column_default_test.rb b/test/command_checker/change_column_default_test.rb index 000f9f4..b762a33 100644 --- a/test/command_checker/change_column_default_test.rb +++ b/test/command_checker/change_column_default_test.rb @@ -16,21 +16,15 @@ def teardown end class ChangeColumnDefault < TestMigration - def up - change_column_default :users, :name, "Test" - end - - def down - # For Active Record < 5, this change_column_default - # is not automatically reversible. - change_column_default :users, :name, nil + def change + change_column_default :users, :name, from: nil, to: "Test" end end def test_with_partial_writes with_partial_writes(true) do if ar_version >= 7 - assert_unsafe ChangeColumnDefault, <<-MSG.strip_heredoc + assert_unsafe ChangeColumnDefault, <<~MSG Partial writes are enabled, which can cause incorrect values to be inserted when changing the default value of a column. Disable partial writes in config/application.rb: @@ -38,7 +32,7 @@ def test_with_partial_writes config.active_record.partial_inserts = false MSG else - assert_unsafe ChangeColumnDefault, <<-MSG.strip_heredoc + assert_unsafe ChangeColumnDefault, <<~MSG config.active_record.partial_writes = false MSG end @@ -52,8 +46,6 @@ def change end def test_with_partial_writes_hash - skip("Active Record < 5 does not support :from :to") if ar_version < 5 - with_partial_writes(true) do assert_unsafe ChangeColumnDefaultHash end diff --git a/test/command_checker/change_column_null_test.rb b/test/command_checker/change_column_null_test.rb index d6825db..87768de 100644 --- a/test/command_checker/change_column_null_test.rb +++ b/test/command_checker/change_column_null_test.rb @@ -33,12 +33,12 @@ def change def test_change_column_null_to_disallow_before_12 with_target_version(11) do - assert_unsafe ChangeColumnNullToFalse, <<-MSG.strip_heredoc + assert_unsafe ChangeColumnNullToFalse, <<~MSG Setting NOT NULL on an existing column blocks reads and writes while every row is checked. A safer approach is to add a NOT NULL check constraint and validate it in a separate transaction. add_not_null_constraint and validate_not_null_constraint take care of that. - class CommandChecker::ChangeColumnNullTest::ChangeColumnNullToFalse < #{migration_parent_string} + class CommandChecker::ChangeColumnNullTest::ChangeColumnNullToFalse < #{migration_parent} disable_ddl_transaction! def change @@ -90,17 +90,12 @@ def test_change_column_null_new_table end class ChangeColumnNullConstraint < TestMigration - def up + def change safety_assured do - execute "ALTER TABLE users ADD CONSTRAINT name_check CHECK (name IS NOT NULL)" + add_not_null_constraint(:users, :name) end change_column_null :users, :name, false end - - def down - execute "ALTER TABLE users DROP CONSTRAINT name_check" - change_column_null :users, :name, true - end end def test_change_column_null_constraint @@ -116,17 +111,10 @@ def test_change_column_null_constraint_before_12 end class ChangeColumnNullConstraintUnvalidated < TestMigration - def up - safety_assured do - execute "ALTER TABLE users ADD CONSTRAINT name_check CHECK (name IS NOT NULL) NOT VALID" - end + def change + add_not_null_constraint(:users, :name, validate: false) change_column_null :users, :name, false end - - def down - execute "ALTER TABLE users DROP CONSTRAINT name_check" - change_column_null :users, :name, true - end end def test_change_column_null_constraint_unvalidated diff --git a/test/command_checker/change_column_test.rb b/test/command_checker/change_column_test.rb index b5fba61..3e80254 100644 --- a/test/command_checker/change_column_test.rb +++ b/test/command_checker/change_column_test.rb @@ -31,13 +31,13 @@ def down; end end def test_change_column_type - assert_unsafe ChangeColumnType, <<-MSG.strip_heredoc + assert_unsafe ChangeColumnType, <<~MSG Changing the type of an existing column blocks reads and writes while the entire table is rewritten. A safer approach can be accomplished in several steps: 1. Create a new column and keep column's data in sync: - class InitializeCommandChecker::ChangeColumnTest::ChangeColumnType < #{migration_parent_string} + class InitializeCommandChecker::ChangeColumnTest::ChangeColumnType < #{migration_parent} def change initialize_column_type_change :files, :cost_per_gb, :integer end @@ -48,7 +48,7 @@ def change 2. Backfill data from the old column to the new column: - class BackfillCommandChecker::ChangeColumnTest::ChangeColumnType < #{migration_parent_string} + class BackfillCommandChecker::ChangeColumnTest::ChangeColumnType < #{migration_parent} disable_ddl_transaction! def up @@ -62,7 +62,7 @@ def down 3. Copy indexes, foreign keys, check constraints, NOT NULL constraint, swap new column in place: - class FinalizeCommandChecker::ChangeColumnTest::ChangeColumnType < #{migration_parent_string} + class FinalizeCommandChecker::ChangeColumnTest::ChangeColumnType < #{migration_parent} disable_ddl_transaction! def change @@ -73,7 +73,7 @@ def change 4. Deploy 5. Finally, if everything is working as expected, remove copy trigger and old column: - class CleanupCommandChecker::ChangeColumnTest::ChangeColumnType < #{migration_parent_string} + class CleanupCommandChecker::ChangeColumnTest::ChangeColumnType < #{migration_parent} def up cleanup_column_type_change :files, :cost_per_gb end @@ -499,12 +499,7 @@ def test_timestamptz_decrease_limit class IntervalIncreasePrecision < TestMigration def up - if OnlineMigrations::Utils.ar_version >= 6.1 - add_column :files, :delete_after, :interval, precision: 0 - else - # precision is ignored for add_column and interval in Active Record < 6.1 - safety_assured { execute('ALTER TABLE "files" ADD COLUMN "delete_after" interval(0)') } - end + add_column :files, :delete_after, :interval, precision: 0 change_column :files, :delete_after, :interval, precision: 3 change_column :files, :delete_after, :interval, precision: 6 @@ -710,7 +705,7 @@ def down; end end def test_add_not_null - assert_unsafe AddNotNull, <<-MSG.strip_heredoc + assert_unsafe AddNotNull, <<~MSG Changing the type is safe, but setting NOT NULL is not. MSG end diff --git a/test/command_checker/check_constraints_test.rb b/test/command_checker/check_constraints_test.rb index c93db39..7f7f721 100644 --- a/test/command_checker/check_constraints_test.rb +++ b/test/command_checker/check_constraints_test.rb @@ -29,12 +29,12 @@ def change end def test_add_check_constraint - assert_unsafe AddCheckConstraint, <<-MSG.strip_heredoc + assert_unsafe AddCheckConstraint, <<~MSG Adding a check constraint blocks reads and writes while every row is checked. A safer approach is to add the check constraint without validating existing rows, and then validating them in a separate transaction. - class CommandChecker::CheckConstraintsTest::AddCheckConstraint < #{migration_parent_string} + class CommandChecker::CheckConstraintsTest::AddCheckConstraint < #{migration_parent} disable_ddl_transaction! def change @@ -96,7 +96,7 @@ def change end def test_add_check_constraint_validate_same_transaction - assert_unsafe AddCheckConstraintValidateSameTransaction, <<-MSG.strip_heredoc + assert_unsafe AddCheckConstraintValidateSameTransaction, <<~MSG Validating a constraint while holding heavy locks on tables is dangerous. Use disable_ddl_transaction! or a separate migration. MSG diff --git a/test/command_checker/foreign_keys_test.rb b/test/command_checker/foreign_keys_test.rb index 07a1e77..3eaad6a 100644 --- a/test/command_checker/foreign_keys_test.rb +++ b/test/command_checker/foreign_keys_test.rb @@ -31,11 +31,11 @@ def change end def test_add_foreign_key - assert_unsafe AddForeignKey, <<-MSG.strip_heredoc + assert_unsafe AddForeignKey, <<~MSG Adding a foreign key blocks writes on both tables. Add the foreign key without validating existing rows, and then validate them in a separate transaction. - class CommandChecker::ForeignKeysTest::AddForeignKey < #{migration_parent_string} + class CommandChecker::ForeignKeysTest::AddForeignKey < #{migration_parent} disable_ddl_transaction! def change @@ -87,7 +87,7 @@ def change end def test_add_foreign_key_validate_same_transaction - assert_unsafe AddForeignKeyValidateSameTransaction, <<-MSG.strip_heredoc + assert_unsafe AddForeignKeyValidateSameTransaction, <<~MSG Validating a foreign key while holding heavy locks on tables is dangerous. Use disable_ddl_transaction! or a separate migration. MSG @@ -192,14 +192,10 @@ def change end def test_add_reference_column - if ar_version >= 5.1 - assert_unsafe AddReferenceColumn, <<-MSG.strip_heredoc - projects.repository_id references a column of different type - foreign keys should be of the same type as the referenced primary key. - Otherwise, there's a risk of errors caused by IDs representable by one type but not the other. - MSG - else - assert_safe AddReferenceColumn - end + assert_unsafe AddReferenceColumn, <<~MSG + projects.repository_id references a column of different type - foreign keys should be of the same type as the referenced primary key. + Otherwise, there's a risk of errors caused by IDs representable by one type but not the other. + MSG end class AddReferenceColumnWithDefault < TestMigration @@ -209,14 +205,10 @@ def change end def test_add_reference_column_with_default - if ar_version >= 5.1 - assert_unsafe AddReferenceColumnWithDefault, <<-MSG.strip_heredoc - projects.repository_id references a column of different type - foreign keys should be of the same type as the referenced primary key. - Otherwise, there's a risk of errors caused by IDs representable by one type but not the other. - MSG - else - assert_safe AddReferenceColumnWithDefault - end + assert_unsafe AddReferenceColumnWithDefault, <<~MSG + projects.repository_id references a column of different type - foreign keys should be of the same type as the referenced primary key. + Otherwise, there's a risk of errors caused by IDs representable by one type but not the other. + MSG end class AddReferenceColumnNonExistentTable < TestMigration @@ -236,11 +228,7 @@ def change end def test_add_reference_column_integer_with_limit - if ar_version >= 5.1 - assert_safe AddReferenceColumnIntegerWithLimit - else - assert_unsafe AddReferenceColumnIntegerWithLimit - end + assert_safe AddReferenceColumnIntegerWithLimit end class AddReference < TestMigration @@ -250,11 +238,7 @@ def change end def test_add_reference - if ar_version >= 5.1 - assert_unsafe AddReference, "projects.repository_id references a column of different type" - else - assert_safe AddReference - end + assert_unsafe AddReference, "projects.repository_id references a column of different type" end class AddReferencePolymorphic < TestMigration diff --git a/test/command_checker/indexes_test.rb b/test/command_checker/indexes_test.rb index de3cb04..88b7321 100644 --- a/test/command_checker/indexes_test.rb +++ b/test/command_checker/indexes_test.rb @@ -30,10 +30,10 @@ def change end def test_add_index_non_concurrently - assert_unsafe AddIndexNonConcurrently, <<-MSG.strip_heredoc + assert_unsafe AddIndexNonConcurrently, <<~MSG Adding an index non-concurrently blocks writes. Instead, use: - class CommandChecker::IndexesTest::AddIndexNonConcurrently < #{migration_parent_string} + class CommandChecker::IndexesTest::AddIndexNonConcurrently < #{migration_parent} disable_ddl_transaction! def change @@ -174,10 +174,10 @@ def change end def test_remove_index_non_concurrently - assert_unsafe RemoveIndexNonConcurrently, <<-MSG.strip_heredoc + assert_unsafe RemoveIndexNonConcurrently, <<~MSG Removing an index non-concurrently blocks writes. Instead, use: - class CommandChecker::IndexesTest::RemoveIndexNonConcurrently < #{migration_parent_string} + class CommandChecker::IndexesTest::RemoveIndexNonConcurrently < #{migration_parent} disable_ddl_transaction! def change @@ -192,8 +192,7 @@ class RemoveIndexConcurrently < TestMigration def change add_index :users, :email, algorithm: :concurrently - # For Active Record <= 4.2 need to specify a :column to be reversible - remove_index :users, column: :email, algorithm: :concurrently + remove_index :users, :email, algorithm: :concurrently end end @@ -227,7 +226,7 @@ def change def test_replace_index @connection.add_index(:projects, :creator_id) - assert_unsafe ReplaceIndex, <<-MSG.strip_heredoc + assert_unsafe ReplaceIndex, <<~MSG Removing an old index before replacing it with the new one might result in slow queries while building the new index. A safer approach is to create the new index and then delete the old one. MSG diff --git a/test/command_checker/inheritance_column_test.rb b/test/command_checker/inheritance_column_test.rb index fb2c2be..60ee840 100644 --- a/test/command_checker/inheritance_column_test.rb +++ b/test/command_checker/inheritance_column_test.rb @@ -20,37 +20,25 @@ def change end def test_add_column - if ar_version >= 5 - assert_unsafe AddColumn, <<-MSG.strip_heredoc - 'type' column is used for single table inheritance. Adding it might cause errors in old instances of your application. - - After the migration was ran and the column was added, but before the code is fully deployed to all instances, - an old instance may be restarted (due to an error etc). And when it will fetch 'User' records from the database, - 'User' will look for a 'Member' subclass (from the 'type' column) and fail to locate it unless it is already defined. - - A safer approach is to: - - 1. ignore the column: - - class User < #{model_parent_string} - self.ignored_columns = ["type"] - end - - 2. deploy - 3. remove the column ignoring from step 1 and apply initial code changes - 4. deploy - MSG - else - assert_unsafe AddColumn, <<-MSG.strip_heredoc - 1. ignore the column: - - class User < #{model_parent_string} - def self.columns - super.reject { |c| c.name == "type" } - end - end - MSG - end + assert_unsafe AddColumn, <<~MSG + 'type' column is used for single table inheritance. Adding it might cause errors in old instances of your application. + + After the migration was ran and the column was added, but before the code is fully deployed to all instances, + an old instance may be restarted (due to an error etc). And when it will fetch 'User' records from the database, + 'User' will look for a 'Member' subclass (from the 'type' column) and fail to locate it unless it is already defined. + + A safer approach is to: + + 1. ignore the column: + + class User < ApplicationRecord + self.ignored_columns = ["type"] + end + + 2. deploy + 3. remove the column ignoring from step 1 and apply initial code changes + 4. deploy + MSG end class AddColumnWithDefaultHelper < TestMigration @@ -98,25 +86,13 @@ def test_custom_inheritance_column prev = ActiveRecord::Base.inheritance_column ActiveRecord::Base.inheritance_column = "my_type_column" - if ar_version >= 5 - assert_unsafe CustomInheritanceColumn, <<-MSG.strip_heredoc - 1. ignore the column: - - class User < #{model_parent_string} - self.ignored_columns = ["my_type_column"] - end - MSG - else - assert_unsafe CustomInheritanceColumn, <<-MSG.strip_heredoc - 1. ignore the column: - - class User < #{model_parent_string} - def self.columns - super.reject { |c| c.name == "my_type_column" } - end - end - MSG - end + assert_unsafe CustomInheritanceColumn, <<~MSG + 1. ignore the column: + + class User < ApplicationRecord + self.ignored_columns = ["my_type_column"] + end + MSG ensure ActiveRecord::Base.inheritance_column = prev end diff --git a/test/command_checker/misc_test.rb b/test/command_checker/misc_test.rb index 65de959..d345f86 100644 --- a/test/command_checker/misc_test.rb +++ b/test/command_checker/misc_test.rb @@ -77,7 +77,7 @@ def change def test_integer_primary_key OnlineMigrations.config.enable_check(:short_primary_key_type) - assert_unsafe IntegerPrimaryKey, <<-MSG.strip_heredoc + assert_unsafe IntegerPrimaryKey, <<~MSG Using short integer types for primary keys is dangerous due to the risk of running out of IDs on inserts. Better to use one of 'bigint', 'bigserial' or 'uuid'. MSG @@ -92,7 +92,7 @@ def change end def test_rename_table - assert_unsafe RenameTable, <<-MSG.strip_heredoc + assert_unsafe RenameTable, <<~MSG Renaming a table that's in use will cause errors in your application. migration_helpers provides a safer approach to do this: @@ -107,7 +107,7 @@ def test_rename_table nor any data/indexes/foreign keys copying will be made, so will be very fast. It will use a VIEW to work with both table names simultaneously: - class InitializeCommandChecker::MiscTest::RenameTable < #{migration_parent_string} + class InitializeCommandChecker::MiscTest::RenameTable < #{migration_parent} def change initialize_table_rename :clients, :users end @@ -118,7 +118,7 @@ def change 6. Deploy 7. Remove the VIEW created on step 3: - class FinalizeCommandChecker::MiscTest::RenameTable < #{migration_parent_string} + class FinalizeCommandChecker::MiscTest::RenameTable < #{migration_parent} def change finalize_table_rename :clients, :users end @@ -158,7 +158,7 @@ def change end def test_rename_column - assert_unsafe RenameColumn, <<-MSG.strip_heredoc + assert_unsafe RenameColumn, <<~MSG Renaming a column that's in use will cause errors in your application. migration_helpers provides a safer approach to do this: @@ -175,7 +175,7 @@ def test_rename_column nor any data/indexes/foreign keys copying will be made, so will be instantaneous. It will use a combination of a VIEW and column aliasing to work with both column names simultaneously: - class InitializeCommandChecker::MiscTest::RenameColumn < #{migration_parent_string} + class InitializeCommandChecker::MiscTest::RenameColumn < #{migration_parent} def change initialize_column_rename :users, :name, :first_name end @@ -186,7 +186,7 @@ def change 6. Remove the column rename config from step 1 7. Remove the VIEW created in step 3 and finally rename the column: - class FinalizeCommandChecker::MiscTest::RenameColumn < #{migration_parent_string} + class FinalizeCommandChecker::MiscTest::RenameColumn < #{migration_parent} def change finalize_column_rename :users, :name, :first_name end @@ -198,7 +198,7 @@ def change def test_rename_column_without_partial_writes with_partial_writes(false) do - assert_unsafe RenameColumn, <<-MSG.strip_heredoc + assert_unsafe RenameColumn, <<~MSG 1. Instruct Rails that you are going to rename a column: OnlineMigrations.config.column_renames = { @@ -222,27 +222,17 @@ def test_rename_column_with_enumerate_columns_in_select_statements previous = ActiveRecord::Base.enumerate_columns_in_select_statements ActiveRecord::Base.enumerate_columns_in_select_statements = true - if ar_version >= 5 - assert_unsafe RenameColumn, <<-MSG.strip_heredoc - 5. Ignore old column + assert_unsafe RenameColumn, <<~MSG + 5. Ignore old column - self.ignored_columns = [:name] - MSG - else - assert_unsafe RenameColumn, <<-MSG.strip_heredoc - 5. Ignore old column + self.ignored_columns = [:name] - super.reject { |c| c.name == "name" } - MSG - end - - assert_unsafe RenameColumn, <<-MSG.strip_heredoc 6. Deploy 7. Remove the column rename config from step 1 8. Remove the column ignore from step 5 9. Remove the VIEW created in step 3 and finally rename the column: - class FinalizeCommandChecker::MiscTest::RenameColumn < #{migration_parent_string} + class FinalizeCommandChecker::MiscTest::RenameColumn < #{migration_parent} def change finalize_column_rename :users, :name, :first_name end @@ -289,7 +279,6 @@ def change end def test_validate_constraint_no_transaction - skip if ar_version < 5.2 assert_safe ValidateConstraintNoTransaction end @@ -300,7 +289,7 @@ def change end def test_execute_query - assert_unsafe ExecuteQuery, <<-MSG.strip_heredoc + assert_unsafe ExecuteQuery, <<~MSG Online Migrations does not support inspecting what happens inside an execute call, so cannot help you here. Make really sure that what you're doing is safe before proceeding, then wrap it in a safety_assured { ... } block. @@ -338,11 +327,11 @@ def change def test_add_unique_constraint skip if ar_version < 7.1 - assert_unsafe AddUniqueConstraint, <<-MSG.strip_heredoc + assert_unsafe AddUniqueConstraint, <<~MSG Adding a unique constraint blocks reads and writes while the underlying index is being built. A safer approach is to create a unique index first, and then create a unique constraint using that index. - class CommandChecker::MiscTest::AddUniqueConstraintAddIndex < #{migration_parent_string} + class CommandChecker::MiscTest::AddUniqueConstraintAddIndex < #{migration_parent} disable_ddl_transaction! def change @@ -350,7 +339,7 @@ def change end end - class CommandChecker::MiscTest::AddUniqueConstraint < #{migration_parent_string} + class CommandChecker::MiscTest::AddUniqueConstraint < #{migration_parent} def up add_unique_constraint :users, name: "unique_email", using_index: "index_users_on_email" end @@ -386,18 +375,18 @@ def change end def test_add_not_null_constraint - assert_unsafe AddNotNullConstraint, <<-MSG.strip_heredoc + assert_unsafe AddNotNullConstraint, <<~MSG Adding a NOT NULL constraint blocks reads and writes while every row is checked. A safer approach is to add the NOT NULL check constraint without validating existing rows, and then validating them in a separate migration. - class CommandChecker::MiscTest::AddNotNullConstraint < #{migration_parent_string} + class CommandChecker::MiscTest::AddNotNullConstraint < #{migration_parent} def change add_not_null_constraint :projects, :user_id, validate: false end end - class CommandChecker::MiscTest::AddNotNullConstraintValidate < #{migration_parent_string} + class CommandChecker::MiscTest::AddNotNullConstraintValidate < #{migration_parent} def change validate_not_null_constraint :projects, :user_id end @@ -423,7 +412,7 @@ def change end def test_validate_not_null_constraint - assert_unsafe ValidateNotNullConstraint, <<-MSG.strip_heredoc + assert_unsafe ValidateNotNullConstraint, <<~MSG Validating a constraint while holding heavy locks on tables is dangerous. Use disable_ddl_transaction! or a separate migration. MSG @@ -449,18 +438,18 @@ def change end def test_add_text_limit_constraint - assert_unsafe AddTextLimitConstraint, <<-MSG.strip_heredoc + assert_unsafe AddTextLimitConstraint, <<~MSG Adding a limit on the text column blocks reads and writes while every row is checked. A safer approach is to add the limit check constraint without validating existing rows, and then validating them in a separate migration. - class CommandChecker::MiscTest::AddTextLimitConstraint < #{migration_parent_string} + class CommandChecker::MiscTest::AddTextLimitConstraint < #{migration_parent} def change add_text_limit_constraint :projects, :description, 255, validate: false end end - class CommandChecker::MiscTest::AddTextLimitConstraintValidate < #{migration_parent_string} + class CommandChecker::MiscTest::AddTextLimitConstraintValidate < #{migration_parent} def change validate_text_limit_constraint :projects, :description end @@ -486,7 +475,7 @@ def change end def test_validate_text_limit_constraint - assert_unsafe ValidateTextLimitConstraint, <<-MSG.strip_heredoc + assert_unsafe ValidateTextLimitConstraint, <<~MSG Validating a constraint while holding heavy locks on tables is dangerous. Use disable_ddl_transaction! or a separate migration. MSG @@ -558,7 +547,7 @@ def test_revert end def test_prints_more_details_link - assert_unsafe RenameColumn, <<-MSG.strip_heredoc + assert_unsafe RenameColumn, <<~MSG 8. Deploy For more details, see https://github.com/fatkodima/online_migrations#renaming-a-column diff --git a/test/command_checker/removing_columns_test.rb b/test/command_checker/removing_columns_test.rb index 01c561f..6954f69 100644 --- a/test/command_checker/removing_columns_test.rb +++ b/test/command_checker/removing_columns_test.rb @@ -31,40 +31,28 @@ def change end def test_remove_column - if ar_version >= 5 - assert_unsafe RemoveColumn, <<-MSG.strip_heredoc - Active Record caches database columns at runtime, so if you drop a column, it can cause exceptions until your app reboots. - A safer approach is to: + assert_unsafe RemoveColumn, <<~MSG + Active Record caches database columns at runtime, so if you drop a column, it can cause exceptions until your app reboots. + A safer approach is to: - 1. Ignore the column(s): + 1. Ignore the column(s): - class User < ApplicationRecord - self.ignored_columns = ["email"] - end + class User < ApplicationRecord + self.ignored_columns = ["email"] + end - 2. Deploy - 3. Wrap column removing in a safety_assured { ... } block + 2. Deploy + 3. Wrap column removing in a safety_assured { ... } block - class CommandChecker::RemovingColumnsTest::RemoveColumn < #{migration_parent_string} - def change - safety_assured { remove_column :users, :email, :string, null: false } - end + class CommandChecker::RemovingColumnsTest::RemoveColumn < #{migration_parent} + def change + safety_assured { remove_column :users, :email, :string, null: false } end + end - 4. Remove columns ignoring - 5. Deploy - MSG - else - assert_unsafe RemoveColumn, <<-MSG.strip_heredoc - 1. Ignore the column(s): - - class User < ActiveRecord::Base - def self.columns - super.reject { |c| ["email"].include?(c.name) } - end - end - MSG - end + 4. Remove columns ignoring + 5. Deploy + MSG end class RemoveColumnNewTable < TestMigration @@ -88,11 +76,11 @@ def change end def test_remove_column_with_index - assert_unsafe RemoveColumnWithIndex, <<-MSG.strip_heredoc + assert_unsafe RemoveColumnWithIndex, <<~MSG Removing a column will automatically remove all of the indexes that involved the removed column. But the indexes would be removed non-concurrently, so you need to safely remove the indexes first: - class CommandChecker::RemovingColumnsTest::RemoveColumnWithIndexRemoveIndexes < #{migration_parent_string} + class CommandChecker::RemovingColumnsTest::RemoveColumnWithIndexRemoveIndexes < #{migration_parent} disable_ddl_transaction! def change @@ -128,8 +116,6 @@ def change end def test_remove_column_with_expression_index - skip("Active Record < 5 does not support expression indexes") if ar_version < 5 - assert_unsafe RemoveColumnWithExpressionIndex, "remove_index :users, name: :index_users_on_lower_email, algorithm: :concurrently" end @@ -149,11 +135,7 @@ def change end def test_remove_columns - if ar_version >= 5 - assert_unsafe RemoveColumns, 'self.ignored_columns = ["name", "email"]' - else - assert_unsafe RemoveColumns, 'super.reject { |c| ["name", "email"].include?(c.name) }' - end + assert_unsafe RemoveColumns, 'self.ignored_columns = ["name", "email"]' end class RemoveColumnsNewTable < TestMigration @@ -192,11 +174,7 @@ def change end def test_remove_timestamps - if ar_version >= 5 - assert_unsafe RemoveTimestamps, 'self.ignored_columns = ["created_at", "updated_at"]' - else - assert_unsafe RemoveTimestamps, 'super.reject { |c| ["created_at", "updated_at"].include?(c.name) }' - end + assert_unsafe RemoveTimestamps, 'self.ignored_columns = ["created_at", "updated_at"]' end class RemoveTimestampsNewTable < TestMigration @@ -231,11 +209,7 @@ def change end def test_remove_reference - if ar_version >= 5 - assert_unsafe RemoveReference, 'self.ignored_columns = ["user_id"]' - else - assert_unsafe RemoveReference, 'super.reject { |c| ["user_id"].include?(c.name) }' - end + assert_unsafe RemoveReference, 'self.ignored_columns = ["user_id"]' end class RemovePolymorphicReference < TestMigration @@ -245,11 +219,7 @@ def change end def test_remove_polymorphic_reference - if ar_version >= 5 - assert_unsafe RemovePolymorphicReference, 'self.ignored_columns = ["attachable_id", "attachable_type"]' - else - assert_unsafe RemovePolymorphicReference, 'super.reject { |c| ["attachable_id", "attachable_type"].include?(c.name) }' - end + assert_unsafe RemovePolymorphicReference, 'self.ignored_columns = ["attachable_id", "attachable_type"]' end class RemoveReferenceNewTable < TestMigration diff --git a/test/command_checker/removing_tables_test.rb b/test/command_checker/removing_tables_test.rb index 8800788..08baf68 100644 --- a/test/command_checker/removing_tables_test.rb +++ b/test/command_checker/removing_tables_test.rb @@ -57,7 +57,7 @@ def change end def test_drop_table_multiple_foreign_keys - assert_unsafe DropProjects, <<-MSG.strip_heredoc + assert_unsafe DropProjects, <<~MSG Dropping a table with multiple foreign keys blocks reads and writes on all involved tables until migration is completed. Remove all the foreign keys first. MSG @@ -74,11 +74,7 @@ def test_drop_table_single_foreign_key end def test_drop_table_self_referencing_foreign_key - # This is not working in Active Record 4.2 - add_reference ignores :to_table option - # @connection.add_reference :repositories, :forked_repository, foreign_key: { to_table: :repositories } - - @connection.add_column :repositories, :forked_repository_id, :integer - @connection.add_foreign_key :repositories, :repositories, column: :forked_repository_id + @connection.add_reference :repositories, :forked_repository, foreign_key: { to_table: :repositories } assert_safe DropRepositories end diff --git a/test/command_recorder_test.rb b/test/command_recorder_test.rb index 693be53..07f7e5c 100644 --- a/test/command_recorder_test.rb +++ b/test/command_recorder_test.rb @@ -120,7 +120,7 @@ def test_finalize_table_rename @connection.initialize_table_rename(:users, :clients) migrate(CleanupRenameTableConcurrently, direction: :up) - refute_includes @connection.views, "users" + assert_not_includes @connection.views, "users" migrate(CleanupRenameTableConcurrently, direction: :down) assert_includes @connection.views, "users" diff --git a/test/config_test.rb b/test/config_test.rb index 2f0fac5..ed135ce 100644 --- a/test/config_test.rb +++ b/test/config_test.rb @@ -77,17 +77,7 @@ def test_start_after_revert_unsafe end end - def test_start_after_multiple_dbs_below_6 - skip if supports_multiple_dbs? - - assert_raises_with_message(RuntimeError, "OnlineMigrations does not support multiple databases for Active Record < 6.1") do - config.start_after = { primary: 20200101000001 } - end - end - def test_start_after_multiple_dbs - skip if !supports_multiple_dbs? - with_multiple_dbs do with_start_after({ primary: 20200101000001 }) do assert_safe RemoveNameFromUsers @@ -100,8 +90,6 @@ def test_start_after_multiple_dbs end def test_start_after_multiple_dbs_unconfigured - skip if !supports_multiple_dbs? - with_multiple_dbs(connects_to: :primary) do assert_raises_with_message(StandardError, /OnlineMigrations.config.start_after is not configured for :primary/i) do with_start_after({ animals: 20200101000001 }) do @@ -129,17 +117,7 @@ def test_target_version_unsafe end end - def test_target_version_multiple_dbs_below_6 - skip if supports_multiple_dbs? - - assert_raises_with_message(RuntimeError, "OnlineMigrations does not support multiple databases for Active Record < 6.1") do - config.target_version = { primary: 10 } - end - end - def test_target_version_multiple_dbs - skip if !supports_multiple_dbs? - with_multiple_dbs do with_target_version({ primary: 11 }) do assert_safe AddColumnDefault @@ -152,8 +130,6 @@ def test_target_version_multiple_dbs end def test_target_version_multiple_dbs_unconfigured - skip if !supports_multiple_dbs? - with_multiple_dbs(connects_to: :primary) do assert_raises_with_message(StandardError, /OnlineMigrations.config.target_version is not configured for :primary/i) do with_target_version({ animals: 10 }) do @@ -253,7 +229,7 @@ def test_verbose_sql_logs out, = capture_io do assert_safe AddEmailToUsers end - refute_match(/SHOW lock_timeout/i, out) + assert_no_match(/SHOW lock_timeout/i, out) ensure OnlineMigrations.config.verbose_sql_logs = previous end diff --git a/test/schema_statements/add_column_with_default_test.rb b/test/schema_statements/add_column_with_default_test.rb index 4d3218d..b778838 100644 --- a/test/schema_statements/add_column_with_default_test.rb +++ b/test/schema_statements/add_column_with_default_test.rb @@ -49,8 +49,6 @@ def test_add_column_with_default_updates_existing_records end def test_add_column_with_default_expression - skip if ar_version < 5.0 - with_postgres(10) do connection.add_column_with_default(:milestones, :created_at, :datetime, default: -> { "now()" }) @@ -59,31 +57,21 @@ def test_add_column_with_default_expression assert_equal "now()", column.default_function milestone = Milestone.create! - refute_nil milestone.created_at + assert_not_nil milestone.created_at end end def test_add_column_with_default_quoted_expression - skip if ar_version < 5.0 - with_postgres(10) do connection.add_column_with_default(:milestones, :created_at, :datetime, default: -> { "'now()'" }) Milestone.reset_column_information column = Milestone.columns_hash["created_at"] assert_nil column.default_function - refute_nil column.default + assert_not_nil column.default milestone = Milestone.create! - refute_nil milestone.created_at - end - end - - def test_add_column_with_default_raises_for_expression_default_in_older_railses - skip if ar_version >= 5.0 - - assert_raises_with_message(ArgumentError, "Expressions as default are not supported") do - connection.add_column_with_default(:milestones, :status, :integer, default: -> { "random()" }) + assert_not_nil milestone.created_at end end diff --git a/test/schema_statements/add_reference_concurrently_test.rb b/test/schema_statements/add_reference_concurrently_test.rb index 86c10a1..b012ccb 100644 --- a/test/schema_statements/add_reference_concurrently_test.rb +++ b/test/schema_statements/add_reference_concurrently_test.rb @@ -34,15 +34,11 @@ def test_add_reference_concurrently assert_includes Milestone.column_names, "project_id" end - def test_add_reference_concurrently_adds_index_in_ar_5 + def test_add_reference_concurrently_adds_index connection.add_reference_concurrently :milestones, :project index = connection.indexes(:milestones).first - if ar_version >= 5.0 - assert_equal "index_milestones_on_project_id", index.name - else - assert_nil index - end + assert_equal "index_milestones_on_project_id", index.name end def test_add_reference_concurrently_add_index_hash diff --git a/test/schema_statements/changing_column_type_test.rb b/test/schema_statements/changing_column_type_test.rb index b469878..0101e42 100644 --- a/test/schema_statements/changing_column_type_test.rb +++ b/test/schema_statements/changing_column_type_test.rb @@ -17,13 +17,7 @@ def setup @connection.create_table(:users, id: :serial, force: :cascade) @connection.create_table(:projects, id: :serial, force: :cascade) do |t| - # Database comments were added in Active Record 5.0 - # (see https://github.com/rails/rails/pull/22911). - if ar_version >= 5 - t.text :name, default: "My project", null: false, comment: "Project's name" - else - t.text :name, default: "My project", null: false - end + t.text :name, default: "My project", null: false, comment: "Project's name" t.string :long_name t.string :description, null: false t.text :settings @@ -33,12 +27,8 @@ def setup t.index :name t.index :long_name - - # Active Record < 5 does not support expression indexes - if ar_version >= 5 - t.index "lower(name)" - t.index "lower(long_name)" - end + t.index "lower(name)" + t.index "lower(long_name)" t.foreign_key :users end @@ -64,10 +54,7 @@ def test_initialize_column_type_change_creates_new_column_before_11 assert_equal 100, column.limit assert column.null assert_equal "My project", column.default - - if ar_version >= 5.0 - assert_equal "Project's name", column.comment - end + assert_equal "Project's name", column.comment end end @@ -80,10 +67,7 @@ def test_initialize_column_type_change_creates_new_column_after_11 assert_equal 100, column.limit assert_not column.null assert_equal "My project", column.default - - if ar_version >= 5.0 - assert_equal "Project's name", column.comment - end + assert_equal "Project's name", column.comment end end @@ -201,11 +185,7 @@ def test_backfill_columns_for_type_change_updates_existing_data @connection.backfill_columns_for_type_change(:projects, :name, :user_id) result = @connection.select_rows("SELECT name_for_type_change, user_id_for_type_change FROM projects WHERE id = #{p.id}").first - if ar_version <= 4.2 - assert_equal ["rails", u.id.to_s], result - else - assert_equal ["rails", u.id], result - end + assert_equal ["rails", u.id], result end def test_backfill_column_for_type_change_type_cast_function @@ -257,13 +237,11 @@ def test_finalize_column_type_change_copies_indexes @connection.initialize_column_type_change(:projects, :name, :string) @connection.finalize_column_type_change(:projects, :name) - indexes_count = (ar_version >= 5 ? 2 : 1) - assert_equal(indexes_count, @connection.indexes(:projects).count { |index| index.columns.include?("name_for_type_change") }) + assert_equal(2, @connection.indexes(:projects).count { |index| index.columns.include?("name_for_type_change") }) end def test_finalize_column_type_change_copies_indexes_with_long_names - max_identifier_length = 63 # could use just `max_identifier_length` method for ActiveRecord >= 5.0. - long_column_name = "a" * (max_identifier_length - "index_projects_on_".length) + long_column_name = "a" * (@connection.max_identifier_length - "index_projects_on_".length) @connection.change_table(:projects) do |t| t.text long_column_name @@ -289,7 +267,7 @@ def test_finalize_column_type_change_copies_check_constraints user = User.create! assert_raises(ActiveRecord::StatementInvalid) do - @connection.execute <<-SQL.strip_heredoc + @connection.execute(<<~SQL) INSERT INTO projects (description, user_id, star_count) VALUES ('Description', #{user.id}, -1) SQL diff --git a/test/schema_statements/foreign_keys_test.rb b/test/schema_statements/foreign_keys_test.rb index 36d336b..3413a24 100644 --- a/test/schema_statements/foreign_keys_test.rb +++ b/test/schema_statements/foreign_keys_test.rb @@ -24,108 +24,15 @@ def teardown connection.drop_table(:users) rescue nil end - def test_add_foreign_key - connection.add_foreign_key :milestones, :projects - assert connection.foreign_key_exists?(:milestones, :projects) - end - - def test_add_foreign_key_when_exists + def test_add_foreign_key_is_idempotent connection.add_foreign_key :milestones, :projects connection.add_foreign_key :milestones, :projects # once again assert connection.foreign_key_exists?(:milestones, :projects) end - def test_add_unvalidated_foreign_key - assert_sql("NOT VALID") do - connection.add_foreign_key :milestones, :projects, validate: false - end - end - - def test_add_foreign_key_custom_name - connection.add_foreign_key :milestones, :projects, name: "custom_fk_name" - - fkey = connection.foreign_keys(:milestones).first - assert_equal "custom_fk_name", fkey.name - end - - def test_add_foreign_key_default_column - connection.add_foreign_key :milestones, :projects - - fkey = connection.foreign_keys(:milestones).first - assert_equal "project_id", fkey.column - end - - def test_add_foreign_key_custom_column - connection.add_foreign_key :milestones, :users, column: :owner_id - - fkey = connection.foreign_keys(:milestones).first - assert_equal "owner_id", fkey.column - end - - def test_add_foreign_key_custom_on_delete - connection.add_foreign_key :milestones, :projects, on_delete: :cascade - - fkey = connection.foreign_keys(:milestones).first - assert_equal :cascade, fkey.on_delete - end - - def test_add_foreign_key_on_delete_nil - connection.add_foreign_key :milestones, :projects, on_delete: nil - - fkey = connection.foreign_keys(:milestones).first - assert_nil fkey.on_delete - end - - def test_add_foreign_key_custom_on_update - connection.add_foreign_key :milestones, :projects, on_update: :cascade - - fkey = connection.foreign_keys(:milestones).first - assert_equal :cascade, fkey.on_update - end - - def test_add_foreign_key_on_update_nil - connection.add_foreign_key :milestones, :projects, on_update: nil - - fkey = connection.foreign_keys(:milestones).first - assert_nil fkey.on_update - end - - def test_add_foreign_key_raises_on_invalid_actions - assert_raises(ArgumentError) do - connection.add_foreign_key :milestones, :projects, on_delete: :invalid - end - - assert_raises(ArgumentError) do - connection.add_foreign_key :milestones, :projects, on_update: :invalid - end - end - - def test_add_foreign_key_deferrable - skip("Active Record < 7.0 does not support DEFERRABLE foreign keys") if ar_version < 7.0 - - connection.add_foreign_key :milestones, :projects - fkey = connection.foreign_keys(:milestones).first - assert_equal false, fkey.deferrable - connection.remove_foreign_key :milestones, :projects - - connection.add_foreign_key :milestones, :projects, deferrable: true - fkey = connection.foreign_keys(:milestones).first - if ar_version >= 7.1 - assert_equal :immediate, fkey.deferrable - else - assert_equal true, fkey.deferrable # rubocop:disable Minitest/AssertTruthy - end - connection.remove_foreign_key :milestones, :projects - - connection.add_foreign_key :milestones, :projects, deferrable: :deferred - fkey = connection.foreign_keys(:milestones).first - assert_equal :deferred, fkey.deferrable - connection.remove_foreign_key :milestones, :projects - end - - def test_validate_foreign_key + def test_validate_foreign_key_disables_statement_timeout connection.add_foreign_key :milestones, :projects, validate: false - assert_sql('ALTER TABLE "milestones" VALIDATE CONSTRAINT') do + assert_sql("SET statement_timeout TO 0") do connection.validate_foreign_key :milestones, :projects end end diff --git a/test/schema_statements/indexes_test.rb b/test/schema_statements/indexes_test.rb index 3f3ba6c..c24e106 100644 --- a/test/schema_statements/indexes_test.rb +++ b/test/schema_statements/indexes_test.rb @@ -89,16 +89,13 @@ def test_remove_non_existing_index private def index_valid?(index_name) - # Active Record <= 4.2 returns a string, instead of automatically casting to boolean - valid = @connection.select_value <<-SQL.strip_heredoc + @connection.select_value(<<~SQL) SELECT indisvalid FROM pg_index i JOIN pg_class c ON i.indexrelid = c.oid WHERE c.relname = #{@connection.quote(index_name)} SQL - - OnlineMigrations::Utils.to_bool(valid) end end end diff --git a/test/schema_statements/misc_test.rb b/test/schema_statements/misc_test.rb index 0ccbdc8..e1c040d 100644 --- a/test/schema_statements/misc_test.rb +++ b/test/schema_statements/misc_test.rb @@ -92,8 +92,6 @@ def test_reset_counters_in_background end def test_delete_orphaned_records_in_background - skip if ar_version <= 4.2 - m = @connection.delete_orphaned_records_in_background(Post.name, :author) assert_equal "DeleteOrphanedRecords", m.migration_name diff --git a/test/schema_statements/renaming_columns_test.rb b/test/schema_statements/renaming_columns_test.rb index aa882cc..ba52b0c 100644 --- a/test/schema_statements/renaming_columns_test.rb +++ b/test/schema_statements/renaming_columns_test.rb @@ -83,12 +83,9 @@ def test_old_code_uses_original_table_for_metadata assert_equal "id", User.primary_key - refute_empty User.columns - refute_empty User.columns_hash - - if ar_version >= 6.0 - refute_empty @schema_cache.indexes("users") - end + assert_not_empty User.columns + assert_not_empty User.columns_hash + assert_not_empty @schema_cache.indexes("users") end def test_old_code_accepts_crud_operations @@ -211,8 +208,6 @@ def test_revert_finalize_columns_rename # Test that it is properly reset in rails tests using fixtures. def test_initialize_column_rename_and_resetting_sequence - skip("Rails 4.2 is not working with newer PostgreSQL") if ar_version <= 4.2 - column_renames("fname" => "first_name") @connection.initialize_column_rename(:users, :fname, :first_name) diff --git a/test/schema_statements/renaming_table_test.rb b/test/schema_statements/renaming_table_test.rb index 6875836..d1effd7 100644 --- a/test/schema_statements/renaming_table_test.rb +++ b/test/schema_statements/renaming_table_test.rb @@ -65,16 +65,14 @@ def test_old_code_uses_new_table_for_metadata assert_equal ProjectNew.primary_key, ProjectOld.primary_key - refute_empty ProjectOld.columns + assert_not_empty ProjectOld.columns assert_equal ProjectNew.columns, ProjectOld.columns - refute_empty ProjectOld.columns_hash + assert_not_empty ProjectOld.columns_hash assert_equal ProjectNew.columns_hash, ProjectOld.columns_hash - if ar_version >= 6.0 - refute_empty @schema_cache.indexes("projects") - assert_equal @schema_cache.indexes("projects_new"), @schema_cache.indexes("projects") - end + assert_not_empty @schema_cache.indexes("projects") + assert_equal @schema_cache.indexes("projects_new"), @schema_cache.indexes("projects") end def test_old_code_and_new_code_accepts_crud_operations @@ -120,8 +118,6 @@ def test_revert_finalize_table_rename # Test that it is properly reset in rails tests using fixtures. def test_initialize_table_rename_and_resetting_sequence - skip("Rails 4.2 is not working with newer PostgreSQL") if ar_version <= 4.2 - @connection.initialize_table_rename(:projects, :projects_new) @schema_cache.clear! diff --git a/test/schema_statements/update_column_in_batches_test.rb b/test/schema_statements/update_column_in_batches_test.rb index 0bd1ae1..abbe5fb 100644 --- a/test/schema_statements/update_column_in_batches_test.rb +++ b/test/schema_statements/update_column_in_batches_test.rb @@ -16,7 +16,7 @@ def setup t.string :name t.integer :points t.datetime :started_at - t.timestamps(null: false) # to please Active Record 4.2 deprecation warning + t.timestamps end Milestone.reset_column_information @@ -75,7 +75,7 @@ def test_update_column_in_batches_expression_value connection.update_column_in_batches(:milestones, :started_at, -> { "CURRENT_TIMESTAMP" }) - refute_nil m1.reload.started_at + assert_not_nil m1.reload.started_at assert_equal m1.started_at, m2.reload.started_at end diff --git a/test/support/minitest_helpers.rb b/test/support/minitest_helpers.rb index a731d20..8c206ab 100644 --- a/test/support/minitest_helpers.rb +++ b/test/support/minitest_helpers.rb @@ -28,10 +28,8 @@ def migrate(migration, direction: :up, version: 1) args = if OnlineMigrations::Utils.ar_version >= 7.1 [ActiveRecord::SchemaMigration.new(connection), ActiveRecord::InternalMetadata.new(connection)] - elsif OnlineMigrations::Utils.ar_version >= 6 - [ActiveRecord::SchemaMigration] else - [] + [ActiveRecord::SchemaMigration] end ActiveRecord::Migrator.new(direction, [migration], *args).migrate true @@ -74,7 +72,7 @@ def assert_sql(*patterns_to_match, &block) failed_patterns = [] patterns_to_match.each do |pattern| pattern = pattern.downcase - failed_patterns << pattern if queries.none? { |sql| sql.downcase.include?(pattern) } + failed_patterns << pattern if queries.none? { |sql| sql.downcase.squish.include?(pattern) } end assert_empty failed_patterns, "Query pattern(s) #{failed_patterns.map(&:inspect).join(', ')} not found.#{queries.empty? ? '' : "\nQueries:\n#{queries.join("\n")}"}" @@ -118,20 +116,17 @@ def ar_version OnlineMigrations::Utils.ar_version end - def migration_parent_string - OnlineMigrations::Utils.migration_parent_string - end - - def model_parent_string - OnlineMigrations::Utils.model_parent_string - end - - def supports_multiple_dbs? - OnlineMigrations::Utils.supports_multiple_dbs? + def migration_parent + "ActiveRecord::Migration[#{OnlineMigrations::Utils.ar_version}]" end end Minitest::Test.class_eval do include MinitestHelpers alias_method :assert_not, :refute + alias_method :assert_not_equal, :refute_equal + alias_method :assert_not_includes, :refute_includes + alias_method :assert_no_match, :refute_match + alias_method :assert_not_nil, :refute_nil + alias_method :assert_not_empty, :refute_empty end diff --git a/test/test_helper.rb b/test/test_helper.rb index 4d7b219..624a33f 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -29,7 +29,7 @@ def self.env # Disallow ActiveSupport deprecations sprouting from this gem if OnlineMigrations::Utils.ar_version >= 7.1 ActiveSupport::Deprecation._instance.disallowed_warnings = :all -elsif OnlineMigrations::Utils.ar_version >= 6.1 +else ActiveSupport::Deprecation.disallowed_warnings = :all end @@ -63,7 +63,7 @@ def prepare_database prepare_database -TestMigration = OnlineMigrations::Utils.migration_parent +TestMigration = ActiveRecord::Migration::Current TestMigration.version = 20200101000001 OnlineMigrations.configure do |config|