ankane / strong_migrations Goto Github PK

Catch unsafe migrations in development

License: MIT License

Ruby 100.00%

strong_migrations's Issues

For adding new columns, warning messages are mixed

__          __     _____ _______ _
 \ \        / /\   |_   _|__   __| |
  \ \  /\  / /  \    | |    | |  | |
   \ \/  \/ / /\ \   | |    | |  | |
    \  /\  / ____ \ _| |_   | |  |_|
     \/  \/_/    \_\_____|  |_|  (_)

Adding a column with a non-null default requires
the entire table and indexes to be rewritten. Instead:

1. Add the column without a default value
2. Commit the transaction
3. Backfill the column
4. Add the default value

2..4 seem to be mixed, which is correct?

The structure of the `find_in_batches` example in README.md

Given that this gem is designed to prevent us all from shooting ourselves in the foot, I was curious why the example of backfilling a default column in the README.md was structured in this way:

User.find_in_batches do |users|
  User.where(id: users.map(&:id)).update_all some_column: "default_value"
end

There seems to be some hidden knowledge in doing it this way vs. the simpler

User.find_in_batches do |users|
  users.update_all some_column: "default_value"
end

I was wondering if this is important in making good migrations, or if I could use the simpler version.

Does strong_migrations find issues with migrations containing raw SQL, or does it only work for migrations that utilize ActiveRecord?

It's unclear to me from reading the readme whether strong_migrations can find issues in migrations containing raw SQL, or whether it instead only work for migrations that utilize ActiveRecord. Could someone please let me know, and perhaps also clarify this in the documentation?

[Idea] Allow setting postgresql_version in config

Add a target_postgresql_version (or something like that) to StrongMigrations config.
i.e. StrongMigrations.target_postgres_version = 10.7 which is checked in postgresql_version

strong_migrations/lib/strong_migrations/checker.rb

Line 194 in 981d4f0

def postgresql_version

Background:
Adding a column with a default is unsafe in PostgreSQL < 11, and will raise an error. This passed on my local machine because I'm running PostgreSQL v11.3. However, on our staging/production servers, we're running PostgreSQL 10.7. So strong_migrations let the migration through in development, but failed when building on staging.

It would be safer to allow setting a production target version in StrongMigrations config so this doesn't slip through in development.

[Idea] Make it easier to disable rules

Awesome gem! We would like to ignore one of the rules, but there is no configuration for that. We though in monkeypatching the code, but the hash with warnings or the method_missing are huge. We will end up forking the gem, but that isn't ideal. The gem should be better modularized so it's easier to override.

Use of 'commit_db_transaction'

While this speeds things up when everything works according to plan, I've found that it breaks migrations if anything that runs after the command goes wrong. If the migration fails and reverts to the previous schema version, the add_column isn't undone, meaning the database is out of sync with the schema. Running rake db:migrate again yields a "column already exists" error.

Do you have an official stance on this situation?

[Idea] Also lint Procfile for "release: rake db:migrate"

Although the Procfile isn't a migration per-say, addingrelease: rake db:migrateto it is recommended and is related to proper migrations.

Ignore Migrations before a certain Timestamp

As a developer adding StrongMigrations to my long-existing application, I would like the ability to only use StrongMigrations moving forward, and not on historical migrations.

StrongMigrations.start_after does not appear to work.

Question: Is it ok to wrap an old migration in `safety_assured` block?

I have asked a question about your library on stackoverflow, can you please take a look?

Thanks!

change_column is overly inclusive

The change_column type error raises for any column change, not just the unsafe ones I'm aware of (changing the column type or the name, making a default value, or adding a constraint of some kind). I am in fact removing a constraint from the column (null: true). Is this meant to be flagged as unsafe for some reason?

[QUESTION] : Can we do the validate foreign key in the same migration where we add it

Can we have the validate foreign key validation in the same migration for addition

class AddForeignKeyOnUsers < ActiveRecord::Migration[5.2]
  def change
    add_foreign_key :users, :orders, validate: false
    validate_foreign_key :users, :orders
  end
end

do these should be in separate migrations so that they happen on a separate transaction

What is an extremely large table?

Thanks for an amazing project! There is this message:

Adding an index with more than three columns only helps on extremely large tables

How large is extremely large? 1G? 1TB?

Have any reading I can do on this? I can then do a PR adding some info to this message and to the readme.

Thanks!
John

Easy way to ignore unsafe operations in pre-existing migrations

My codebase has 164 migrations already, many of which include "unsafe" operations (likely created before the app was even in production).

