Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Providing support for :inverse_of as an option to associations.
You can now add an :inverse_of option to has_one, has_many and belongs_to associations.  This is best described with an example:

class Man < ActiveRecord::Base
  has_one :face, :inverse_of => :man
end

class Face < ActiveRecord::Base
  belongs_to :man, :inverse_of => :face
end

m = Man.first
f = m.face

Without :inverse_of m and f.man would be different instances of the same object (f.man being pulled from the database again).  With these new :inverse_of options m and f.man are the same in memory instance.

Currently :inverse_of supports has_one and has_many (but not the :through variants) associations.  It also supplies inverse support for belongs_to associations where the inverse is a has_one and it's not a polymorphic.

Signed-off-by: Murray Steele <muz@h-lame.com>
Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
  • Loading branch information
h-lame authored and jeremy committed May 4, 2009
1 parent eb201e6 commit ccea983
Show file tree
Hide file tree
Showing 18 changed files with 418 additions and 12 deletions.
12 changes: 9 additions & 3 deletions activerecord/lib/active_record/associations.rb
@@ -1,4 +1,10 @@
module ActiveRecord
class InverseOfAssociationNotFoundError < ActiveRecordError #:nodoc:
def initialize(reflection)
super("Could not find the inverse association for #{reflection.name} (#{reflection.options[:inverse_of].inspect} in #{reflection.class_name})")
end
end

class HasManyThroughAssociationNotFoundError < ActiveRecordError #:nodoc:
def initialize(owner_class_name, reflection)
super("Could not find the association #{reflection.options[:through].inspect} in model #{owner_class_name}")
Expand Down Expand Up @@ -1488,7 +1494,7 @@ def nullify_has_many_dependencies(record, reflection_name, association_class, pr
:finder_sql, :counter_sql,
:before_add, :after_add, :before_remove, :after_remove,
:extend, :readonly,
:validate
:validate, :inverse_of
]

def create_has_many_reflection(association_id, options, &extension)
Expand All @@ -1502,7 +1508,7 @@ def create_has_many_reflection(association_id, options, &extension)
@@valid_keys_for_has_one_association = [
:class_name, :foreign_key, :remote, :select, :conditions, :order,
:include, :dependent, :counter_cache, :extend, :as, :readonly,
:validate, :primary_key
:validate, :primary_key, :inverse_of
]

def create_has_one_reflection(association_id, options)
Expand All @@ -1521,7 +1527,7 @@ def create_has_one_through_reflection(association_id, options)
@@valid_keys_for_belongs_to_association = [
:class_name, :foreign_key, :foreign_type, :remote, :select, :conditions,
:include, :dependent, :counter_cache, :extend, :polymorphic, :readonly,
:validate, :touch
:validate, :touch, :inverse_of
]

def create_belongs_to_reflection(association_id, options)
Expand Down
Expand Up @@ -399,11 +399,14 @@ def find_target
find(:all)
end

@reflection.options[:uniq] ? uniq(records) : records
records = @reflection.options[:uniq] ? uniq(records) : records
records.each do |record|
set_inverse_instance(record, @owner)
end
records
end

private

def create_record(attrs)
attrs.update(@reflection.options[:conditions]) if @reflection.options[:conditions].is_a?(Hash)
ensure_owner_is_not_new
Expand Down Expand Up @@ -433,6 +436,7 @@ def add_record_to_target_with_callbacks(record)
@target ||= [] unless loaded?
@target << record unless @reflection.options[:uniq] && @target.include?(record)
callback(:after_add, record)
set_inverse_instance(record, @owner)
record
end

Expand Down
14 changes: 14 additions & 0 deletions activerecord/lib/active_record/associations/association_proxy.rb
Expand Up @@ -53,6 +53,7 @@ class AssociationProxy #:nodoc:

def initialize(owner, reflection)
@owner, @reflection = owner, reflection
reflection.check_validity!
Array(reflection.options[:extend]).each { |ext| proxy_extend(ext) }
reset
end
Expand Down Expand Up @@ -274,6 +275,19 @@ def flatten_deeper(array)
def owner_quoted_id
@owner.quoted_id
end

