Add new Rails/OrderById cop

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][])
 

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

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]

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
 
 |===

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

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'

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