Default AR order behaviour

A May Of WTFs
21

Matthew, two objections. One is the use of the word “Arel”, since all the methods this is being used with are AR methods, so having a fix that was also part of the AR api seems preferable, and if there is an explicit desire to bring Arel internal then alternate AR method signatures for order/pluck that would allow raw sql (with whatever prefix/suffix is used) seems better than having one dangling reference to Arel out at this level of abstraction.

The other is that there is not a global config to turn this check off and just allow the sql through. I appreciate the security warning that is the goal here, and the difficulty in detecting ok vs. not-ok sql strings. And think it laudable to encourage folks to review their sql. But on my tiny team, we know that sql can be dangerous and think about it before introducing it, and we would like to turn the whole thing off – and indeed the first thing we did was override those methods in our ApplicationRecord to remove this warning.

1 Like
23

Exactly!

Those are basically my thoughts.

24

In order not to have to escape half of our code with Arel.sql, and because this subject seems to still be an unresolved debate, this is the sort of hack I did in the meantime for our current Postgres-backed app where most of our raw SQL ordering was about getting random records:

class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true

  scope :random_order, -> { order(Arel.sql('RANDOM()')) }
  scope :random_pick, -> { random_order.first }
end

Note that the SQL above is not performant for large tables of course, but the point is that I had to use Arel.sql to do this, which felt like a WTF. Incidentally maybe Rails should have those random query methods baked in.

The rest of our other raw SQL order code is escaped with Arel.sql in an ad-hoc fashion. This makes us cringe every time, but that’s what it is!

1 Like
25

"random()".sql_safe would also be a more appealing alternative.

1 Like
26

I appreciate how concrete this suggestion is, after a lot of back-and-forth about generalities. Thank you!

1 Like
27

YESSS. I have a very large project that I can’t upgrade bc I’m using so much SQL (and supporting url param querying). Is that a smell? Sure/probably. But I don’t have a way to stay in sync w Rails unless there’s some kind of crutch (to_unsafe_sql) available without rewriting EVERYTHING.

1 Like