Add new Rails/ReversibleMigrationMethodDefinition cop

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

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

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]

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
 
 |===

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

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'

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