#340 ✓resolved

Ensure description of associations hierarchy is still correct

Reported by Obie | July 9th, 2010 @ 02:05 PM | in The Rails 3 Way

From description of the patch at https://rails.lighthouseapp.com/projects/8994/tickets/1642-hasoneth...

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 https://rails.lighthouseapp.com/projects/8994/tickets/895-has_one-t... .

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 https://rails.lighthouseapp.com/projects/8994/tickets/895-has_one-t.... 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 https://rails.lighthouseapp.com/projects/8994/tickets/1643-associat... 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.

Comments and changes to this ticket

  • Obie

    Obie September 15th, 2010 @ 01:02 AM

    • State changed from “new” to “resolved”
    • Assigned user set to “Obie”
    • Tag changed from up for grabs, chapter07 to chapter07
    • Milestone order changed from “273” to “0”

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

Shared Ticket Bins

People watching this ticket