Rework table aliasing to account for truncated table aliases. Add smarter table aliasing when doing eager loading of STI associations. This allows you to use the association name in the order/where clause. [Jonathan Viney / Rick Olson] closes #4108

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@3921 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

6 files changed, 80 insertions, 36 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 668df3698f..3267a712da 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,9 @@
 *SVN*
 
+* Rework table aliasing to account for truncated table aliases.  Add smarter table aliasing when doing eager loading of STI associations. This allows you to use the association name in the order/where clause. [Jonathan Viney / Rick Olson] #4108 Example (SpecialComment is using STI):
+
+    Author.find(:all, :include => { :posts => :special_comments }, :order => 'special_comments.body')
+
 * Add AbstractAdapter#table_alias_for to create table aliases according to the rules of the current adapter. [Rick]
 
 * Provide access to the underlying database connection through Adapter#raw_connection. Enables the use of db-specific methods without complicating the adapters. #2090 [Koz]
@@ -8,7 +12,7 @@
 
 * Added connection#current_database that'll return of the current database (only works in MySQL, SQL Server, and Oracle so far -- please help implement for the rest of the adapters) #3663 [Tom ward]
 
-* Fixed that Migration#execute would have the table name prefix appended to its query #4110 [mark.imbriaco@pobox.com]
+* Fixed that Migration#execute w ould have the table name prefix appended to its query #4110 [mark.imbriaco@pobox.com]
 
 * Make all tinyint(1) variants act like boolean in mysql (tinyint(1) unsigned, etc.) [Jamis Buck]
 
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 34fba0414c..4f8f41fcea 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -1200,24 +1200,28 @@ module ActiveRecord
           end
 
           class JoinAssociation < JoinBase
-            attr_reader :reflection, :parent, :aliased_table_name, :aliased_prefix, :aliased_join_table_name
-            delegate    :options, :klass, :to=>:reflection
+            attr_reader :reflection, :parent, :aliased_table_name, :aliased_prefix, :aliased_join_table_name, :parent_table_name
+            delegate    :options, :klass, :to => :reflection
 
             def initialize(reflection, join_dependency, parent = nil)
               super(reflection.klass)
               @parent             = parent
               @reflection         = reflection
               @aliased_prefix     = "t#{ join_dependency.joins.size }"
-              @aliased_table_name = table_name # start with the table name
+              @aliased_table_name = sti? ? pluralize(reflection.name) : table_name # start with the table name
+              @parent_table_name  = sti? ? pluralize(parent.active_record.name.underscore) : parent.active_record.table_name
               unless join_dependency.table_aliases[aliased_table_name].zero?
                 # if the table name has been used, then use an alias
-                # if the alias has been used, add a '_n' suffix to the end.
-                @aliased_table_name = "#{parent.active_record.to_s.underscore}_#{reflection.name}_#{join_dependency.table_aliases[aliased_table_name]}".gsub(/_1$/, '')
+                @aliased_table_name = active_record.connection.table_alias_for "#{pluralize(reflection.name)}_#{parent_table_name}"
+                table_index = join_dependency.table_aliases[aliased_table_name]
+                @aliased_table_name = @aliased_table_name[0..active_record.connection.table_alias_length-3] + "_#{table_index+1}" if table_index > 0
               end
               if reflection.macro == :has_and_belongs_to_many || (reflection.macro == :has_many && reflection.options[:through])
                 @aliased_join_table_name = reflection.macro == :has_and_belongs_to_many ? reflection.options[:join_table] : parent.active_record.reflect_on_association(reflection.options[:through]).klass.table_name
                 unless join_dependency.table_aliases[aliased_join_table_name].zero?
-                  @aliased_join_table_name = "join_#{parent.active_record.to_s.underscore}_#{reflection.name}_#{join_dependency.table_aliases[aliased_join_table_name]}".gsub(/_1$/, '')
+                  @aliased_join_table_name = active_record.connection.table_alias_for "#{pluralize(reflection.name)}_#{parent_table_name}_join"
+                  table_index = join_dependency.table_aliases[aliased_join_table_name]
+                  @aliased_join_table_name = @aliased_join_table_name[0..active_record.connection.table_alias_length-3] + "_#{table_index+1}" if table_index > 0
                 end
                 join_dependency.table_aliases[aliased_join_table_name] += 1
               end
@@ -1227,12 +1231,13 @@ module ActiveRecord
             def association_join
               join = case reflection.macro
                 when :has_and_belongs_to_many
