Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• You flagged this item as spam.
  

  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

• You flagged this item as spam.
  

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

  Whoops, here is the patch.

  • 2596-include-with-conditions-hash.diff 2.1 KB

• You flagged this item as spam.
  

  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 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?

• You flagged this item as spam.
  

  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?
        string.scan(/([\.a-zA-Z_]+).?\./).flatten
      end
  
  

  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 = attrs.map 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)
            end
  
            attribute_condition("#{table_name}.#{connection.quote_column_name(attr)}", 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
  
  

• You flagged this item as spam.
  

  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.

  Anthony

  • 2596-include-with-conditions-hash-2.diff 2.9 KB
• 

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

  • State changed from “new” to “committed”

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

  [state:committed]

• You flagged this item as spam.
  

  blj May 10th, 2009 @ 01:17 PM

  Thanks guys.