Is there an easy way to "assure safety" for all migrations before a certain version number?

[Idea] Add unless index_exists? on index creation

I recently used this gem to add concurrently an index on a large and busy table on Heroku.

The operation lost connection midway but the migration finished.

Seems like some parts of Rails book keeping failed and now I get failing new builds since it think the migration need to be done but the index exists.

I think that adding unless index_exists? to the recommendation will be useful for future similar issues others may have.

No way to disable for migration as a whole

Very excited about this gem and adding it to a large app. Unfortunately, I am stuck combing through hundreds of migrations to mark them as safe. Is there an easier way, such as marking the whole migration as safe without having to edit individual lines?

Documentation for tri state problem for booleans

Hello,
It's not clear to me how to add null: false and default: false to a boolean column using strong_migrations. Using your approach, I would 1) create the column 2) add a default 3) flip everything to false 4) add null: false but when I add try to add the null: false, it complains. How would you recommend handling this?

add_column should raise warning

I was wondering why add_column does not always raise a warning to add self.ignored_columns = [:column].

ActiveRecord::PreparedStatementCacheExpired gets raised if you run the migration and roll out your servers.

rake db:migrate -> fatal: exception reentered

After running:

rake db:migrate --trace

I got:

Running via Spring preloader in process 20919
** Invoke db:migrate (first_time)
** Invoke environment (first_time)
** Execute environment
** Invoke db:load_config (first_time)
** Execute db:load_config
** Execute db:migrate
rake aborted!
can't modify frozen object
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/task.rb:188:in `extend_object'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/task.rb:188:in `extend'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/task.rb:188:in `add_chain_to'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/task.rb:182:in `rescue in invoke_with_call_chain'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/task.rb:171:in `invoke_with_call_chain'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/task.rb:165:in `invoke'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:150:in `invoke_task'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:106:in `block (2 levels) in top_level'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:106:in `each'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:106:in `block in top_level'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:115:in `run_with_threads'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:100:in `top_level'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:78:in `block in run'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:176:in `standard_exception_handling'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:75:in `run'
~/code/beautydate/bin/rake:12:in `<top (required)>'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/dependencies.rb:268:in `load'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/dependencies.rb:268:in `block in load'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/dependencies.rb:240:in `load_dependency'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/dependencies.rb:268:in `load'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/spring-1.6.2/lib/spring/command_wrapper.rb:40:in `call'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/spring-1.6.2/lib/spring/application.rb:185:in `block in serve'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/spring-1.6.2/lib/spring/application.rb:156:in `fork'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/spring-1.6.2/lib/spring/application.rb:156:in `serve'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/spring-1.6.2/lib/spring/application.rb:131:in `block in run'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/spring-1.6.2/lib/spring/application.rb:125:in `loop'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/spring-1.6.2/lib/spring/application.rb:125:in `run'
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/spring-1.6.2/lib/spring/application/boot.rb:18:in `<top (required)>'
~/.rbenv/versions/2.2.3/lib/ruby/site_ruby/2.2.0/rubygems/core_ext/kernel_require.rb:54:in `require'
~/.rbenv/versions/2.2.3/lib/ruby/site_ruby/2.2.0/rubygems/core_ext/kernel_require.rb:54:in `require'
-e:1:in `<main>'
fatal: exception reentered
~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:228:in `display_exception_backtrace': undefined method `join' for nil:NilClass (NoMethodError)
  from ~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:210:in `display_exception_details'
  from ~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:211:in `display_exception_details'
  from ~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:198:in `display_error_message'
  from ~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:185:in `rescue in standard_exception_handling'
  from ~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:176:in `standard_exception_handling'
  from ~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rake-10.5.0/lib/rake/application.rb:75:in `run'
  from ~/code/beautydate/bin/rake:12:in `<top (required)>'
  from ~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/dependencies.rb:268:in `load'
  from ~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/dependencies.rb:268:in `block in load'
  from ~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/dependencies.rb:240:in `load_dependency'
  from ~/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/activesupport-4.2.5.1/lib/active_support/dependencies.rb:268:in `load'
  from ~/.rbenv/versions/2.2.3/lib/ruby/site_ruby/2.2.0/rubygems/core_ext/kernel_require.rb:54:in `require'
  from ~/.rbenv/versions/2.2.3/lib/ruby/site_ruby/2.2.0/rubygems/core_ext/kernel_require.rb:54:in `require'
  from -e:1:in `<main>'

I am using Ruby 2.2.3 on the Mac OS X Yosemite, with Rails 4.2.5.1

NOT AN ISSUE , JUST A QUERY

