This project is archived and is in readonly mode.

#3296 ✓invalid
Morgan Grubb

Failing functional test of ActiveRecord conditions merging

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

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.

Comments and changes to this ticket

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>

Pages