Add new Rails/ReversibleMigrationMethodDefinition cop

[hey-leon]

This cop attempts to automate part of the review process of rails migrations. Two example cases this cop helps prevent are:

1. a typo in a change method e.g. chance instead of change
2. a migration that only has one direction implemented.

These can both be a bad time when not caught before merging to production.

Case 1 can lead to the deployment of code that relies on a column that has not been added yet.
Case 2 can lead to hard to revert faulty deployments.

Rails/ReversibleMigrationMethodDefinition cop

   # 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
 
    class SomeMigration < ActiveRecord::Migration[6.0]
      def up
        # up migration
      end
 
      def down
        # down migration
      end
    end

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.
• If this is a new cop, consider making a corresponding update to the Rails Style Guide.

[andyw8]

I haven't tried it, but would https://github.com/ankane/strong_migrations help prevent any of these issues?

[hey-leon]

I haven't tried it, but would https://github.com/ankane/strong_migrations help prevent any of these issues?

Good thought. I don't believe it can be configured to catch these cases (we use it at our company and have these issues).

From what I understand strong migrations uses instrumentation to enforce best practices. I don't think it's possible to catch these types of issues with instrumentation as it cannot catch issues on methods not called?

[koic]

Suggested change

	* [#457](https://github.com/rubocop/rubocop-rails/pull/457): New cop `Rails/ImplementedMigration` checks for migrations that have a typo in a method name or are half implemented. ([@leonp1991][])
	* [#457](https://github.com/rubocop/rubocop-rails/pull/457): Add new `Rails/ReversibleMigrationMethodDefinition` cop. ([@leonp1991][])

[koic]

Can you update the description?

Suggested change

	Description: 'Checks for migrations that have a typo in a method name or are half implemented.'
	Description: 'Checks whether the migration implements either a `change` method or both an `up` and a `down` method.

[koic]

Can you add Include path?

Suggested change

	VersionAdded: '2.10'
	VersionAdded: '2.10'
	Include:
	- db/migrate/*.rb

[koic]

I think typos are not the main focus of the cop, so can you remove this example?

[koic]

Suggested change

	# migration
	# reversible migration

[koic]

The name ImplementedMigration looks abstract in purpose. How about the name ReversibleMigrationMethodDefinition?

Suggested change

	class ImplementedMigration < Base
	class ReversibleMigrationMethodDefinition < Base

[koic]

Can you enclose method names in backticks?

Suggested change

	MSG = 'Migrations must contain either a change method, or ' \
	'both an up and a down method.'
	MSG = 'Migrations must contain either a `change` method, or ' \
	'both an `up` and a `down` method.'

[koic]

Can you refactor it?

Suggested change

	return if change_method?(node)
	return if up_and_down_methods?(node)
	return if change_method?(node) || up_and_down_methods?(node)

[koic]

Suggested change

	it 'registers an offense with a typo\'d change method' do
	it "registers an offense with a typo'd change method" do

[koic]

Can you remove the redundant blank line in this PR' test examples?

Suggested change

	class SomeMigration < ActiveRecord::Migration[6.0]
	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a change method, or both an up and a down method.
	def up
	class SomeMigration < ActiveRecord::Migration[6.0]
	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a change method, or both an up and a down method.
	def up

[koic]

Can you remove the redundant test case and leave only one of them?

Suggested change

	expect_offense(<<~RUBY)
	class SomeMigration < ActiveRecord::Migration[5.1]
	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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
	expect_offense(<<~RUBY)
	class SomeMigration < ActiveRecord::Migration[7.1]
	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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
	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

[koic]

Ditto :-)

Suggested change

	expect_no_offenses(<<~RUBY)
	class SomeMigration < ActiveRecord::Migration[5.2]
	def change
	add_column :users, :email, :text, null: false
	end
	end
	RUBY
	expect_no_offenses(<<~RUBY)
	class SomeMigration < ActiveRecord::Migration[7.1]
	def change
	add_column :users, :email, :text, null: false
	end
	end
	RUBY
	expect_no_offenses(<<~RUBY)
	class SomeMigration < ActiveRecord::Migration[5.2]
	def change
	add_column :users, :email, :text, null: false
	end
	end
	RUBY

[koic]

Rails has not officially maintained Rails 5.1 series.
https://guides.rubyonrails.org/maintenance_policy.html

RuboCop Rails also has negative support, so please specify it to 5.2 or higher for the example.

[koic]

That's just my two cents. Rails 7.1 is a world no one has seen yet 😅 Can you update to 6.1?

Suggested change

	class SomeMigration < ActiveRecord::Migration[7.1]
	class SomeMigration < ActiveRecord::Migration[6.1]

[hey-leon]

lol this is true it is not in the world yet and may not ever be a specific version (however likely). Was just trying to make sure it wont break on a future release 🤠.

[hey-leon]

@koic I appreciate all the feedback. I think the new name has made the cop's intention much clearer. The latest version should address all comments and anywhere else that made sense after.

[koic]

Thanks!

[aki77]

Include typo.

[rhymes]

Yep, without it it flags all Ruby files :=)

[hey-leon]

😅 My bad thanks @rhymes