detect when adding a reference that lock writes

[milmazz]

What I'm doing

Adding a new check to avoid locks when an Ecto Migration includes a foreign key.

Why?

As the README suggests, adding a migration like this:

def change do
  alter table("posts") do
    add :group_id, references("groups")
  end
end

It would block writes on both tables. This PR includes a new check to detect these cases and report them as dangerous.

Approach

Add a new function in the AST Parser to detect when a column is added and includes references to another table; then, we check if the validate: false option is given.

How to review this PR / I’d like feedback on

This is my first PR here, so any constructive feedback is welcome.

[Artur-Sulej]

Hi @milmazz! Thank you for your PR. You have a valid point about validate: false. This option indeed makes the operation non blocking and such case should not be marked as unsafe. Good catch.

Please notice, that there is already a check column_reference_added. Can you please extending its implementation to react on validate: false (instead of adding a new danger)? Code: https://github.com/Artur-Sulej/excellent_migrations/blob/master/lib/ast_parser.ex#L132-L140 Test: https://github.com/Artur-Sulej/excellent_migrations/blob/master/test/ast_parser_test.exs#L43-L56

[milmazz]

@Artur-Sulej Hello. Sorry that I completely forgot about this.

I followed your suggestion, and please let me know if that's what you want.

[milmazz]

Also, please let me know if you like the use of defguard to improve the readability of the AST parser code. Here is an example, I can prepare another PR as a followup.