diff --git a/CHANGELOG.md b/CHANGELOG.md index 052943ee6f..5f0ca2c03d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### New features * [#52](https://github.com/rubocop-hq/rubocop-rails/issues/52): Add new `Rails/AfterCommitOverride` cop. ([@fatkodima][]) +* [#323](https://github.com/rubocop-hq/rubocop-rails/pull/323): Add new `Rails/OrderById` cop. ([@fatkodima][]) * [#274](https://github.com/rubocop-hq/rubocop-rails/pull/274): Add new `Rails/WhereNot` cop. ([@fatkodima][]) * [#311](https://github.com/rubocop-hq/rubocop-rails/issues/311): Make `Rails/HelperInstanceVariable` aware of memoization. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index 171b5824ba..1fc399b632 100644 --- a/config/default.yml +++ b/config/default.yml @@ -396,6 +396,14 @@ Rails/NotNullColumn: Include: - db/migrate/*.rb +Rails/OrderById: + Description: >- + Do not use the `id` column for ordering. + Use a timestamp column to order chronologically. + StyleGuide: 'https://rails.rubystyle.guide/#order-by-id' + Enabled: false + VersionAdded: '2.8' + Rails/Output: Description: 'Checks for calls to puts, print, etc.' Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 3a028a53d6..587ef63f19 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -61,6 +61,7 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide]. * xref:cops_rails.adoc#railsmatchroute[Rails/MatchRoute] * xref:cops_rails.adoc#railsnegateinclude[Rails/NegateInclude] * xref:cops_rails.adoc#railsnotnullcolumn[Rails/NotNullColumn] +* xref:cops_rails.adoc#railsorderbyid[Rails/OrderById] * xref:cops_rails.adoc#railsoutput[Rails/Output] * xref:cops_rails.adoc#railsoutputsafety[Rails/OutputSafety] * xref:cops_rails.adoc#railspick[Rails/Pick] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index a42ee1c1fe..ffce39dc9c 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -2266,6 +2266,43 @@ add_reference :products, :category, null: false, default: 1 | Array |=== +== Rails/OrderById + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Disabled +| Yes +| No +| 2.8 +| - +|=== + +This cop checks for places where ordering by `id` column is used. + +Don't use the `id` column for ordering. The sequence of ids is not guaranteed +to be in any particular order, despite often (incidentally) being chronological. +Use a timestamp column to order chronologically. As a bonus the intent is clearer. + +NOTE: Make sure the changed order column does not introduce performance +bottlenecks and appropriate database indexes are added. + +=== Examples + +[source,ruby] +---- +# bad +scope :chronological, -> { order(id: :asc) } +scope :chronological, -> { order(primary_key => :asc) } + +# good +scope :chronological, -> { order(created_at: :asc) } +---- + +=== References + +* https://rails.rubystyle.guide/#order-by-id + == Rails/Output |=== diff --git a/lib/rubocop/cop/rails/order_by_id.rb b/lib/rubocop/cop/rails/order_by_id.rb new file mode 100644 index 0000000000..4cc13db266 --- /dev/null +++ b/lib/rubocop/cop/rails/order_by_id.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # This cop checks for places where ordering by `id` column is used. + # + # Don't use the `id` column for ordering. The sequence of ids is not guaranteed + # to be in any particular order, despite often (incidentally) being chronological. + # Use a timestamp column to order chronologically. As a bonus the intent is clearer. + # + # NOTE: Make sure the changed order column does not introduce performance + # bottlenecks and appropriate database indexes are added. + # + # @example + # # bad + # scope :chronological, -> { order(id: :asc) } + # scope :chronological, -> { order(primary_key => :asc) } + # + # # good + # scope :chronological, -> { order(created_at: :asc) } + # + class OrderById < Base + include RangeHelp + + MSG = 'Do not use the `id` column for ordering. '\ + 'Use a timestamp column to order chronologically.' + + def_node_matcher :order_by_id?, <<~PATTERN + (send _ :order + { + (sym :id) + (hash (pair (sym :id) _)) + (send _ :primary_key) + (hash (pair (send _ :primary_key) _)) + }) + PATTERN + + def on_send(node) + return unless node.method?(:order) + + add_offense(offense_range(node)) if order_by_id?(node) + end + + private + + def offense_range(node) + range_between(node.loc.selector.begin_pos, node.source_range.end_pos) + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 50957f1c77..6b5679ed80 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -49,6 +49,7 @@ require_relative 'rails/match_route' require_relative 'rails/negate_include' require_relative 'rails/not_null_column' +require_relative 'rails/order_by_id' require_relative 'rails/output' require_relative 'rails/output_safety' require_relative 'rails/pick' diff --git a/spec/rubocop/cop/rails/order_by_id_spec.rb b/spec/rubocop/cop/rails/order_by_id_spec.rb new file mode 100644 index 0000000000..56f7d1400f --- /dev/null +++ b/spec/rubocop/cop/rails/order_by_id_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::OrderById do + subject(:cop) { described_class.new } + + it 'registers an offense when ordering by `:id` with implicit direction' do + expect_offense(<<~RUBY) + User.order(:id) + ^^^^^^^^^^ Do not use the `id` column for ordering. Use a timestamp column to order chronologically. + RUBY + end + + it 'registers an offense when ordering by `:id` with explicit direction' do + expect_offense(<<~RUBY) + User.order(id: :asc) + ^^^^^^^^^^^^^^^ Do not use the `id` column for ordering. Use a timestamp column to order chronologically. + RUBY + end + + it 'registers an offense when ordering by `primary_key` with implicit direction' do + expect_offense(<<~RUBY) + scope :chronological, -> { order(primary_key) } + ^^^^^^^^^^^^^^^^^^ Do not use the `id` column for ordering. Use a timestamp column to order chronologically. + RUBY + end + + it 'registers an offense when ordering by `primary_key` with explicit direction' do + expect_offense(<<~RUBY) + scope :chronological, -> { order(primary_key => :asc) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use the `id` column for ordering. Use a timestamp column to order chronologically. + RUBY + end + + it 'does not register an offense when ordering by non id column' do + expect_no_offenses(<<~RUBY) + User.order(:created_at) + RUBY + end + + it 'does not register an offense when ordering by multiple columns, including id' do + expect_no_offenses(<<~RUBY) + User.order(id: :asc, created_at: :desc) + RUBY + end +end