This project is archived and is in readonly mode.

ActiveRecord not using bind vars for association loading?

#2222

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

Reported by Nate Wiger · March 12th, 2009 @ 05:39 PM

Activity

CancelProfileIsBroken
- 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.

April 15th, 2009 @ 06:18 PM
CancelProfileIsBroken
- 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.

April 15th, 2009 @ 06:19 PM
Matt Aimonetti (mattetti)
- 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...

April 15th, 2009 @ 06:41 PM
Matt Aimonetti (mattetti)
- State changed from new to invalid
April 15th, 2009 @ 06:41 PM
Nate Wiger
- 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?
April 15th, 2009 @ 06:48 PM
Matt Aimonetti (mattetti)
- 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.

April 15th, 2009 @ 06:52 PM