From 1cfeea9694e60a00c9993c79a73712f8c4762eea Mon Sep 17 00:00:00 2001 From: Phil Coggins Date: Fri, 17 Jul 2020 15:07:30 -0600 Subject: [PATCH] Register new offenses for --- CHANGELOG.md | 2 + docs/modules/ROOT/pages/cops_rails.adoc | 49 ++++++++++++ lib/rubocop/cop/rails/reversible_migration.rb | 79 +++++++++++++++++++ .../cop/rails/reversible_migration_spec.rb | 36 +++++++++ 4 files changed, 166 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c54c22db92..6bfb3e0f04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ ### Changes * [#312](https://github.com/rubocop-hq/rubocop-rails/pull/312): Mark `Rails/MailerName` as unsafe for auto-correct. ([@eugeneius][]) +* [#294](https://github.com/rubocop-hq/rubocop-rails/pull/294): Update `Rails/ReversibleMigration` to register offenses for `remove_columns` and `remove_index`. ([@philcoggins][]) ## 2.7.1 (2020-07-26) @@ -258,3 +259,4 @@ [@fatkodima]: https://github.com/fatkodima [@taylorthurlow]: https://github.com/taylorthurlow [@krim]: https://github.com/krim +[@philcoggins]: https://github.com/philcoggins diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index 872986ac01..ff94f25627 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -3379,6 +3379,55 @@ def change end ---- +[source,ruby] +---- +# remove_columns + +# bad +def change + remove_columns :users, :name, :email +end + +# good +def change + reversible do |dir| + dir.up do + remove_columns :users, :name, :email + end + + dir.down do + add_column :users, :name, :string + add_column :users, :name, :email + end + end +end + +# good (Rails >= 6.1, see https://github.com/rails/rails/pull/36589) +def change + remove_columns :users, :name, :email, type: :string +end +---- + +[source,ruby] +---- +# remove_index + +# bad +def change + remove_index :users, name: :index_users_on_email +end + +# good +def change + remove_index :users, :email +end + +# good +def change + remove_index :users, column: :email +end +---- + === Configurable attributes |=== diff --git a/lib/rubocop/cop/rails/reversible_migration.rb b/lib/rubocop/cop/rails/reversible_migration.rb index a221a40c51..04f39aef11 100644 --- a/lib/rubocop/cop/rails/reversible_migration.rb +++ b/lib/rubocop/cop/rails/reversible_migration.rb @@ -129,6 +129,51 @@ module Rails # end # end # + # @example + # # remove_columns + # + # # bad + # def change + # remove_columns :users, :name, :email + # end + # + # # good + # def change + # reversible do |dir| + # dir.up do + # remove_columns :users, :name, :email + # end + # + # dir.down do + # add_column :users, :name, :string + # add_column :users, :email, :string + # end + # end + # end + # + # # good (Rails >= 6.1, see https://github.com/rails/rails/pull/36589) + # def change + # remove_columns :users, :name, :email, type: :string + # end + # + # @example + # # remove_index + # + # # bad + # def change + # remove_index :users, name: :index_users_on_email + # end + # + # # good + # def change + # remove_index :users, :email + # end + # + # # good + # def change + # remove_index :users, column: :email + # end + # # @see https://api.rubyonrails.org/classes/ActiveRecord/Migration/CommandRecorder.html class ReversibleMigration < Cop MSG = '%s is not reversible.' @@ -153,6 +198,14 @@ class ReversibleMigration < Cop (send nil? :change_table $_ ...) PATTERN + def_node_matcher :remove_columns_call, <<~PATTERN + (send nil? :remove_columns ... $_) + PATTERN + + def_node_matcher :remove_index_call, <<~PATTERN + (send nil? :remove_index _ $_) + PATTERN + def on_send(node) return unless within_change_method?(node) return if within_reversible_or_up_only_block?(node) @@ -162,6 +215,8 @@ def on_send(node) check_reversible_hash_node(node) check_remove_column_node(node) check_remove_foreign_key_node(node) + check_remove_columns_node(node) + check_remove_index_node(node) end def on_block(node) @@ -237,6 +292,30 @@ def check_change_table_node(node, block) end end + def check_remove_columns_node(node) + remove_columns_call(node) do |args| + unless all_hash_key?(args, :type) && target_rails_version >= 6.1 + action = target_rails_version >= 6.1 ? 'remove_columns(without type)' : 'remove_columns' + + add_offense( + node, + message: format(MSG, action: action) + ) + end + end + end + + def check_remove_index_node(node) + remove_index_call(node) do |args| + if args.hash_type? && !all_hash_key?(args, :column) + add_offense( + node, + message: format(MSG, action: 'remove_index(without column)') + ) + end + end + end + def check_change_table_offense(receiver, node) method_name = node.method_name return if receiver != node.receiver && diff --git a/spec/rubocop/cop/rails/reversible_migration_spec.rb b/spec/rubocop/cop/rails/reversible_migration_spec.rb index e21071410b..e42a6644ff 100644 --- a/spec/rubocop/cop/rails/reversible_migration_spec.rb +++ b/spec/rubocop/cop/rails/reversible_migration_spec.rb @@ -212,4 +212,40 @@ def change end RUBY end + + context 'remove_columns' do + context 'Rails >= 6.1', :rails61 do + it_behaves_like 'accepts', 'remove_columns(with type)', <<~RUBY + remove_columns(:posts, :title, :body, type: :text) + RUBY + + it_behaves_like 'offense', 'remove_columns(without type)', <<~RUBY + remove_columns(:posts, :title, :body) + RUBY + end + + context 'Rails < 6.1', :rails60 do + it_behaves_like 'offense', 'remove_columns', <<~RUBY + remove_columns(:posts, :title, :body, type: :text) + RUBY + end + + it_behaves_like 'offense', 'remove_columns', <<~RUBY + remove_columns(:posts, :title, :body) + RUBY + end + + context 'remove_index' do + it_behaves_like 'accepts', 'remove_index (with column)', <<~RUBY + remove_index(:posts, column: :body) + RUBY + + it_behaves_like 'accepts', 'remove_index (with two args)', <<~RUBY + remove_index(:posts, :body) + RUBY + + it_behaves_like 'offense', 'remove_index(without column)', <<~RUBY + remove_index(:posts, name: :index_columns_on_body) + RUBY + end end