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

Ticket #11516 (closed enhancement: fixed)

Opened 2 years ago

Last modified 2 years ago

[PATCH] HasManyThroughAssociation consistency refactor M-M-M-M-MEGA PATCH

Reported by: rubyruy Assigned to: nzkoz
Priority: normal Milestone: 2.x
Component: ActiveRecord Version: edge
Severity: normal Keywords: aciverecord has_many through refactor
Cc: lifofifo, cch1, nzkoz

Description

This patch makes the HasManyThroughAssociation behave more consistently with the other AssociationCollection methods by delegating as much as possible to the AssociationCollection class.

The detailed implications of this follow:

  • :before_remove callbacks now get called for unsaved associates during .collection.delete() (for both HasManyThroughAssociation and HasAndBelongsToManyAssociation)
  • HasManyAssociation#build() has been moved to AssociationCollection#build() and AssociationCollection subclasses all defer to it. HasManyThroughAssociation#build() and HasAndBelongsToManyAssociation#build() call load_target before defering to super.
  • HasManyThroughAssociation#reset() has been removed and will defer to the equivalent AssociationCollection#reset() instead.
  • HasManyThroughAssociation#delete() and HasManyThroughAssociation#<<() have been removed and will defer to AssociationCollection, which in turn will use the newly added HasManyThroughAssociation#insert_record() and HasManyThroughAssociation#delete_records() protected methods.
    • Because of this HasManyThroughAssociation#replace() now works consistently with the other AssociationCollections
    • And in turn because of this the has_many macro will add writer methods for the association.
  • Added check for attempting to associate or disassociate an item on a has_many :through association where the source reflection relates back to the owner via has_many (instead of the usual belongs_to).
    • I felt that it's just a little too presumptuous to just guess what to do with the join model. The belongs_to scenario entails some guesswork as well, but since that case is basically the equivalent of has_and_belongs_to_many with a visible join model it seems quite save to just assume the same behaviour as has_and_belongs_to_many.
  • Added test cases for new functionality (I would have liked to do a more thorough job with this but I think that would require a pretty major overhaul of the active record tests)
  • Removed test cases which ensured the previously non-existing features would be caught early (like .collection_ids= not being defined for HasManyThrough)

Attachments

has_many_through_refactor.diff (20.4 kB) - added by rubyruy on 04/03/08 19:05:50.
has_many_through_refactor.2.diff (21.5 kB) - added by rubyruy on 04/04/08 03:15:52.
has_many_through_refactor.3.diff (24.1 kB) - added by rubyruy on 04/05/08 22:41:24.

Change History

04/03/08 00:37:36 changed by rubyruy

  • summary changed from [PATCH] HasManyThroughAsspcoaton consistency refactor M-M-M-M-MEGA PATCH to [PATCH] HasManyThroughAssociation consistency refactor M-M-M-M-MEGA PATCH.

spelling

04/03/08 01:16:28 changed by lifofifo

  • cc set to lifofifo.

04/03/08 01:35:46 changed by cch1

It was, admittedly, only after a brief look, but I was surprised that HMT is not now a subclass of AssociationCollection. I think lifofifo had been contemplating just such a restructuring, and it did seem logical. But you've tackled the problem differently...

04/03/08 01:36:07 changed by cch1

  • cc changed from lifofifo to lifofifo, cch1.

04/03/08 19:05:50 changed by rubyruy

  • attachment has_many_through_refactor.diff added.

04/03/08 19:09:52 changed by rubyruy

I fixed the sqlite3 test failures (and I'm pretty sure mysql as well) - problem was simply that my tests were creating empty records against :null=>false columns. Presumably the postgresql adapter just ignores those which is i didn't catch this the first time around. I have also updated the xs in the associations.rb rdoc.

Anything else I can do? :)

04/03/08 19:11:45 changed by lifofifo

  • owner changed from core to lifofifo.

04/04/08 03:15:52 changed by rubyruy

  • attachment has_many_through_refactor.2.diff added.

04/04/08 03:17:43 changed by rubyruy

Now inherits from HasManyAssociation, no longer calls load_target on build.

Added lots of assert_queries for all basic operations.

04/05/08 11:05:16 changed by nzkoz

  • cc changed from lifofifo, cch1 to lifofifo, cch1, nzkoz.
  • owner changed from lifofifo to nzkoz.
  • status changed from new to assigned.

I definitely like where this patch is going. Unifying the two different associations is a great idea and you've done really good work here.

A few comments though. The test cases have lots of assert_queries (which is great) but the numbers seem a little arbitrary, perhaps include some comments why we have those numbers for some of the less obvious cases. f.ex 

assert_queries(2) { 
  posts(:thinking).people << new_person 
}

Stylistically, we use do/end for multi line blocks.

There's a few TODOs in there, do you intend to fix those up before we apply them? Or do we need to document the restrictions they impose?

Finally, while we're adding a m-m-m-m-mega patch, perhaps you could break out the has_many and has_many through tests to their own files rather than one mammoth 'associations test'?

Again, really awesome work!

04/05/08 22:41:24 changed by rubyruy

  • attachment has_many_through_refactor.3.diff added.

04/05/08 22:48:41 changed by rubyruy

Aright I resolved the conflicts, tests still run everything looks ok.

I changed the block syntax and explained the queries - makes a lot more sense to me as well now.

About the TODOs - I removed them from now. I added a warning to the :destroy option for has_many - mind you, this stuff was NEVER documented to begin with and actually documenting the fact that has_many :through is a completely different beast from just plain has_many is something that is sorely lacking and IMO beyond the scope of this patch. (However i see somebody is working on removing the :nodocs: from the proxy classes themselves so maybe not for much longer)

Having said that I would rather work on removing those inconsistencies then documenting them (but not in this patch).

Similarly, splitting up / re-organizing the association tests is also something I would rather do wholesale. As things stand I think it's better to be consistent and not treat the HMT tests differently from the other ones.

I would love to tackle the association tests next (for a much more thorough organization), but I would really like just having this go in first.

04/06/08 00:27:15 changed by pratik

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [9230]) Refactor HasManyThroughAssociation to inherit from HasManyAssociation. Association callbacks and <association>_ids= now work with hm:t. Closes #11516 [rubyruy]