On 2/25/06, Rick Olson <[EMAIL PROTECTED]> wrote: > > I haven't tested the patch, but it aliases the table names in the SQL, > > so it may fix the problems that 1562 addresses (hopefully someone else > > can give a more definitive answer). I hope it does and it gets > > included in Rails 1.1, that way I won't have to keep updating the 1562 > > patch and associated plugin (which would probably be painful because > > of the changes made between 1.0 and Edge). > > Yes, this will fix that issue. I've used this on one of my apps and > it worked almost flawlessly. I updated a fixed patch that fixes on > other issue.
After taking a look at the patch more closely (without testing it), I can see that it is still broken when it comes to multiple associations against the same table, in the following manners: 1) has_and_belongs_to_many associations don't use table aliases, so you can't eager load multiple habtm associations that use the same final table or the same join table 2) Eager loading tables with STI conditions isn't done correctly (even with Rick's fix) 3) Eager loading associations that have :conditions specified isn't done correctly Take a look at the latest patch from 1562 and compare it against the latest from 3193, specifically the association_join method. 3193 adds useful features not included in 1562 (which was purely a bugfix patch), so it is certainly what you would want to base future development on, but the problems mentioned above should be fixed before it is applied (or at least plan to fix them in the future). Unfortunately, the 3193 patch is a lot more complex and invasive and contains no documentation, and I'm not sure how easy it would be to fix the above problems > Even though it aliases the table names, the STI where statements are > using the table name, causing STI queries to fail. > > class Content < AR::Base > end > > class Product < Content > has_many :comments > end > > class Comment < Content > end > > Doing this gets me a query ending with: > > (contents.type = 'Product') and (contents.type = 'Comment') > > I fixed this to use table aliases, so now you get: > > (contents.type = 'Product') and (t1.type = 'Comment') Using t1..tn for table aliases is probably not a good idea. How will this affect code that wants to use the alias in :conditions or :order? Is it easy for users to determine how table names will be aliased so they can specify the correct alias? The 1562 patch based the associated table alias on the table name and the association name, so it was easy to see what the alias would be for any associated table. 3193 is a step backwards in this regard. > However, this was leaving out otherwise valid products that have 0 comments. > > (contents.type = 'Product') and (t1.type = 'Comment' or t1.type IS NULL) > > This will get all products, and comments if they're available. I > added a sample test case to prove this as well. The correct way to handle this is to add the t1.type='Comment' to the appropriate JOIN ON clause. If the patch specifies the STI conditions in the WHERE clause instead of the appropriate JOIN ON clause, it is broken. Granted, it probably wouldn't display incorrect behavior unless there were actually entries in the table with NULL for the type attribute, but it's still incorrect. Conditions for joining tables during eager loading should always be specified in the JOIN ON conditions, specifying the table join conditions in the WHERE clause is broken behavior. This is true for both STI conditions and other restrictions specified via the association's :conditions. > This patch gets my vote. It gets my vote too, as it is superior to the current rails behavior, but it should be changed so that it fixes all the problems that 1562 fixed. _______________________________________________ Rails-core mailing list [email protected] http://lists.rubyonrails.org/mailman/listinfo/rails-core
