Add warnings when useless statement are used (#29)

* Add warnings when useless statement are used

* Remove unused method

* Move test to dedicated file

* Using super

* Add foreign_key

* Add remove_index

* Fix rubocop

* Fix

* Messing with methods again

Author: Thomas Hareau

README.md

@@ -122,7 +122,7 @@ Note: the addition of the not null constraint may timeout. In that case, you may
 
 </details>
 
-<details><summary>Safe <code>add_index</code> and <code>remove_index</code></summary>
+<details><summary id="safe_add_remove_index">Safe <code>add_index</code> and <code>remove_index</code></summary>
 
 Creating an index requires a `SHARE` lock on the target table which blocks all write on the table while the index is created (which can take some time on a large table). This is usually not practical in a live environment. Thus, **Safe PG Migrations** ensures indexes are created concurrently.
 
@@ -135,7 +135,7 @@ If you still get lock timeout while adding / removing indexes, it might be for o
 
 </details>
 
-<details><summary>safe <code>add_foreign_key</code> (and <code>add_reference</code>)</summary>
+<details><summary id="safe_add_foreign_key">safe <code>add_foreign_key</code> (and <code>add_reference</code>)</summary>
 
 Adding a foreign key requires a `SHARE ROW EXCLUSIVE` lock, which **prevent writing in the tables** while the migration is running.
 

lib/safe-pg-migrations/base.rb

@@ -6,6 +6,7 @@
 require 'safe-pg-migrations/plugins/statement_insurer'
 require 'safe-pg-migrations/plugins/statement_retrier'
 require 'safe-pg-migrations/plugins/idem_potent_statements'
+require 'safe-pg-migrations/plugins/useless_statements_logger'
 
 module SafePgMigrations
   # Order matters: the bottom-most plugin will have precedence
@@ -14,6 +15,7 @@ module SafePgMigrations
     IdemPotentStatements,
     StatementRetrier,
     StatementInsurer,
+    UselessStatementsLogger,
   ].freeze
 
   class << self
@@ -78,6 +80,7 @@ def exec_migration(connection, direction)
     end
 
     def disable_ddl_transaction
+      UselessStatementsLogger.warn_useless '`disable_ddl_transaction`' if super
       true
     end
 

lib/safe-pg-migrations/plugins/useless_statements_logger.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module SafePgMigrations
+  module UselessStatementsLogger
+    def self.warn_useless(action, link = nil, *args)
+      SafePgMigrations.say "/!\\ No need to explicitly use #{action}, safe-pg-migrations does it for you", *args
+      SafePgMigrations.say "\t see #{link} for more details", *args if link
+    end
+
+    def add_index(*, **options)
+      warn_for_index(**options)
+      super
+    end
+
+    def remove_index(table_name, options = {})
+      warn_for_index(options) if options.is_a? Hash
+      super
+    end
+
+    def add_foreign_key(*, **options)
+      if options[:validate] == false
+        UselessStatementsLogger.warn_useless '`validate: :false`', 'https://github.com/doctolib/safe-pg-migrations#safe_add_foreign_key'
+      end
+      super
+    end
+
+    def warn_for_index(**options)
+      return unless options[:algorithm] == :concurrently
+
+      UselessStatementsLogger.warn_useless '`algorithm: :concurrently`', 'https://github.com/doctolib/safe-pg-migrations#safe_add_remove_index'
+    end
+  end
+end

test/useless_statement_logger_test.rb

