diff --git a/changelog/new_cop_rails_order_arguments.md b/changelog/new_cop_rails_order_arguments.md new file mode 100644 index 0000000000..1265538e9d --- /dev/null +++ b/changelog/new_cop_rails_order_arguments.md @@ -0,0 +1 @@ +* [#1501](https://github.com/rubocop/rubocop-rails/pull/1501): Add new cop `Rails/OrderArguments`. ([@lovro-bikic][]) diff --git a/config/default.yml b/config/default.yml index 73478937b5..f321b3ad3a 100644 --- a/config/default.yml +++ b/config/default.yml @@ -748,6 +748,13 @@ Rails/NotNullColumn: Include: - db/**/*.rb +Rails/OrderArguments: + Description: 'Prefer symbol arguments over strings in `order` method.' + StyleGuide: 'https://rails.rubystyle.guide/#order-arguments' + Enabled: pending + VersionAdded: '<>' + Safe: false + Rails/OrderById: Description: >- Do not use the `id` column for ordering. diff --git a/lib/rubocop/cop/rails/order_arguments.rb b/lib/rubocop/cop/rails/order_arguments.rb new file mode 100644 index 0000000000..c4e14c0dfc --- /dev/null +++ b/lib/rubocop/cop/rails/order_arguments.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Prefer symbol arguments over strings in `order` method. + # + # @safety + # Cop is unsafe because the receiver might not be an Active Record query. + # + # @example + # # bad + # User.order('name') + # User.order('name DESC') + # + # # good + # User.order(:name) + # User.order(name: :desc) + # + class OrderArguments < Base + extend AutoCorrector + + MSG = 'Prefer `%s` instead.' + + RESTRICT_ON_SEND = %i[order].freeze + + def_node_matcher :string_order, <<~PATTERN + (call _ :order (str $_value)+) + PATTERN + + ORDER_EXPRESSION_REGEX = /\A(\w+) ?(asc|desc)?\z/i.freeze + + def on_send(node) + return unless (current_expressions = string_order(node)) + return unless (preferred_expressions = replacement(current_expressions)) + + offense_range = find_offense_range(node) + add_offense(offense_range, message: format(MSG, prefer: preferred_expressions)) do |corrector| + corrector.replace(offense_range, preferred_expressions) + end + end + alias on_csend on_send + + private + + def find_offense_range(node) + node.first_argument.source_range.join(node.last_argument.source_range) + end + + def replacement(order_expressions) + order_arguments = order_expressions.flat_map { |expr| expr.split(',') } + order_arguments.map! { |arg| extract_column_and_direction(arg.strip) } + + return if order_arguments.any?(&:nil?) + + convert_to_preferred_arguments(order_arguments).join(', ') + end + + def convert_to_preferred_arguments(order_expressions) + use_hash = false + order_expressions.map do |column, direction| + if direction == :asc && !use_hash + ":#{column}" + else + use_hash = true + "#{column}: :#{direction}" + end + end + end + + def extract_column_and_direction(order_expression) + return unless (column, direction = ORDER_EXPRESSION_REGEX.match(order_expression)&.captures) + + [column.downcase, direction&.downcase&.to_sym || :asc] + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index d3b24ec9e9..da4c2a627f 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -82,6 +82,7 @@ require_relative 'rails/multiple_route_paths' require_relative 'rails/negate_include' require_relative 'rails/not_null_column' +require_relative 'rails/order_arguments' require_relative 'rails/order_by_id' require_relative 'rails/output' require_relative 'rails/output_safety' diff --git a/spec/rubocop/cop/rails/order_arguments_spec.rb b/spec/rubocop/cop/rails/order_arguments_spec.rb new file mode 100644 index 0000000000..74b451df73 --- /dev/null +++ b/spec/rubocop/cop/rails/order_arguments_spec.rb @@ -0,0 +1,131 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::OrderArguments, :config do + it 'registers an offense for `order` with a string argument' do + expect_offense(<<~RUBY) + User.order('first_name') + ^^^^^^^^^^^^ Prefer `:first_name` instead. + RUBY + + expect_correction(<<~RUBY) + User.order(:first_name) + RUBY + end + + it 'registers an offense for `order` with multiple string arguments' do + expect_offense(<<~RUBY) + User.order('first_name', 'last_name') + ^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `:first_name, :last_name` instead. + RUBY + + expect_correction(<<~RUBY) + User.order(:first_name, :last_name) + RUBY + end + + it 'registers an offense for `order` with a string argument with DESC direction' do + expect_offense(<<~RUBY) + User.order('name DESC') + ^^^^^^^^^^^ Prefer `name: :desc` instead. + RUBY + + expect_correction(<<~RUBY) + User.order(name: :desc) + RUBY + end + + it 'registers an offense for `order` with a string argument with ASC order' do + expect_offense(<<~RUBY) + User.order('name ASC') + ^^^^^^^^^^ Prefer `:name` instead. + RUBY + + expect_correction(<<~RUBY) + User.order(:name) + RUBY + end + + it 'registers an offense for `order` with DESC column followed by implicit ASC column' do + expect_offense(<<~RUBY) + User.order('first_name DESC, last_name') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `first_name: :desc, last_name: :asc` instead. + RUBY + + expect_correction(<<~RUBY) + User.order(first_name: :desc, last_name: :asc) + RUBY + end + + it 'registers an offense for `order` with explicit ASC column followed by implicit ASC column' do + expect_offense(<<~RUBY) + User.order('first_name ASC, last_name') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `:first_name, :last_name` instead. + RUBY + + expect_correction(<<~RUBY) + User.order(:first_name, :last_name) + RUBY + end + + it 'registers an offense for safe navigation `order` with a string argument' do + expect_offense(<<~RUBY) + User&.order('first_name') + ^^^^^^^^^^^^ Prefer `:first_name` instead. + RUBY + + expect_correction(<<~RUBY) + User&.order(:first_name) + RUBY + end + + it 'registers an offense for `order` with multiple string arguments, one of which has multiple sorts' do + expect_offense(<<~RUBY) + User.order('first_name, middle_name', 'last_name') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `:first_name, :middle_name, :last_name` instead. + RUBY + + expect_correction(<<~RUBY) + User.order(:first_name, :middle_name, :last_name) + RUBY + end + + it 'registers an offense for `order` with lowercase direction' do + expect_offense(<<~RUBY) + User.order('name desc') + ^^^^^^^^^^^ Prefer `name: :desc` instead. + RUBY + + expect_correction(<<~RUBY) + User.order(name: :desc) + RUBY + end + + it 'registers an offense for `order` with uppercase column' do + expect_offense(<<~RUBY) + User.order('NAME DESC') + ^^^^^^^^^^^ Prefer `name: :desc` instead. + RUBY + + expect_correction(<<~RUBY) + User.order(name: :desc) + RUBY + end + + it 'does not register an offense for `order` with symbol argument' do + expect_no_offenses(<<~RUBY) + User.order(:first_name) + RUBY + end + + it 'does not register an offense for `order` with a string and a symbol argument' do + expect_no_offenses(<<~RUBY) + User.order(:first_name, 'last_name') + RUBY + end + + it 'does not register an offense for `order` with string argument expression' do + expect_no_offenses(<<~RUBY) + User.order('LEFT(first_name, 1)') + RUBY + end +end