Permalink
Browse files

Set inverse associations when using scopes

Currently using scopes through a has_many association will not set
the inverse association, e.g:

  product = Product.first
  product.variants.enabled.first.product.equal? product
  => false

This commit fixes it by copying the association into the scope and
then attempting to set the inverse association when the records are
loaded.
  • Loading branch information...
1 parent ea84b0c commit 9f607fb4ddb3789b0859e05bf7810354a867bfc2 @pixeltrix pixeltrix committed Aug 26, 2012
@@ -15,6 +15,7 @@ def initialize(association)
def scope
scope = klass.unscoped
+ scope.association = @association
scope.merge! eval_scope(klass, reflection.scope) if reflection.scope
add_constraints(scope)
end
@@ -35,8 +35,8 @@ module Associations
# instantiation of the actual post records.
class CollectionProxy < Relation
def initialize(association) #:nodoc:
- @association = association
super association.klass, association.klass.arel_table
+ @association = association
merge! association.scope
end
@@ -896,7 +896,7 @@ def ==(other)
end
# Returns a new array of objects from the collection. If the collection
- # hasn't been loaded, it fetches the records from the database.
+ # hasn't been loaded, it fetches the records from the database.
#
# class Person < ActiveRecord::Base
# has_many :pets
@@ -17,7 +17,7 @@ class Relation
include FinderMethods, Calculations, SpawnMethods, QueryMethods, Batches, Explain, Delegation
attr_reader :table, :klass, :loaded
- attr_accessor :default_scoped
+ attr_accessor :default_scoped, :association
alias :model :klass
alias :loaded? :loaded
alias :default_scoped? :default_scoped
@@ -29,6 +29,7 @@ def initialize(klass, table, values = {})
@implicit_readonly = nil
@loaded = false
@default_scoped = false
+ @association = nil
end
def insert(values)
@@ -572,6 +573,10 @@ def exec_queries
@records = default_scoped.to_a
end
+ if association
+ @records.each { |record| association.set_inverse_instance(record) }
+ end
+
@loaded = true
@records
end
@@ -35,6 +35,10 @@ def initialize(relation, other)
other = other.with_default_scope
end
+ if other.association
+ relation.association = other.association
+ end
+
@relation = relation
@values = other.values
end
@@ -261,8 +261,24 @@ def test_parent_instance_should_be_shared_with_replaced_via_accessor_children
def test_parent_instance_should_be_shared_with_first_and_last_child
man = Man.first
- assert man.interests.first.man.equal? man
- assert man.interests.last.man.equal? man
+ assert man.interests.first.man.equal?(man), "Man should be the same instance when using association.first"
+ assert man.interests.last.man.equal?(man), "Man should be the same instance when using association.last"
+ end
+
+ def test_parent_instance_should_be_shared_with_all_children
+ man = Man.first
+ assert man.interests.to_a.all?{ |interest| interest.man.equal?(man) }, "Man should be the same instance when using association.to_a"
+ end
+
+ def test_parent_instance_should_be_shared_with_first_and_last_child_when_using_scope
+ man = Man.first
+ assert man.interests.by_topic.first.man.equal?(man), "Man should be the same instance when using association.scope.first"
+ assert man.interests.by_topic.last.man.equal?(man), "Man should be the same instance when using association.scope.last"
+ end
+
+ def test_parent_instance_should_be_shared_with_all_children_when_using_scope
+ man = Man.first
+ assert man.interests.by_topic.to_a.all?{ |interest| interest.man.equal?(man) }, "Man should be the same instance when using association.scope.to_a"
end
def test_trying_to_use_inverses_that_dont_exist_should_raise_an_error
@@ -2,4 +2,8 @@ class Interest < ActiveRecord::Base
belongs_to :man, :inverse_of => :interests
belongs_to :polymorphic_man, :polymorphic => true, :inverse_of => :polymorphic_interests
belongs_to :zine, :inverse_of => :interests
+
+ def self.by_topic
+ order(arel_table[:topic])
+ end
end

3 comments on commit 9f607fb

Member

@pixeltrix hey, just took a look at this. t.b.h. I am not convinced that this is something that needs to be / should be solved. I am cautious about it for two reasons:

  1. The code involves giving relation more knowledge about associations, which is messy and a warning sign to me
  2. I think there could be scenarios where this behaviour is surprising and/or wrong. At the least if we are going to implement it then we should clear the association when Relation#only or #except is called.

The way I see it, once you call scope methods on an association (e.g. product.variants.enabled) that object ceases to be an association and is now a scope, and therefore should behave as all other scopes...

We can discuss further in Campfire though if you like

Owner

I definitely think it needs to be addressed - the only alternatives are:

  1. Declaring multiple associations:

    class Product < ActiveRecord::Base
      has_many :variants, :inverse_of => :product
      has_many :enabled_variants, :inverse_of => :product, :conditions => { :enabled => true }
    end
  2. Filtering the association in the parent

    class Product < ActiveRecord::Base
      has_many :variants, :inverse_of => :product
    
      def enabled_variants
        variants.select(&:enabled?)
      end
    end
  3. Extending the association

    class Product < ActiveRecord::Base
      has_many :variants, :inverse_of => :product do
        def enabled
          scoped.where(:enabled => true).each{ |variant| proxy_association.set_inverse_instance(variant) }
        end
      end
    end

All of these are either messy and/or inefficient.

We can discuss further in Campfire though if you like

Yes please, DM me on Twitter/email me the next time you're going to be in the Campfire room.

Contributor

I tied to work around this by adding with_association_targets method to AR Relation to allow forcing a custom target instance for any association: https://gist.github.com/wojt-eu/5076568

Not that I think it's a good solution (-:

Please sign in to comment.