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

missing bind values #363

Closed
firien opened this issue Mar 26, 2015 · 10 comments
Closed

missing bind values #363

firien opened this issue Mar 26, 2015 · 10 comments

Comments

@firien
Copy link

firien commented Mar 26, 2015

while upgrading a rails app to 4.2 i ran into some malformed SQL. My app has many polymorphic associations and i use unions religiously to leverage composite indices. The ruby code below can be run with both arel 5 and 6 by toggling the gem 'activerecord' lines at the top. The code uses an in-memory sqlite database so you should be able to run it directly from your favorite text editor.

I have included the SQL output below and version 6 has no query parameter assignments

require 'sqlite3'
gem 'activerecord', '4.2.1'
# toggle activerecord versions
# gem 'activerecord', '4.1.10'
require 'active_record'

ActiveRecord::Base.establish_connection(
  adapter: 'sqlite3',
  database: ':memory:'
)
puts "-- arel #{Arel::VERSION}"

ActiveRecord::Migration.verbose = false
ActiveRecord::Schema.define do
  create_table :tasks do |t|
    t.integer :assigned_id
    t.string  :assigned_type
  end
  create_table :people do |t|
    t.string :name
  end
  create_table :roles do |t|
    t.integer :person_id
  end
end

Task = Class.new ActiveRecord::Base
Role = Class.new ActiveRecord::Base

class Person < ActiveRecord::Base
  def tasks
    str = Arel::VERSION.to_i == 5 ? 'Role' : Arel::Nodes::Quoted.new('Role')
    person_arel = Task.where(assigned_id: self.id, assigned_type: 'Person')
    _roles = Role.where(person_id: self.id).select(:id).as('_roles')
    roles_arel = Task.joins(Arel::Nodes::InnerJoin.new(_roles, Arel::Nodes::On.new(
      Arel::Nodes::Equality.new(_roles[:id], Task.arel_table[:assigned_id]).and(
      Arel::Nodes::Equality.new(Task.arel_table[:assigned_type], str))
    )))
    union = Arel::Nodes::Union.new(person_arel.ast, roles_arel.ast)
    _tasks = Arel::Nodes::TableAlias.new(union, :tasks)
    Task.from(_tasks)
  end
end

me = Person.create(name: 'me')
puts me.tasks.to_sql
-- arel 6.0.0
SELECT "tasks".* FROM (
  SELECT "tasks".* FROM "tasks"
  WHERE "tasks"."assigned_id" =  AND "tasks"."assigned_type" =
  UNION
  SELECT "tasks".* FROM "tasks"
  INNER JOIN (
    SELECT "roles"."id" FROM "roles" WHERE "roles"."person_id" =
  ) _roles ON _roles."id" = "tasks"."assigned_id" AND "tasks"."assigned_type" = 'Role'
) "tasks"

-- arel 5.0.1
SELECT "tasks".* FROM (
  SELECT "tasks".* FROM "tasks"
  WHERE "tasks"."assigned_id" = 1 AND "tasks"."assigned_type" = 'Person'
  UNION
  SELECT "tasks".* FROM "tasks"
  INNER JOIN (
    SELECT "roles"."id" FROM "roles"  WHERE "roles"."person_id" = 1
  ) _roles ON _roles."id" = "tasks"."assigned_id" AND "tasks"."assigned_type" = 'Role'
) "tasks"
@rafaelfranca
Copy link
Member

cc @sgrif

@sgrif
Copy link

sgrif commented Mar 27, 2015

What version are you upgrading from? It looks like you're grabbing the AST from the relation, but leaving the bind values behind, but this shouldn't have worked in 4.1 either.

(Also insert requisite warning about how you're reaching deeply into AR internals in unsupported ways that will likely break between versions)

@firien
Copy link
Author

firien commented Mar 30, 2015

my rails app uses 4.1.7, but 4.1.10 still produces the desired SQL. Here is a gist that uses SelectManager#union, instead of passing ast into Arel::Nodes::Union.new
Arel 6.x from Rails 4.2 produces SQL without bind values.

@sgrif
Copy link

sgrif commented Mar 30, 2015

Thanks, I'll look into this.

@firien
Copy link
Author

firien commented Mar 30, 2015

interesting, calling #to_sql on the TableAlias from AREL 5.0.1 produces the desired SQL with bind values.
but calling #to_sql on the TableAlias from AREL 6.0.0 produces the SQL with placeholder ? - I guess ActiveRecord #from is now supplying the bind values. (#from seems to have got a lot of work lately rails/rails@bdc5141)

If this is an ActiveRecord issue I can close this and open in the rails repo

@sgrif
Copy link

sgrif commented Mar 30, 2015

It's not really an Active Record or an Arel issue, it's just incorrect usage. Like I mentioned originally, yeah the problem is that you're not passing the bind values. I would have expected that query to use bind values in 4.1, which was the thing I was going to look into, but it appears that query didn't use bind values in 4.1

@danielnc
Copy link

danielnc commented Feb 8, 2017

@sgrif Sorry to reopen such old issue I am trying to investigate an issue where in Rails 4.2 bind values are not passed to down to two different libraries that use AREL and ActiveRecord: Squeel and BabySqueel

How would you rewrite this query to allow bind values to be used by AST?

@DavidMikeSimon
Copy link

@sgrif I am running into the same issue as @danielnc. Point taken that the interface may not be stable, but even so, is there a way to use the current interface to get an AST with bind values attached?

@voltechs
Copy link

I'd love to know this as well. I'm up to my elbows in Arel at the moment—if I figure anything out I'll report back.

@HuBandiT
Copy link

HuBandiT commented Nov 20, 2020

.arel.ast instead of .ast is what I found I can use for this in arel 6.0.4 (Rails 4.2).

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

No branches or pull requests

7 participants