Close
Glad You're Ready. Let's Get Started!

Let us know how we can contact you.

Thank you!

We'll respond shortly.

LABS
has_one-ish :through

Rails has had the has_one :through association for a while now, and you probably use it on occasion. But, has it ever given you the heebie-jeebies a little bit? Maybe something happens now and then that doesn’t seem quite right? Well, the reason for this is that HasOneThroughAssociation, the class that ActiveRecord uses (shockingly) to implement has_one :through associations, is a subclass of HasManyThroughAssociation.

Whhaaaaaaaaaat?

Yes, oddly enough, a has_one :through association is a collection that is special-cased to always return just one element. Seems kind of dirty, doesn’t it?

I have two, related, problems with this inheritance relationship. First, it violates Liskov substitutability. One cannot in any way argue that a has_one :through association IS A has_many :through association; substituting one for the other would not be likely to maintain a program’s correctness. Second, the inheritance relationship exists to share implementation, not interface. Among others, the Gang of Four have described, persuasively, why this is a terrible idea.

More practically, I’ve now run into two serious bugs caused by this relationship. The first one caused newly created has_one :through associations to return an empty array rather than nil. I fixed that here, with the help of wunderkind David Stevenson. The second I ran into in the last few days while working on this patch, which I’ve written about here. It turns out that trying to get #method_missing to behave sensibly for all non-collection associations is difficult when one of those associations inherits a pile of collection-specific functionality.

So, in order to make my #method_missing patch work I had to rewrite HasOneThroughAssociation with a more appropriate superclass (I used HasOneAssociation). You can find that patch here. Hopefully this change will make its way into the Rails codebase and will make lives easier for generations to come.

Comments
  1. Will Green says:

    > One cannot in any way argue that a has_one :through
    > association IS A has_many :through association;
    > substituting one for the other would not be likely to
    > maintain a program’s correctness.

    I beg to differ. You make the argument yourself:

    > a `has_one :through` association is a collection
    > that is special-cased to always return just one
    > element

    Polymorphism, baby!

    It’s like squares and rectangles. A square is a rectangle, but a rectangle cannot be a square. A square is a rectangle with special restrictions; both are quadrilaterals where all four of its angles are right angles, but a square extends this to say that all four sides have equal length.

    A `has_one :through` is a `has_many :through` relationship, but it can only return one element. A `has_one :though` is a `has_many :through`, but a `has_many :though` cannot be a `has_one :through`

  2. Adam Milligan says:

    You’ve fallen into a classic inheritance fallacy. A square is a rectangle in mathematics, but not in object oriented design. The purpose of inheritance is to allow objects to share interfaces, or behavior if you prefer. Consider this example:

    def resize(rectangle)
    rectangle.height = 10
    rectangle.width = 3
    puts rectangle.area
    end

    What happens if you pass a square into that method? I’m not entirely certain, but you won’t end up with a shape that has an area of 30. The square is not substitutable for the rectangle, so it is not an appropriate subclass of rectangle.

    Analogously, consider any method that treats an association as a collection by calling #size or #each. Passing a has_one :through association to that method would make no sense. A HOT is not substitutable for a HMT, and therefore inheritance is not the appropriate approach.

  3. Will Green says:

    No, that is poor class design. The square should throw an exception when you attempt to resize it into something that is not a square. That’s part of the restrictions of the square, so the square class should ensure it. Otherwise, it is not a square.

    Same thing for the `has_one :through` method. It has special checks in place so that you cannot assign a collection to it. If you could, then it would not be a `has_one :through`

    That is good OO design.

  4. Adam Milligan says:

    Why throw an exception when a user tries to resize a square to violate its constraints, rather than simply *not define* the behavior that allows the user to violate the constraints? By that logic you should define Square#radius to throw an exception as well.

    Please explain how that is good OO design.

    Even if Square#width= were to throw an exception, passing an instance of square to the code snippet above wouldn’t lead to a correct program. Thus, it violates Liskov Substitution, and an inheritance relationship is not appropriate.

    I’m willing to be proven wrong on this point, but it seems to me a fairly clear application of well understood OO rules. I would love to hear a well-supported argument that shows otherwise.

  5. Will Green says:

    Define correct program.

    If you mean allowing an invalid operation on an object to go through, unchecked, without some indication that it is invalid, then sure, you have a correct program.

    However, I argue that the code snippet violates the rules of what defines a Square; if your Square class doesn’t have check constraints on the dimensions to ensure that they are equal, then your class has not correctly modeled a Square, and, therefor, your program is incorrect.

    It makes sense to call `Square#resize` on a square. It does not make sense to call `Square#radius`. So, yes, trying to call `Square#radius` should throw an exception (specifically in the case of Ruby, a NoMethodError) , as should any other Shape where calling `radius` on it would make no sense. If you allow such calls to go through without raising some sort of error, then you have not correctly modeled the Shape in question. Again, you have an incorrect program.

  6. Adam Milligan says:

    Clearly we’ll have to agree to disagree.

  7. Will Green says:

    Clearly ;)

    Interesting debate, though.

Post a Comment

Your Information (Name required. Email address will not be displayed with comment.)

* Copy This Password *

* Type Or Paste Password Here *