reverse_sql_order fails to reverse reversible order

[sebastianiorga]

The way ActiveRecord::QueryMethods#reverse_sql_order and ActiveRecord::QueryMethods#does_not_support_reverse? test for reversibility/perform it, breaks in cases like

 scope :evens_first_with_comma, lambda {
    custom_ordering = <<~SQL
      CASE
        WHEN (category IN (0, 2))
        THEN
          category
        ELSE category + 100
      END ASC
    SQL

    order(Arel.sql(custom_ordering))
  }

because they split on the comma in IN (0, 2). Presumably, that split is intended to check for function calls or something.

Ordering by CASE works when built via Arel, which I think is the proper way to do it anyway. I just wasn't aware Arel could do this when I first wrote the code that triggered the bug.

I figured I'd raise an issue in case someone else was googling around due to this bug.

Not sure what a good way to fix this would be, probably not worth fixing and adding more complexity to reverse_sql_order, tbh.

Steps to reproduce

# Your reproduction script goes here

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Fails in 5.2 and 6.1.5
  gem "rails", '~> 5.2'
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.integer :category
  end
end

class Post < ActiveRecord::Base
  scope :evens_first_with_comma, lambda {
    custom_ordering = <<~SQL
      CASE
        WHEN (category IN (0, 2))
        THEN
          category
        ELSE category + 100
      END ASC
    SQL

    order(Arel.sql(custom_ordering))
  }

  scope :evens_first_without_comma, lambda {
    custom_ordering = <<~SQL
      CASE
        WHEN (category = 0) OR (category = 2)
        THEN
          category
        ELSE category + 100
      END ASC
    SQL

    order(Arel.sql(custom_ordering))
  }

  scope :evens_first_with_arel_case, lambda {
    posts = arel_table
    custom_ordering = Arel::Nodes::Case
                        .new
                        .when(posts[:category].in([0, 2])).then(posts[:category])
                        .else(posts[:category] + 200)

    order(custom_ordering)
  }
end

class BugTest < Minitest::Test
  def test_association_stuff
    post3 = Post.create! category: 3
    post2 = Post.create! category: 2
    post0 = Post.create! category: 0
    post1 = Post.create! category: 1

    assert_equal Post.evens_first_without_comma.to_a, [post0, post2, post1, post3]
    assert_equal Post.evens_first_without_comma.reverse.to_a, [post0, post2, post1, post3].reverse
    assert_equal Post.evens_first_without_comma.last(2), [post1, post3]

    # Arel version works, and produces the same SQL
    assert_equal Post.evens_first_with_arel_case.to_a, [post0, post2, post1, post3]
    assert_equal Post.evens_first_with_arel_case.reverse.to_a, [post0, post2, post1, post3].reverse
    assert_equal Post.evens_first_with_arel_case.last(2), [post1, post3]

    assert_equal Post.evens_first_with_comma.to_a, [post0, post2, post1, post3]
    assert_equal Post.evens_first_with_comma.reverse.to_a, [post0, post2, post1, post3].reverse
    puts 'The next 1 will error'
    assert_equal Post.evens_first_with_comma.last(2), [post1, post3]
    # raises

    # Error:
    # BugTest#test_association_stuff:
    # ActiveRecord::IrreversibleOrderError: Order "CASE\n  WHEN (category IN (0, 2))\n  THEN\n    category\n  ELSE category + 100\nEND ASC\n" can not be reversed automatically
  end
end

# Problem code is below(from rails/AR 5.2.x):
#
#
# module ActiveRecord
#   module QueryMethods
#     def reverse_sql_order(order_query)
#       if order_query.empty?
#         return [arel_attribute(primary_key).desc] if primary_key
#         raise IrreversibleOrderError,
#           "Relation has no current order and table has no primary key to be used as default order"
#       end

#       order_query.flat_map do |o|
#         case o
#         when Arel::Attribute
#           o.desc
#         when Arel::Nodes::Ordering
#           o.reverse
#         when String
#           if does_not_support_reverse?(o)
#             raise IrreversibleOrderError, "Order #{o.inspect} can not be reversed automatically"
#           end
#           o.split(",").map! do |s|
#             s.strip!
#             s.gsub!(/\sasc\Z/i, " DESC") || s.gsub!(/\sdesc\Z/i, " ASC") || (s << " DESC")
#           end
#         else
#           o
#         end
#       end
#     end

#     def does_not_support_reverse?(order)
#       # Account for String subclasses like Arel::Nodes::SqlLiteral that
#       # override methods like #count.
#       order = String.new(order) unless order.instance_of?(String)

#       # Uses SQL function with multiple arguments.
#       (order.include?(",") && order.split(",").find { |section| section.count("(") != section.count(")") }) ||
#         # Uses "nulls first" like construction.
#         /nulls (first|last)\Z/i.match?(order)
#     end
#   end
# end

Expected behavior

All the asserts should pass.

Actual behavior

The last assert triggers IrreversibleOrderError

System configuration

Rails version: Checked on 5.2/6.1.5/7.0

Ruby version: 2.5.1/2.7

[ghiculescu]

Thanks for the report, are you able to confirm if this is an issue on rails 7 too?

[sebastianiorga]

Yes. Same error with ruby 2.7 and rails 7.0.2.3.

[ghiculescu]

You're right that the split is due to functions with arguments, see #25987 / #18928 (comment)

I suppose you could add a check to see if the comma is immediately preceded by an IN 🤷 . It feels like a pretty niche corner case though.

[sebastianiorga]

Agreed, I don't think it's worth fixing especially since you can use Arel to achieve the same result without a problem.

I created the issue because I was looking for an explanation or workaround and couldn't find any information on it, when searching for the method name or error. Figured it might help someone in the future.

[ghiculescu]

Thanks for creating it! I will close since no action's required, but it should still be findable via google / the issues search here.