def set_inverse_instance(record, instance)
return if record.nil? || !we_can_set_the_inverse_on_this?(record)
inverse_relationship = @reflection.inverse_of
unless inverse_relationship.nil?
record.send(:"set_#{inverse_relationship.name}_target", instance)
end
end

# Override in subclasses
def we_can_set_the_inverse_on_this?(record)
false
end
end
end
end
Expand Up @@ -31,6 +31,8 @@ def replace(record)
@updated = true
end

set_inverse_instance(record, @owner)

loaded
record
end
Expand All @@ -41,18 +43,26 @@ def updated?

private
def find_target
@reflection.klass.find(
the_target = @reflection.klass.find(
@owner[@reflection.primary_key_name],
:select => @reflection.options[:select],
:conditions => conditions,
:include => @reflection.options[:include],
:readonly => @reflection.options[:readonly]
)
set_inverse_instance(the_target, @owner)
the_target
end

def foreign_key_present
!@owner[@reflection.primary_key_name].nil?
end

# NOTE - for now, we're only supporting inverse setting from belongs_to back onto
# has_one associations.
def we_can_set_the_inverse_on_this?(record)
@reflection.has_inverse? && @reflection.inverse_of.macro == :has_one
end
end
end
end
Expand Up @@ -116,6 +116,11 @@ def construct_scope
:create => create_scoping
}
end

def we_can_set_the_inverse_on_this?(record)
inverse = @reflection.inverse_of
return !inverse.nil?
end
end
end
end
@@ -1,11 +1,6 @@
module ActiveRecord
module Associations
class HasManyThroughAssociation < HasManyAssociation #:nodoc:
def initialize(owner, reflection)
reflection.check_validity!
super
end

alias_method :new, :build

def create!(attrs = nil)
Expand Down Expand Up @@ -251,6 +246,11 @@ def has_cached_counter?
def cached_counter_attribute_name
"#{@reflection.name}_count"
end

# NOTE - not sure that we can actually cope with inverses here
def we_can_set_the_inverse_on_this?(record)
false
end
end
end
end
Expand Up @@ -74,13 +74,15 @@ def owner_quoted_id

private
def find_target
@reflection.klass.find(:first,
the_target = @reflection.klass.find(:first,
:conditions => @finder_sql,
:select => @reflection.options[:select],
:order => @reflection.options[:order],
:include => @reflection.options[:include],
:readonly => @reflection.options[:readonly]
)
set_inverse_instance(the_target, @owner)
the_target
end

def construct_sql
Expand Down Expand Up @@ -117,8 +119,15 @@ def new_record(replace_existing)
self.target = record
end

set_inverse_instance(record, @owner)

record
end

def we_can_set_the_inverse_on_this?(record)
inverse = @reflection.inverse_of
return !inverse.nil?
end
end
end
end
21 changes: 21 additions & 0 deletions activerecord/lib/active_record/reflection.rb
Expand Up @@ -212,6 +212,13 @@ def reset_column_information
end

def check_validity!
check_validity_of_inverse!
end

def check_validity_of_inverse!
if has_inverse? && inverse_of.nil?
raise InverseOfAssociationNotFoundError.new(self)
end
end

def through_reflection
Expand All @@ -225,6 +232,18 @@ def source_reflection
nil
end

def has_inverse?
!@options[:inverse_of].nil?
end

def inverse_of
if has_inverse?
@inverse_of ||= klass.reflect_on_association(options[:inverse_of])
else
nil
end
end

private
def derive_class_name
class_name = name.to_s.camelize
Expand Down Expand Up @@ -300,6 +319,8 @@ def check_validity!
unless [:belongs_to, :has_many].include?(source_reflection.macro) && source_reflection.options[:through].nil?
raise HasManyThroughSourceAssociationMacroError.new(self)
end

check_validity_of_inverse!
end

def through_reflection_primary_key
Expand Down

15 comments on commit ccea983

@darrylring
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Nice work.

@NZKoz
Copy link
Member

@NZKoz NZKoz commented on ccea983 May 5, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, really nice work murray ;)

