From 321707509566453973b8181e42728bf8e0c0baed Mon Sep 17 00:00:00 2001 From: seapy Date: Thu, 16 Aug 2018 16:39:44 +0900 Subject: [PATCH] Add change_column, change_column_null case. null false with default value * Change column with null false and default value lead to slow database, If table size is big. * Because this situation update to default value all rows * https://github.com/rails/rails/blob/5-2-stable/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb#L440 * `change_column` is direct call when newer version migration * if previous 5.2 migration call `change_column_null` --- README.md | 1 + lib/zero_downtime_migrations.rb | 2 + .../validation/change_column.rb | 55 +++++++ .../validation/change_column_null.rb | 64 ++++++++ .../validation/change_column_null_spec.rb | 143 ++++++++++++++++++ .../validation/change_column_spec.rb | 87 +++++++++++ 6 files changed, 352 insertions(+) create mode 100644 lib/zero_downtime_migrations/validation/change_column.rb create mode 100644 lib/zero_downtime_migrations/validation/change_column_null.rb create mode 100644 spec/zero_downtime_migrations/validation/change_column_null_spec.rb create mode 100644 spec/zero_downtime_migrations/validation/change_column_spec.rb diff --git a/README.md b/README.md index 6ae1acb..814b6a1 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,7 @@ It checks for common things like: * Mixing data changes with index or schema migrations * Performing data or schema migrations with the DDL transaction disabled * Using `each` instead of `find_each` to loop thru `ActiveRecord` objects +* Check slow database, when change column options null is false with default value These exceptions display clear instructions of how to perform the same operation the "zero downtime way". diff --git a/lib/zero_downtime_migrations.rb b/lib/zero_downtime_migrations.rb index bd329fe..3477f2c 100644 --- a/lib/zero_downtime_migrations.rb +++ b/lib/zero_downtime_migrations.rb @@ -8,6 +8,8 @@ require_relative "zero_downtime_migrations/validation" require_relative "zero_downtime_migrations/validation/add_column" require_relative "zero_downtime_migrations/validation/add_index" +require_relative "zero_downtime_migrations/validation/change_column" +require_relative "zero_downtime_migrations/validation/change_column_null" require_relative "zero_downtime_migrations/validation/ddl_migration" require_relative "zero_downtime_migrations/validation/find_each" require_relative "zero_downtime_migrations/validation/mixed_migration" diff --git a/lib/zero_downtime_migrations/validation/change_column.rb b/lib/zero_downtime_migrations/validation/change_column.rb new file mode 100644 index 0000000..cec960f --- /dev/null +++ b/lib/zero_downtime_migrations/validation/change_column.rb @@ -0,0 +1,55 @@ +module ZeroDowntimeMigrations + class Validation + class ChangeColumn < Validation + def validate! + return unless options.key?(:null) + return if options[:null] || options[:default].nil? + error!(message) + end + + private + + def message + <<-MESSAGE.strip_heredoc + Changing a column with a false null and default value is unsafe! + + This action lead to slow database, If table size is big. + + If you're 100% positive that this migration is safe, then wrap the + call to `change_column` in a `safety_assured` block. + + class Change#{column_title}To#{table_title} < ActiveRecord::Migration + def change + safety_assured { change_column :#{table}, :#{column}, :#{column_type}, #{column_options} } + end + end + + MESSAGE + end + + def column + args[1] + end + + def column_options + options.map { |k, v| "#{k}: #{v.inspect}" }.join ', ' + end + + def column_title + column.to_s.camelize + end + + def column_type + args[2] + end + + def table + args[0] + end + + def table_title + table.to_s.camelize + end + end + end +end diff --git a/lib/zero_downtime_migrations/validation/change_column_null.rb b/lib/zero_downtime_migrations/validation/change_column_null.rb new file mode 100644 index 0000000..e361465 --- /dev/null +++ b/lib/zero_downtime_migrations/validation/change_column_null.rb @@ -0,0 +1,64 @@ +module ZeroDowntimeMigrations + class Validation + class ChangeColumnNull < Validation + def validate! + null = args[2] + default = args[3] + return if null || default.nil? + error!(message) + end + + private + + def message + <<-MESSAGE.strip_heredoc + Changing a column with a false null and default value is unsafe! + + This action lead to slow database, If table size is big. + + If you're 100% positive that this migration is safe, then wrap the + call to `change_column_null` in a `safety_assured` block. + + class Change#{column_title}To#{table_title} < ActiveRecord::Migration + def change + safety_assured { change_column_null :#{table}, :#{column}, #{column_null}, default: #{column_default} } + end + end + + or `change_column`, if you use `change_column` + + class Change#{column_title}To#{table_title} < ActiveRecord::Migration + def change + safety_assured { change_column :#{table}, :#{column}, :column_type, #{column_null}, default: #{column_default} } + end + end + + MESSAGE + end + + def column + args[1] + end + + def column_default + args[3].inspect + end + + def column_null + args[2].inspect + end + + def column_title + column.to_s.camelize + end + + def table + args[0] + end + + def table_title + table.to_s.camelize + end + end + end +end diff --git a/spec/zero_downtime_migrations/validation/change_column_null_spec.rb b/spec/zero_downtime_migrations/validation/change_column_null_spec.rb new file mode 100644 index 0000000..b9116f3 --- /dev/null +++ b/spec/zero_downtime_migrations/validation/change_column_null_spec.rb @@ -0,0 +1,143 @@ +RSpec.describe ZeroDowntimeMigrations::Validation::ChangeColumnNull do + let(:error) { ZeroDowntimeMigrations::UnsafeMigrationError } + + context "with a true null" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.0]) do + def change + change_column_null :users, :email, true + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a false null" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.0]) do + def change + change_column_null :users, :email, false + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a true null has default" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.0]) do + def change + change_column_null :users, :email, true, '' + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a false null has default" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.0]) do + def change + change_column_null :users, :email, false, '' + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.to raise_error(error) + end + end + + context "without a null" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.0]) do + def change + change_column :users, :email, :text + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a default" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.0]) do + def change + change_column :users, :email, :text, default: '' + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a true null" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.0]) do + def change + change_column :users, :email, :text, null: true + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a true null and has default" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.0]) do + def change + change_column :users, :email, :text, null: true, default: '' + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a false null" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.0]) do + def change + change_column :users, :email, :text, null: false + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a false null and has default" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.0]) do + def change + change_column :users, :email, :text, null: false, default: '' + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.to raise_error(error) + end + end +end diff --git a/spec/zero_downtime_migrations/validation/change_column_spec.rb b/spec/zero_downtime_migrations/validation/change_column_spec.rb new file mode 100644 index 0000000..8403f9b --- /dev/null +++ b/spec/zero_downtime_migrations/validation/change_column_spec.rb @@ -0,0 +1,87 @@ +RSpec.describe ZeroDowntimeMigrations::Validation::ChangeColumnNull do + let(:error) { ZeroDowntimeMigrations::UnsafeMigrationError } + + context "without a null" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.2]) do + def change + change_column :users, :email, :text + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a default" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.2]) do + def change + change_column :users, :email, :text, default: '' + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a true null" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.2]) do + def change + change_column :users, :email, :text, null: true + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a true null and has default" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.2]) do + def change + change_column :users, :email, :text, null: true, default: '' + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a false null" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.2]) do + def change + change_column :users, :email, :text, null: false + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.not_to raise_error(error) + end + end + + context "with a false null and has default" do + let(:migration) do + Class.new(ActiveRecord::Migration[5.2]) do + def change + change_column :users, :email, :text, null: false, default: '' + end + end + end + + it "raises an unsafe migration error" do + expect { migration.migrate(:up) }.to raise_error(error) + end + end +end