Permalink
Browse files

Construct an actual ActiveRecord::Relation object for the association…

… scope, rather than a hash which is passed to apply_finder_options. This allows more flexibility in how the scope is created, for example because scope.where(a, b) and scope.where(a).where(b) mean different things.
  • Loading branch information...
jonleighton authored and tenderlove committed Jan 6, 2011
1 parent 4411184 commit 770e6893b9f2aaaebe3de10576931dc7194451bc
@@ -333,23 +333,16 @@ def proxy_respond_to?(method, include_private = false)
protected
- def finder_options
- {
- :conditions => construct_conditions,
- :select => construct_select,
- :readonly => @reflection.options[:readonly],
- :order => @reflection.options[:order],
- :limit => @reflection.options[:limit],
- :include => @reflection.options[:include],
- :joins => @reflection.options[:joins],
- :group => @reflection.options[:group],
- :having => @reflection.options[:having],
- :offset => @reflection.options[:offset]
- }
- end
-
- def construct_select
- @reflection.options[:select] ||
+ def association_scope
+ options = @reflection.options.slice(:order, :limit, :joins, :group, :having, :offset)
+ super.apply_finder_options(options)
+ end
+
+ def select_value
+ super || uniq_select_value
+ end
+
+ def uniq_select_value
@reflection.options[:uniq] && "DISTINCT #{@reflection.quoted_table_name}.*"
end
@@ -165,9 +165,7 @@ def send(method, *args)
end
def scoped
- target_scope.
- apply_finder_options(@finder_options).
- create_with(@creation_attributes)
+ target_scope & @association_scope
end
protected
@@ -180,27 +178,32 @@ def sanitize_sql(sql, table_name = @reflection.klass.table_name)
@reflection.klass.send(:sanitize_sql, sql, table_name)
end
- # Construct the data used for the scope for this association
+ # Construct the scope for this association.
#
- # Note that we don't actually build the scope here, we just construct the options and
- # attributes. We must only build the scope when it's actually needed, because at that
- # point the call may be surrounded by scope.scoping { ... } or with_scope { ... } etc,
- # which affects the scope which actually gets built.
+ # Note that the association_scope is merged into the targed_scope only when the
+ # scoped method is called. This is because at that point the call may be surrounded
+ # by scope.scoping { ... } or with_scope { ... } etc, which affects the scope which
+ # actually gets built.
def construct_scope
- if target_klass
- @finder_options = finder_options
- @creation_attributes = creation_attributes
- end
+ @association_scope = association_scope if target_klass
+ end
+
+ def association_scope
+ scope = target_klass.unscoped

This comment has been minimized.

Show comment Hide comment
@bborn

bborn Feb 10, 2011

This line is giving me errors when trying to assign an object through a polymorphic belongs_to association:

class Comment
  belongs_to :commentable, :polymorphic => true
end

class User
  has_many :comments, :as => :commentable
end

comment = Comment.new
user = User.new
comment.commentable = user
comment.save!

-> NoMethodError: undefined method `unscoped' for Object:Class on line 185 of association_proxy.rb

It seems that target_klass is returning an Object instead of a User, but I don't understand the associations code well enough to figure out why. Any ideas?

@bborn

bborn Feb 10, 2011

This line is giving me errors when trying to assign an object through a polymorphic belongs_to association:

class Comment
  belongs_to :commentable, :polymorphic => true
end

class User
  has_many :comments, :as => :commentable
end

comment = Comment.new
user = User.new
comment.commentable = user
comment.save!

-> NoMethodError: undefined method `unscoped' for Object:Class on line 185 of association_proxy.rb

It seems that target_klass is returning an Object instead of a User, but I don't understand the associations code well enough to figure out why. Any ideas?

This comment has been minimized.

Show comment Hide comment
@jonleighton

jonleighton Feb 11, 2011

Member

Hi, thanks for the bug report. I'll look into it.

@jonleighton

jonleighton Feb 11, 2011

Member

Hi, thanks for the bug report. I'll look into it.

This comment has been minimized.

Show comment Hide comment
@jonleighton

jonleighton Feb 14, 2011

Member

Hi,

I am unable to reproduce this bug, with the following test (the models are different because these models already exist in the AR test suite):

def test_assign_polymorphic
  tagging = Tagging.new
  post    = Post.new :title => "foo", :body => "foo"

  tagging.taggable = post
  tagging.save!

  assert_equal post, tagging.reload.taggable
end

If you still think the bug exists, could you please create a failing test case against the AR test suite and open a ticket on Lighthouse with the test attached? Or provide more details at least, because as far as I can see this works. You can assign the ticket to me.

Thanks,
Jon

@jonleighton

jonleighton Feb 14, 2011

Member

Hi,

I am unable to reproduce this bug, with the following test (the models are different because these models already exist in the AR test suite):