@imajes
Copy link
Contributor

@imajes imajes commented on ccea983 May 5, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice :)

@jrmehle
Copy link
Contributor

@jrmehle jrmehle commented on ccea983 May 5, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be the default behavior for has_one, has_many, and belongs_to? What downsides are there to doing this?

@maxim
Copy link
Contributor

@maxim maxim commented on ccea983 May 5, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yesss. It's in! : )

@adrianpacala
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like jrmehle said: why not use this as default?

@arthurschreiber
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is truely a very nice change. Thanks!

@h-lame
Copy link
Contributor Author

@h-lame h-lame commented on ccea983 May 5, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this the default would require AR to try and work out what the inverse of a given association is when it's not explicitly specified. It's not too hard for the simple-case, it's what the plugin I wrote that formed the basis of this patch did, but there are many edge-cases. Also the whole concept of bi-directionality needs testing out before we go the whole way.

That said hopefully once enough people have tested this it will become the default. Think of it as inverse deprecation ;)

@bjeanes
Copy link
Contributor

@bjeanes bjeanes commented on ccea983 May 5, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something that should have been the default a long time ago. Better yet, having an active record identity map so there was only ever one object per row in existence at a time (well, in the same thread). We need to push for this to be the default behaviour. Testing all the edge cases aside, that would only be a positive change imo

@NZKoz
Copy link
Member

@NZKoz NZKoz commented on ccea983 May 5, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we can make this the 'default' some time before we ship 3.0. But with this option in early, it lets us test the "having an inverse" code independently of the "figuring out what the inverse is" code.

As for an identity map, the issue has come up DOZENS of times on the mailing lists. We'd be happy to investigate it but there are several cases which we support now which aren't easy to do with an identity map without adding some notion of a persistence session with attaching and detaching and the associated errors. This could well be a positive change, but it's not just:

@objects[id] ||= find(id)

@dkubb
Copy link
Contributor

@dkubb dkubb commented on ccea983 May 6, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NZKoz: the way we deal with the persistence session in DataMapper is to wrap the code in a repository block: repository() { .. }

IMHO this is not ideal, but it works well enough and doesn't cause us to leak memory in long running processes.

What I'd really like to see is a decent WeakHash implementation for MRI and JRuby. With it (I think) it might be possible to make an Identity Map that doesn't require an explicit scope be defined -- like inside a block -- and would work well for long running processes.

@fcheung
Copy link
Contributor

@fcheung fcheung commented on ccea983 May 6, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! As thing stands I don't think this will work with :include (ie the inverses won't get set). I'll have a think about that, but (at least for non massive join version of include) I don't think it will be too much hassle.

@NZKoz
Copy link
Member

@NZKoz NZKoz commented on ccea983 May 7, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkubb: Do you handle unmarshalled objects? e.g. cache-hits from yaml or marshal.load?

If you have person:1 in memcache as part of some serialized association for accounts, and follow a belongs_to association that refers to person:1, you can end up with duplicated instances. This is why hibernate has concepts like attach() and why it throws a bunch of exceptions. There are several other problems but that kind of thing is a little tricky to do right without messing up the usability of the AR API. Especially when combined with useful things like :select and find_by_sql.

So it's not going to be easy, or perhaps even possible to support 100% identity, without removing some features or adding horrible random exceptions which people can and will hit in 'real life'. I think we can move in the right direction gradually and get to 90% without 'making it hard to use'. That last 10% could be quite a slog though.

@NZKoz
Copy link
Member

@NZKoz NZKoz commented on ccea983 May 7, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fcheung: good point, let me know if you do take a look at it

@fcheung
Copy link
Contributor

@fcheung fcheung commented on ccea983 May 7, 2009

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've stuck a patch up at https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/2621 I've had a chat with h-lame who has also had a stab at it and we independently came up with pretty much the same thing

Please sign in to comment.