Bugfix: UselessStatementLogger now warns when disable_ddl_transaction is called

lib/safe-pg-migrations/base.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'safe-pg-migrations/configuration'
+require 'safe-pg-migrations/helpers/logger'
 require 'safe-pg-migrations/helpers/satisfied_helper'
 require 'safe-pg-migrations/helpers/index_helper'
 require 'safe-pg-migrations/helpers/batch_over'
@@ -31,15 +32,27 @@ class << self
     def setup_and_teardown(migration, connection, &block)
       @pg_version_num = get_pg_version_num(connection)
       @alternate_connection = nil
-      @current_migration = migration
-      stdout_sql_logger = VerboseSqlLogger.new.setup if verbose?
-      PLUGINS.each { |plugin| connection.extend(plugin) }
 
-      connection.with_setting :lock_timeout, SafePgMigrations.config.pg_lock_timeout, &block
+      with_current_migration(migration) do
+        stdout_sql_logger = VerboseSqlLogger.new.setup if verbose?
+
+        VerboseSqlLogger.new.setup if verbose?
+        PLUGINS.each { |plugin| connection.extend(plugin) }
+
+        connection.with_setting :lock_timeout, SafePgMigrations.config.pg_lock_timeout, &block
+      ensure
+        stdout_sql_logger&.teardown
+      end
     ensure
       close_alternate_connection
+    end
+
+    def with_current_migration(migration, &block)
+      @current_migration = migration
+
+      yield block
+    ensure
       @current_migration = nil
-      stdout_sql_logger&.teardown
     end
 
     def alternate_connection
@@ -53,16 +66,6 @@ def close_alternate_connection
       @alternate_connection = nil
     end
 
-    ruby2_keywords def say(*args)
-      return unless current_migration
-
-      current_migration.say(*args)
-    end
-
-    ruby2_keywords def say_method_call(method, *args)
-      say "#{method}(#{args.map(&:inspect) * ', '})", true
-    end
-
     def verbose?
       unless current_migration.class._safe_pg_migrations_verbose.nil?
         return current_migration.class._safe_pg_migrations_verbose
@@ -98,8 +101,11 @@ def exec_migration(connection, direction)
     end
 
     def disable_ddl_transaction