@@ -0,0 +1,134 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+class UselessStatementLoggerTest < MiniTest::Unit::TestCase
+  def setup
+    SafePgMigrations.instance_variable_set(:@config, nil)
+    @connection = ActiveRecord::Base.connection
+    @verbose_was = ActiveRecord::Migration.verbose
+    @connection.create_table(:schema_migrations) { |t| t.string :version }
+    ActiveRecord::SchemaMigration.create_table
+    ActiveRecord::Migration.verbose = false
+    @connection.execute("SET statement_timeout TO '70s'")
+    @connection.execute("SET lock_timeout TO '70s'")
+  end
+
+  def teardown
+    ActiveRecord::SchemaMigration.drop_table
+    @connection.execute('SET statement_timeout TO 0')
+    @connection.execute("SET lock_timeout TO '30s'")
+    @connection.drop_table(:messages, if_exists: true)
+    @connection.drop_table(:users, if_exists: true)
+    ActiveRecord::Migration.verbose = @verbose_was
+  end
+
+  def test_ddl_transactions
+    @migration =
+      Class.new(ActiveRecord::Migration::Current) do
+        disable_ddl_transaction!
+
+        def change
+          create_table(:users) { |t| t.string :email }
+        end
+      end.new
+
+    write_calls = record_calls(SafePgMigrations, :say) { run_migration }.map(&:first)
+
+    assert_includes(
+      write_calls,
+      '/!\ No need to explicitly use `disable_ddl_transaction`, safe-pg-migrations does it for you'
+    )
+  end
+
+  def test_no_warning_when_no_ddl_transaction
+    @migration =
+      Class.new(ActiveRecord::Migration::Current) do
+        def change
+          create_table(:users) { |t| t.string :email }
+        end
+      end.new
+
+    write_calls = record_calls(SafePgMigrations, :say) { run_migration }.map(&:first)
+
+    refute_includes write_calls, '/!\ No need to explicitly disable DDL transaction, safe-pg-migrations does it for you'
+  end
+
+  def test_add_index_concurrently
+    @connection.create_table(:users) { |t| t.string :email }
+    @migration =
+      Class.new(ActiveRecord::Migration::Current) do
+        def change
+          add_index :users, :email, algorithm: :concurrently
+        end
+      end.new
+
+    write_calls = record_calls(SafePgMigrations, :say) { run_migration }.map(&:first)
+
+    assert_includes(
+      write_calls,
+      '/!\ No need to explicitly use `algorithm: :concurrently`, safe-pg-migrations does it for you'
+    )
+  end
+
+  def test_no_warning_when_no_index_concurrently
+    @connection.create_table(:users) { |t| t.string :email }
+    @migration =
+      Class.new(ActiveRecord::Migration::Current) do
+        def change
+          add_index :users, :email
+        end
+      end.new
+
+    write_calls = record_calls(SafePgMigrations, :say) { run_migration }.map(&:first)
+
+    refute_includes(
+      write_calls,
+      '/!\ No need to explicitly use `algorithm: :concurrently`, safe-pg-migrations does it for you'
+    )
+  end
+
+  def test_add_foreign_key_validate_false
+    @connection.create_table(:users) { |t| t.string :email }
+    @connection.create_table(:messages) do |t|
+      t.string :message
+      t.bigint :user_id
+    end
+
+    @migration =
+      Class.new(ActiveRecord::Migration::Current) do
+        def change
+          add_foreign_key :messages, :users, validate: false
+        end
+      end.new
+
+    write_calls = record_calls(SafePgMigrations, :say) { run_migration }.map(&:first)
+
+    assert_includes(
+      write_calls,
+      '/!\ No need to explicitly use `validate: :false`, safe-pg-migrations does it for you'
+    )
+  end
+
+  def test_add_foreign_key_no_validation
+    @connection.create_table(:users) { |t| t.string :email }
+    @connection.create_table(:messages) do |t|
+      t.string :message
+      t.bigint :user_id
+    end
+
+    @migration =
+      Class.new(ActiveRecord::Migration::Current) do
+        def change
+          add_foreign_key :messages, :users
+        end
+      end.new
+
+    write_calls = record_calls(SafePgMigrations, :say) { run_migration }.map(&:first)
+
+    refute_includes(
+      write_calls,
+      '/!\ No need to explicitly use `validate: :false`, safe-pg-migrations does it for you'
+    )
+  end
+end