-                  " LEFT OUTER JOIN %s %s ON %s.%s = %s.%s " % [
-                     options[:join_table], aliased_join_table_name, aliased_join_table_name,
+                  " LEFT OUTER JOIN %s ON %s.%s = %s.%s " % [
+                     table_alias_for(options[:join_table], aliased_join_table_name),
+                     aliased_join_table_name,
                      options[:foreign_key] || reflection.active_record.to_s.classify.foreign_key,
                      reflection.active_record.table_name, reflection.active_record.primary_key] +
-                  " LEFT OUTER JOIN %s %s ON %s.%s = %s.%s " % [
-                     table_name, aliased_table_name, aliased_table_name, klass.primary_key,
+                  " LEFT OUTER JOIN %s ON %s.%s = %s.%s " % [
+                     table_name_and_alias, aliased_table_name, klass.primary_key,
                      aliased_join_table_name, options[:association_foreign_key] || klass.table_name.classify.foreign_key
                      ]
                 when :has_many, :has_one
@@ -1244,42 +1249,44 @@ module ActiveRecord
                         polymorphic_foreign_key  = through_reflection.options[:as].to_s + '_id'
                         polymorphic_foreign_type = through_reflection.options[:as].to_s + '_type'
 
-                        " LEFT OUTER JOIN %s %s ON (%s.%s = %s.%s AND %s.%s = %s) "  % [
-                          through_reflection.klass.table_name, aliased_join_table_name,
+                        " LEFT OUTER JOIN %s ON (%s.%s = %s.%s AND %s.%s = %s) "  % [
+                          table_alias_for(through_reflection.klass.table_name, aliased_join_table_name),
                           aliased_join_table_name, polymorphic_foreign_key,
                           parent.aliased_table_name, parent.primary_key,
                           aliased_join_table_name, polymorphic_foreign_type, klass.quote(parent.active_record.base_class.name)] +
-                        " LEFT OUTER JOIN %s %s ON %s.%s = %s.%s " % [table_name, aliased_table_name,
+                        " LEFT OUTER JOIN %s ON %s.%s = %s.%s " % [table_name_and_alias,
                           aliased_table_name, primary_key, aliased_join_table_name, options[:foreign_key] || reflection.klass.to_s.classify.foreign_key
                         ]
                       else # has_many :through against a normal join
-                        " LEFT OUTER JOIN %s %s ON %s.%s = %s.%s "  % [
-                          through_reflection.klass.table_name, aliased_join_table_name, aliased_join_table_name, 
+                        " LEFT OUTER JOIN %s ON %s.%s = %s.%s "  % [
+                          table_alias_for(through_reflection.klass.table_name, aliased_join_table_name), aliased_join_table_name,
                           through_reflection.options[:foreign_key] || through_reflection.active_record.to_s.classify.foreign_key,
                           parent.aliased_table_name, parent.primary_key] +
-                        " LEFT OUTER JOIN %s %s ON %s.%s = %s.%s " % [
-                          table_name, aliased_table_name,
+                        " LEFT OUTER JOIN %s ON %s.%s = %s.%s " % [
+                          table_name_and_alias,
                           aliased_table_name, primary_key, 
                           aliased_join_table_name, options[:foreign_key] || klass.to_s.classify.foreign_key
                         ]
                       end
                       
                     when reflection.macro == :has_many && reflection.options[:as]
-                      " LEFT OUTER JOIN %s %s ON %s.%s = %s.%s AND %s.%s = %s" % [table_name, aliased_table_name,
+                      " LEFT OUTER JOIN %s ON %s.%s = %s.%s AND %s.%s = %s" % [
+                        table_name_and_alias,
                         aliased_table_name, "#{reflection.options[:as]}_id",
                         parent.aliased_table_name, parent.primary_key,
                         aliased_table_name, "#{reflection.options[:as]}_type",
                         klass.quote(parent.active_record.base_class.name)
                       ]
                     else
-                      " LEFT OUTER JOIN %s %s ON %s.%s = %s.%s " % [table_name, aliased_table_name,
+                      " LEFT OUTER JOIN %s ON %s.%s = %s.%s " % [
+                        table_name_and_alias,
                         aliased_table_name, options[:foreign_key] || reflection.active_record.to_s.classify.foreign_key,
                         parent.aliased_table_name, parent.primary_key
                       ]
                   end
                 when :belongs_to
-                  " LEFT OUTER JOIN %s %s ON %s.%s = %s.%s " % [table_name, aliased_table_name,
-                     aliased_table_name, reflection.klass.primary_key,
+                  " LEFT OUTER JOIN %s ON %s.%s = %s.%s " % [
+                     table_name_and_alias, aliased_table_name, reflection.klass.primary_key,
                      parent.aliased_table_name, options[:foreign_key] || klass.to_s.classify.foreign_key
                     ]
                 else
@@ -1288,10 +1295,27 @@ module ActiveRecord
               join << %(AND %s.%s = %s ) % [
                 aliased_table_name, 
                 reflection.active_record.connection.quote_column_name(reflection.active_record.inheritance_column), 
-                klass.quote(klass.name)] unless klass.descends_from_active_record?
+                klass.quote(klass.name)] if sti?
               join << "AND #{eval("%(#{reflection.options[:conditions]})")} " if reflection.options[:conditions]
               join
             end
+            
+            protected
+              def sti?
+                !klass.descends_from_active_record?
+              end
+              
+              def pluralize(table_name)
+                ActiveRecord::Base.pluralize_table_names ? table_name.to_s.pluralize : table_name
+              end
+              
+              def table_alias_for(table_name, table_alias)
+                "#{table_name} #{table_alias if table_name != table_alias}".strip
+              end
+
+              def table_name_and_alias
+                table_alias_for table_name, @aliased_table_name
+              end
           end
         end
     end
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
index d0b8a1ba1c..8c94f07db9 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -14,12 +14,8 @@ module ActiveRecord
       end
 
       # Truncates a table alias according to the limits of the current adapter.  
-      def table_alias_for(table_name, index = 1)
-        if index > 1
-          "#{table_name[0..table_alias_length-3]}_#{index}"
-        else
-          table_name[0..table_alias_length-1]
-        end
+      def table_alias_for(table_name)
+        table_name[0..table_alias_length-1]
       end
 
       # def tables(name = nil) end
diff --git a/activerecord/test/adapter_test.rb b/activerecord/test/adapter_test.rb
index 2c98045cee..af94904eee 100644
--- a/activerecord/test/adapter_test.rb
+++ b/activerecord/test/adapter_test.rb
@@ -47,16 +47,18 @@ class AdapterTest < Test::Unit::TestCase
   end
 
   def test_table_alias
-    old = @connection.table_alias_length
-    def @connection.table_alias_length() 10; end
+    def @connection.test_table_alias_length() 10; end
+    class << @connection
+      alias_method :old_table_alias_length, :table_alias_length
+      alias_method :table_alias_length,     :test_table_alias_length
+    end
       
     assert_equal 'posts',      @connection.table_alias_for('posts')
-    assert_equal 'posts',      @connection.table_alias_for('posts', 1)
-    assert_equal 'posts_2',    @connection.table_alias_for('posts', 2)
     assert_equal 'posts_comm', @connection.table_alias_for('posts_comments')
-    assert_equal 'posts_co_2', @connection.table_alias_for('posts_comments', 2)
     
-    def @connection.table_alias_length() old; end
+    class << @connection
+      alias_method :table_alias_length, :old_table_alias_length
+    end
   end
 
   # test resetting sequences in odd tables in postgreSQL
diff --git a/activerecord/test/associations_cascaded_eager_loading_test.rb b/activerecord/test/associations_cascaded_eager_loading_test.rb
index 8f0a41f593..166d2b2262 100644
--- a/activerecord/test/associations_cascaded_eager_loading_test.rb
+++ b/activerecord/test/associations_cascaded_eager_loading_test.rb
@@ -85,4 +85,22 @@ class CascadedEagerLoadingTest < Test::Unit::TestCase
     assert_equal [topics(:second)], replies
     assert_equal topics(:first), assert_no_queries { replies.first.topic }
   end
+
+  def test_eager_association_loading_with_multiple_stis_and_order
+    author = Author.find(:first, :include => { :posts => [ :special_comments , :very_special_comment ] }, :order => 'authors.name, special_comments.body, very_special_comments.body', :conditions => 'posts.id = 4')
+    assert_equal authors(:david), author
+    assert_no_queries do
+      author.posts.first.special_comments
+      author.posts.first.very_special_comment
+    end
+  end
+  
+  def test_eager_association_loading_of_stis_with_multiple_references
+    authors = Author.find(:all, :include => { :posts => { :special_comments => { :post => [ :special_comments, :very_special_comment ] } } }, :order => 'special_comments.body, very_special_comments.body', :conditions => 'posts.id = 4')
+    assert_equal [authors(:david)], authors
+    assert_no_queries do
+      authors.first.posts.first.special_comments.first.post.special_comments
+      authors.first.posts.first.special_comments.first.post.very_special_comment
+    end
+  end
 end
diff --git a/activerecord/test/associations_test.rb b/activerecord/test/associations_test.rb
index 89d85bd567..6b5b8a8d70 100755
--- a/activerecord/test/associations_test.rb
+++ b/activerecord/test/associations_test.rb
@@ -1471,6 +1471,6 @@ class HasAndBelongsToManyAssociationsTest < Test::Unit::TestCase
   end
 
   def test_join_table_alias
-    assert_equal 3, Developer.find(:all, :include => {:projects => :developers}, :conditions => 'join_project_developers.joined_on IS NOT NULL').size
+    assert_equal 3, Developer.find(:all, :include => {:projects => :developers}, :conditions => 'developers_projects_join.joined_on IS NOT NULL').size
   end
 end