Rails/ReversibleMigration crashing on migration

[scottmatthewman]

I'm in the process of updating some gem dependencies in a long-standing Rails 4.2 project. One of these is RuboCop: we have been on v0.52.1 for a while.

Upon updating to the latest (v0.52.9), running rubocop --auto-gen-config against the full code base crashed out.

An error occurred while Rails/ReversibleMigration cop was inspecting <APP_DIR>/db/migrate/20140407154401_add_price_fields_from_orders_to_invoices.rb:3:4.
undefined method `method_name' for #<RuboCop::AST::Node:0x00007fba643d9a70>
Did you mean?  method
/Users/skillsmatter/workspace/rubocop/lib/rubocop/cop/rails/reversible_migration.rb:249:in `check_change_table_offense'
/Users/skillsmatter/workspace/rubocop/lib/rubocop/cop/rails/reversible_migration.rb:242:in `block (2 levels) in check_change_table_node'
/Users/skillsmatter/workspace/rubocop/lib/rubocop/ast/node.rb:185:in `block in each_child_node'
/Users/skillsmatter/workspace/rubocop/lib/rubocop/ast/node.rb:182:in `each'
/Users/skillsmatter/workspace/rubocop/lib/rubocop/ast/node.rb:182:in `each_child_node'
[...snipped]

As you can see from the (partially obfuscated) file path, this is a migration that dates from 2014, and which previously has been able to be parsed successfully until now.

The file itself that causes the error:

class AddPriceFieldsFromOrdersToInvoices < ActiveRecord::Migration
  def change
    change_table :invoices do |t|
      decimals_params = { precision: 10, scale: 2 }

      t.string :product_code
      t.string :product_name
      t.datetime :product_date

      t.string :currency_symbol
      t.string :currency_code

      t.string :price_strategy
      t.string :price_caption
      t.decimal :item_standard_price, decimals_params
      t.decimal :item_actual_price, decimals_params

      t.string :promo_code
      t.decimal :discount_amount, decimals_params
      t.decimal :discount_percentage, precision: 5, scale: 2
      t.string :discount_caption

      t.decimal :total_discount, decimals_params
      t.decimal :total_exclude_discount, decimals_params
      t.decimal :total_vat, decimals_params
      t.decimal :total_exclude_vat, decimals_params
      t.decimal :total_include_vat, decimals_params
    end
  end
end

However, if I create a test using this migration code in reversible_migration_spec.rb, this passes without crashing:

it_behaves_like 'accepts', 'change_tabe(large list of attributes)', <<-RUBY
  change_table :invoices do |t|
    decimals_params = { precision: 10, scale: 2 }

    t.string :product_code
    t.string :product_name
    t.datetime :product_date

    t.string :currency_symbol
    t.string :currency_code

    t.string :price_strategy
    t.string :price_caption
    t.decimal :item_standard_price, decimals_params
    t.decimal :item_actual_price, decimals_params

    t.string :promo_code
    t.decimal :discount_amount, decimals_params
    t.decimal :discount_percentage, precision: 5, scale: 2
    t.string :discount_caption

    t.decimal :total_discount, decimals_params
    t.decimal :total_exclude_discount, decimals_params
    t.decimal :total_vat, decimals_params
    t.decimal :total_exclude_vat, decimals_params
    t.decimal :total_include_vat, decimals_params
  end
RUBY

$ bundle exec rspec spec/rubocop/cop/rails/reversible_migration_spec.rb
Run options: include {:focus=>true}

All examples were filtered out; ignoring {:focus=>true}

Randomized with seed 26395
........................

Finished in 0.06453 seconds (files took 1.04 seconds to load)
24 examples, 0 failures

Using git bisect, I can establish that this particular migration file was parsed correctly until 4c68d5b226ebeba8284719078d13476f0641c6f2 (Remove Node#method_name in favour of node extensions). Given the commit message it does seem like they're related, but I don't know enough about Node or node extensions to immediately say why or how the issue is arising in a genuine file, but not in an RSpec test.

Expected behavior

The Rails/ReversibleMigration cop should be run against the file concerned and not crash (actual output may vary depending on RuboCop config settings):

$ rubocop --only Rails/ReversibleMigration db/migrate/20140407154401_add_price_fields_from_orders_to_invoices.rb
Inspecting 1 file
C

Offenses:

db/migrate/20140407154401_add_price_fields_from_orders_to_invoices.rb:3:5: C: Rails/ReversibleMigration: change_table is not reversible. (https://github.com/bbatsov/rails-style-guide#reversible-migration, http://api.rubyonrails.org/classes/ActiveRecord/Migration/CommandRecorder.html)
    change_table :invoices do |t|
    ^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected

Actual behavior

The cop crashes:

$ rubocop --only Rails/ReversibleMigration db/migrate/20140407154401_add_price_fields_from_orders_to_invoices.rb
Inspecting 1 file
An error occurred while Rails/ReversibleMigration cop was inspecting /Users/skillsmatter/workspace/skillsmatter/db/migrate/20140407154401_add_price_fields_from_orders_to_invoices.rb:3:4.
To see the complete backtrace run rubocop -d.
.

1 file inspected, no offenses detected

1 error occurred:
An error occurred while Rails/ReversibleMigration cop was inspecting /Users/skillsmatter/workspace/skillsmatter/db/migrate/20140407154401_add_price_fields_from_orders_to_invoices.rb:3:4.
undefined method `metadata' for nil:NilClass
/Users/skillsmatter/workspace/rubocop/lib/rubocop/cli.rb:268:in `display_error_summary'

RuboCop version

0.59.2 (using Parser 2.5.1.2, running on ruby 2.4.4 x86_64-darwin17)

[koic]

@scottmatthewman I opened a PR #6335 that wrote the reproduction test and the patch. Thanks to what you were detailed investigating, the fix was straightforward. I write your account both credit in the changelog. Thank you!

[scottmatthewman]

Thanks, @koic!