SQL Literals should not be type casted

[rzane]

The issue makes itself apparent when querying integer colums, which will call #to_i on the SqlLiteral. Take the following example:

Post.arel_table[:id].eq(Arel.sql('coalesce(1, 0)')).to_sql

Active Record 4.2:

"posts"."id" = coalesce(1, 0)

Active Record 5.0:

"posts"."id" = 0

Related: https://github.com/rails/rails/pull/26992

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[dolzenko]

I assume its the same issue with e.g. matches:

Rails 4:

Blocked::Base.where(Blocked::Base.arel_table[:values].matches("%.com%"))
  Blocked::Base Load (1.1ms)  SELECT `blocked`.* FROM `blocked` WHERE (`blocked`.`values` LIKE '%.com%')

Rails 5

Blocked::Base.where(Blocked::Base.arel_table[:values].matches("%.com%"))
  Blocked::Base Load (0.7ms)  SELECT `blocked`.* FROM `blocked` WHERE (`blocked`.`values` LIKE '\"%.com%\"')

The value passed to LIKE gets quoted twice (I assume by adapter and by Arel).

[rzane]

@dolzenko I don't think so. That looks like a separate issue.

This PR is specifically related to Arel::Nodes::SqlLiteral, but the query you've shown isn't using SqlLiteral.

[dolzenko]

@rzane thanks, tried submitting this to Rails issues https://github.com/rails/rails/issues/27987

[libc]

I have a more palatable solution to this (context):

--- a/lib/arel/nodes/casted.rb
+++ b/lib/arel/nodes/casted.rb
@@ -30,7 +30,7 @@ module Arel

     def self.build_quoted other, attribute = nil
       case other
-        when Arel::Nodes::Node, Arel::Attributes::Attribute, Arel::Table, Arel::Nodes::BindParam, Arel::SelectManager, Arel::Nodes::Quoted
+        when Arel::Nodes::Node, Arel::Attributes::Attribute, Arel::Table, Arel::Nodes::BindParam, Arel::SelectManager, Arel::Nodes::Quoted, Arel::Nodes::SqlLiteral
           other
         else
           case attribute

I think the problem is not that it quotes literals, but more that it adds Arel::Nodes::Casted around them when constructing AST.

@matthewd would a PR with the change above (and a test) be acceptable?

The current problem is

          attribute = Attribute.new nil, nil
          literal = Arel.sql('NOW()')
          node = attribute.eq(literal)

constructs <Equality left=<Attribute> right=<Casted val=<SqlLiteral>>>. I think it should be <Equality left=<Attribute> right=<SqlLiteral>>

[rzane]

@libc I think you're right. That does seem to be the root of the problem.

[sgrif]

Closing in favor of #500