brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.04k stars 355 forks source link

Passing raw strings to reorder deprecated in Rails 5.2 #290

Closed jeffreyguenther closed 6 years ago

jeffreyguenther commented 6 years ago

I'm using acts_as_list in a project running on rails/master. The following deprecation warning is being provided:

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw
SQL) called with non-attribute argument(s): "\"content_blocks\".\"position\" DESC". Non-attribute
arguments will be disallowed in Rails 6.0. This method should not be called with user-provided
values, such as request parameters or model attributes. Known-safe values can be passed by
wrapping them in Arel.sql(). (called from bottom_item at
/home/rof/cache/bundler/ruby/2.3.0/gems/acts_as_list
0.9.9/lib/acts_as_list/active_record/acts/list.rb:283)

Looking at line 283, it appears that the reorder call could be simplified with the hash order syntax. I haven't read through the whole codebase yet to verify this will work, though.

scope.in_list.reorder("#{quoted_position_column_with_table_name} DESC").first

to

scope.in_list.reorder(position_column => :desc).first

If not, Arel.sql will need to be used.

brendon commented 6 years ago

We specify the full table name and column name due to a namespacing issue someone had in the past. I can't quite remember much about it but it's somewhere in the Issues section. It might have had something to do with joins and both tables having a position column? Does Rails handle this well now?

Anyway, perhaps this needs exploring. I'd prefer your first method but we just need to be sure we're not breaking anything. Would you like to put together a PR that uses these simpler methods and perhaps add some tests around join tables and colliding column names?