This project is archived and is in readonly mode.

#2222 ✓hold
Nate Wiger

ActiveRecord not using bind vars for association loading?

Reported by Nate Wiger | March 12th, 2009 @ 05:39 PM | in 2.x

For the lazy loading of associations, AR generates SQL something like this:

select * from posts where posts.user_id in (1,4,8,12,13,16)

However, according to the DBA's here (we use Oracle), this actually passes the numbers in the SQL string, rather than using bind vars.

I've been tracing thru vendor/rails/activerecord/lib and have found this snippet:

    # associations.rb
    def add_limited_ids_condition!(sql, options, join_dependency)
      unless (id_list = select_limited_ids_list(options, join_dependency)).empty?
        sql << "#{condition_word(sql)} #{connection.quote_table_name table_name}.#{primary_key} IN (#{id_list}) "
      else
        throw :invalid_query
      end
    end

Unless I'm reading this wrong, it appears to put the ID's directly in the SQL.

By comparison, association_preload.rb has logic to construct bind vars:

  def in_or_equals_for_ids(ids)
    ids.size > 1 ? "IN (?)" : "= ?"
  end

Anyone familiar with AR have ideas on a patch? I will dive in, but need some hints. At the very least, a confirmation that what I'm seeing is accurate, and is not using bind vars in this situation.

Thanks, Nate

Comments and changes to this ticket

  • CancelProfileIsBroken

    CancelProfileIsBroken April 15th, 2009 @ 06:18 PM

    • Tag set to “2.3.x, activecord, active_record, association, associations, associations_preload, association_preload, preload, preload_associaitons, preload_associations”
    • State changed from “new” to “invalid”

    ActiveRecord doesn't use bind vars at all; passing the numbers in the SQL string is by design.

    We could look at a patch to overall support bind vars in AR, but that would be a fairly major undertaking.

  • CancelProfileIsBroken

    CancelProfileIsBroken April 15th, 2009 @ 06:19 PM

    • Tag changed from “2.3.x, activecord, active_record, association, associations, associations_preload, association_preload, preload, preload_associaitons, preload_associations” to “2.3.x, activecord, active_record, association”

    Hmmm...Lighthouse got a little overexcited on the tags there.

  • Matt Aimonetti (mattetti)

    Matt Aimonetti (mattetti) April 15th, 2009 @ 06:41 PM

    • Tag changed from “2.3.x, activecord, active_record, association” to “2.3.x, activecord, active_record, association, associations, associations_preload, association_preload, preload, preload_associaitons, preload_associations”
    • State changed from “invalid” to “new”
    • Assigned user set to “Matt Aimonetti (mattetti)”

    Furthermore, for people wondering why they do see some bind vars being used, here is the reply from the Oracle enhanced driver author: http://groups.google.com/group/o...

  • Matt Aimonetti (mattetti)

    Matt Aimonetti (mattetti) April 15th, 2009 @ 06:41 PM

    • State changed from “new” to “invalid”
  • Nate Wiger

    Nate Wiger April 15th, 2009 @ 06:48 PM

    • Tag changed from “2.3.x, activecord, active_record, association, associations, associations_preload, association_preload, preload, preload_associaitons, preload_associations” to “2.3.x, activecord, active_record, association”
    • Assigned user cleared.

    That doesn't sound exactly right.

    There are many methods within the AR code that separate out the SQL strings from the "bind" values, explicitly so that SQL injection attacks are avoided. That's what makes this do something useful (and safe):

    @kids = Person.find(:all, :conditions => ["age < ?", 5])

    As evidence, consider the method I posted originally:

    def in_or_equals_for_ids(ids)

     ids.size > 1 ? "IN (?)" : "= ?"

    end

    That's also where sanitize_sql and friends come into play, at least from my reading of the code.

    While "databases" such as mysql might not support true bind variables, this separation nonetheless enables adapters like the Oracle one to do interesting things by using these separate data structures to create real bind variables.

    All I'm looking for is that the few methods that don't follow this pattern be updated to follow this pattern.

    Also note I volunteered to write the patch. I already spoke with Matt Aimonetti about this, and he and I are going to look at it in a week or so.

    Can you please reopen this ticket?

  • Matt Aimonetti (mattetti)

    Matt Aimonetti (mattetti) April 15th, 2009 @ 06:52 PM

    • State changed from “invalid” to “hold”
    • Assigned user set to “Matt Aimonetti (mattetti)”

    Alright, I'm putting this ticket on hold while we look at it. If sanitizing the sql query allows for the Oracle adapter to do some magic using cursor_sharing = similar, I don't see why we shouldn't do it.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Tags

Pages