From 7199b3bc4be7f2e53321b08903a7fc624047a34d Mon Sep 17 00:00:00 2001 From: Eugene Date: Thu, 23 Jul 2026 12:33:28 +0300 Subject: [PATCH] Recognize tables passed via keyword arguments and ignore comments Migrations that touch tables only through helper methods (e.g. partition-conversion DSLs calling with table: :events) rendered an empty tables column, since the parser only knew a closed list of ActiveRecord DDL methods. A generic keyword pattern (table:, to_table:, from_table:) now catches the whole helper family. Bare from:/to: are deliberately excluded: change_column_default would yield phantom tables. Ruby comments are stripped before scanning, so explanatory comments mentioning ALTER TABLE or commented-out DSL calls no longer produce phantom tables; #{} interpolation is preserved. Also: dual-table methods now contribute their first argument even when the second is passed as to_table:, and change_column_default / change_column_null joined the single-table method list. --- lib/railbow/migration_parser.rb | 22 +++++++- spec/railbow/migration_parser_spec.rb | 73 +++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/lib/railbow/migration_parser.rb b/lib/railbow/migration_parser.rb index 5ac1db0..7926c63 100644 --- a/lib/railbow/migration_parser.rb +++ b/lib/railbow/migration_parser.rb @@ -8,6 +8,7 @@ class MigrationParser SINGLE_TABLE_METHODS = %w[ create_table drop_table change_table add_column remove_column rename_column change_column + change_column_default change_column_null add_index remove_index add_reference remove_reference add_belongs_to remove_belongs_to @@ -19,7 +20,9 @@ class MigrationParser add_foreign_key remove_foreign_key ].freeze - SINGLE_TABLE_PATTERN = /\b(?:#{SINGLE_TABLE_METHODS.join("|")})\s+[:"](\w+)/ + # Dual-table methods are included here too: their first argument is always a + # table, even when the second is passed as `to_table:` instead of positionally. + SINGLE_TABLE_PATTERN = /\b(?:#{(SINGLE_TABLE_METHODS + DUAL_TABLE_METHODS).join("|")})\s+[:"](\w+)/ DUAL_TABLE_PATTERN = /\b(?:#{DUAL_TABLE_METHODS.join("|")})\s+[:"](\w+)["\s,]+[:"](\w+)/ # SQL keywords followed by a table name @@ -33,6 +36,17 @@ class MigrationParser /\bCREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?["']?(\w+)["']?/i ].freeze + # Helper/DSL calls that name their target table via a keyword argument, e.g. + # `convert_to_monthly_partitions(table: :apples)` or + # `remove_foreign_key :accounts, to_table: :owners`. Catches project-level + # migration helpers regardless of the method name. Bare `from:`/`to:` are + # deliberately not matched — `change_column_default :users, :status, + # from: nil, to: "active"` would yield phantom tables. + KEYWORD_TABLE_PATTERN = /\b(?:table|to_table|from_table):\s*[:"']?(\w+)/ + + # Ruby `#` comments up to end of line, but not `#{}` interpolation. + COMMENT_PATTERN = /#(?!\{).*/ + # AR class-level query/mutation methods that strongly signal a constant is a model. # Kept narrow on purpose — methods like `find`, `all`, `first`, `count`, `create`, # `new`, `transaction`, `order`, `includes` collide with stdlib/non-model constants @@ -60,6 +74,11 @@ def self.extract_tables(filepath) def self.extract_tables_from_content(content) return [] if content.nil? || content.empty? + # Comments would otherwise produce phantom tables — an explanatory + # "# ALTER TABLE foo ..." or a commented-out create_table reads exactly + # like the real thing to the patterns below. + content = content.gsub(COMMENT_PATTERN, "") + tables = [] content.scan(SINGLE_TABLE_PATTERN) { |match| tables << match[0] } @@ -67,6 +86,7 @@ def self.extract_tables_from_content(content) SQL_TABLE_PATTERNS.each do |pattern| content.scan(pattern) { |match| tables << match[0] } end + content.scan(KEYWORD_TABLE_PATTERN) { |match| tables << match[0] } tables.uniq! # Model-based detection is heuristic (a constant that happens to respond diff --git a/spec/railbow/migration_parser_spec.rb b/spec/railbow/migration_parser_spec.rb index 31f24c1..5556b04 100644 --- a/spec/railbow/migration_parser_spec.rb +++ b/spec/railbow/migration_parser_spec.rb @@ -172,6 +172,79 @@ def write_migration(content) end end + describe "keyword-argument table detection" do + it "extracts table from a helper call with table: keyword and symbol value" do + content = "convert_to_monthly_partitions(table: :apples, indexes: INDEXES)" + expect(described_class.extract_tables_from_content(content)).to eq(["apples"]) + end + + it "extracts table from table: keyword with string value" do + content = 'convert_to_monthly_partitions(table: "apples")' + expect(described_class.extract_tables_from_content(content)).to eq(["apples"]) + end + + it "extracts both tables from remove_foreign_key with to_table: keyword" do + content = "remove_foreign_key :accounts, to_table: :owners" + expect(described_class.extract_tables_from_content(content)).to contain_exactly("accounts", "owners") + end + + it "does not extract from:/to: values as tables" do + content = 'change_column_default :users, :status, from: nil, to: "active"' + expect(described_class.extract_tables_from_content(content)).to eq(["users"]) + end + + it "counts keyword tables toward the model-detection guard" do + content = <<~RUBY + partition_helper(table: :orders) + partition_helper(table: :users) + Setting.find_each { |s| s.update_column(:x, 1) } + RUBY + expect(described_class.extract_tables_from_content(content)).to contain_exactly("orders", "users") + end + + it "extracts table from a realistic partition-conversion migration" do + content = <<~RUBY + class PartitionApplesByMonth < ActiveRecord::Migration[8.1] + include PartitionConversion + + # Rewrites the table into a partitioned one (ALTER TABLE is not enough: + # postgres cannot convert a plain table in place). + INDEXES = [ + [:banana_id, "index_apples_on_banana_id"], + [:created_at, "index_apples_on_created_at"] + ].freeze + + def up + convert_to_monthly_partitions(table: :apples, indexes: INDEXES) + end + end + RUBY + expect(described_class.extract_tables_from_content(content)).to eq(["apples"]) + end + end + + describe "comment stripping" do + it "ignores SQL keywords inside comments" do + content = <<~RUBY + # This used to run ALTER TABLE ghosts, kept for reference. + def up; end + RUBY + expect(described_class.extract_tables_from_content(content)).to eq([]) + end + + it "ignores commented-out DSL calls" do + content = "# create_table :ghosts do |t|; end" + expect(described_class.extract_tables_from_content(content)).to eq([]) + end + + it "keeps content after string interpolation on the same line" do + content = <<~'RUBY' + execute "COMMENT ON TABLE #{tbl} IS 'x'; UPDATE settings SET a = 1" + RUBY + expect(described_class.extract_tables_from_content(content)).to eq(["settings"]) + end + end + describe "model-based table detection" do it "infers table name from ActiveRecord model with find_each" do content = <<~RUBY