Hey guys, If you make extensive use of eager loaded associations in your application, and run on edge, we could really use your help testing a very nice, but quite intrusive patch that''s been submitted. http://dev.rubyonrails.org/ticket/3913 It''s a nice refactoring of the code, and a great new feature as it lets you nest your includes as deep as you want. # cascaded in two levels with two has_many associations>> Author.find(:all, :include=>{:posts=>[:comments, :categorizations]})=> authors +- posts +- comments +- categorizations So can you please apply the patch, run your unit tests, play around with your application, watch your development log and report back. A brief post to the ticket including your email address, and any hiccups you received would be greatly appreciated. Unit tests for any failing cases will be much appreciated. If it works for you, please comment on the ticket (again, include your email) and let us know. -- Cheers Koz
On 21-feb-2006, at 19:46, Michael Koziarski wrote:> http://dev.rubyonrails.org/ticket/3913Ah, that''s AnnaChan''s (irc nick) patch. I will most definitely test this! -- Regards, Charles.
Does this tie in with http://dev.rubyonrails.org/ticket/1562 which allows including multiple associations that reference a single table, but (AFAIK) isn''t compatible with edge rails? If this doesn''t solve the same problem as 1562, would this make solving it easier? On 22/02/2006, at 8:30 AM, Charles M. Gerungan wrote:> On 21-feb-2006, at 19:46, Michael Koziarski wrote: > >> http://dev.rubyonrails.org/ticket/3913 > > Ah, that''s AnnaChan''s (irc nick) patch. I will most definitely test > this! > > -- > Regards, Charles. > > > _______________________________________________ > Rails-core mailing list > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core
On 2/22/06, Pete Yandell <pete@notahat.com> wrote:> Does this tie in with http://dev.rubyonrails.org/ticket/1562 which > allows including multiple associations that reference a single table, > but (AFAIK) isn''t compatible with edge rails? If this doesn''t solve > the same problem as 1562, would this make solving it easier?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).
> 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. 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'') 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. This patch gets my vote. -- Rick Olson http://techno-weenie.net
On 2/25/06, Rick Olson <technoweenie@gmail.com> 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.
> 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 > Rails-core@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails-core >You make some good points. I''ll see if I can somehow integrate your fixes into this patch. It may just be easier to include the patch now, and add a smaller patch to add better table aliasing later. I also have a patch for :include support w/ polymorphic joins and has_many :through that I''ll have to redo as well. -- Rick Olson http://techno-weenie.net
FYI: this has been applied as of [3769]: http://dev.rubyonrails.org/changeset/3769 Please report back if this causes any breakage. Now we can tackle some of the eager loading issues regarding polymorphic joins and STI models. -- Rick Olson http://techno-weenie.net