diff --git a/CHANGELOG.md b/CHANGELOG.md index a356ec6..bde649b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ # main +* Added cop `Migration/ChangeTableReferences` ([#47](https://github.com/petalmd/rubocop-petal/pull/47)) * Added cop `Migration/AlwaysBulkChangeTable` ([#46](https://github.com/petalmd/rubocop-petal/pull/46)) # v1.0.0 (2023-05-15) diff --git a/README.md b/README.md index 9c81684..c379d52 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ bundle exec rake 'new_cop[Rails/MyNewCop]' ``` Have a look to [RuboCop documentation](https://docs.rubocop.org/rubocop/1.23/development.html) to get started with -_node pattern_. +_node pattern_. [Rubocop AST](https://github.com/rubocop/rubocop-ast/blob/master/docs/modules/ROOT/pages/node_pattern.adoc) ## Release diff --git a/config/default.yml b/config/default.yml index bab392a..1b7f252 100644 --- a/config/default.yml +++ b/config/default.yml @@ -11,6 +11,12 @@ Migration/AlwaysBulkChangeTable: Include: - db/migrate/** +Migration/ChangeTableReferences: + Description: 'Prevent using `t.references` or `t.belongs_to` in a change_table.' + Enabled: true + Include: + - db/migrate/** + Migration/ForeignKeyOption: Description: 'Specify the foreign key option to create the constraint.' Enabled: true diff --git a/lib/rubocop/cop/migration/change_table_references.rb b/lib/rubocop/cop/migration/change_table_references.rb new file mode 100644 index 0000000..acdc807 --- /dev/null +++ b/lib/rubocop/cop/migration/change_table_references.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Migration + # Prevent using `t.references` or `t.belongs_to` in a change_table. + # Internally, `t.references` use multiple `ALTER TABLE` statements. + # Since Rails cannot transform automatically `t.references` inside + # a `change_table bulk: true` we can manually create the equivalent + # `ALTER TABLE` statement using `t.bigint`, `t.index` and `t.foreign_key`. + # + # #bad + # change_table :subscriptions, bulk: true do |t| + # t.references :user, null: false, foreign_key: true + # end + # + # #good + # change_table :subscriptions, bulk: true do |t| + # t.bigint :user_id, null: false + # t.index :user_id + # t.foreign_key :users, column: :user_id + # end + class ChangeTableReferences < Base + MSG = 'Use a combination of `t.bigint`, `t.index` and `t.foreign_key` in a change_table.' + + # @!method add_references_in_block?(node) + def_node_search :add_references_in_block?, <<~PATTERN + (send lvar /references|belongs_to/ ...) + PATTERN + + # @!method change_table?(node) + def_node_search :change_table?, <<~PATTERN + (send nil? :change_table ...) + PATTERN + + def on_block(node) + return unless change_table?(node) + + references_node = node.children.detect { |n| add_references_in_block?(n) } + return unless references_node + + arguments = references_node.child_nodes[1] + references_methods_range = references_node.source_range.with(end_pos: arguments.source_range.begin_pos - 1) + add_offense(references_methods_range) + end + end + end + end +end diff --git a/spec/rubocop/cop/migration/change_table_references_spec.rb b/spec/rubocop/cop/migration/change_table_references_spec.rb new file mode 100644 index 0000000..c45d7ea --- /dev/null +++ b/spec/rubocop/cop/migration/change_table_references_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Migration::ChangeTableReferences, :config do + it 'registers an offense when using references in a change_table' do + expect_offense(<<~RUBY) + change_table :subscriptions, bulk: true do |t| + t.references :user, null: false, foreign_key: true + ^^^^^^^^^^^^ Use a combination of `t.bigint`, `t.index` and `t.foreign_key` in a change_table. + end + RUBY + + expect_offense(<<~RUBY) + change_table :subscriptions, bulk: true do |t| + t.belongs_to :user, null: false, foreign_key: true + ^^^^^^^^^^^^ Use a combination of `t.bigint`, `t.index` and `t.foreign_key` in a change_table. + end + RUBY + end + + it 'does not register an offense when using references in a create_table' do + expect_no_offenses(<<~RUBY) + create_table :subscriptions do |t| + t.belongs_to :user, null: false, foreign_key: true + end + RUBY + end + + it 'does not register an offense when not using references in a create_table' do + expect_no_offenses(<<~RUBY) + change_table :subscriptions do |t| + t.index :user_id + end + RUBY + end +end