Regards to Bigint Primary Keys (Postgres & MySQL), is there a way I can raise an error if there are any new primary KEY additions which don't use BIGINT as the primary key?

Similarly, if I am adding a new column like created_at to a table A, Can I raise an error like it should follow a Standard like adding TimeZone to the date type?

undefined method 'columns' for class

First off, I love the gem and appreciate all of your open source work.

When I run a DB migration it get the following error:

NameError: undefined method `columns' for class `#<Class:0x00007f9c1b3dea78>'
bin/rails:4:in `<main>'
Tasks: TOP => db:schema:dump => strong_migrations:alphabetize_columns

I am using makara, with the makara_postgis adapter and version 0.1.9 of strong_migrations, but also updated and tried with 0.2.2 with the same results. I have it alphabetizing the columns after a schema dump obviously and that is the part that fails. The migration "succeeds" but looks as if it fails because the schema doesn't dump.

Full backtrace

NameError: undefined method `columns' for class `#<Class:0x00007fd33a842320>'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/strong_migrations-0.2.2/lib/tasks/strong_migrations.rake:14:in `alias_method'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/strong_migrations-0.2.2/lib/tasks/strong_migrations.rake:14:in `singleton class'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/strong_migrations-0.2.2/lib/tasks/strong_migrations.rake:13:in `block (2 levels) in <main>'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:271:in `block in execute'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:271:in `each'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:271:in `execute'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:213:in `block in invoke_with_call_chain'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/2.5.0/monitor.rb:226:in `mon_synchronize'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:193:in `invoke_with_call_chain'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:237:in `block in invoke_prerequisites'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:235:in `each'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:235:in `invoke_prerequisites'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:212:in `block in invoke_with_call_chain'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/2.5.0/monitor.rb:226:in `mon_synchronize'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:193:in `invoke_with_call_chain'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:182:in `invoke'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/activerecord-5.2.0/lib/active_record/railties/databases.rake:68:in `block (2 levels) in <main>'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:271:in `block in execute'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:271:in `each'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:271:in `execute'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:213:in `block in invoke_with_call_chain'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/2.5.0/monitor.rb:226:in `mon_synchronize'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:193:in `invoke_with_call_chain'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:182:in `invoke'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/activerecord-5.2.0/lib/active_record/railties/databases.rake:61:in `block (2 levels) in <main>'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:271:in `block in execute'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:271:in `each'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:271:in `execute'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:213:in `block in invoke_with_call_chain'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/2.5.0/monitor.rb:226:in `mon_synchronize'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:193:in `invoke_with_call_chain'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/task.rb:182:in `invoke'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/application.rb:160:in `invoke_task'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/application.rb:116:in `block (2 levels) in top_level'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/application.rb:116:in `each'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/application.rb:116:in `block in top_level'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/application.rb:125:in `run_with_threads'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/application.rb:110:in `top_level'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/railties-5.2.0/lib/rails/commands/rake/rake_command.rb:23:in `block in perform'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rake-12.3.1/lib/rake/application.rb:186:in `standard_exception_handling'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/railties-5.2.0/lib/rails/commands/rake/rake_command.rb:20:in `perform'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/railties-5.2.0/lib/rails/command.rb:48:in `invoke'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/railties-5.2.0/lib/rails/commands.rb:18:in `<main>'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/bootsnap-1.3.0/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:21:in `require'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/bootsnap-1.3.0/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:21:in `block in require_with_bootsnap_lfi'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/bootsnap-1.3.0/lib/bootsnap/load_path_cache/loaded_features_index.rb:65:in `register'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/bootsnap-1.3.0/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:20:in `require_with_bootsnap_lfi'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/bootsnap-1.3.0/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:29:in `require'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/activesupport-5.2.0/lib/active_support/dependencies.rb:283:in `block in require'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/activesupport-5.2.0/lib/active_support/dependencies.rb:249:in `load_dependency'
/Users/chadwilken/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/activesupport-5.2.0/lib/active_support/dependencies.rb:283:in `require'
bin/rails:4:in `<main>'

Would it be possible to catch enum columns that don't have a NOT NULL constraint and default value?

I just realized that I wasn't setting a NOT NULL constraint for any of my enum columns. This came to my attention when my API test returned null for one of my state columns, and I realized that null is never a valid value (this state must always start at pending.) I think it's a much better practice to define an integer value that represents nothing, e.g. none: 0, if that's necessary. This way you can also control the string value that represents 0, and the ActiveRecord helper methods are consistent when you use a _prefix. E.g. you can call model.foo_type_none? instead of !model.foo? (which checks for the opposite of presence.)