-      UselessStatementsLogger.warn_useless '`disable_ddl_transaction`' if super
-      true
+      SafePgMigrations.with_current_migration(self) do
+        UselessStatementsLogger.warn_useless '`disable_ddl_transaction`' if super
+
+        true
+      end
     end
 
     SAFE_METHODS = %i[

lib/safe-pg-migrations/helpers/blocking_activity_formatter.rb

@@ -5,20 +5,20 @@ module Helpers
     module BlockingActivityFormatter
       def log_queries(queries)
         if queries.empty?
-          SafePgMigrations.say 'Could not find any blocking query.', true
+          Logger.say 'Could not find any blocking query.', sub_item: true
         else
-          SafePgMigrations.say(
-            "Statement was being blocked by the following #{'query'.pluralize(queries.size)}:",
-            true
-          )
-          SafePgMigrations.say '', true
+          Logger.say <<~MESSAGE.rstrip, sub_item: true
+            Statement was being blocked by the following #{'query'.pluralize(queries.size)}:
+          MESSAGE
+
+          Logger.say '', sub_item: true
           output_blocking_queries(queries)
-          SafePgMigrations.say(
-            'Beware, some of those queries might run in a transaction. In this case the locking query might be ' \
-            'located elsewhere in the transaction',
-            true
-          )
-          SafePgMigrations.say '', true
+          Logger.say <<~MESSAGE, sub_item: true
+            Beware, some of those queries might run in a transaction. In this case the locking query might be located
+            elsewhere in the transaction
+          MESSAGE
+
+          Logger.say '', sub_item: true
         end
       end
 
@@ -27,8 +27,10 @@ def log_queries(queries)
       def output_blocking_queries(queries)
         if SafePgMigrations.config.blocking_activity_logger_verbose
           queries.each do |pid, query, start_time|
-            SafePgMigrations.say "Query with pid #{pid || 'null'} started #{format_start_time start_time}:  #{query}",
-                                 true
+            Logger.say(
+              "Query with pid #{pid || 'null'} started #{format_start_time start_time}: #{query}",
+              sub_item: true
+            )
           end
         else
           output_confidentially_blocking_queries(queries)
@@ -37,13 +39,13 @@ def output_blocking_queries(queries)
 
       def output_confidentially_blocking_queries(queries)
         queries.each do |start_time, locktype, mode, pid, transactionid|
-          SafePgMigrations.say(
+          Logger.say(
             "Query with pid #{pid || 'null'} " \
             "started #{format_start_time(start_time)}: " \
             "lock type: #{locktype || 'null'}, " \
             "lock mode: #{mode || 'null'}, " \
             "lock transactionid: #{transactionid || 'null'}",
-            true
+            sub_item: true
           )
         end
       end

lib/safe-pg-migrations/helpers/logger.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module SafePgMigrations
+  module Helpers
+    module Logger
+      class << self
+        def say(message, sub_item: false)
+          return unless SafePgMigrations.current_migration
+
+          log message, sub_item: sub_item
+        end
+
+        def say_method_call(method, *args, **options)
+          args += [options] unless options.empty?
+
+          say "#{method}(#{args.map(&:inspect) * ', '})", sub_item: true
+        end
+
+        private
+
+        def log(message, sub_item:)
+          SafePgMigrations.current_migration.say message, sub_item
+        end
+      end
+    end
+  end
+end

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

@@ -5,8 +5,8 @@
 
 module SafePgMigrations
   module BlockingActivityLogger
-    include ::SafePgMigrations::Helpers::BlockingActivityFormatter
-    include ::SafePgMigrations::Helpers::BlockingActivitySelector
+    include Helpers::BlockingActivityFormatter
+    include Helpers::BlockingActivitySelector
 
     %i[
       add_column
@@ -63,14 +63,14 @@ def log_blocking_queries_after_lock
 
       blocking_queries_retriever_thread.kill
     rescue ActiveRecord::LockWaitTimeout
-      SafePgMigrations.say 'Lock timeout.', true
+      Helpers::Logger.say 'Lock timeout.', sub_item: true
       queries =
         begin
           blocking_queries_retriever_thread.value
         rescue StandardError => e
-          SafePgMigrations.say(
+          Helpers::Logger.say(
             "Error while retrieving blocking queries: #{e}",
-            true
+            sub_item: true
           )
           nil
         end

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

@@ -2,7 +2,7 @@
 
 module SafePgMigrations
   module IdempotentStatements
-    include SafePgMigrations::Helpers::IndexHelper
+    include Helpers::IndexHelper
 
     ruby2_keywords def add_index(table_name, column_name, *args)
       options = args.last.is_a?(Hash) ? args.last : {}
@@ -12,9 +12,9 @@ module IdempotentStatements
       return super unless index_name_exists?(index_definition.table, index_definition.name)
 
       if index_valid?(index_definition.name)
-        SafePgMigrations.say(
+        Helpers::Logger.say(
           "/!\\ Index '#{index_definition.name}' already exists in '#{table_name}'. Skipping statement.",
-          true
+          sub_item: true
         )
         return
       end
@@ -26,13 +26,18 @@ module IdempotentStatements
     ruby2_keywords def add_column(table_name, column_name, type, *)
       return super unless column_exists?(table_name, column_name)
 
-      SafePgMigrations.say("/!\\ Column '#{column_name}' already exists in '#{table_name}'. Skipping statement.", true)
+      Helpers::Logger.say(
+        "/!\\ Column '#{column_name}' already exists in '#{table_name}'. Skipping statement.",
+        sub_item: true
+      )
     end
 
     ruby2_keywords def remove_column(table_name, column_name, type = nil, *)
       return super if column_exists?(table_name, column_name)
 
-      SafePgMigrations.say("/!\\ Column '#{column_name}' not found on table '#{table_name}'. Skipping statement.", true)
+      Helpers::Logger.say(
+        "/!\\ Column '#{column_name}' not found on table '#{table_name}'. Skipping statement.", sub_item: true
+      )
     end
 
     ruby2_keywords def remove_index(table_name, *args)
@@ -41,41 +46,44 @@ module IdempotentStatements
 
       return super if index_name_exists?(table_name, index_name)
 
-      SafePgMigrations.say("/!\\ Index '#{index_name}' not found on table '#{table_name}'. Skipping statement.", true)
+      Helpers::Logger.say(
+        "/!\\ Index '#{index_name}' not found on table '#{table_name}'. Skipping statement.", sub_item: true
+      )
     end
 
     ruby2_keywords def add_foreign_key(from_table, to_table, *args)
       options = args.last.is_a?(Hash) ? args.last : {}
       suboptions = options.slice(:name, :column)
       return super unless foreign_key_exists?(from_table, suboptions.present? ? nil : to_table, **suboptions)
 
-      SafePgMigrations.say(
+      Helpers::Logger.say(
         "/!\\ Foreign key '#{from_table}' -> '#{to_table}' already exists. Skipping statement.",
-        true
+        sub_item: true
       )
     end
 
     def remove_foreign_key(from_table, to_table = nil, **options)
       return super if foreign_key_exists?(from_table, to_table, **options)
 
       reference_name = to_table || options[:to_table] || options[:column] || options[:name]
-      SafePgMigrations.say(
+      Helpers::Logger.say(
         "/!\\ Foreign key '#{from_table}' -> '#{reference_name}' does not exist. Skipping statement.",
-        true
+        sub_item: true
       )
     end
 
     ruby2_keywords def create_table(table_name, *args)
       options = args.last.is_a?(Hash) ? args.last : {}
       return super if options[:force] || !table_exists?(table_name)
 
-      SafePgMigrations.say "/!\\ Table '#{table_name}' already exists.", true
+      Helpers::Logger.say "/!\\ Table '#{table_name}' already exists.", sub_item: true
 
       td = create_table_definition(table_name, *args)
 
       yield td if block_given?
 
-      SafePgMigrations.say(td.indexes.empty? ? '-- Skipping statement' : '-- Creating indexes', true)
+      Helpers::Logger.say(td.indexes.empty? ? '-- Skipping statement' : '-- Creating indexes',
+                          sub_item: true)
 
       td.indexes.each do |column_name, index_options|
         add_index(table_name, column_name, **index_options)
@@ -88,27 +96,29 @@ def add_check_constraint(table_name, expression, **options)
 
       return super if constraint_definition.nil?
 
-      SafePgMigrations.say "/!\\ Constraint '#{constraint_definition.name}' already exists. Skipping statement.", true
+      Helpers::Logger.say <<~MESSAGE, sub_item: true
+        /!\\ Constraint '#{constraint_definition.name}' already exists. Skipping statement.
+      MESSAGE
     end
 
     def change_column_null(table_name, column_name, null, *)
       column = column_for(table_name, column_name)
 
       return super if column.null != null
 
-      SafePgMigrations.say(
-        "/!\\ Column '#{table_name}.#{column.name}' is already set to 'null: #{null}'. Skipping statement.",
-        true
-      )
+      Helpers::Logger.say <<~MESSAGE, sub_item: true
+        /!\\ Column '#{table_name}.#{column.name}' is already set to 'null: #{null}'. Skipping statement.
+      MESSAGE
     end
 
     def validate_check_constraint(table_name, **options)
       constraint_definition = check_constraint_for!(table_name, **options)
 
       return super unless constraint_definition.validated?
 
-      SafePgMigrations.say "/!\\ Constraint '#{constraint_definition.name}' already validated. Skipping statement.",
-                           true
+      Helpers::Logger.say <<~MESSAGE, sub_item: true
+        /!\\ Constraint '#{constraint_definition.name}' already validated. Skipping statement.
+      MESSAGE
     end
 
     def change_column_default(table_name, column_name, default_or_changes)
@@ -122,11 +132,9 @@ def change_column_default(table_name, column_name, default_or_changes)
 
       return super if new_alter_statement != previous_alter_statement
 
-      SafePgMigrations.say(
-        "/!\\ Column '#{table_name}.#{column.name}' is already set to 'default: #{column.default}'. " \
-        'Skipping statement.',
-        true
-      )
+      Helpers::Logger.say <<~MESSAGE, sub_item: true
+        /!\\ Column '#{table_name}.#{column.name}' is already set to 'default: #{column.default}'. Skipping statement.
+      MESSAGE
     end
   end
 end

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

@@ -23,12 +23,13 @@ def add_check_constraint(table_name, expression, **options)
 
       options = check_constraint_options(table_name, expression, options)
 
-      SafePgMigrations.say_method_call :add_check_constraint, table_name, expression, **options, validate: false
+      Helpers::Logger.say_method_call :add_check_constraint, table_name, expression, **options,
+validate: false
       super table_name, expression, **options, validate: false
 
       return unless options.fetch(:validate, true)
 
-      SafePgMigrations.say_method_call :validate_check_constraint, table_name, name: options[:name]
+      Helpers::Logger.say_method_call :validate_check_constraint, table_name, name: options[:name]
       validate_check_constraint table_name, name: options[:name]
     end
 
@@ -67,15 +68,15 @@ def add_check_constraint(table_name, expression, **options)
         options[:algorithm] = :concurrently
       end
 
-      SafePgMigrations.say_method_call(:add_index, table_name, column_name, **options)
+      Helpers::Logger.say_method_call(:add_index, table_name, column_name, **options)
 
       without_timeout { super(table_name, column_name, **options) }
     end
 
     ruby2_keywords def remove_index(table_name, *args)
       options = args.last.is_a?(Hash) ? args.last : { column: args.last }
       options[:algorithm] = :concurrently unless options.key?(:algorithm)
-      SafePgMigrations.say_method_call(:remove_index, table_name, **options)
+      Helpers::Logger.say_method_call(:remove_index, table_name, **options)
 
       without_timeout { super(table_name, **options) }
     end
@@ -87,12 +88,13 @@ def change_column_null(table_name, column_name, null, default = nil)
 
       add_check_constraint table_name, "#{column_name} IS NOT NULL"
 
-      SafePgMigrations.say_method_call :change_column_null, table_name, column_name, false
+      Helpers::Logger.say_method_call :change_column_null, table_name, column_name, false
       with_setting(:statement_timeout, SafePgMigrations.config.pg_statement_timeout) do
         super table_name, column_name, false
       end
 
-      SafePgMigrations.say_method_call :remove_check_constraint, table_name, "#{column_name} IS NOT NULL"
+      Helpers::Logger.say_method_call :remove_check_constraint, table_name,
+                                      "#{column_name} IS NOT NULL"
       remove_check_constraint table_name, "#{column_name} IS NOT NULL"
     end