Don't join twice when searching on multiple association fields. Patch

submitted by Benjamin Curtis.

[git-p4: depot-paths = "//src/Sphincter/dev/": change = 3650]

History.txt

@@ -1,3 +1,9 @@
+== 1.1.1
+
+* 1 bug fix:
+  * Don't join twice when searching on multiple association fields.  Patch
+    submitted by Benjamin Curtis.
+
 == 1.1.0 / 2007-08-13
 
 * 2 major enhancements:

lib/sphincter/configure.rb

@@ -75,6 +75,7 @@ def initialize(klass, options)
       @table = @klass.table_name
       @conn = @klass.connection
       @tables = @table.dup
+      @joined_tables = @table.dup
 
       defaults = {
         :conditions => [],
@@ -137,8 +138,7 @@ def add_include(as_name, as_field)
       case as_assoc.macro
       when :belongs_to then
         @fields << add_field(as_field, as_klass, as_table)
-        @tables << " LEFT JOIN #{as_table} ON" \
-                   " #{@table}.#{as_assoc_key} = #{as_table}.#{as_klass_key}"
+        add_join(as_table, as_klass_key, as_assoc_key)
 
       when :has_many then
         if as_assoc.options.include? :through then
@@ -162,12 +162,9 @@ def add_include(as_name, as_field)
           id_col = @conn.quote_column_name "#{poly_name}_id"
           type_col = @conn.quote_column_name "#{poly_name}_type"
 
-          @tables << " LEFT JOIN #{as_table} ON"\
-                     " #{@table}.#{as_klass_key} = #{as_table}.#{id_col} AND" \
-                     " #{@conn.quote @klass.name} = #{as_table}.#{type_col}"
+          add_join(as_table, id_col, as_klass_key, " AND #{@conn.quote @klass.name} = #{as_table}.#{type_col}")
         else
-          @tables << " LEFT JOIN #{as_table} ON" \
-                     " #{@table}.#{as_klass_key} = #{as_table}.#{as_assoc_key}"
+          add_join(as_table, as_assoc_key, as_klass_key)
         end
 
         @group = true
@@ -178,6 +175,13 @@ def add_include(as_name, as_field)
       end
     end
 
+    def add_join(dest_table, dest_field, src_field, extra_query = nil)
+      return if @joined_tables.include?(dest_table)
+      @tables << " LEFT JOIN #{dest_table} ON" \
+                 " #{@table}.#{src_field} = #{dest_table}.#{dest_field}#{extra_query}"
+      @joined_tables << dest_table
+    end
+
     def configure
       conn = @klass.connection
       pk = conn.quote_column_name @klass.primary_key

test/test_sphincter_configure.rb

@@ -343,6 +343,21 @@ def test_add_include_belongs_to
     assert_equal false, @index.group
   end
 
+  def test_add_include_belongs_to_twice_only_joins_once
+    @index.add_include 'belongs_to', 'string'
+    @index.add_include 'belongs_to', 'text'
+
+    assert_equal ["belongs_tos.`string` AS `belongs_tos_string`", "belongs_tos.`text` AS `belongs_tos_text`"], @index.fields
+
+    tables = "models LEFT JOIN belongs_tos ON " \
+               "models.`belongs_to_id` = belongs_tos.`id`"
+    assert_equal tables, @index.tables
+
+    assert_equal [], @index.where
+
+    assert_equal false, @index.group
+  end
+
   def test_add_include_has_many
     @index.add_include 'manys', 'string'
 
@@ -358,6 +373,23 @@ def test_add_include_has_many
     assert_equal true, @index.group
   end
 
+  def test_add_include_has_many_twice_only_joins_once
+    @index.add_include 'manys', 'string'
+    @index.add_include 'manys', 'text'
+
+    fields = [
+      "GROUP_CONCAT(has_manys.`string` SEPARATOR ' ') AS `has_manys_string`",
+      "GROUP_CONCAT(has_manys.`text` SEPARATOR ' ') AS `has_manys_text`"
+    ]
+    assert_equal fields, @index.fields
+
+    tables = "models LEFT JOIN has_manys ON models.`id` = has_manys.`manys_id`"
+    assert_equal tables, @index.tables
+
+    assert_equal [], @index.where
+    assert_equal true, @index.group
+  end
+
   def test_add_include_has_many_polymorphic
     @index.add_include 'polys', 'string'
 
@@ -373,6 +405,25 @@ def test_add_include_has_many_polymorphic
     assert_equal true, @index.group
   end
 
+  def test_add_include_has_many_polymorphic_twice_only_joins_once
+    @index.add_include 'polys', 'string'
+    @index.add_include 'polys', 'text'
+
+    fields = [
+      "GROUP_CONCAT(polys.`string` SEPARATOR ' ') AS `polys_string`",
+      "GROUP_CONCAT(polys.`text` SEPARATOR ' ') AS `polys_text`"
+      ]
+    assert_equal fields, @index.fields
+
+    tables = "models LEFT JOIN polys ON " \
+               "models.`id` = polys.`polyable_id` AND " \
+               "'Model' = polys.`polyable_type`"
+    assert_equal tables, @index.tables
+
+    assert_equal [], @index.where
+    assert_equal true, @index.group
+  end
+
   def test_add_include_nonexistent_association
     e = assert_raise Sphincter::Error do
       @index.add_include 'nonexistent', 'string'