Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• 

  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

• You flagged this item as spam.
  

  David Stevenson December 29th, 2008 @ 05:01 AM

  +1

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

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

  "HasOneThroughAssociation inheriting from HasManyThroughAssociation seems logical to me".

  It does?

• You flagged this item as spam.
  

  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)

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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

• You flagged this item as spam.
  

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

  +1 for applying this patch.

• You flagged this item as spam.
  

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

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

• 

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

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  jpeasy April 21st, 2009 @ 06:00 PM

  +1 This code is really hard to work with.

• You flagged this item as spam.
  

  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 April 21st, 2009 @ 06:12 PM

  • Assigned user cleared.

• You flagged this item as spam.
  

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

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

• You flagged this item as spam.
  

  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.

  • has_one_through.patch 17.6 KB
• 

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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 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 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 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 October 9th, 2010 @ 10:14 PM

  • Tag cleared.

  Automatic cleanup of spam.

• 

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

  • Tag cleared.

  Automatic cleanup of spam.

• 

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

  Automatic cleanup of spam.

• 

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

  • Milestone set to “3.0.2”

  [bulk edit]

• 

  Jeff Kreeftmeijer November 8th, 2010 @ 08:48 AM

  Automatic cleanup of spam.