diff --git a/CHANGELOG.md b/CHANGELOG.md index 37b11eab6f..16102af1ea 100644 --- a/CHANGELOG.md +++ b/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][]) diff --git a/config/default.yml b/config/default.yml index 3693828e89..ce9004ee4f 100644 --- a/config/default.yml +++ b/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 diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index ed6839853b..b12d3d082d 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/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] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index e9fa34d385..d085c6df25 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/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 |=== diff --git a/lib/rubocop/cop/rails/active_record_callbacks_order.rb b/lib/rubocop/cop/rails/active_record_callbacks_order.rb new file mode 100644 index 0000000000..c105b60cf5 --- /dev/null +++ b/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 = '`%s` is supposed to appear before `%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 diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 61f6ef833d..7918a81cdf 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/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' diff --git a/spec/rubocop/cop/rails/active_record_callbacks_order_spec.rb b/spec/rubocop/cop/rails/active_record_callbacks_order_spec.rb new file mode 100644 index 0000000000..bd7e1f2931 --- /dev/null +++ b/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