Artur-Sulej / excellent_migrations

An Elixir tool for checking safety of database migrations.
MIT License
232 stars 25 forks source link

Concurrently attributes missing checker #12

Closed hiagomeels closed 2 years ago

hiagomeels commented 2 years ago

8

It will add a checker to verify if the following module attributes are set up to true when the migration contains an operation to add or drop an index concurrently.

@disable_ddl_transaction true
@disable_migration_lock true

As said here, the unique way that I found to include this feature was going through the entire code_ast.

The checker consists of post-walk with the AST and checking:

defp detect_invalid_index_concurrently_inner(disable_migrationlock, , [true]} = ast_part, acc) do {ast_part, %{acc | has_lock?: true}} end


I also created this initial accumulator map with these default params to be easier to handle with this logic:
```Elixir
%{
      line: nil,
      is_index_concurrently?: false,
      has_transaction?: false,
      has_lock?: false
}

For now, I have named these news danger types index_concurrently_without_disable_ddl_transaction and index_concurrently_without_disable_migration_lock, but I'm not sure which will be a more appropriate name for them.

Let me know your thoughts on that.

hiagomeels commented 2 years ago

Should each of these full file detections checker live in their own modules? I think breaking it down into multiple modules will also be more readable and testable. Any thoughts?

Artur-Sulej commented 2 years ago

One more thing – index_concurrently_without_disable_ddl_transaction and index_concurrently_without_disable_migration_lock should be added to README.md.

hiagomeels commented 2 years ago

@Artur-Sulej

One more thing – index_concurrently_without_disable_ddl_transaction and index_concurrently_without_disable_migration_lock should be added to README.md.

I have created this PR to do it.