HasOneThroughAssociation should not be a child of HasManyThroughAssociation

Good call - that has always seemed a bit messy to me (and has been responsible for a fair few bugs). Haven't looked at patch in detail yet, but tests all pass

+1

+1 I've had problems with has_one through in 2.1 and 2.2 (edge seems to resolve them, but the existing implementation seemed like a bad hack)

To put not too fine a point on it, this is a fundamentally improper use of inheritance; the relationship exists to share implementation rather than interface. Ruby provides modules for this exact situation. Pulling the shared functionality into a shared module is straightforward, and simplifies the code by removing special case interface overrides.

This inheritance relationship has already caused its share of bugs, as mentioned by commenters on the ticket. I fixed two here: http://rails.lighthouseapp.com/p... More importantly, the added complexity created by importing all of the collection logic and interface into a non-collection association class just adds to rigidity and potential for odd bugs in the future.

"HasOneThroughAssociation inheriting from HasManyThroughAssociation seems logical to me".

It does?

Adam Milligan is a my refactoring hero. Adam Milligan refactoring scope is not limited to computers. Adam Milligan once refactored water into wine. Adam Milligan's refactoring patch SHOULD BE ACCEPTED!!! (in all serious)

+1 for accepting this patch. HasOneThroughAssociation < HasOneAssociation makes far more sense to me than HasOneThroughAssociation < HasManyThroughAssociation.

This seems like a good thing to merge in now that edge is all shaken up by Yehuda's changes for Rails 3

+1 for applying this patch.

+1 to any reasonably intelligent refactoring out of special cases in AR

Just so you all know, the blind +1s here have no significance whatsoever and will only have a negative effect, if any. Quoting from the "contributor guide" :

If your comment simply says +1, then odds are other reviewers aren't going to take it too seriously. Show that you took the time to review the patch.

I stick to what I said in http://groups.google.com/group/r...

+1 Code cleanup and being dry tends to be a good thing. I like the ThroughAssociationScope extraction, because it moves common code into one place and removes the need to have special case if/else statements that require more explicit tests and make maintenance more difficult.

+1 This code is really hard to work with.

I'd also say that Rails in general could use a good deal of refactoring.

Navigating some sections gives me a headache. There is some serious code debt in the framework.

+1 for cleaner code. rails should be pretty on the inside too.

1) Please take a look at activerecord/lib/active_record/associations.rb, line 1236. This is the code that defies the association setter method for all associations. On line 1243 the code explicitly checks the type of the association_proxy_class and branches to different logic if it's a HasOneThroughAssociation. For all other association types the code makes a call to a polymorphic method without regard to object type, as it should. Changing implementation based on type interrogation in this way is a clear sign of code fragility, makes the method longer and less obvious, and causes the class abstraction to leak. My patch removes the explicit check and reduces six lines of code from 1243 to 1249 to two. At the same time it makes private the #create_through_record method in the HasOneThroughAssociation class, simplifying the public interface to the class.

From a more pragmatic point of view, this branching logic was a direct cause of one of the bugs I fixed in #895 has_one :through errors .

2) Please look at activerecord/lib/active_record/associations/has_many_through_association.rb. This is a long file, 257 lines. My patch pulls all of the Through-specific login into a module, which reduces the size of this file to 112 lines, and makes an obvious distinction between different aspects of the implementation. The patch also reduced the size of the #find_target method from twelve lines to two, delegating the scope construction to the shared #construct_scope method, rather than building the scope inline.

3) Please look at activerecord/lib/active_record/associations/has_one_through_association.rb. As mentioned before this class exposes the #create_through_association method in the public interface, though it performs implementation that should be handled by the class within its abstraction. My patch makes this method private. The class also includes special-case overrides of #find, #find_target, and #reset_target! to mask the functionality inherited from HasManyThroughAssociation (the override of #reset_target! was the source of another bug I fixed in #895 has_one :through errors. My patch removes the need for #find and #reset_target!, since the inherited behavior from HasOneAssociation is already correct, and changes the implementation of #find_target to mirror the implementation of #find_target on the HasManyThroughAssociation class, which is now a peer. In this case, it calls find(:first) on the reflections class, using the scope generated by ThroughAssociationScope#construct_scope. I believe this is more clear than sending #find_target to the HasManyThroughAssociation superclass with an explicit limit override.

4) This patch makes HasOneThroughAssociation a peer of HasManyThroughAssociation in the inheritance hierarchy, as it properly should be. HasOneAssociation is a peer of HasManyAssociation; doing the same with their Through counterparts is far more obvious than deepening the hierarchy only under HasManyAssociation. As well, the current inheritance tree imports all of the code from association_collection.rb and has_many_association.rb as well as hash_many_through_association.rb into has_one_through_association.rb. Moving HasOneThroughAssociation up the inheritance hierarchy to its proper place removes a lot of inherited cruft that can only cause complication (for instance, it made #1643 Association proxies should correctly respond to method defined via #method_mi... unnecessarily difficult) and confuse programmers who interrogate the HasOneThroughAssociation class for its capabilities.

5) All of this is in addition to my argument that the current implementation is a fundamental misuse of inheritance. This is not an esoteric argument, the concept of encapsulation over inheritance for sharing of implementation is well worn, baked into Liskov Substitutability, permeating Design Patterns, advocated regularly by the likes of Dave Thomas, etc. It's been proven enough times to matter that it makes code easier to understand, easier to change, and more resilient to defects. Ignore that at your peril, I suppose.

+1

Yes, please apply this patch. I occasionally delve deep into the association code, but always with enough intervening time to forget the details. It's no fun and anything that makes the code clearer and easier to understand is a good thing in my book.

Automatic cleanup of spam.

Automatic cleanup of spam.