This project is archived and is in readonly mode.

#1642 ✓committed
Adam Milligan

HasOneThroughAssociation should not be a child of HasManyThroughAssociation

Reported by Adam Milligan | December 27th, 2008 @ 08:16 AM | in 3.0.2

This patch changes the superclass of HasOneThroughAssociation to HasOneAssociation, moves the functionality shared between the two Through associations to a module named ThroughAssociationScope, and removes the special case overloads of the HasManyThroughAssociation methods in HasOneThroughAssociation.

No new tests, since this patch only include refactorings, no new functionality.

Comments and changes to this ticket

  • Frederick Cheung

    Frederick Cheung December 27th, 2008 @ 11:14 AM

    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

  • David Stevenson
  • Joseph Palermo

    Joseph Palermo December 30th, 2008 @ 07:25 AM

    +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)

  • Pratik

    Pratik March 7th, 2009 @ 12:09 PM

    • Assigned user set to “Pratik”
    • State changed from “new” to “wontfix”

    HasOneThroughAssociation inheriting from HasManyThroughAssociation seems logical to me. What are the advantages of this patch ? If there's some bug to be fixed by doing this, I'd happily apply. Otherwise, let's just wait till this refactoring is actually needed.

    Thanks !

  • Adam Milligan

    Adam Milligan March 9th, 2009 @ 06:38 AM

    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.

  • Alex Chaffee

    Alex Chaffee April 20th, 2009 @ 07:25 PM

    "HasOneThroughAssociation inheriting from HasManyThroughAssociation seems logical to me".

    It does?

  • john f

    john f April 20th, 2009 @ 08:27 PM

    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)

  • ronin-41535 (at lighthouseapp)

    ronin-41535 (at lighthouseapp) April 20th, 2009 @ 08:37 PM

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

  • Zach Brock

    Zach Brock April 20th, 2009 @ 11:11 PM

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

  • Arthur Schreiber

    Arthur Schreiber April 21st, 2009 @ 07:10 AM

    +1 for applying this patch.

  • Tim Connor

    Tim Connor April 21st, 2009 @ 08:29 AM

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

  • Pratik

    Pratik April 21st, 2009 @ 01:53 PM

    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...

  • Brian Takita

    Brian Takita April 21st, 2009 @ 05:59 PM

    +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.

  • jpeasy

    jpeasy April 21st, 2009 @ 06:00 PM

    +1 This code is really hard to work with.

  • Brian Takita

    Brian Takita April 21st, 2009 @ 06:01 PM

    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.

  • Pratik

    Pratik April 21st, 2009 @ 06:12 PM

    • Assigned user cleared.
  • parker thompson

    parker thompson April 21st, 2009 @ 10:23 PM

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

  • Adam Milligan

    Adam Milligan May 11th, 2009 @ 12:34 AM

    • Assigned user set to “Jeremy Kemper”
    • Tag changed from activerecord, associations, has_one_through, patch to activerecord, associations, has_one_through, patch, refactoring

    Based on the discussion at RailsConf I'm pressing for this refactoring once more. I've reapplied the changes to the current HEAD of Rails and recreated the patch file. Little change was needed; all tests still pass.

    A search through GitHub (http://github.com/search?type=Co...) turned up no plugins that this change will affect.

    I've also taken the liberty of setting Jeremy Kemper as the owner of the ticket, since he seems to be someone who might actually advocate for it. Jeremy, sorry if you're not the right person for this. Ideally you can pass it off onto whoever is.

  • Pratik

    Pratik May 13th, 2009 @ 02:00 PM

    • Assigned user changed from “Jeremy Kemper” to “Pratik”

    If people with a lot of extra time are looking for some work that might actually add some real value, I'd be happy to help out. But I don't believe this "refactoring" is making the code any easier to follow.

  • Adam Milligan

    Adam Milligan May 13th, 2009 @ 04:13 PM

    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.

  • Michael Schuerig

    Michael Schuerig May 13th, 2009 @ 10:59 PM

    +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.

  • Pratik

    Pratik May 20th, 2009 @ 04:03 PM

    • Assigned user changed from “Pratik” to “Michael Koziarski”
    • State changed from “wontfix” to “open”

    I'm sorry if my last message seems rude. I am not in favor of patches which only satisfy some syntactical fetish of a few. But after a quick look through your last message, I think someone else should evaluate your patch. I guess Koz is probably the guy.

  • Michael Koziarski

    Michael Koziarski May 21st, 2009 @ 02:34 AM

    • Assigned user changed from “Michael Koziarski” to “Jeremy Kemper”
    • Milestone cleared.

    Jeremy actually took a question on this at railsconf, so he's the guy.

    I'm +1 on it so long as we're not needlessly breaking plugins. We have to be careful to weigh the cost of plugin breakage against the benefits of clearer code.

    If it's a clear win here, then let's go for it.

  • Repository

    Repository June 12th, 2009 @ 09:54 PM

    • State changed from “open” to “committed”

    (from [54a5446641e4386285231385700b95a223931bff]) HasOneThroughAssociation still shouldn't derive from HasManyThroughAssociation.

    [#1642 state:committed]

    Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
    http://github.com/rails/rails/commit/54a5446641e4386285231385700b95...

  • Ryan Bigg

    Ryan Bigg October 9th, 2010 @ 10:14 PM

    • Tag cleared.

    Automatic cleanup of spam.

  • Aditya Sanghi

    Aditya Sanghi October 12th, 2010 @ 10:17 AM

    • Tag cleared.

    Automatic cleanup of spam.

  • Ryan Bigg

    Ryan Bigg October 13th, 2010 @ 06:25 AM

    Automatic cleanup of spam.

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2
  • Jeff Kreeftmeijer

Create your profile

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

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Referenced by

Pages