I suppose this is up for debate, because technically nil is a valid value for an enum column. It just means that the column hasn't been defined yet. But I think that this will lead to errors more often than being a useful default.

My idea is to scan the Rails model source code for enum calls and parse out the attribute. It would find the following enums:

class Model < ApplicationRecord
  enum state: {
    pending: 0,
    complete: 1,
  }

  enum :foo => %w[bar baz]
  enum 'bar' => %w[hello world]
  enum "baz" => %w[one last example]
end

Then it would raise 4 errors for the following migration:

class CreateModels < ActiveRecord::Migration[5.2]
  def change
    create_table :models do |t|
      t.integer :state
      t.integer :foo
      t.integer :bar
      t.integer :baz
    end

    change_column_default :models, :state, from: 0, to: 0
    change_column_default :models, :foo, from: 0, to: 0
    change_column_default :models, :bar, from: 0, to: 0
    change_column_default :models, :baz, from: 0, to: 0
  end
end

Saying that all of the integer columns must be written as:

      t.integer :state, null: false
      t.integer :foo, null: false
      t.integer :bar, null: false
      t.integer :baz, null: false

This would have been a very helpful heuristic for me, because I just forgot that integers can be nil, even when I'm setting the default to 0. Obviously it wouldn't handle all of the cases (custom DSLs, metaprogramming, or calls to add_column and change_column_null), but it would be nice to catch 99% of these cases.

And also maybe as a configurable option, if you would rather leave it disabled by default.

Adding table constraints

Hi, I just ran across a scenario that might be interesting to add to this gem.

We are using Postgres and if you add a new constraint to an existing table it will lock the whole table with an exclusive lock. For example:

