diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b38d2d5cf..b892536a23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### New features +* [#457](https://github.com/rubocop/rubocop-rails/pull/457): Add new `Rails/ReversibleMigrationMethodDefinition` cop. ([@leonp1991][]) * [#446](https://github.com/rubocop/rubocop-rails/issues/446): Add new `Rails/RequireDependency` cop. ([@tubaxenor][]) * [#458](https://github.com/rubocop/rubocop-rails/issues/458): Add new `Rails/TimeZoneAssignment` cop. ([@olivierbuffon][]) @@ -353,3 +354,4 @@ [@ohbarye]: https://github.com/ohbarye [@tubaxenor]: https://github.com/tubaxenor [@olivierbuffon]: https://github.com/olivierbuffon +[@leonp1991]: https://github.com/leonp1991 diff --git a/config/default.yml b/config/default.yml index 139dd21743..5572cf1f48 100644 --- a/config/default.yml +++ b/config/default.yml @@ -620,6 +620,13 @@ Rails/ReversibleMigration: Include: - db/migrate/*.rb +Rails/ReversibleMigrationMethodDefinition: + Description: 'Checks whether the migration implements either a `change` method or both an `up` and a `down` method.' + Enabled: false + VersionAdded: '2.10' + include: + - db/migrate/*.rb + Rails/SafeNavigation: Description: "Use Ruby's safe navigation operator (`&.`) instead of `try!`." Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index fabcdae60d..eb92578364 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -86,6 +86,7 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide]. * xref:cops_rails.adoc#railsrequestreferer[Rails/RequestReferer] * xref:cops_rails.adoc#railsrequiredependency[Rails/RequireDependency] * xref:cops_rails.adoc#railsreversiblemigration[Rails/ReversibleMigration] +* xref:cops_rails.adoc#railsreversiblemigrationmethoddefinition[Rails/ReversibleMigrationMethodDefinition] * xref:cops_rails.adoc#railssafenavigation[Rails/SafeNavigation] * xref:cops_rails.adoc#railssafenavigationwithblank[Rails/SafeNavigationWithBlank] * xref:cops_rails.adoc#railssavebang[Rails/SaveBang] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index 8334dff47e..f9fc7b11b9 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -3694,6 +3694,72 @@ end * https://rails.rubystyle.guide#reversible-migration * https://api.rubyonrails.org/classes/ActiveRecord/Migration/CommandRecorder.html +== Rails/ReversibleMigrationMethodDefinition + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Disabled +| Yes +| No +| 2.10 +| - +|=== + +This cop checks whether the migration implements +either a `change` method or both an `up` and a `down` +method. + +=== Examples + +[source,ruby] +---- +# bad +class SomeMigration < ActiveRecord::Migration[6.0] + def up + # up migration + end + + # <----- missing down method +end + +class SomeMigration < ActiveRecord::Migration[6.0] + # <----- missing up method + + def down + # down migration + end +end + +# good +class SomeMigration < ActiveRecord::Migration[6.0] + def change + # reversible migration + end +end + +# good +class SomeMigration < ActiveRecord::Migration[6.0] + def up + # up migration + end + + def down + # down migration + end +end +---- + +=== Configurable attributes + +|=== +| Name | Default value | Configurable values + +| include +| `db/migrate/*.rb` +| Array +|=== + == Rails/SafeNavigation |=== diff --git a/lib/rubocop/cop/rails/reversible_migration_method_definition.rb b/lib/rubocop/cop/rails/reversible_migration_method_definition.rb new file mode 100644 index 0000000000..4a2b90d1ba --- /dev/null +++ b/lib/rubocop/cop/rails/reversible_migration_method_definition.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # This cop checks whether the migration implements + # either a `change` method or both an `up` and a `down` + # method. + # + # @example + # # bad + # class SomeMigration < ActiveRecord::Migration[6.0] + # def up + # # up migration + # end + # + # # <----- missing down method + # end + # + # class SomeMigration < ActiveRecord::Migration[6.0] + # # <----- missing up method + # + # def down + # # down migration + # end + # end + # + # # good + # class SomeMigration < ActiveRecord::Migration[6.0] + # def change + # # reversible migration + # end + # end + # + # # good + # class SomeMigration < ActiveRecord::Migration[6.0] + # def up + # # up migration + # end + # + # def down + # # down migration + # end + # end + class ReversibleMigrationMethodDefinition < Base + MSG = 'Migrations must contain either a `change` method, or ' \ + 'both an `up` and a `down` method.' + + def_node_matcher :migration_class?, <<~PATTERN + (class + (const nil? _) + (send + (const (const nil? :ActiveRecord) :Migration) + :[] + (float _)) + _) + PATTERN + + def_node_matcher :change_method?, <<~PATTERN + [ #migration_class? `(def :change (args) _) ] + PATTERN + + def_node_matcher :up_and_down_methods?, <<~PATTERN + [ #migration_class? `(def :up (args) _) `(def :down (args) _) ] + PATTERN + + def on_class(node) + return if change_method?(node) || up_and_down_methods?(node) + + add_offense(node) + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 3c602d9182..4f8321a134 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -75,6 +75,7 @@ require_relative 'rails/request_referer' require_relative 'rails/require_dependency' require_relative 'rails/reversible_migration' +require_relative 'rails/reversible_migration_method_definition' require_relative 'rails/safe_navigation' require_relative 'rails/safe_navigation_with_blank' require_relative 'rails/save_bang' diff --git a/spec/rubocop/cop/rails/reversible_migration_method_definition_spec.rb b/spec/rubocop/cop/rails/reversible_migration_method_definition_spec.rb new file mode 100644 index 0000000000..32d5d00a78 --- /dev/null +++ b/spec/rubocop/cop/rails/reversible_migration_method_definition_spec.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::ReversibleMigrationMethodDefinition, :config do + it 'does not register an offense with a change method' do + expect_no_offenses(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[6.0] + def change + add_column :users, :email, :text, null: false + end + end + RUBY + end + + it 'registers an offense with only an up method' do + expect_offense(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[6.0] + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a `change` method, or both an `up` and a `down` method. + + def up + add_column :users, :email, :text, null: false + end + end + RUBY + end + + it 'registers an offense with only a down method' do + expect_offense(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[6.0] + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a `change` method, or both an `up` and a `down` method. + + def down + remove_column :users, :email + end + end + RUBY + end + + it 'does not register an offense with an up and a down method' do + expect_no_offenses(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[6.0] + def up + add_column :users, :email, :text, null: false + end + + def down + remove_column :users, :email + end + end + RUBY + end + + it "registers an offense with a typo'd change method" do + expect_offense(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[6.0] + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a `change` method, or both an `up` and a `down` method. + def chance + add_column :users, :email, :text, null: false + end + end + RUBY + end + + it 'does not register an offense with helper methods' do + expect_no_offenses(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[6.0] + def change + add_users_column :email, :text, null: false + end + + private + + def add_users_column(column_name, null: false) + add_column :users, column_name, type, null: null + end + end + RUBY + end + + it 'registers offenses correctly with any migration class' do + expect_offense(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[5.2] + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a `change` method, or both an `up` and a `down` method. + def chance + add_column :users, :email, :text, null: false + end + end + RUBY + end + + it 'does not register offenses correctly with any migration class' do + expect_no_offenses(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[5.2] + def change + add_column :users, :email, :text, null: false + end + end + RUBY + end +end