Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #6461 (new defect)

Opened 2 years ago

Last modified 10 months ago

[PATCH] Support nested has_many_through associations

Reported by: obrie Assigned to: technoweenie@gmail.com
Priority: normal Milestone: 2.x
Component: ActiveRecord Version:
Severity: normal Keywords: tested nested has_many through associations
Cc: matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, jcoglan, ian.w.white@gmail.com, c.shoemaker@cox.net, danger, terceiro@colivre.coop.br, moises@colivre.coop.br, mroch@cmu.edu

Description

Attempting to perform a has_many :through with a source association as a has_many :through currently fails:

class Author < ActiveRecord::Base
  has_many :posts
  has_many :categories, :through => :posts
  has_many :recommended_posts, :through => :categories, :source => :posts
  has_many :recommended_authors, :through => :recommended_posts, :source => :author
  has_many :posts_of_recommended_authors, :through => :recommended_authors, :source => :posts

  has_many :commenters, :through => :posts
end

class Post < ActiveRecord::Base
  belongs_to :author
  belongs_to :category
  has_many :comments
  has_many :commenters, :through => :comments, :source => :user
end

class Category < ActiveRecord::Base
  has_many :posts
end

class Comment < ActiveRecord::Base
  belongs_to :user
  belongs_to :post
end
>> a=Author.create
=> #<Author:0x585c0b4 @new_record=false, @new_record_before_save=true, @errors=#<ActiveRecord::Errors:0x584ebd0 @base=#<
Author:0x585c0b4 ...>, @errors={}>, @attributes={"id"=>1}>
>> a.categories
=> []
>> a.recommended_posts
ActiveRecord::StatementInvalid: Mysql::Error: Unknown column 'categories.author_id' in 'where clause': SELECT posts.* FROM posts  INNER JOIN categories ON posts.category_id = categories.id    WHERE ((categories.author_id = 1))
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/connection_adapters/abstract_adapter.rb:122:in `log'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/connection_adapters/mysql_adapter.rb:224:in `execute'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/connection_adapters/mysql_adapter.rb:378:in `select'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/connection_adapters/mysql_adapter.rb:215:in `select_all'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/base.rb:424:in `find_by_sql'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/base.rb:993:in `find_every'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/base.rb:415:in `find'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/associations/has_many_through_association.rb:97:in `find_target'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/associations/association_proxy.rb:131:in `load_target'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/associations/association_proxy.rb:122:in `method_missing'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/associations/has_many_through_association.rb:90:in `method_missing'
        from C:/Program Files/ruby/lib/ruby/1.8/irb.rb:298:in `output_value'
        from C:/Program Files/ruby/lib/ruby/1.8/irb.rb:151:in `eval_input'
        from C:/Program Files/ruby/lib/ruby/1.8/irb.rb:259:in `signal_status'
        from C:/Program Files/ruby/lib/ruby/1.8/irb.rb:147:in `eval_input'
        from C:/Program Files/ruby/lib/ruby/1.8/irb.rb:146:in `eval_input'
        from C:/Program Files/ruby/lib/ruby/1.8/irb.rb:70:in `start'
        from C:/Program Files/ruby/lib/ruby/1.8/irb.rb:69:in `catch'
        from C:/Program Files/ruby/lib/ruby/1.8/irb.rb:69:in `start'
        from C:/Program Files/ruby/bin/irb.bat:20>>