def test_assign_polymorphic
  tagging = Tagging.new
  post    = Post.new :title => "foo", :body => "foo"

  tagging.taggable = post
  tagging.save!

  assert_equal post, tagging.reload.taggable
end

If you still think the bug exists, could you please create a failing test case against the AR test suite and open a ticket on Lighthouse with the test attached? Or provide more details at least, because as far as I can see this works. You can assign the ticket to me.

Thanks,
Jon

This comment has been minimized.

Show comment Hide comment
@bborn

bborn Feb 15, 2011

Hi, I think I may have tracked this down do a default value being set for the commentable_id (or, in your case, it would be the taggable_id). For some reason the migration specified a default of 0 for the commentable_id; removing that seems to have solved the problem.

@bborn

bborn Feb 15, 2011

Hi, I think I may have tracked this down do a default value being set for the commentable_id (or, in your case, it would be the taggable_id). For some reason the migration specified a default of 0 for the commentable_id; removing that seems to have solved the problem.

+ scope = scope.create_with(creation_attributes)
+ scope = scope.apply_finder_options(@reflection.options.slice(:conditions, :readonly, :include))
+ scope = scope.where(construct_owner_conditions)
+ scope = scope.select(select_value) if select_value = self.select_value
+ scope
end
- # Implemented by subclasses
- def finder_options
- raise NotImplementedError
+ def select_value
+ @reflection.options[:select]
end
# Implemented by (some) subclasses
def creation_attributes
- {}
+ { }
end
def aliased_table
@@ -226,6 +229,29 @@ def target_scope
target_klass.scoped
end
+ # Returns a hash linking the owner to the association represented by the reflection
+ def construct_owner_attributes(reflection = @reflection)
+ attributes = {}
+ if reflection.macro == :belongs_to
+ attributes[reflection.association_primary_key] = @owner[reflection.foreign_key]
+ else
+ attributes[reflection.foreign_key] = @owner[reflection.active_record_primary_key]
+
+ if reflection.options[:as]
+ attributes["#{reflection.options[:as]}_type"] = @owner.class.base_class.name
+ end
+ end
+ attributes
+ end
+
+ # Builds an array of arel nodes from the owner attributes hash
+ def construct_owner_conditions(table = aliased_table, reflection = @reflection)
+ conditions = construct_owner_attributes(reflection).map do |attr, value|
+ table[attr].eq(value)
+ end
+ table.create_and(conditions)
+ end
+
private
# Forwards any missing method call to the \target.
def method_missing(method, *args)
@@ -58,23 +58,6 @@ def find_target
scoped.first.tap { |record| set_inverse_instance(record) }
end
- def finder_options
- {
- :conditions => construct_conditions,
- :select => @reflection.options[:select],
- :include => @reflection.options[:include],
- :readonly => @reflection.options[:readonly]
- }
- end
-
- def construct_conditions
- conditions = aliased_table[@reflection.association_primary_key].
- eq(@owner[@reflection.foreign_key])
-
- conditions = conditions.and(Arel.sql(sql_conditions)) if sql_conditions
- conditions
- end
-
def foreign_key_present?
!@owner[@reflection.foreign_key].nil?
end
@@ -88,14 +88,14 @@ def construct_owner_conditions
super(join_table)
end
- def finder_options
- super.merge(
- :joins => construct_joins,
- :readonly => ambiguous_select?(@reflection.options[:select]),
- :select => @reflection.options[:select] || [
- @reflection.klass.arel_table[Arel.star],
- join_table[Arel.star]]
- )
+ def association_scope
+ scope = super.joins(construct_joins)
+ scope = scope.readonly if ambiguous_select?(@reflection.options[:select])
+ scope
+ end
+
+ def select_value
+ super || [@reflection.klass.arel_table[Arel.star], join_table[Arel.star]]
end
# Join tables with additional columns on top of the two foreign keys must be considered
@@ -9,34 +9,6 @@ def set_owner_attributes(record)
construct_owner_attributes.each { |key, value| record[key] = value }
end
end
-
- # Returns a hash linking the owner to the association represented by the reflection
- def construct_owner_attributes(reflection = @reflection)
- attributes = {}
- if reflection.macro == :belongs_to
- attributes[reflection.association_primary_key] = @owner.send(reflection.foreign_key)
- else
- attributes[reflection.foreign_key] = @owner.send(reflection.active_record_primary_key)
-
- if reflection.options[:as]
- attributes["#{reflection.options[:as]}_type"] = @owner.class.base_class.name
- end
- end
- attributes
- end
-
- # Builds an array of arel nodes from the owner attributes hash
- def construct_owner_conditions(table = aliased_table, reflection = @reflection)
- construct_owner_attributes(reflection).map do |attr, value|
- table[attr].eq(value)
- end
- end
-
- def construct_conditions
- conditions = construct_owner_conditions
- conditions << Arel.sql(sql_conditions) if sql_conditions
- aliased_table.create_and(conditions)
- end
end
end
end
@@ -69,9 +69,7 @@ def delete_records(records)
end
end
- def creation_attributes
- construct_owner_attributes
- end
+ alias creation_attributes construct_owner_attributes
end
end
end
@@ -68,19 +68,11 @@ def find_target
scoped.first.tap { |record| set_inverse_instance(record) }
end
- def finder_options
- {
- :conditions => construct_conditions,
- :select => @reflection.options[:select],
- :include => @reflection.options[:include],
- :readonly => @reflection.options[:readonly],
- :order => @reflection.options[:order]
- }
+ def association_scope
+ super.order(@reflection.options[:order])
end
- def creation_attributes
- construct_owner_attributes
- end
+ alias creation_attributes construct_owner_attributes
def new_record
record = scoped.scoping { yield @reflection }
@@ -9,12 +9,12 @@ def target_scope
super & @reflection.through_reflection.klass.scoped
end
- def finder_options
- super.merge(
- :joins => construct_joins,
- :include => @reflection.options[:include] ||
- @reflection.source_reflection.options[:include]
- )
+ def association_scope
+ scope = super.joins(construct_joins).where(conditions)
+ unless @reflection.options[:include]
+ scope = scope.includes(@reflection.source_reflection.options[:include])
+ end
+ scope
end
# This scope affects the creation of the associated records (not the join records). At the
@@ -98,18 +98,13 @@ def conditions
end
def build_conditions
- association_conditions = @reflection.options[:conditions]
through_conditions = build_through_conditions
source_conditions = @reflection.source_reflection.options[:conditions]
uses_sti = !@reflection.through_reflection.klass.descends_from_active_record?
- if association_conditions || through_conditions || source_conditions || uses_sti
+ if through_conditions || source_conditions || uses_sti
all = []
-
- [association_conditions, source_conditions].each do |conditions|
- all << interpolate_sql(sanitize_sql(conditions)) if conditions
- end
-
+ all << interpolate_sql(sanitize_sql(source_conditions)) if source_conditions
all << through_conditions if through_conditions
all << build_sti_condition if uses_sti
@@ -227,14 +227,6 @@ def test_dependence_with_missing_association_and_nullify
firm.destroy
end
- def test_finding_with_interpolated_condition
- firm = Firm.find(:first)
- superior = firm.clients.create(:name => 'SuperiorCo')
- superior.rating = 10
- superior.save
- assert_equal 10, firm.clients_with_interpolated_conditions.first.rating
- end
-
def test_assignment_before_child_saved
firm = Firm.find(1)
firm.account = a = Account.new("credit_limit" => 1000)
@@ -181,8 +181,8 @@ def test_association_reflection_in_modules
def test_reflection_of_all_associations
# FIXME these assertions bust a lot
- assert_equal 37, Firm.reflect_on_all_associations.size
- assert_equal 27, Firm.reflect_on_all_associations(:has_many).size
+ assert_equal 36, Firm.reflect_on_all_associations.size
+ assert_equal 26, Firm.reflect_on_all_associations(:has_many).size
assert_equal 10, Firm.reflect_on_all_associations(:has_one).size
assert_equal 0, Firm.reflect_on_all_associations(:belongs_to).size
end
@@ -259,7 +259,8 @@ def test_should_maintain_default_scope_on_associations
end
def test_should_default_scope_on_associations_is_overriden_by_association_conditions
- assert_equal [], people(:michael).fixed_bad_references
+ reference = references(:michael_unicyclist).becomes(BadReference)
+ assert_equal [reference], people(:michael).fixed_bad_references
end
def test_should_maintain_default_scope_on_eager_loaded_associations
@@ -49,7 +49,6 @@ class Firm < Company
has_many :exclusively_dependent_clients_of_firm, :foreign_key => "client_of", :class_name => "Client", :order => "id", :dependent => :delete_all
has_many :limited_clients, :class_name => "Client", :limit => 1
has_many :clients_like_ms, :conditions => "name = 'Microsoft'", :class_name => "Client", :order => "id"
- has_many :clients_with_interpolated_conditions, :class_name => "Client", :conditions => 'rating > #{rating}'
has_many :clients_like_ms_with_hash_conditions, :conditions => { :name => 'Microsoft' }, :class_name => "Client", :order => "id"
has_many :clients_using_sql, :class_name => "Client", :finder_sql => 'SELECT * FROM companies WHERE client_of = #{id}'
has_many :clients_using_multiline_sql, :class_name => "Client", :finder_sql => '

0 comments on commit 770e689

Please sign in to comment.