Merge pull request #285 from fatkodima/active-record-callbacks-order-cop

Add new `Rails/ActiveRecordCallbacksOrder` cop

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### New features
 
 * [#283](https://github.com/rubocop-hq/rubocop-rails/pull/283): Add new `Rails/FindById` cop. ([@fatkodima][])
+* [#285](https://github.com/rubocop-hq/rubocop-rails/pull/285): Add new `Rails/ActiveRecordCallbacksOrder` cop. ([@fatkodima][])
 * [#276](https://github.com/rubocop-hq/rubocop-rails/pull/276): Add new `Rails/RenderPlainText` cop. ([@fatkodima][])
 * [#76](https://github.com/rubocop-hq/rubocop-rails/issues/76): Add new `Rails/DefaultScope` cop. ([@fatkodima][])
 * [#275](https://github.com/rubocop-hq/rubocop-rails/pull/275): Add new `Rails/MatchRoute` cop. ([@fatkodima][])

config/default.yml

@@ -37,6 +37,14 @@ Rails/ActiveRecordAliases:
   VersionAdded: '0.53'
   SafeAutoCorrect: false
 
+Rails/ActiveRecordCallbacksOrder:
+  Description: 'Order callback declarations in the order in which they will be executed.'
+  StyleGuide: 'https://rails.rubystyle.guide/#callbacks-order'
+  Enabled: 'pending'
+  VersionAdded: '2.7'
+  Include:
+    - app/models/**/*.rb
+
 Rails/ActiveRecordOverride:
   Description: >-
                   Check for overriding Active Record methods instead of using

docs/modules/ROOT/pages/cops.adoc

@@ -4,6 +4,7 @@
 
 * xref:cops_rails.adoc#railsactionfilter[Rails/ActionFilter]
 * xref:cops_rails.adoc#railsactiverecordaliases[Rails/ActiveRecordAliases]
+* xref:cops_rails.adoc#railsactiverecordcallbacksorder[Rails/ActiveRecordCallbacksOrder]
 * xref:cops_rails.adoc#railsactiverecordoverride[Rails/ActiveRecordOverride]
 * xref:cops_rails.adoc#railsactivesupportaliases[Rails/ActiveSupportAliases]
 * xref:cops_rails.adoc#railsapplicationcontroller[Rails/ApplicationController]

docs/modules/ROOT/pages/cops_rails.adoc

@@ -89,6 +89,52 @@ Book.update_attributes!(author: 'Alice')
 Book.update!(author: 'Alice')
 ----
 
+== Rails/ActiveRecordCallbacksOrder
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Pending
+| Yes
+| Yes
+| 2.7
+| -
+|===
+
+This cop checks that Active Record callbacks are declared
+in the order in which they will be executed.
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+class Person < ApplicationRecord
+  after_commit :after_commit_callback
+  before_validation :before_validation_callback
+end
+
+# good
+class Person < ApplicationRecord
+  before_validation :before_validation_callback
+  after_commit :after_commit_callback
+end
+----
+
+=== Configurable attributes
+
+|===
+| Name | Default value | Configurable values
+
+| Include
+| `app/models/**/*.rb`
+| Array
+|===
+
+=== References
+
+* https://rails.rubystyle.guide/#callbacks-order
+
 == Rails/ActiveRecordOverride
 
 |===

lib/rubocop/cop/rails/active_record_callbacks_order.rb

@@ -0,0 +1,145 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # This cop checks that Active Record callbacks are declared
+      # in the order in which they will be executed.
+      #
+      # @example
+      #   # bad
+      #   class Person < ApplicationRecord
+      #     after_commit :after_commit_callback
+      #     before_validation :before_validation_callback
+      #   end
+      #
+      #   # good
+      #   class Person < ApplicationRecord
+      #     before_validation :before_validation_callback
+      #     after_commit :after_commit_callback
+      #   end
+      #
+      class ActiveRecordCallbacksOrder < Cop
+        MSG = '`%<current>s` is supposed to appear before `%<previous>s`.'
+
+        CALLBACKS_IN_ORDER = %i[
+          after_initialize
+          before_validation
+          after_validation
+          before_save
+          around_save
+          before_create
+          around_create
+          after_create
+          before_update
+          around_update
+          after_update
+          before_destroy
+          around_destroy
+          after_destroy
+          after_save
+          after_commit
+          after_rollback
+          after_find
+          after_touch
+        ].freeze
+
+        CALLBACKS_ORDER_MAP = Hash[
+          CALLBACKS_IN_ORDER.map.with_index { |name, index| [name, index] }
+        ].freeze
+
+        def on_class(class_node)
+          previous_index = -1
+          previous_callback = nil
+
+          defined_callbacks(class_node).each do |node|
+            callback = node.method_name
+            index = CALLBACKS_ORDER_MAP[callback]
+
+            if index < previous_index
+              message = format(MSG, current: callback,
+                                    previous: previous_callback)
+              add_offense(node, message: message)
+            end
+            previous_index = index
+            previous_callback = callback
+          end
+        end
+
+        # Autocorrect by swapping between two nodes autocorrecting them
+        def autocorrect(node)
+          previous = left_siblings_of(node).find do |sibling|
+            callback?(sibling)
+          end
+
+          current_range = source_range_with_comment(node)
+          previous_range = source_range_with_comment(previous)
+
+          lambda do |corrector|
+            corrector.insert_before(previous_range, current_range.source)
+            corrector.remove(current_range)
+          end
+        end
+
+        private
+
+        def defined_callbacks(class_node)
+          class_def = class_node.body
+
+          if class_def
+            class_def.each_child_node.select { |c| callback?(c) }
+          else
+            []
+          end
+        end
+
+        def callback?(node)
+          node.send_type? && CALLBACKS_ORDER_MAP.key?(node.method_name)
+        end
+
+        def left_siblings_of(node)
+          siblings_of(node)[0, node.sibling_index]
+        end
+
+        def siblings_of(node)
+          node.parent.children
+        end
+
+        def source_range_with_comment(node)
+          begin_pos = begin_pos_with_comment(node)
+          end_pos = end_position_for(node)
+
+          Parser::Source::Range.new(buffer, begin_pos, end_pos)
+        end
+
+        def end_position_for(node)
+          end_line = buffer.line_for_position(node.loc.expression.end_pos)
+          buffer.line_range(end_line).end_pos
+        end
+
+        def begin_pos_with_comment(node)
+          annotation_line = node.first_line - 1
+          first_comment = nil
+
+          processed_source.comments_before_line(annotation_line)
+                          .reverse_each do |comment|
+            if comment.location.line == annotation_line
+              first_comment = comment
+              annotation_line -= 1
+            end
+          end
+
+          start_line_position(first_comment || node)
+        end
+
+        def start_line_position(node)
+          buffer.line_range(node.loc.line).begin_pos - 1
+        end
+
+        def buffer
+          processed_source.buffer
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -6,6 +6,7 @@
 
 require_relative 'rails/action_filter'
 require_relative 'rails/active_record_aliases'
+require_relative 'rails/active_record_callbacks_order'
 require_relative 'rails/active_record_override'
 require_relative 'rails/active_support_aliases'
 require_relative 'rails/application_controller'

spec/rubocop/cop/rails/active_record_callbacks_order_spec.rb

@@ -0,0 +1,110 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::ActiveRecordCallbacksOrder do
+  subject(:cop) { described_class.new }
+
+  it 'registers an offense and corrects when declared callbacks are not correctly ordered' do
+    expect_offense(<<~RUBY)
+      class User < ApplicationRecord
+        scope :admins, -> { where(admin: true) }
+
+        after_commit :after_commit_callback
+        after_save :after_save_callback
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `after_save` is supposed to appear before `after_commit`.
+
+        def some_method
+        end
+
+        before_validation :before_validation_callback
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `before_validation` is supposed to appear before `after_save`.
+        some_other_macros :foo
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      class User < ApplicationRecord
+        scope :admins, -> { where(admin: true) }
+
+        before_validation :before_validation_callback
+        after_save :after_save_callback
+        after_commit :after_commit_callback
+
+        def some_method
+        end
+
+        some_other_macros :foo
+      end
+    RUBY
+  end
+
+  it 'correcly autocorrects when there is a comment for callback method' do
+    new_source = autocorrect_source(<<~RUBY)
+      class User < ApplicationRecord
+        # This is a
+        # multiline
+        # comment for after_commit.
+        after_commit :after_commit_callback
+        # This is another
+        # multiline
+        # comment for after_save.
+        after_save :after_save_callback
+      end
+    RUBY
+
+    expect(new_source).to eq(<<~RUBY)
+      class User < ApplicationRecord
+        # This is another
+        # multiline
+        # comment for after_save.
+        after_save :after_save_callback
+        # This is a
+        # multiline
+        # comment for after_commit.
+        after_commit :after_commit_callback
+      end
+    RUBY
+  end
+
+  it 'correcly autocorrects when there are multiple callbacks of the same type' do
+    new_source = autocorrect_source(<<~RUBY)
+      class User < ApplicationRecord
+        after_commit :after_commit_callback1
+        after_save :after_save_callback
+        after_commit :after_commit_callback2
+      end
+    RUBY
+
+    expect(new_source).to eq(<<~RUBY)
+      class User < ApplicationRecord
+        after_save :after_save_callback
+        after_commit :after_commit_callback1
+        after_commit :after_commit_callback2
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when declared callbacks are correctly ordered' do
+    expect_no_offenses(<<~RUBY)
+      class User < ApplicationRecord
+        scope :admins, -> { where(admin: true) }
+
+        before_validation :before_validation_callback
+        after_save :after_save_callback
+
+        def some_method
+        end
+
+        after_commit :after_commit_callback
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when there are no callbacks' do
+    expect_no_offenses(<<~RUBY)
+      class User < ApplicationRecord
+        def some_method
+        end
+      end
+    RUBY
+  end
+end