?> a.commenters
ActiveRecord::HasManyThroughSourceAssociationMacroError: Invalid source reflection macro :has_many :through for has_many :commenters, :through => :posts.  Use :source to specify the source reflection.
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/reflection.rb:181:in `check_validity!'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/associations/has_many_through_association.rb:6:in `initialize'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/associations.rb:926:in `new'
        from C:/Program Files/ruby/lib/ruby/gems/1.8/gems/activerecord-1.14.4.5263/lib/active_record/associations.rb:926:in `commenters'
        from (irb):5

The proposed patch fixes the problem by allowing a has_many :through go through an arbitrary number of other has_many :through associations:

>> a=Author.create
=> #<Author:0x5854a44 @new_record=false, @new_record_before_save=true, @errors=#<ActiveRecord::Errors:0x5847560 @base=#<Author:0x5854a44 ...>, @errors={}>, @attributes={"id"=>1}>
>> a=Author.find(1)
=> #<Author:0x58435b4 @attributes={"id"=>"1"}>
>> a.categories
=> [#<Category:0x582ee0c @attributes={"id"=>"1"}>]
>> a.recommended_posts
=> [#<Post:0x582c6ac @attributes={"id"=>"1", "category_id"=>"1", "author_id"=>"1"}>, #<Post:0x582c670 @attributes={"id"=>"2", "category_id"=>"1", "author_id"=>"2"}>]
>> a.recommended_authors
=> [#<Author:0x582a03c @attributes={"id"=>"1"}>, #<Author:0x582a000 @attributes={"id"=>"2"}>]
>> a.commenters
=> [#<User:0x5811b7c @attributes={"id"=>"1"}>]

Attachments

arbitrary_has_many_through.diff (8.7 kB) - added by obrie on 10/20/06 15:56:02.
Initial attempt.
nested_has_many_through_updated.diff (15.5 kB) - added by obrie on 02/15/07 02:55:14.
Updated with a few more tests and some fixes.
nested_has_many_through_20070428b.diff (19.4 kB) - added by mattwestcott on 04/28/07 15:14:26.
now applies against trunk as of r6608, and fixes test_multiple_table_references_in_nested_through_associations
nested_has_many_through_20070428clean.diff (17.9 kB) - added by mattwestcott on 04/28/07 23:11:06.
major code cleanup
6461_nested_has_many_through_20070721.diff (18.4 kB) - added by mattwestcott on 07/21/07 02:08:01.
fix for sticky find(:conditions) clauses, and for updated fixtures as of r7119
nested_has_many_through_with_correct_primary_keys.diff (18.7 kB) - added by jcoglan on 08/09/07 14:38:38.
Update of Matt's previous patch that also fixes #7621
has_many_through_STI_subclasses_fix_for_6461.patch (1.0 kB) - added by mhoroschun on 09/02/07 12:38:03.
0001-Nested-has_many-through-associations.patch (23.8 kB) - added by shoe on 03/14/08 14:18:56.
Nested has_many :through, ported to edge
0001-Nested-has_many-through-associations.diff (21.7 kB) - added by danger on 03/14/08 19:52:51.
copy of the previous patch in GNU diff format
0001-Fix-table-aliasing-for-quoted-tables-in-JoinAssociat.patch (1.4 kB) - added by shoe on 03/14/08 21:44:57.

Change History

10/20/06 15:56:02 changed by obrie

  • attachment arbitrary_has_many_through.diff added.

Initial attempt.

10/20/06 19:28:09 changed by obrie

  • keywords changed from tested has_many through associations to tested nested has_many through associations.
  • summary changed from [PATCH] Support through associations with source through associations to [PATCH] Support nested has_many_through associations.

(follow-up: ↓ 3 ) 10/23/06 00:09:21 changed by david

  • owner changed from bitsweat to technoweenie@gmail.com.

We need tests for this. Thanks for working on it.

(in reply to: ↑ 2 ) 01/30/07 11:43:34 changed by orangechicken

What's the word on this? This is very handy (I made it into a plugin ... should probably share it)

01/30/07 15:36:04 changed by aaron

I think we basically need a peer review of the code and a better set of unit tests. I admittedly have not had time to work on it myself.

01/30/07 20:18:56 changed by orangechicken

The good thing is that, at least up to 1.2.1, the patch applies without change. Obrie, come back and write some more tests! :)

(in reply to: ↑ description ) 02/14/07 17:04:02 changed by DerGuteMoritz

This is definitely useful, perhaps I'll find some time to create more test within the next few days!

02/14/07 18:12:46 changed by obrie

Sorry, kind of threw that patch on here and never addressed the tests. I'll see if I can come up with something as well.

02/15/07 02:55:14 changed by obrie

  • attachment nested_has_many_through_updated.diff added.

Updated with a few more tests and some fixes.

02/15/07 02:59:45 changed by obrie

The latest patch fixes issues with the conditions not being properly copied from all through/source associations. In addition, it fixes issues when both the through and source associations go to additional nested through associations.

I've added a few more tests that verified the fixes, but admittedly there's probably still additional work that can be done to test how nested through associations work with the helper methods (build, create, <<, etc.).

Unfortunately, that's all the time I can spend on it for a while. I threw in some comments in case anyone wants to take a whack at it.

04/21/07 20:54:55 changed by mattwestcott

  • cc set to matt@west.co.tt.

I'm very keen to see this supported in ActiveRecord, and in fact I've been working on this same thing myself (guess I should have taken a closer look through the active tickets first...). My own efforts are mostly working (and as a bonus, allow going through habtm/has_one relationships as per ticket:4376) but don't account for table aliasing like this patch, and involve some fairly nasty subverting of the AssociationReflection class. I'll gladly share my code if desired though...

Anyway - I'm willing to invest some time into tying up loose ends on this patch, and as an initial offering I hope to have an updated version that applies against current trunk shortly (currently I'm getting a failure on test_multiple_table_references_in_nested_through_associations). What else needs to be done here - just more unit tests?

04/28/07 15:14:26 changed by mattwestcott

  • attachment nested_has_many_through_20070428b.diff added.

now applies against trunk as of r6608, and fixes test_multiple_table_references_in_nested_through_associations

04/28/07 15:27:00 changed by mattwestcott

Sorry, this latest patch was a bit more of a slog than expected - the one failing test wasn't a routine fix as hoped, but appears to have been down to a logical flaw in obrie's approach (find_deepest_through tries to determine a unique table alias, but since it doesn't traverse the entire tree of source/through associations it can't be guaranteed to get it right). Consequently I've merged in a chunk of my earlier effort - all tests succeed now, but it could probably do with a bit more cleaning up. Will see what I can do...

04/28/07 23:11:06 changed by mattwestcott

  • attachment nested_has_many_through_20070428clean.diff added.

major code cleanup

04/28/07 23:24:20 changed by mattwestcott

OK, as promised this latest version has been quite dramatically cleaned up - in particular I've boldly removed the construct_sql method, as it was doing a series of broken-looking things with :finder_sql and :counter_sql, which ultimately didn't amount to anything because those options are meaningless in a has_many :through association.

I'm confident that the patched has_many_through_association.rb is now as readable as the unpatched version, if not more so.

05/13/07 04:25:13 changed by mhoroschun

  • cc changed from matt@west.co.tt to matt@west.co.tt, mhoroschun@canprint.com.au.

05/14/07 23:43:37 changed by knyt

  • cc changed from matt@west.co.tt, mhoroschun@canprint.com.au to matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net.

05/31/07 04:04:37 changed by knyt

Nice patch. Any chance you could get it to work along with HABTM associations? :)

 
def First < AR; has_many :seconds; has_many :thirds, :through => :seconds; has_many :fourths, :through => :thirds; end

def Second < AR; belongs_to :first; has_many :thirds; has_many :fourths, :through => :thirds; end

def Third < AR; belongs_to :second; has_and_belongs_to_many :fourths; end

def Fourth < AR; has_and_belongs_to_many :thirds; end

a = First.find_first
a.fourths

# ActiveRecord::HasManyThroughSourceAssociationMacroError: Invalid source reflection
# macro :has_and_belongs_to_many for has_many :fourths, :through => :thirds.  Use
# :source to specify the source reflection.

06/10/07 22:30:55 changed by mattwestcott

I've now assembled this into a plugin (already in production on one site with another on the way, BTW :-) ):

http://code.torchbox.com/svn/rails/plugins/nested_has_many_through/

Currently it's lacking unit tests of its own, which I hope to correct shortly (fx: slapped wrists)...

knyt: Sure, the shake-up of code I've done in this patch should actually make HABTM easier to implement. It's definitely on the agenda (and I think there may already be a ticket for it somewhere), although since you can easily replace a HABTM relationship with a proper join model, there isn't an immediate itch to be scratched there.

07/17/07 06:00:38 changed by evan

  • cc changed from matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net to matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, evan.

07/21/07 02:08:01 changed by mattwestcott

  • attachment 6461_nested_has_many_through_20070721.diff added.

fix for sticky find(:conditions) clauses, and for updated fixtures as of r7119

07/21/07 02:13:31 changed by mattwestcott

This latest version of the patch fixes a test which broke in r7119 due to a change in the fixtures (- now applies correctly as of r7200), and fixes a bug in the find method where :conditions clauses would persist in subsequent queries (because it mutated the cached array returned by construct_conditions).

08/09/07 14:38:38 changed by jcoglan

  • attachment nested_has_many_through_with_correct_primary_keys.diff added.

Update of Matt's previous patch that also fixes #7621

08/09/07 14:41:26 changed by jcoglan

  • cc changed from matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, evan to matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, evan, jcoglan.

I've updated Matt's latest patch with a fix for #7621 - the fix for that goes in the old construct_joins method so it needs to go in here. I've only tested this via my current project, but maybe someone could use the tests from my patch for #7621 to enhance this one?

09/02/07 12:38:03 changed by mhoroschun

  • attachment has_many_through_STI_subclasses_fix_for_6461.patch added.

09/02/07 12:39:58 changed by mhoroschun

Ticket #7908 supplies a patch which fixes has_many :through associations when using multiple levels of STI. That patch doesn't apply correctly after applying the patches on this ticket.

I've attached has_many_through_STI_subclasses_fix_for_6461.patch, a version of the patch to #7908 which applies correctly after applying the patches above.

12/11/07 14:40:00 changed by ian.w.white@gmail.com

  • cc changed from matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, evan, jcoglan to matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, evan, jcoglan, ian.w.white@gmail.com.

02/16/08 01:32:49 changed by lindvall

+1

The plugin works great.

02/28/08 15:01:08 changed by steph(an)

Plugin breaks in Rails 2.0.2 because extract_options_from_args! no longer exists. You can use args.extract_options! instead.

03/11/08 16:44:58 changed by shoe

  • cc changed from matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, evan, jcoglan, ian.w.white@gmail.com to matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, evan, jcoglan, ian.w.white@gmail.com, c.shoemaker@cox.net.

03/11/08 18:34:07 changed by shoe

Ok, I've only been playing with this for an hour or so, but I can't resist reporting my findings so far. This is exactly what I've been searching for for a few weeks now. I've been hobbling along with the standard hacks: finder_sql just doesn't compose well and caching my own custom finders is a pain.

Add to that the frustration of seeing no good reason why has_many :through shouldn't be chainable, and I started down the very path to implement this. I was quite excited to find this pre-existing code, and even more pleased when it worked beautifully for me.

I'm even using this with STI classes in the mix, and I was also able to disambiguate column names in :conditions using the tablename prefixes.

In short, this is awesome. Thanks, Matt.

+1

03/11/08 21:53:37 changed by subimage

I've wondered many times why rails doesn't do this already.

+1

03/11/08 22:03:05 changed by shoe

  • keywords changed from tested nested has_many through associations to verified tested nested has_many through associations.

03/11/08 22:05:47 changed by shoe

Note: I tested with 2.0.2 and I did eventually have change to the args.extract_options! mentioned above.

03/12/08 14:44:09 changed by lifofifo

  • keywords changed from verified tested nested has_many through associations to tested nested has_many through associations.

Could someone please post which one of the 7 patches applies cleanly ? I tried second last patch, which doesn't apply cleanly. Add back "verified" keyword once there is a patch that applies cleanly.

Thanks.

03/14/08 14:18:56 changed by shoe

  • attachment 0001-Nested-has_many-through-associations.patch added.

Nested has_many :through, ported to edge

03/14/08 14:21:19 changed by shoe

  • keywords changed from tested nested has_many through associations to verified tested nested has_many through associations.

This is a major update to the patch attached to #6461: nested_has_many_through_with_correct_primary_keys.diff

That patch needed substantial re-working to apply cleanly to edge. While the main algorithm (recurse through the associations to construct the join) remains the same, the following changed:

  • Test files moved to new places and updated for fixture changes.
  • Use the newer #finder_needs_type_condition? method for checking if we need an STI filter condition.
  • Document the limitations w.r.t. #delete and #push
  • Aliased table quoting: the edge code got a lot more rigorous about quoting than it was when this code was written, so this required a complete audit for aliased table name quoting.
  • AR::Base#type_condition needed to be taught to optionally use an aliased table name.
  • The fix for #7621 had to be reworked for this code. (The patch for that sitting in #6461 isn't correct, because it doesn't use the aliased table name.)
  • There's a somewhat unrelated generalization of the regex looking for already-used table names to know if new joins have to have their tables aliased. The old regex failed to detect quoted table names, so it would reuse a quoted table name without an alias.
  • General style and formatting changes to conform to surrounding code.

03/14/08 14:30:07 changed by shoe

  • keywords changed from verified tested nested has_many through associations to tested nested has_many through associations.

Considering all the changes, please retest and re +1 so we can re-verify.

03/14/08 19:32:01 changed by danger

  • cc changed from matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, evan, jcoglan, ian.w.white@gmail.com, c.shoemaker@cox.net to matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, evan, jcoglan, ian.w.white@gmail.com, c.shoemaker@cox.net, danger.

03/14/08 19:52:51 changed by danger

  • attachment 0001-Nested-has_many-through-associations.diff added.

copy of the previous patch in GNU diff format

03/14/08 19:55:27 changed by danger

shoe: I'm getting some failing tests on the latest patch. Thanks so much for working on this, it's a huge help.

03/14/08 20:55:12 changed by shoe

danger: could you paste a failing test? I tested with postgresql and I only get one failure (something about BigMoney) that has been there for a while.

03/14/08 21:05:20 changed by danger

This is against revision 9025: http://pastie.caboo.se/165829

03/14/08 21:26:22 changed by evan

  • cc changed from matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, evan, jcoglan, ian.w.white@gmail.com, c.shoemaker@cox.net, danger to matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, jcoglan, ian.w.white@gmail.com, c.shoemaker@cox.net, danger.

03/14/08 21:41:48 changed by shoe

danger: ok, that makes complete sense. Here's what's happening: This patch happens to uncover a table aliasing bug in associations.rb. This bug is triggerable by any query that joins the same table twice, which isn't that common. However, nested hmt makes this more common, and it is in fact used by the test cases several times.

I went ahead and tried to fix this bug, too, but I fixed it in a way that only worked for postgresql. Here's a quick solution:

Find the regex in assoications.rb: %r{join(\s+\w+)?\s+"?#{aliased_table_name.downcase}"?\s+on}

and add in some more quote characters, like: %r{join(\s+\w+)?\s+["'`]?#{aliased_table_name.downcase}['"`]?\s+on}

