This project is archived and is in readonly mode.

#2596 ✓committed

:include and nested conditions hash

Reported by blj | May 1st, 2009 @ 10:56 PM | in 2.x

# All these works
User.find(:all,:include=>:bank_accounts, :conditions=>'"ICICI"') => [some users]
User.find(:all,:include=>:bank_accounts, :conditions=>{'' => "ICICI"}) => [some users again]
User.find(:all,:joins=>:bank_accounts, :conditions=>{:bank_accounts=>{:name=>'ICICI'}}) => [some users again and again]

# This does not work 
User.find(:all,:include=>:bank_accounts, :conditions=>{:bank_accounts=>{:name=>'ICICI'}}) => throws error with invalid SQL

#ActiveRecord::StatementInvalid: Mysql::Error: Unknown column '' in 'where clause': SELECT * FROM `users` WHERE (`bank_accounts`.`name` = 'ICICI') 

Comments and changes to this ticket

  • Anthony Crumley

    Anthony Crumley May 4th, 2009 @ 06:50 PM

    • Tag set to patch

    The issue here is that the include table is not recognized as being in the conditions. The code that identifies the tables in conditions is expecting strings in the format of "table_name".column or table_name.column. A conditions hash like {:bank_accounts=>{:name=>'ICICI'}} results in the table name bank_accounts which does not match the expected formats.

    The solution provided in this patch uses the rails function for converting hashes to sql conditions to normalize the hash conditions so the same logic applied to string and array conditions will apply to hashes as well.


  • Anthony Crumley
  • Ben Wyrosdick

    Ben Wyrosdick May 4th, 2009 @ 07:21 PM

    that looks solid to me ... converting the hash to a sql string making it act like the rest of the conditions should be a good fix

  • Michael Koziarski

    Michael Koziarski May 4th, 2009 @ 07:46 PM

    Can't we just change the code that recognizes the table names in the strings, to recognize these hashes?

  • Anthony Crumley

    Anthony Crumley May 4th, 2009 @ 08:20 PM

    The problem is that it would not be able to tell the difference between column names from the find model and table names from included models. They both can appear as a simple name in the resulting string.

    The following method is the one that pulls out the table names based on a regex.

        def tables_in_string(string)
          return [] if string.blank?

    We can code the logic to determine whether or not the hash keys are table names, column names, or table.column strings and handle them appropriately but that logic is already in the function being used below. Maybe this is a good opportunity to refactor that logic out.

        def sanitize_sql_hash_for_conditions(attrs, table_name = quoted_table_name)
          attrs = expand_hash_conditions_for_aggregates(attrs)
          conditions = do |attr, value|
            unless value.is_a?(Hash)
              attr = attr.to_s
              # Extract table name from qualified attribute names.
              if attr.include?('.')
                table_name, attr = attr.split('.', 2)
                table_name = connection.quote_table_name(table_name)
              attribute_condition("#{table_name}.#{connection.quote_column_name(attr)}", value)
              sanitize_sql_hash_for_conditions(value, connection.quote_table_name(attr.to_s))
          end.join(' AND ')
          replace_bind_variables(conditions, expand_range_bind_variables(attrs.values))
  • Anthony Crumley

    Anthony Crumley May 5th, 2009 @ 02:28 AM

    After thinking on it a while, the following may be a better solution. I added a tables_in_hash method to handle the hash case.

    Please take a look and see if this looks better.


  • Michael Koziarski

    Michael Koziarski May 10th, 2009 @ 02:38 AM

    • State changed from “new” to “committed”

    Great work Anthony, applied to 2-3-stable and master.


  • blj

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=""></a>