Add check for validated foreign key constraint #62
Conversation
| @@ -119,6 +119,16 @@ def method_missing(method, *args, &block) | |||
| raise_error :change_column_null, | |||
| code: backfill_code(table, column, default) | |||
| end | |||
| when :add_foreign_key | |||
There was a problem hiding this comment.
I'm thinking we'll have to reject any call to this method on Rails < 5.2 since validate: true is implied.
iamvery
force-pushed
the
iamvery/validating-constraints
branch
from
b744528
to
5cc38c8
Compare
lib/strong_migrations/migration.rb
Outdated
| validated = options.fetch(:validate) { true } | ||
|
|
||
| five_point_two = Gem::Version.new("5.2") | ||
| if postgresql? && ActiveRecord.version < five_point_two |
There was a problem hiding this comment.
I feel like this is a better way to do gem version comparisons than boolean operations on ActiveRecord::VERSION
There was a problem hiding this comment.
Please keep it consistent for now.
|
@ankane finally found some time to update this PR. Let me know what you think! |
| @@ -368,8 +374,13 @@ def test_down | |||
| assert_safe SafeUp, direction: :down | |||
| end | |||
|
|
|||
| def test_add_foreign_key | |||
| skip unless postgres? | |||
There was a problem hiding this comment.
It has been a really long time since I've used mysql. How confident are we that the unsafe foreign key does not apply to it?
There was a problem hiding this comment.
I'm not sure if it's safe with MySQL, it's fine to start with just Postgres.
| @@ -368,8 +374,13 @@ def test_down | |||
| assert_safe SafeUp, direction: :down | |||
| end | |||
|
|
|||
| def test_add_foreign_key | |||
| skip unless postgres? | |||
There was a problem hiding this comment.
I'm not sure if it's safe with MySQL, it's fine to start with just Postgres.
lib/strong_migrations/migration.rb
Outdated
| validated = options.fetch(:validate) { true } | ||
|
|
||
| five_point_two = Gem::Version.new("5.2") | ||
| if postgresql? && ActiveRecord.version < five_point_two |
There was a problem hiding this comment.
Please keep it consistent for now.
| @@ -119,6 +119,16 @@ def method_missing(method, *args, &block) | |||
| raise_error :change_column_null, | |||
| code: backfill_code(table, column, default) | |||
| end | |||
| when :add_foreign_key | |||
It's hard to see the benefit of this with the current set of constraints. However, it is more apparently when including minor versions in the check as we do in ankane#62.
Older versions of Rails defaulted to the validate: true behavior, so all use of this method is unsafe in that case.
The add_foreign_key check is a bit complicated, so the custom example is more straightforward on a simple add_column example.
iamvery
force-pushed
the
iamvery/validating-constraints
branch
from
42f963f
to
01070a7
Compare
| options ||= {} | ||
| validated = options.fetch(:validate) { true } | ||
|
|
||
| if postgresql? && ActiveRecord::VERSION::MAJOR > 5 || (ActiveRecord::VERSION::MAJOR == 5 && ActiveRecord::VERSION::MINOR >= 2) |
There was a problem hiding this comment.
Fwiw, this would be much simpler by comparing Gem::Version objects. See #74
Co-authored-by: Jay Hayes <ur@iamvery.com>
Following #59, this PR adds a new check to protect against the addition of a validated foreign key constraint which acquires an AccessExclusiveLock which may result in significant performance problems on large, active tables.