ALTER TABLE products ADD CONSTRAINT positive_price CHECK (price > 0);`

However, a much better approach is using the NOT VALID option. With that option it's possible to add a new constraint that only applies to new and updated records while temporarily skipping the expensive check for all existing rows. The existing rows can be checked using a separate VALIDATE CONSTRAINT command with a much less intrusive SHARE UPDATE EXCLUSIVE lock. For example:

ALTER TABLE products ADD CONSTRAINT positive_price CHECK (price > 0) NOT VALID;
ALTER TABLE products VALIDATE CONSTRAINT positive_price;

Source: https://www.postgresql.org/docs/9.6/static/sql-altertable.html

I'm not sure if this scenario is a good fit for this gem, though, as there is no default Rails method for adding such constraints and thus intercepting it is much harder. Also it probably does not apply to all SQL database systems. What do you think?

Safer defaults for lock timeouts

Commit b57486d added some helpful suggestions around setting lock timeouts in migrations. The activerecord-safer_migrations gem provides some nice capabilities to set default lock/statement timeouts and override them on a per-migration basis. Do you think it would be worth adding similar functionality to this gem? I'd love to only include one gem in my projects for safe/strong migrations :)

Are unsafe operations stackable between themselves?

Hi!

First, thanks for that Gem and the neat documentation.

We're looking for best practices here, we're unsure of a use case so let me try to explain it.

When reading the unsafe operations documentation, to rename a column we need to:
1st deployment:

Create the new column,
Write to both columns
Backfill the data from old to new column

2nd deployment

Write only on new column
Drop the old column <<<<===

Question is pretty simple: When we drop the column, should we apply the sames rules as described here for removing a column: https://github.com/ankane/strong_migrations#removing-a-column
(So a 2-step deployment, to first ignore the column from the AR cache, then a second deployment to actually drop the column)?

So, when we want to rename a column, the process is more a 4-steps deployments than a 2-steps deployments.

The same question applies for all operations described in the documentation.

What do you think?

LMK if something is obscure, happy to clarify.

Adding a null constraint w/out defaults is dangerous

https://apidock.com/rails/ActiveRecord/ConnectionAdapters/SchemaStatements/change_column_null

We ran into this recently ^^. Fixed here, hopefully? #28

Thanks so much for maintaining this btw 😄

Rails 5 in_batches method

First off, thanks a ton for this fantastic guide.

I was recently following the instructions for adding a column and wondered if there was a cleaner way to batch update yet.

Turns out in rails 5 there's a new in_batches method that can yield enumerators of relation objects, which lets you rewrite

User.find_in_batches do |users|
  User.where(id: users.map(&:id)).update_all some_column: "default_value"
end

User.in_batches.update_all some_column: "default_value"

If you also prefer the aesthetics of this new method, I can go ahead and open a PR with that option for Rails 5. But if you don't want to clutter the guide with more "For Rails < 5 use this, for Rails >= 5 use that", I understand. Just thought I'd share this new method :)

`ANALYZE` after creating an index, is it useful?

Hi there!

I've noticed that you suggest running ANALYZE after creating an index but is it really useful? Of course, it always makes sense to run ANALYZE frequently or tune it to be more efficient which is a separate topic, but how is it connected with the newly created index? I'm afraid that in no way.

What ANALYZE do is gathering some statistics about the content of the table column. The Planner uses it to make an assumption about efficiency. Since column content is not affected by any index, in the majority of the cases it doesn't make sense to run ANALYZE after index creation. It would be more efficient to run it after some bulk updates/inserts/deletes.

Am I wrong with my thoughts?

Doesn't know i've fixed the problem.

WARNING:  there is no transaction in progress
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:


 __          __     _____ _______ _
 \ \        / /\   |_   _|__   __| |
  \ \  /\  / /  \    | |    | |  | |
   \ \/  \/ / /\ \   | |    | |  | |
    \  /\  / ____ \ _| |_   | |  |_|
     \/  \/_/    \_\_____|  |_|  (_)

Adding a non-concurrent index locks the table. Instead, use:

  def change
    commit_db_transaction
    add_index :users, :some_column, algorithm: :concurrently
  end

Here's my code:

class IndexShippingInfosOnDropOffManifestId < ActiveRecord::Migration
  def change
    commit_db_transaction
    add_index :shipping_infos, column: :drop_off_manifest_id, algorithm: :concurrently, where: %("shipping_infos"."drop_off_manifest_id" IS NOT NULL)
  end
end

[Idea] Including dropping an index as a potentially dangerous operation

Hi!

Thanks for your work with strong_migrations - it's a great gem and I'm glad this exists!

Proposal: add drop_index to the list of dangerous operations that strong_migrations catches. As you probably know, this is dangerous because it can turn index lookups into table scans if done improperly.

We've definitely incurred downtime as a result of migrations dropping indexes while using strong_migrations, and would LOVE to roll this check into this gem. Thanks for your work! 👍

[Idea] Support list of tables that require special care

Hey Andrew, I'd like to list tables that require an extra set of eyes so that they aren't inadvertently messed up.

doughyde#1

[QUESTION] : Does Custom Checks don't run for safety assured blocks ?

I am doing a remove column as below

def up
      remove_column :sample, :region_id 
end

Strong migrations enforces me to put a safety assured block as below once I do that successfully

Deploy the code, then wrap this step in a safety_assured { ... } block.

class RemoveColumnrulesets < ActiveRecord::Migration[5.1]
  def change
    safety_assured { remove_column :sample, :region_id }
  end
end

In the initializer I have a custom check to stop someone from removing the column for the above table it doesn't really stop it the migrations works successfully

custom check below :

StrongMigrations.add_check do |method, args|
  if method == :remove_column && args[0].to_s == "sample"
  stop! Don't remove columns on sample table 
 end
end

Consider running arbitrary SQL a dangerous operation

This gem provides awesome protection against dangerous operations when using the Rails migration DSL. Unfortunately it's easy for developers to inadvertently bypass this safety net when running arbitrary SQL e.g.

class AddNumWheelsToCars < ActiveRecord::Migration
  def up
    execute('ALTER TABLE cars ADD COLUMN num_wheels integer DEFAULT 4 NOT NULL')
  end

  def down
    remove_column(:cars, :num_wheels)
  end
end

It would be great if execute was considered a dangerous operation that required developers to explicitly indicate that they know what they're doing. I'd be happy to put up a PR if you're open to the change.

Default column with not null constraint recommendation is incorrect

With a migration that looks like this:

class AddMetricUnitsToPassengerProfiles < ActiveRecord::Migration[5.1]
  def change
    add_column :passenger_profiles, :metric_units, :boolean, default: true, null: false
  end
end

We are getting this suggestion:

== 20190507182930 AddMetricUnitsToPassengerProfiles: migrating ================
rails aborted!
StandardError: An error has occurred, this and all later migrations canceled:

=== Dangerous operation detected #strong_migrations ===

Adding a column with a non-null default causes the entire table to be rewritten.
Instead, add the column without a default value, then change the default.

class AddMetricUnitsToPassengerProfiles < ActiveRecord::Migration[5.1]
  def up
    add_column :passenger_profiles, :metric_units, :boolean, null: false
    change_column_default :passenger_profiles, :metric_units, true
  end

  def down
    remove_column :passenger_profiles, :metric_units
  end
end

Then backfill the existing rows in the Rails console or a separate migration with disable_ddl_transaction!.

class BackfillAddMetricUnitsToPassengerProfiles < ActiveRecord::Migration[5.1]
  disable_ddl_transaction!

  def change
    PassengerProfile.in_batches.update_all metric_units: true
  end
end

The problem is that the first operation removes the default, but doesn't remove the null: false constraint. So the first line: add_column :passenger_profiles, :metric_units, :boolean, null: false will explode due to null values in a new column.

Removing that part of the line removes the null: false constraint on the column.

I looked at previous issues but didn't find anything. We're running the most recent version of the gem: 0.3.1.

Anyone have any insights into this? I know how to write the migration, but I just want to make the recommendation better for other engineers that stumble into this.

[Idea] Warn on the addition of a validated foreign key

I recently ran into a problem adding a new foreign key constraint at scale. It seems that by default, when adding a foreign key, the entire table is validated for the new constraint which performs some table locking.

From the PostgreSQL docs

If the constraint is marked NOT VALID, the potentially-lengthy initial check to verify that all rows in the table satisfy the constraint is skipped.

In my particular case, I was adding the foreign key to a new column which would be NULL for all existing records anyway thus not needing the validation at all. Here's the full example that caused me problems:

add_column :foos, :bar_id, :integer
add_foreign_key :foos, :bars

And here is the preferred way:

add_column :foos, :bar_id, :integer
add_foreign_key :foos, :bars, validate: false

Note: validate: false which results in NOT VALID option being added to the produced ALTER TABLE statement.

What do you think?

NoMethodError: undefined method `perform' for nil:NilClass