that will probably work, but, really, its not such a great solution. A better idea would probably be to just get the quoted table name from the connection, and check for both the quoted and unquoted versions.

I'll post a patch to do just that in a second. The patch will apply on top of this one.

03/14/08 21:44:57 changed by shoe

  • attachment 0001-Fix-table-aliasing-for-quoted-tables-in-JoinAssociat.patch added.

03/20/08 15:25:02 changed by terceiro

  • cc changed from matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, jcoglan, ian.w.white@gmail.com, c.shoemaker@cox.net, danger to matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, jcoglan, ian.w.white@gmail.com, c.shoemaker@cox.net, danger, terceiro@colivre.coop.br, moises@colivre.coop.br.

03/20/08 18:08:12 changed by terceiro

We've just written a test for this funcionality. Actually I wrote it against the published plugin, but I think it tests the feature.

We also fixed the extract_options_from_args! issue, and it kind of works with Rails 2.0.2.

FWIW the plugin is being managed (with piston, pointing to the original svn mentioned above) at:

http://svn.colivre.coop.br/svn/noosfero/trunk/vendor/plugins/nested_has_many_through/

-- terceiro and moises

04/11/08 11:00:45 changed by mattwestcott

Very happy to see things progressing on this ticket - thanks all!

Just thought I'd flag up that I previously raised the table aliasing bug (or one that looks a lot like it...) in #8267, and have an alternative patch there (complete with slightly contrived unit test that doesn't depend on nested HMT).

04/13/08 01:20:19 changed by mroch

  • cc changed from matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, jcoglan, ian.w.white@gmail.com, c.shoemaker@cox.net, danger, terceiro@colivre.coop.br, moises@colivre.coop.br to matt@west.co.tt, mhoroschun@canprint.com.au, kennyt@php.net, jcoglan, ian.w.white@gmail.com, c.shoemaker@cox.net, danger, terceiro@colivre.coop.br, moises@colivre.coop.br, mroch@cmu.edu.

04/28/08 23:13:02 changed by ian.w.white@gmail.com

This is a great patch/plugin. Kudos to all who have worked on it.

I was finding that the latest patches, and the plugin were not compatible with edge, so I've created a plugin with a bunch of acceptance specs over on github.

http://github.com/ianwhite/nested_has_many_through

There's a few specs testing the core features, and they pass on 2.0.2, 2-0-stable, and edge. There's plenty still left to do, so fork away, or I can give out commit access on repo. Cheers, Ian

05/16/08 21:28:21 changed by cpisto

This is an awesome feature, unfortunately all of the patches and plugins here break using has_many :through with a belongs_to association, for example:

class Test < ActiveRecord::Base

belongs_to :thing has_many :thingbits, :through => :thing

end