Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• 

  Pratik December 23rd, 2008 @ 12:32 PM

  • Title changed from “[patch] support for :inverse option in associations” to “Support for :inverse option in associations”
• 

  José Valim January 5th, 2009 @ 03:00 PM

  • Tag changed from “activerecord, associations, belongs_to, has_many, has_one, inverse” to “activerecord, associations, belongs_to, has_many, has_one, inverse, patch”

  +1

• You flagged this item as spam.
  

  Chris Hapgood January 6th, 2009 @ 02:00 PM

  The concept motivating this patch is huge -it goes right to the heart of object persistence and identity. I'm glad that Murray has put the issue on the table, but my instinct tells me that this issue needs to be addressed comprehensively or not at all. So, either AR supports...

  
  User.first.object_id == User.first.object_id
  

  or we accept that AR is not a transparent object persistence layer but rather an opaque Object-Relational Mapping layer.

  ---

  ActiveRecord is what it is. I look forward to the merbification of Rails in this respect and I hope that other persistence layers will eventually be as practical with Rails as ActiveRecord now is.

  -1

• 

  Michael Koziarski January 6th, 2009 @ 08:39 PM

  Chris,

  While I appreciate you taking the time to post your comments here, I can't accept your absolutist approach.
  

  There are dozens of real world scenarios that would benefit HUGELY from bidirectional associations, and several cases which make a full identity map much harder than you seem to think. In short:

  
    u = User.find(1)
    # another process changes users where id = 1
    u2 = User.find(1), :select=>"users.*, MAX(sessions.updated_at)", :joins=>:sessions)
  

  Should we still return the same instance in this case? Or do we raise an exception?

  There's a reason that hibernate has attach/detach API calls each of which throw several state-related exceptions.

  There's a lot of merit to fixing this problem which is real and painful today, rather than holding off till we have a perfect solution to every possible corner case.

• You flagged this item as spam.
  

  Chris Hapgood January 6th, 2009 @ 09:20 PM

  Koz, I in no way meant to imply that a full identity map was easy -quite the contrary. I see it as a very hard problem.

  I think we disagree on the huge benefit of a partial solution. But since I'm not the one doing the work, I'm pretty poorly placed to object.

  Consider my -1 as an expression of "I wouldn't want to have to do it."

• 

  Michael Koziarski January 7th, 2009 @ 01:50 AM

  The inverse on associations will mean that walking associations will always return the same instance. So things like

  user.posts.first.user.posts.map(&:user).first.posts.first.user will only fetch user and posts once. Seems like a pretty big win :)

• You flagged this item as spam.
  

  Chris Kampmeier January 7th, 2009 @ 09:04 AM

  Why is :inverse specified explicitly? In your example, it's always the same as the class declaring the association.

• You flagged this item as spam.
  

  Murray Steele January 7th, 2009 @ 09:41 AM

  Chris H. I actually think that a full identity map is a separate goal to supporting inverse associations. Inverse associations might open the door to get people thinking about identity maps, but I don't think we should wait for a full identity map solution before going ahead with inverses.
  

  It might be worth describing the use-case I wrote the plugin for:

  
  
  class ProjectPlan < ActiveRecord::Base
    has_many :iterations, :validate => true
  
    validate :duration_isnt_too_short
  
    def duration_isnt_too_short
      (self.end_date - self.start_date) > 3.days
    end
  
    def duration_range
      (self.start_date..self.end_date)
    end
  end
  
  class Iteration < ActiveRecord::Base
    belongs_to :project_plan
  
    validate :my_duration_is_within_project
  
    def my_duration_is_within_project
      self.project_plan.duration_range.include?(self.start_date) && 
      self.project_plan.duration_range.include?(self.end_date)
    end
  end
  
  

  Imagine we have a project plan with some iterations and we change the duration of the plan so that one or more of it's iterations are no longer valid (their durations are outside the plan). Without some kind of inverse association shared-instance stuff going on the validation for the iterations will always pass as they'll be using the DB version of the project plan that has no changes, and that's no fun.

  ---

  Chris K. you're of course right. The original plugin I wrote tried to guess the inverse, but after a chat with Koz I decided to make the first cut at getting this into rails as non-magical as possible. Maybe there's a better way though, "convention over configuration" and all that, we could just assume that in the lack of an :inverse option that the default is the same as the class declaring the association.
  

  However, because it doesn't work on every association type (yet!) maybe there is merit in making it explicit (sorry fingers!) so as not to confuse?

