diff --git a/changelog/new_in_order_of_cop.md b/changelog/new_in_order_of_cop.md new file mode 100644 index 0000000000..a3a2d8ee3e --- /dev/null +++ b/changelog/new_in_order_of_cop.md @@ -0,0 +1 @@ +* [#1554](https://github.com/rubocop/rubocop-rails/pull/1554): Add new `Rails/InOrderOf` cop to suggest using `in_order_of` for ordering instead of manually constructing SQL. ([@davidenglishmusic][]) diff --git a/config/default.yml b/config/default.yml index 65568b6913..47457b7df3 100644 --- a/config/default.yml +++ b/config/default.yml @@ -665,6 +665,12 @@ Rails/IgnoredSkipActionFilterOption: - '**/app/controllers/**/*.rb' - '**/app/mailers/**/*.rb' +Rails/InOrderOf: + Description: 'Use `in_order_of` for ordering instead of manually constructing SQL.' + Reference: 'https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-in_order_of' + Enabled: pending + VersionAdded: '<>' + Rails/IndexBy: Description: 'Prefer `index_by` over `each_with_object`, `to_h`, or `map`.' Enabled: true diff --git a/lib/rubocop/cop/rails/in_order_of.rb b/lib/rubocop/cop/rails/in_order_of.rb new file mode 100644 index 0000000000..9f6cb17f71 --- /dev/null +++ b/lib/rubocop/cop/rails/in_order_of.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Checks for manual construction of ordering logic and suggests using `in_order_of` instead. + # + # `in_order_of` was introduced in Rails 7.0 to allow custom ordering of records + # without using raw SQL with `CASE` statements or `Arel.sql`. + # + # @example + # # bad + # Post.order( + # Arel.sql( + # <<~SQL + # case status + # when 'draft' then 1 + # when 'published' then 2 + # when 'archived' then 3 + # else 4 end + # SQL + # ) + # ) + # + # # good + # Post.in_order_of(:status, %w[draft published archived]) + # + # # bad - using MySQL FIELD function + # Post.order( + # Arel.sql("FIELD(status, 'draft', 'published', 'archived')") + # ) + # + # # good + # Post.in_order_of(:status, %w[draft published archived]) + # + # # bad - with string interpolation + # Post.order(Arel.sql("FIELD(status, '#{statuses.join("', '")}')")) + # + # # good + # Post.in_order_of(:status, statuses) + # + class InOrderOf < Base + extend TargetRailsVersion + + minimum_target_rails_version 7.0 + + MSG = 'Use `in_order_of` for ordering instead of manually constructing SQL.' + RESTRICT_ON_SEND = %i[order].freeze + + # Matches: case column when ... or case #{var} when ... + CASE_WHEN_PATTERN = /\bcase\s+(\w+|#\{[^}]+\})\s+when\b/im.freeze + # Matches: FIELD(column, ...) + FIELD_FUNCTION_PATTERN = /\bFIELD\s*\(/i.freeze + + AREL_CONST_NAME = 'Arel' + HEREDOC_START = '<<' + + def on_send(node) + return unless node.method?(:order) + return unless (sql_string = extract_sql_string(node)) + return unless custom_order_sql?(sql_string) + + add_offense(node.loc.selector) + end + + private + + def extract_sql_string(node) + sql_arg = sql_argument(node) + extract_string_content(sql_arg) if sql_arg + end + + def sql_argument(node) + arg = node.first_argument + return unless arg&.send_type? && arg.method?(:sql) + return unless arel_const?(arg.receiver) + + arg.first_argument if arg.first_argument&.type?(:str, :dstr) + end + + def extract_string_content(sql_arg) + case sql_arg.type + when :str then sql_arg.str_content + when :dstr then extract_dstr_content(sql_arg) + end + end + + def extract_dstr_content(sql_arg) + source = sql_arg.source + source.start_with?(HEREDOC_START) ? heredoc_content(sql_arg) : source[1...-1] + end + + def heredoc_content(sql_arg) + sql_arg.each_child_node(:str).map(&:str_content).join + end + + def arel_const?(node) + node&.const_type? && node.const_name == AREL_CONST_NAME + end + + def custom_order_sql?(sql_string) + sql_string.match?(CASE_WHEN_PATTERN) || sql_string.match?(FIELD_FUNCTION_PATTERN) + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index e3cea8a378..2e6f9860cb 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -73,6 +73,7 @@ require_relative 'rails/i18n_locale_texts' require_relative 'rails/ignored_columns_assignment' require_relative 'rails/ignored_skip_action_filter_option' +require_relative 'rails/in_order_of' require_relative 'rails/index_by' require_relative 'rails/index_with' require_relative 'rails/inquiry' diff --git a/spec/rubocop/cop/rails/in_order_of_spec.rb b/spec/rubocop/cop/rails/in_order_of_spec.rb new file mode 100644 index 0000000000..6cade8282e --- /dev/null +++ b/spec/rubocop/cop/rails/in_order_of_spec.rb @@ -0,0 +1,130 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::InOrderOf, :config do + context 'Rails >= 7.0', :rails70 do + context 'when using Arel.sql with CASE statement' do + it 'registers an offense' do + expect_offense(<<~RUBY) + Post.order( + ^^^^^ Use `in_order_of` for ordering instead of manually constructing SQL. + Arel.sql( + <<~SQL + case status + when 'draft' then 1 + when 'published' then 2 + when 'archived' then 3 + else 4 end + SQL + ) + ) + RUBY + end + end + + context 'when using Arel.sql with inline CASE statement' do + it 'registers an offense' do + expect_offense(<<~RUBY) + User.order(Arel.sql("CASE status WHEN 'active' THEN 1 ELSE 2 END")) + ^^^^^ Use `in_order_of` for ordering instead of manually constructing SQL. + RUBY + end + end + + context 'when using Arel.sql with FIELD function' do + it 'registers an offense' do + expect_offense(<<~RUBY) + Post.order(Arel.sql("FIELD(status, 'draft', 'published', 'archived')")) + ^^^^^ Use `in_order_of` for ordering instead of manually constructing SQL. + RUBY + end + end + + context 'when using ::Arel.sql with CASE statement' do + it 'registers an offense' do + expect_offense(<<~RUBY) + Post.order(::Arel.sql("case priority when 'high' then 1 else 2 end")) + ^^^^^ Use `in_order_of` for ordering instead of manually constructing SQL. + RUBY + end + end + + context 'when using Arel.sql without CASE or FIELD' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + User.order(Arel.sql("created_at DESC")) + RUBY + end + end + + context 'when using regular order methods' do + it 'does not register an offense with symbol' do + expect_no_offenses(<<~RUBY) + User.order(:created_at) + RUBY + end + + it 'does not register an offense with hash' do + expect_no_offenses(<<~RUBY) + User.order(created_at: :desc) + RUBY + end + + it 'does not register an offense with string' do + expect_no_offenses(<<~RUBY) + User.order('created_at DESC') + RUBY + end + end + + context 'when using in_order_of' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + Post.in_order_of(:status, %w[draft published archived]) + RUBY + end + end + + context 'when using Arel.sql with FIELD and string interpolation' do + it 'registers an offense' do + expect_offense(<<~'RUBY') + TITLES = ['foo', 'bar'] + Post.order(Arel.sql("FIELD(title,'#{TITLES.join(",")}')")) + ^^^^^ Use `in_order_of` for ordering instead of manually constructing SQL. + RUBY + end + + it 'registers an offense with join using single quotes' do + expect_offense(<<~'RUBY') + Post.order(Arel.sql("FIELD(status,'#{UNPUBLISHED_STATES.join("','")}')")) + ^^^^^ Use `in_order_of` for ordering instead of manually constructing SQL. + RUBY + end + + it 'registers an offense with exact user pattern' do + expect_offense(<<~'RUBY') + TITLES = ['foo', 'bar'] + Post.order(Arel.sql("FIELD(title,'#{TITLES.join("','")}')")) + ^^^^^ Use `in_order_of` for ordering instead of manually constructing SQL. + RUBY + end + end + + context 'when using Arel.sql with CASE and string interpolation' do + it 'registers an offense' do + expect_offense(<<~'RUBY') + column = 'status' + User.order(Arel.sql("CASE #{column} WHEN 'active' THEN 1 ELSE 2 END")) + ^^^^^ Use `in_order_of` for ordering instead of manually constructing SQL. + RUBY + end + end + end + + context 'Rails <= 6.1', :rails61 do + it 'does not register an offense for Arel.sql with CASE' do + expect_no_offenses(<<~RUBY) + Post.order(Arel.sql("case status when 'draft' then 1 else 2 end")) + RUBY + end + end +end