Rework table aliasing to account for truncated table aliases. Add sma…

…rter 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

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]
 

activerecord/lib/active_record/associations.rb

@@ -1200,24 +1200,28 @@ def instantiate(row)
           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 @@ def initialize(reflection, join_dependency, parent = nil)
             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 @@ def association_join
                         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 @@ def association_join
               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

activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb

@@ -14,12 +14,8 @@ def table_alias_length
       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

activerecord/test/adapter_test.rb

@@ -47,16 +47,18 @@ def test_current_database
   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

activerecord/test/associations_cascaded_eager_loading_test.rb

@@ -85,4 +85,22 @@ def test_eager_association_loading_with_belongs_to_sti
     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

activerecord/test/associations_test.rb

@@ -1471,6 +1471,6 @@ def test_update_attributes_after_push_without_duplicate_join_table_rows
   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