• You flagged this item as spam.
  

  Gaspard Bucher February 9th, 2009 @ 01:47 PM

  1+

  A partial solution is definitely welcome !

  I like the :inverse option since it makes this feature explicit and avoids breaking parts that do not need it.

  My use case (set defaults in interdependent models):

  
  
  class Node < ActiveRecord::Base
    has_many :versions
    has_one  :redaction, :class_name => 'Version', :conditions => ...
    accepts_nested_attributes_for :redaction
    before_create :set_default_name
    
    def redaction_attributes=(attrs)
      if @redaction = redaction
        @redaction.build(attrs)
      else
        @redaction = versions.build(attrs)
      end
    end
    
    private
      def set_default_name
        self.name = @redaction.title.to_s.url_name if @redaction && self.name.blank?
      end
  end
  
  class Version < ActiveRecord::Base
    belongs_to :node
    before_create :set_default_title
    
    private
      def set_default_title
        # without something like parental control, this does not work.
        self.title = node.name if self.title.blank?
      end
  end
  

• 

  Michael Koziarski April 27th, 2009 @ 09:46 AM

  After an extended holiday (then a big hole to work my way out of after it), I'm finally back on deck enough to have a look at this.

  Fundamentally this all looks pretty damned good and I'm sorry I missed the boat for 2.3

  I'm wondering if the option should instead be :inverse_of instead of :inverse. So that the inverse method can return the actual association rather than just the name of it?

  Also is there a reason you're using alias method chain in inverse_association.rb? From my point of view this stuff is fundamental and should just be 'wired in' to the source rather than added metaprogrammatically after the fact.

  On the whole the tests look really good, but I'm wondering if we should be a little defensive when passed an explicit :inverse?

  Finally, I think setting one side as an inverse should do the same to the other side, unless you can think of a reason not to?

  Great work, and sorry again that I dropped off the face of the earth :)

• You flagged this item as spam.
  

  Chris Hapgood April 27th, 2009 @ 01:46 PM

  Keep in mind that "inverse" could have two meanings in this context. The first being the other end of the association as in Murray's plugin. The second being the set of models NOT being in the association (true set inverse). I've actually written a plugin for the latter, which allows you to do things like user.magazines.subscribed.inverse to get a list (association proxy) of magazines to which the user is NOT subscribed.

  Other terms that might convey the same idea without implying set inversion: complement, opposite, reflection (nice, but carries some baggage), mirror, etc.

  -The absolutist

• 

  Michael Koziarski April 27th, 2009 @ 11:11 PM

  Inverse is a fairly standard term when dealing with ORM associations. Hibernate in particular uses it all over (it's where I got the term from). I don't think most of our users are thinking set theory when working with AR ;)

• You flagged this item as spam.
  

  Murray Steele April 30th, 2009 @ 12:11 PM

  Michael Koziarski. I'll put together a new patch with your suggestions. However, just to clear some of them up:

  1. I'll use :inverse_of instead of :inverse as the option (I like this, I wasn't keen on :inverse).
  2. I like having the .inverse method of the reflection actually return the reflection object of the association rather than just the name, so I'll do that too.
  3. I'll get rid of inverse_associations.rb and put the code directly into the other relevant associations files. I kind of liked having all the inverse stuff in one file (although, it's really not as I've had to patch some associations directly), but I think this was just an artefact of my pulling the patch directly out of a plugin (and being lazy).
  4. When you say "be a little defensive" I presume you mean it should complain if there is no such association on the associated model? Sound's sensible to me.
  5. I'm not sure about setting the inverse of the other side automatically. Given:

  
  class ProjectPlan < ActiveRecord::Base
    has_one :gantt_chart
  end
  
  class GanttChart < ActiveRecord::Base
    belongs_to :project_plan, :inverse_of :gantt_chart
  end
  

  Right now, the gantt_chart association on ProjectPlan doesn't know that it has an inverse, but the project_plan association on GanttChart does. You're suggesting that even though we didn't explicitly tell ProjectPlan's gantt_chart association that it was an inverse of GantChart's project_plan it would know anyway because we did tell GantChart's project_plan that it was the inverse of ProjectPlan's gantt_chart.
  

  I wonder if that's too magical? What we'd be saying is we require an explicit invocation of :inverse_of on our association definitions before we'll do anything, but if you've done that we might then add inverse information to some other associations that you've not explicitly specified the :inverse_of options for. That seems inconsistent to me.

  Anyway, I'll knock up a patch with 1-4 in it and see what you (or anyone else) thinks about 5.