Hi,

We are in process of upgrading a rails 4.2. app to latest rails version. We successfully upgraded to 5.0 and while upgrading to 5.1 we are blocked by this error, which seems to be coming from strong_migrations.

If i remove strong_migrations from the app, everything works.

Ruby: 2.6.3
Rails: 5.1.7

Caused by:
NoMethodError: undefined method `perform' for nil:NilClass
/.../.rvm/gems/ruby-2.6.3/gems/strong_migrations-0.4.1/lib/strong_migrations/migration.rb:14:in `method_missing'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:600:in `method_missing'
/....../db/migrate/20190802132723_add_xyz_table.rb:2:in `<class:AddXyzTable>'
/....../db/migrate/20190802132723_add_xyz.rb:1:in `<top (required)>'
/.../.rvm/gems/ruby-2.6.3/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:292:in `require'
/.../.rvm/gems/ruby-2.6.3/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:292:in `block in require'
/.../.rvm/gems/ruby-2.6.3/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:258:in `load_dependency'
/.../.rvm/gems/ruby-2.6.3/gems/activesupport-5.1.7/lib/active_support/dependencies.rb:292:in `require'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:962:in `load_migration'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:958:in `migration'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:953:in `disable_ddl_transaction'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:1305:in `use_transaction?'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:1297:in `ddl_transaction'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:1229:in `execute_migration_in_transaction'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:1201:in `block in migrate_without_lock'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:1200:in `each'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:1200:in `migrate_without_lock'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:1148:in `block in migrate'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:1317:in `with_advisory_lock'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:1148:in `migrate'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:1007:in `up'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/migration.rb:985:in `migrate'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/tasks/database_tasks.rb:171:in `migrate'
/.../.rvm/gems/ruby-2.6.3/gems/strong_migrations-0.4.1/lib/strong_migrations/database_tasks.rb:4:in `migrate'
/.../.rvm/gems/ruby-2.6.3/gems/activerecord-5.1.7/lib/active_record/railties/databases.rake:58:in `block (2 levels) in <top (required)>'
/.../.rvm/gems/ruby-2.6.3/gems/rake-12.3.3/exe/rake:27:in `<top (required)>'

Our migration is simple, something like:

class AddXyzTable < ActiveRecord::Migration[4.2]
  create_table :xyz do |t|
    t.integer "abc_id",             null: false
    t.integer "efg_id", null: false
  end

  add_index "xyz", ["abc_id", "efg_id"], name: "index_xyz_on_abc_id_and_efg_id", using: :btree
  add_index "xyz", ["efg_id", "abc_id"], name: "index_xyz_on_efg_id_and_abc_id", using: :btree
end

Till now, it seems to be nothing related to our code, if it turns out to be, I will report here.

Thanks!

change_column_null (without 4th argument) still locks postgres

This is related to:
#63
#69

I've just run the following on a postgres DB with 7M rows.

class AddUserVisibleToEvents < ActiveRecord::Migration[5.1]
  def up
    add_column :events, :user_visible, :boolean, null: true
    change_column_default :events, :user_visible, false
  end

  def down
    remove_column :events, :user_visible
  end
end

The above was fast. 👍

Then I ran:

class BackfillAddUserVisibleToEvents < ActiveRecord::Migration[5.1]
  disable_ddl_transaction!

  def change
    Event.in_batches.update_all user_visible: true
  end
end

As expected, this took several hours, however it didn't stop the production DB running 👍

Then I ran this:

class AddNullFalseToUserVisibleInEvents < ActiveRecord::Migration[5.1]	
  def change	
    change_column_null(:events, :user_visible, false)	
  end	
end

... and it locked the DB 👎

Note that I have not used the 4th argument but it was still "dangerous".

I removed that migration and then ran:

class AddIndexToUserVisibleInEvents < ActiveRecord::Migration[5.1]
  disable_ddl_transaction!

  def change
    add_index :events, :user_visible, algorithm: :concurrently
  end
end

And it ran for about 15 minutes and did not lock the DB 👍

So my question... now that the index is in place, will change_column_null(:events, :user_visible, false) be faster now (since there are no NULL values!) or is there some other way I can SET NOT NULL on the column?

If the index will help, can we warn not to run change_column_null until the index is in place? Alternatively, if there is some SQL that will add NOT NULL without checking all the existing values, could that be suggested if change_column_null is used?

Thanks

How to create a field with null: false and default: 0

Hello guys, Before rails 1_000 it was possible create field with default value and without null values, I known now 21 centry, and we send Tesla to space, and all world is changed, how now i can create i simple field? Just to known. Also, will this be possible in rails 100_000 ? Thanks.!

integer to bigint

We had already two occurrences when we hit the maximum value for some primary key who was integer type (value is 2147483647). Cf. https://www.postgresql.org/docs/9.1/static/datatype-numeric.html

Would be nice to warn the user and let them create table with primary key bigint type right away cause it's really painful to change the column type later on ...

Are unsafe operations stackable between themselves?

Hi!

First, thanks for that Gem and the neat documentation.

We're looking for best practices here, we're unsure of a use case so let me try to explain it.

When reading the unsafe operations documentation, to rename a column we need to:
1st deployment:

Create the new column,
Write to both columns
Backfill the data from old to new column

2nd deployment

Write only on new column
Drop the old column <<<<===

So, when we want to rename a column, the process is more a 4-steps deployments than a 2-steps deployments.

The same question applies for all operations described in the documentation.

What do you think?

LMK if something is obscure, happy to clarify.

Customized warning messages for each unsafe migration

Has there been any thought to allowing customization of the warning messages for each type of migration? There are a few migrations (like rename_table for instance) that have onerous enough workarounds that we instead just recommend scheduling downtime.

Instructions for adding a column with a default value are different in README and in the warning message

The instructions for adding a column with a default value in the README are:

1. Add the column without a default value
2. Add the default value
3. Commit the transaction
4. Backfill the column

Meanwhile, running an unsafe migration will return this list of instructions:

1. Add the column without a default value
2. Commit the transaction
3. Backfill the column
4. Add the default value

I think the console instructions are correct, because you need to commit the transaction before adding the default value, or the DB will start backfilling inside the transaction, probably locking.

[Idea] suggest developers to use timestamptz not timestamp type

Description :
Suggest Developers to Use timestamptz datatype not timestamp datatype will creating any new migration, this is always a pain having timestamp without timezone. Data while migrating between two different databases it is always a pain having columns with timestamp type.

@ankane will be ok to get this merged if I create a PR for this ?

strong_migration should coerce the developer into writing `up/down` instead of change for irreversible migrations

It's dangerous to deploy migrations knowing you can't automatically roll back.

[Idea] drop_table as a dangerous operation?

I know this was also mentioned briefly in #49, but I think drop_table should be included in the list of dangerous operations. When a drop_table call is detected, we could recommend that the user first removes any code that uses the table and then deploys the migration separately.

I've just been bit by this (I dropped a table that was still used by the rest of the servers and the load balancer kicked them all out) and it would have been really useful to have strong_migrations warn me about it.

When adding a column with a non-null default the suggested migration does not work

PostgreSQL version 9.5.13, Ruby 2.3.8 and Rails 4.2.11.

When running this migration

class AddIsAdminToUsers < ActiveRecord::Migration
  def change
    add_column :users, :is_admin, :boolean, default: false, null: false
  end
end

I get this from strong_migrations:

== 20190314162335 AddIsAdminToUsers: migrating - Shard: master ================
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

=== Dangerous operation detected #strong_migrations ===

Adding a column with a non-null default causes the entire table to be rewritten.
Instead, add the column without a default value, then change the default.

class AddIsAdminToUsers < ActiveRecord::Migration
  def up
    add_column :users, :is_admin, :boolean, null: false
    change_column_default :users, :is_admin, false
  end

  def down
    remove_column :users, :is_admin
  end
end

Then backfill the existing rows in the Rails console or a separate migration with disable_ddl_transaction!.

class BackfillAddIsAdminToUsers < ActiveRecord::Migration
  disable_ddl_transaction!

  def change
    User.find_in_batches do |records|
      User.where(id: records.map(&:id)).update_all is_admin: false
    end
  end
end

But I create those suggested migrations and run them, the first one fails with the error:

== 20190314162335 AddIsAdminToUsers: migrating - Shard: master ================
-- add_column(:users, :is_admin, :boolean, {:null=>false})
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

PG::NotNullViolation: ERROR:  column "is_admin" contains null values
: ALTER TABLE "users" ADD "is_admin" boolean NOT NULL

This is because of null: false in the suggested line

add_column :users, :is_admin, :boolean, null: false

Should instead this line be the following instead?

add_column :users, :is_admin, :boolean, null: true

and an additional third migration should be suggested to set the NOT NULL constraint?

change_column_null :users, :is_admin, false

Internal use of `execute` preventing `change` autoreversibility

Hi!

There are paths when the Migration module internally uses connection.execute. When these paths are hit while rolling back a migration that uses change, it causes ActiveRecord to throw IrreversibleMigration: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/migration/command_recorder.rb#L88. ActiveRecord is ignorant of strong_migrations and thinks the user is trying to automatically reverse an arbitrary execute statement.

The offending lines are: https://github.com/ankane/strong_migrations/blob/master/lib/strong_migrations/migration.rb#L84

https://github.com/ankane/strong_migrations/blob/master/lib/strong_migrations/migration.rb#L97

so for now this prevents:

Rolling back add_index within change if StrongMigrations.auto_analyze = true and using Postgresql.
Rolling back add_column with type JSON within change if using Postgresql.

I'm working around these by using explicit up/down for now, but it's easy to forget and get tripped up.

Stacktrace with remove_column and optional column type

Hi there,

I just added this gem and immediately bumped into an issue. :-)

When using remove_column with the column type (to allow the down migration to recreate the column) such as:

def change
  remove_column :table, :column, :integer
end

It is failing with a stack trace:

NoMethodError: undefined method `each' for :integer:Symbol
.../gems/strong_migrations-0.3.0/lib/strong_migrations/migration.rb:191:in `options_str'
.../gems/strong_migrations-0.3.0/lib/strong_migrations/migration.rb:47:in `method_missing'
.../db/migrate/20181015190940_remove_column_from_table.rb:3:in `change'
...

From looking at the code, it appears to assume the third argument will always be an option hash instead of a column type symbol.

Fresh install, no warning

Apologies I must be missing something obvious... installed the gem, created this migration:

class AddBadStuff < ActiveRecord::Migration[5.2]
  def change
    add_column :events, :dumb_text, :text, default: "lame"
  end
end

and ran rake db:migrate... no warning. Is this not how the gem is intended to be used? Additional installation instructions?

Rails 5.2.2.1
Postgres 9.x

[Proposal]: Make all reference constraints deferrable

The foreign key constraints often raise issues when moving data, requiring the programmers to remove them to complete the data load tasks. A better approach is to make them deferrable (only in Postgres) and SET CONSTRAINTS ALL DEFERRED; in a transaction block before running your data load script.

The proposed addition would warn the user if the migration includes a non-deferrable foreign_key constraint.

[Proposal]: unsafe change_column_null default value

Rails change_column_null method accepts four arguments, with the fourth one replacing existing NULLs with some other value. Looks kinda useful feature although the implementation of such a replacement might be pretty dangerous on large tables (postgres, mysql):

UPDATE #{quote_table_name(table_name)}
SET #{quote_column_name(column_name)}=#{quote_default_expression(default, column)}
WHERE #{quote_column_name(column_name)} IS NULL

What we're doing here is a WHERE query on a table which might be lacking of needed index on column_name and UPDATE for (potentially) millions of records at once.
A little more safe way might be to require user to make this UPDATE in batches manually before firing change_column_null up.
WDYT?

ankane / strong_migrations Goto Github PK

strong_migrations's Issues

Recommend Projects

Recommend Topics

Recommend Org