rails / arel

A Relational Algebra
2.06k stars 390 forks source link

Limit expression generated with bind param but no value (works with 6.0.4/breaks with 7.1.4) #507

Closed aselder closed 6 years ago

aselder commented 6 years ago

With ARel 7.1.4 and ActiveRecord 5.0.6:

We are upgrading an app from Rails 4.2 to 5.0, and we have a fairly complex query that works with Arel 6.0.4 and Rails 4.2.9, but breaks when I upgrade. I've reduced it to a fairly simple reproduction case. In the query below, SessionRating is a standard AR class.

in ARel 6.0.4/AR 4.2.9

[1] pry(#<Test>)> SessionRating.limit(1).arel.to_sql
=> "SELECT  `session_ratings`.* FROM `session_ratings` LIMIT 1"
[2] pry(#<Test>)> SessionRating.limit(1).to_sql
=> "SELECT  `session_ratings`.* FROM `session_ratings` LIMIT 1"

in ARel 7.1.4/AR 5.0.6

[9] pry(#<Test>)> SessionRating.limit(1).arel.to_sql
=> "SELECT  `session_ratings`.* FROM `session_ratings` LIMIT ?"
[10] pry(#<Test>)> SessionRating.limit(1).to_sql
=> "SELECT  `session_ratings`.* FROM `session_ratings` LIMIT 1"
aselder commented 6 years ago

Arel 8.0.0/AR 5.14 exhibits the same behavior

matthewd commented 6 years ago

Yeah, this is expected behaviour: the Arel relation doesn't carry the bind parameter values (and we're progressively moving more things from SQL interpolation to use bind parameters -- in this case, LIMIT).

kbrock commented 6 years ago

@aselder Yes. When using Arel to build inner queries, this bites us.

To get around, we have had to build a sql literal:

Arel::Nodes::SqlLiteral.new("(#{arel.to_sql})")

The cause of this is over in Rails, where it builds its own BindParam values ( relation/query_methods.rb ?) instead of using Arel itself to build the sql (with the bind parameters). Not trying to say that is a trivial task, just trying to state the problem.

Would be nice if Arel could return an AST with bind variables that were more usable to Rails. Sorry for not better details, I had researched this over a year ago.

@matthewd if you have specific instructions (or a whim) you can send me off on a wild goose chase on this one.

matthewd commented 6 years ago

Per #523, Arel development is moving to rails/rails.

If this issue is still relevant, please consider reopening it over there. (Note that the Rails repository does not accept feature request issues, and requires reproduction steps that rely on Active Record's documented API.)