• 

  Michael Koziarski April 30th, 2009 @ 11:23 PM

  Sounds great Murray.

  I'm all for punting on magic for a while as we let the explicit declarations get played with. I do want to make sure that 3.0 ships with some niceness (auto-finding inverses for simple cases, etc). But we can wait till we know that what we do with those inverses is safe.

  And yeah, That's what I meant by 4.

• You flagged this item as spam.
  

  Murray Steele May 1st, 2009 @ 04:22 PM

  I've reworked the patch. The one thing that I'm not sure about is that now it throws an exception if you supply an :inverse_of that doesn't exist, but it just silently ignores it if it's a :belongs_to that inverses onto a :has_many (the inverse type I don't have a good idea for supporting). I wonder if in this case it should also complain?

  This of course opens up a whole other set of questions about how and when AR should complain about what the programmer is doing. Should it complain when AR can work out that the :inverse_of is wrong (wrong macro, wrong class, &c). Of course, if it did that for inverses, where else should/could AR be more hand-holdy?

  Anyway, this new patch should answer everything Koz suggested above.

  Moar eyez plz!

  • support_for_inverse_relationships_on_active_record_objects_v2.diff 28.1 KB
• 

  Michael Koziarski May 4th, 2009 @ 05:44 PM

  So the only thing which 'surprised' me playing around with this patch was this snippet:

  
  >> t = Thing.first
  => #<Thing id: 1, user_id: 1, name: "First thing", created_at: "2009-05-04 16:35:37", updated_at: "2009-05-04 16:35:37">
  >> t.object_id
  => 18883980
  >> t.user.things.map(&:object_id)
  => [18753270]
  

• 

  Michael Koziarski May 4th, 2009 @ 05:46 PM

  GAH, preview would be nice ;)

  
  >> t = Thing.first
  => #<Thing id: 1, user_id: 1, name: "First thing", created_at: "2009-05-04 16:35:37", updated_at: "2009-05-04 16:35:37">
  >> t.object_id
  => 18883980
  >> t.user.things.map(&:object_id)
  => [18753270]
  

  I'm not sure whether it'd be feasible to make this work with just associations code, probably not.

  But everything else looked awesome and worked as I expected.

• You flagged this item as spam.
  

  Murray Steele May 4th, 2009 @ 08:43 PM

  I think this might be because #object_id is one of the few methods that AssociationProxy doesn't delegate:

  http://github.com/rails/rails/bl...

• 

  Michael Koziarski May 4th, 2009 @ 10:11 PM

  Right, ignore the use of proxy_target, that's fine. It's the has_many side of things that's not working right. But I don't think that's a fixable issue.

• You flagged this item as spam.
  

  Murray Steele May 4th, 2009 @ 10:53 PM

  Yeah, with a bit more thought I get the point your code snippet makes.

  Like you though, I can't really come up with a way of making a belongs_to with an inverse of a has_many make any sense. At least, not without doing something incredibly complex that borders on a full identity map solution and I'm not sure about the full implications of that.

  Apart from that, glad you liked it!

• 

  Michael Koziarski May 4th, 2009 @ 11:24 PM

  Yeah, I think we can punt on that until / unless someone does a full identity map solution.

• 

  Jeremy Kemper May 4th, 2009 @ 11:36 PM

  Great job. http://github.com/rails/rails/co...

• 

  Jeremy Kemper May 4th, 2009 @ 11:36 PM

  • State changed from “new” to “committed”

• You flagged this item as spam.
  

  sarah (at ultrasaurus) July 12th, 2009 @ 04:03 PM

  Could this be applied to 2.3.x ?

  The following fix depends on it, and it would be great if these could appear as 2.3 fixes at some point:
  https://rails.lighthouseapp.com/projects/8994/tickets/2815-nested-m...

• 

  Michael Koziarski July 13th, 2009 @ 12:54 AM

  No, this is a feature addition (and a fairly large one) so we can't
  really push it to 2-3-stable.