Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Passing raw strings to reorder deprecated in Rails 5.2 #290

Closed
jeffreyguenther opened this issue Nov 17, 2017 · 1 comment
Closed

Passing raw strings to reorder deprecated in Rails 5.2 #290

jeffreyguenther opened this issue Nov 17, 2017 · 1 comment

Comments

@jeffreyguenther
Copy link

jeffreyguenther commented Nov 17, 2017

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
Copy link
Owner

brendon commented Nov 19, 2017

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants