[Proposal] Extract table name normalization logic from Migration#method_missing

rubyonrails-core
1

Background

In ActiveRecord::Migration#method_missing, arguments are currently passed through proper_table_name, even for methods such as quote that are not intended to receive table names.

This behavior led to #55054, where quoting non-string values resulted in unexpected behavior due to unnecessary normalization.

A similar case was addressed in #10798, where specific methods (enable_extension, disable_extension) were excluded from normalization to avoid similar issues.

Motivation

Over time, method_missing has come to handle both delegation and argument normalization, which has introduced some complexity and made it harder to reason about its behavior.

In particular, the current normalization logic:

    # activerecord/lib/active_record/migration.rb
    def method_missing(method, *arguments, &block)
      say_with_time "#{method}(#{format_arguments(arguments)})" do
      
        ############ argument normalization ############
        unless connection.respond_to? :revert
          unless arguments.empty? || [:execute, :enable_extension, :disable_extension].include?(method)
            arguments[0] = proper_table_name(arguments.first, table_name_options)
            if method == :rename_table ||
              (method == :remove_foreign_key && !arguments.second.is_a?(Hash))
              arguments[1] = proper_table_name(arguments.second, table_name_options)
            end
          end
        end
        ######################################
        
        return super unless execution_strategy.respond_to?(method)
        execution_strategy.send(method, *arguments, &block)
      end
    end

is embedded inside method_missing with nested conditionals and ad-hoc exclusions. This makes it difficult to quickly understand which methods are subject to normalization and why.

By extracting this logic into a dedicated helper method and making the list of excluded methods explicit, we can improve both readability and maintainability.

Proposal

I would like to propose a small refactoring to improve the readability and maintainability of argument normalization in Migration#method_missing.

Currently, normalization logic is embedded directly inside method_missing with nested conditionals and special-case exclusions, which makes it harder to understand and maintain.

I am proposing to move this logic into a dedicated helper method, normalize_arguments, and to define an explicit constant METHODS_WITHOUT_TABLE_NAME listing methods that should skip normalization.

The code would look like this:

    def method_missing(method, *arguments, &block)
      say_with_time "#{method}(#{format_arguments(arguments)})" do
        normalize_arguments(method, arguments)

        return super unless execution_strategy.respond_to?(method)
        execution_strategy.send(method, *arguments, &block)
      end
    end
    
    private

      METHODS_WITHOUT_TABLE_NAME = [:execute, :enable_extension, :disable_extension, :quote].freeze
      private_constant :METHODS_WITHOUT_TABLE_NAME

      def normalize_arguments(method, arguments)
        # CommandRecorder records raw args inside revert;
        # the real adapter normalizes them on replay.
        return if reverting?

        return if arguments.empty?
        return if METHODS_WITHOUT_TABLE_NAME.include?(method)

        arguments[0] = proper_table_name(arguments.first, table_name_options)
        arguments[1] = proper_table_name(arguments.second, table_name_options) if method == :rename_table ||
          (method == :remove_foreign_key && !arguments.second.is_a?(Hash))
      end

Why?

  • It makes the responsibility of method_missing clearer: focusing on delegation and logging.
  • The normalization logic is now isolated and easier to read, test, and reason about.
  • The list of methods excluded from normalization is now explicit and easy to update.
  • Future similar exceptions (such as quote in #55054) can be handled in a uniform way.

Discussion

I would very much appreciate any feedback from the community on whether this direction makes sense. If this sounds reasonable, I would be happy to continue working on this.

I have already opened an initial PR to demonstrate this approach here: #55076. Any feedback or suggestions on the idea or implementation would be very welcome.

Thank you for your time and for helping maintain such a great project!

2 Likes