Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

Failing functional test of ActiveRecord conditions merging

#3296

With Rails 2.3.4:

Given this schema

class CreateExamples < ActiveRecord::Migration
 def self.up
  create_table :examples do |t|
  end
 end
 def self.down
  drop_table :examples
 end
end
class CreateExampleUrls < ActiveRecord::Migration
 def self.up
  create_table :example_urls do |t|
   t.string :url
   t.integer :example_id
  end
 end
 def self.down
  drop_table :example_urls
 end
end

And these models

class Example < ActiveRecord::Base
  has_many :example_urls
end
class ExampleUrl < ActiveRecord::Base
  belongs_to :example
end

This query passes

ExampleUrl.first :conditions => { :url => "this will not explode?" }

While this one fails

Example.first :joins => :example_urls, :conditions => { :example_urls => { :url => "this will explode?" } }

This one, however, also passes

Example.first :joins => :example_urls, :conditions => ["example_urls.url = ?", "this will not explode?"]

I tracked the problem down to a couple of methods in ActiveRecord::Base

        def sanitize_sql_hash_for_conditions(attrs, default_table_name = quoted_table_name)

      attrs = expand_hash_conditions_for_aggregates(attrs)

      conditions = attrs.map do |attr, value|
        table_name = default_table_name

        unless value.is_a?(Hash)
          attr = attr.to_s

          # Extract table name from qualified attribute names.
          if attr.include?('.')
            attr_table_name, attr = attr.split('.', 2)
            attr_table_name = connection.quote_table_name(attr_table_name)
          else
            attr_table_name = table_name
          end

          attribute_condition(&quot;#{attr_table_name}.#{connection.quote_column_name(attr)}&quot;, value)
        else
          sanitize_sql_hash_for_conditions(value, connection.quote_table_name(attr.to_s))
        end
      end.join(' AND ')

      replace_bind_variables(conditions, expand_range_bind_variables(attrs.values))
    end

    def replace_bind_variables(statement, values) #:nodoc:
      raise_if_bind_arity_mismatch(statement, statement.count('?'), values.size)
      bound = values.dup
      statement.gsub('?') { quote_bound_value(bound.shift) }
    end</code>



What's happening is the recursive call to sanitize_sql_hash_for_conditions is calling replace_bind_variables on the inner conditions hash, thus interpolating the value into the string query. Then the outer call attempts to interpolate it's variables which is where the problem starts.

replace_bind_variables counts the number of question marks in the [previously] interpolated condition string, finds the one in the "example_urls.url = 'this will explode?'" piece and, not seeing any variables to bind to, raises PreparedStatementInvalid.

When considering how to fix this issue I've ruled out replacing question marks with strings at the point of interpolation and replacing them just before finalizing the query as there is no guarantee that whatever replacement string I used will not legitimately appear in the query itself. This leaves two other options: count the question marks in the already interpolated strings and don't include those when binding variables (difficult but not impossible), or put off interpolating until after the string is completed (much more difficult but definitely the right thing to do).

I don't have the time to fix this myself so I'm hoping someone else can pick this up and run with it.

Reported by Morgan Grubb · September 28th, 2009 @ 05:54 PM

State: invalid
Milestone: none
Assigned to: nobody
Importance: none

Activity

  1. Martin Luder
    Martin Luder

    We just ran into this issue with Rails 2.3.5. Does nobody care?

    June 11th, 2010 @ 11:35 AM

  2. Neeraj Singh
    Neeraj Singh

    I tested it with Rails 2-3-stable branch and it works fine.

    June 11th, 2010 @ 12:29 PM

  3. Neeraj Singh
    Neeraj Singh
    • State changed from new to invalid
    • Importance changed from to

    Marking it as invalid. If you are able to reproduce this problem with rails 2-3-stable or rails master then it could be reopened.

    July 22nd, 2010 @ 07:28 PM