rzane / baby_squeel

:pig: An expressive query DSL for Active Record
MIT License
503 stars 50 forks source link

Arel converts SQL literal to 0 when compared with a numeric column #34

Closed odedniv closed 8 years ago

odedniv commented 8 years ago

Issue

Arel doesn't seem to like having baby squeel's SQL literal proxy on the right side of an operator when the left side is a numeric column (grouping the literal with the new grouping method fixes it, one of the reasons I wanted it here is because it 'fixed' a lot of similar issues on the original Squeel).

Sometimes infras case classes, which proxies don't handle well (case X when Arel::Nodes::SqlLiteral then ...), and it seems like it uses to_i of the literal because it translates to '0' on the SQL.

Reproduction

require 'bundler/inline'
require 'minitest/spec'
require 'minitest/autorun'

gemfile true do
  source 'https://rubygems.org'
  gem 'activerecord', '~> 5.0.0' # which Active Record version?
  gem 'sqlite3'
  gem 'baby_squeel', github: 'rzane/baby_squeel'
end

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')

ActiveRecord::Schema.define do
  create_table :dogs, force: true do |t|
  end
end

class Dog < ActiveRecord::Base
end

class BabySqueelTest < Minitest::Spec
  it 'works' do
    scope = Dog.where.has { id == sql('Fido') }

    scope.to_sql.must_equal %{
      SELECT `dogs`.* FROM `dogs` WHERE `dogs`.`id` = Fido
    }.squish
  end
end
rzane commented 8 years ago

Good find. However, I'm not sure what the best solution is here, because this actually doesn't work in Arel.

it 'works with straight up arel' do
  id = Dog.arel_table[:id]
  scope = Dog.where id.eq(Arel.sql('Fido'))

  scope.to_sql.must_equal %{
    SELECT `dogs`.* FROM `dogs` WHERE `dogs`.`id` = Fido
  }.squish
end
rzane commented 8 years ago

Also, thank you for providing me with a reproduction case. That really, really helps.

odedniv commented 8 years ago

I believe I found something like this before on squeel too, that's why I could guess so much to reach the description on the issue :smile:

Wrapping the literal in a grouping, which causes additional brackets in the SQL fixes the issue (might not be a bad idea before appending raw SQL). I don't love this patch, but encountering this problem caused quite a headache (the SQL fails silently, the condition always fails with no visible error).

Would suggest testing it on squeel and if it works figure out how @ernie fixed it.

rzane commented 8 years ago

Okay, so I think this is an ActiveRecord bug. I've submitted a PR here: https://github.com/rails/rails/pull/26992.

This works fine on 4.2, so this bug only affects 5.0.

odedniv commented 8 years ago

Awesome! I love the quick correspondence you had there, I usually did not get the same result posting a bug/PR to the rails team.

I feel comfortable closing this and leaving it to AREL.

rzane commented 8 years ago

:+1: thanks for reporting this