Permalink
Browse files

CollectionProxy < Relation

This helps bring the interfaces of CollectionProxy and Relation closer
together, and reduces the delegation backflips we need to perform.

For example, first_or_create is defined thus:

class ActiveRecord::Relation
  def first_or_create(...)
    first || create(...)
  end
end

If CollectionProxy < Relation, then post.comments.first_or_create will
hit the association's #create method which will actually add the new record
to the association, just as post.comments.create would.

With the previous delegation, post.comments.first_or_create expands to
post.comments.scoped.first_or_create, where post.comments.scoped has no
knowledge of the association.
  • Loading branch information...
1 parent a8637cf commit c86a32d7451c5d901620ac58630460915292f88b @jonleighton jonleighton committed May 11, 2012
@@ -16,12 +16,6 @@ module Associations
# If you need to work on all current children, new and existing records,
# +load_target+ and the +loaded+ flag are your friends.
class CollectionAssociation < Association #:nodoc:
- attr_reader :proxy
-
- def initialize(owner, reflection)
- super
- @proxy = CollectionProxy.new(self)
- end
# Implements the reader method, e.g. foo.items for Foo.has_many :items
def reader(force_reload = false)
@@ -31,7 +25,7 @@ def reader(force_reload = false)
reload
end
- proxy
+ CollectionProxy.new(self)
@alindeman

alindeman Mar 12, 2013

Contributor

Hey @jonleighton. Could you explain a bit more about the rationale behind this change? It makes the behavior of foo.bars return a new object each time, a behavior that differs from Rails 3.

This has implications if folks write a test like:

post = Post.new
post.comments.expects(:find_by_id)

# ... some code that calls post.comments.find_by_id ...

Because post.comments is a new object each time, the mock expectation will never be satisfied.

This might be desirable, but it is a change from Rails 3. I want to hear your thoughts before I write up some documentation around it (for RSpec, specifically).

Thank you.

@pixeltrix

pixeltrix Mar 12, 2013

Owner

@alindeman that sounds like each time you do post.comments.each do |comment| it would requery the database - is this what you're seeing?

@alindeman

alindeman Mar 12, 2013

Contributor

Not quite. It seems like the underlying target is maintained, but the CollectionProxy on top is different:

Loading development environment (Rails 4.0.0.beta1)
2.0.0-p0 :001 > p = Post.first
  Post Load (0.1ms)  SELECT "posts".* FROM "posts" ORDER BY "posts"."id" ASC LIMIT 1
 => #<Post id: 1, title: nil, created_at: "2013-03-12 03:02:50", updated_at: "2013-03-12 03:02:50">
2.0.0-p0 :002 > comments1 = p.comments
  Comment Load (1.3ms)  SELECT "comments".* FROM "comments" WHERE "comments"."post_id" = ?  [["post_id", 1]]
 => #<ActiveRecord::Associations::CollectionProxy []>
2.0.0-p0 :003 > comments2 = p.comments
 => #<ActiveRecord::Associations::CollectionProxy []>
2.0.0-p0 :004 > comments1.__id__ == comments2.__id__
 => false
2.0.0-p0 :005 > comments1.target.__id__ == comments2.target.__id__
 => true

There's only one database call to grab comments. And the target is the same object. But the proxy is not.

@pixeltrix

pixeltrix Mar 12, 2013

Owner

@alindeman I wonder whether it's to prevent infinite loops like the comment below suggests for scoping - @jonleighton will give you a definitive answer in a few hours once he's woken up (middle of the night in the UK at the moment 😄).

@alindeman

alindeman Mar 12, 2013

Contributor

@alindeman I wonder whether it's to prevent infinite loops like the comment below suggests for scoping - @jonleighton will give you a definitive answer in a few hours once he's woken up (middle of the night in the UK at the moment ).

Sounds great! Thank you @pixeltrix

@jonleighton

jonleighton Mar 12, 2013

Member

I'm not sure off the top of my head. I think I just thought it was harmless to return a new object every time, but will need to investigate a bit more. I'll have a look at it on Friday.

@oddlyzen

oddlyzen Mar 12, 2013

In line 23 above, marked as deleted, wouldn't that always assign a new object, thus return a new object?

However, if it was written like:

@Proxy ||= CollectionProxy.new(self)

That would return the same instance as was returned the first time...

or am I missing the context? It seems like both instances of the way it is written above will always return a new object.

@alindeman

alindeman Mar 13, 2013

Contributor

I think this CollectionProxy object itself is memoized by the model when model.association(:foos) ... so by setting @proxy once in the constructor, it was reused each time here.

I can show that in Rails 3, calling ar_model.foos returns the same object each time; in Rails 4, it's a new proxy object each time.

This came up because of an issue filed against rspec (rspec/rspec-rails#707). I have some more details there, though this is not an RSpec-specific issue (I imagine it would happen with any mocking library).

As an aside, I'm not necessarily pushing for this to be partially reverted. At this point, I'm just curious to find more info. If it does make sense to memoize @proxy again, I think some upgrade pain will be saved. If, however, there's a good architectural reason to keep it this way, I can totally accept that too.

@jonleighton

jonleighton Mar 15, 2013

Member

@alindeman I couldn't see a good reason why I changed this. I think I just thought it didn't matter and it was less error-prone to return a new object each time. Anyway, I've changed it back in 133a175.

@alindeman

alindeman Mar 15, 2013

Contributor

Thank you so much @jonleighton !

end
# Implements the writer method, e.g. foo.items= for Foo.has_many :items
@@ -33,14 +33,7 @@ module Associations
#
# is computed directly through SQL and does not trigger by itself the
# instantiation of the actual post records.
- class CollectionProxy # :nodoc:
- alias :proxy_extend :extend
-
- instance_methods.each { |m| undef_method m unless m.to_s =~ /^(?:nil\?|send|object_id|to_a)$|^__|^respond_to|proxy_/ }
-
- delegate :group, :order, :limit, :joins, :where, :preload, :eager_load, :includes, :from,
- :lock, :readonly, :having, :pluck, :to => :scoped
-
+ class CollectionProxy < Relation # :nodoc:
delegate :target, :load_target, :loaded?, :to => :@association
delegate :select, :find, :first, :last,
@@ -52,7 +45,8 @@ class CollectionProxy # :nodoc:
def initialize(association)
@association = association
- Array(association.options[:extend]).each { |ext| proxy_extend(ext) }
+ super association.klass, association.klass.arel_table
+ merge! association.scoped
@tenderlove

tenderlove Feb 15, 2014

Owner

This call to merge! has caused a major performance regression in 4.0. What is it for? Issue #14033

@jonleighton

jonleighton Feb 15, 2014

Member

@tenderlove sorry. I've replied on the issue - as I said there I'm not sure that this is a real-world (severe) perf regression, but I might well be wrong... happy to help resolve it either way.

@tenderlove

tenderlove via email Feb 15, 2014

Owner
@arthurnn

arthurnn Feb 18, 2014

Member

I thought that too. So I updated the issue, enabling query_cache so the hits on the DB should not take time. And there is a expressive difference.

end
alias_method :new, :build
@@ -61,15 +55,24 @@ def proxy_association
@association
end
+ # We don't want this object to be put on the scoping stack, because
+ # that could create an infinite loop where we call an @association
+ # method, which gets the current scope, which is this object, which
+ # delegates to @association, and so on.
+ def scoping
+ @association.scoped.scoping { yield }
+ end
+
+ def spawn
+ scoped
+ end
+
def scoped(options = nil)
association = @association
- scope = association.scoped
- scope.extending! do
+ super.extending! do
define_method(:proxy_association) { association }
end
- scope.merge!(options) if options
- scope
end
def respond_to?(name, include_private = false)
@@ -81,7 +84,7 @@ def respond_to?(name, include_private = false)
def method_missing(method, *args, &block)
match = DynamicMatchers::Method.match(self, method)
if match && match.is_a?(DynamicMatchers::Instantiator)
- scoped.send(method, *args) do |r|
+ super do |r|
proxy_association.send :set_owner_attributes, r
proxy_association.send :add_to_target, r
yield(r) if block_given?
@@ -101,10 +104,14 @@ def method_missing(method, *args, &block)
end
else
- scoped.readonly(nil).public_send(method, *args, &block)
+ super
end
end
+ def ==(other)
+ load_target == other
+ end
+
# Forwards <tt>===</tt> explicitly to the \target because the instance method
# removal above doesn't catch it. Loads the \target if needed.
def ===(other)
@@ -350,7 +350,7 @@ def save_collection_association(reflection)
end
records_to_destroy.each do |record|
- association.proxy.destroy(record)
+ association.destroy(record)
end
end
@@ -34,9 +34,6 @@ def self.references(attributes)
private
def self.build(attribute, value)
case value
- when ActiveRecord::Relation
- value = value.select(value.klass.arel_table[value.klass.primary_key]) if value.select_values.empty?
- attribute.in(value.arel.ast)
when Array, ActiveRecord::Associations::CollectionProxy
values = value.to_a.map {|x| x.is_a?(ActiveRecord::Model) ? x.id : x}
ranges, values = values.partition {|v| v.is_a?(Range)}
@@ -59,6 +56,9 @@ def self.build(attribute, value)
array_predicates = ranges.map { |range| attribute.in(range) }
array_predicates << values_predicate
array_predicates.inject { |composite, predicate| composite.or(predicate) }
+ when ActiveRecord::Relation
+ value = value.select(value.klass.arel_table[value.klass.primary_key]) if value.select_values.empty?
+ attribute.in(value.arel.ast)
when Range
attribute.in(value)
when ActiveRecord::Model
@@ -40,7 +40,7 @@ def create_with_value=(value)
alias extensions extending_values
def includes(*args)
- args.empty? ? self : clone.includes!(*args)
+ args.empty? ? self : spawn.includes!(*args)
end
def includes!(*args)
@@ -51,7 +51,7 @@ def includes!(*args)
end
def eager_load(*args)
- args.blank? ? self : clone.eager_load!(*args)
+ args.blank? ? self : spawn.eager_load!(*args)
end
def eager_load!(*args)
@@ -60,7 +60,7 @@ def eager_load!(*args)
end
def preload(*args)
- args.blank? ? self : clone.preload!(*args)
+ args.blank? ? self : spawn.preload!(*args)
end
def preload!(*args)
@@ -79,7 +79,7 @@ def preload!(*args)
# User.includes(:posts).where("posts.name = 'foo'").references(:posts)
# # => Query now knows the string references posts, so adds a JOIN
def references(*args)
- args.blank? ? self : clone.references!(*args)
+ args.blank? ? self : spawn.references!(*args)
end
def references!(*args)
@@ -120,7 +120,7 @@ def select(value = Proc.new)
if block_given?
to_a.select { |*block_args| value.call(*block_args) }
else
- clone.select!(value)
+ spawn.select!(value)
end
end
@@ -130,7 +130,7 @@ def select!(value)
end
def group(*args)
- args.blank? ? self : clone.group!(*args)
+ args.blank? ? self : spawn.group!(*args)
end
def group!(*args)
@@ -139,7 +139,7 @@ def group!(*args)
end
def order(*args)
- args.blank? ? self : clone.order!(*args)
+ args.blank? ? self : spawn.order!(*args)
end
def order!(*args)
@@ -165,7 +165,7 @@ def order!(*args)
# generates a query with 'ORDER BY id ASC, name ASC'.
#
def reorder(*args)
- args.blank? ? self : clone.reorder!(*args)
+ args.blank? ? self : spawn.reorder!(*args)
end
def reorder!(*args)
@@ -175,7 +175,7 @@ def reorder!(*args)
end
def joins(*args)
- args.compact.blank? ? self : clone.joins!(*args)
+ args.compact.blank? ? self : spawn.joins!(*args)
end
def joins!(*args)
@@ -186,7 +186,7 @@ def joins!(*args)
end
def bind(value)
- clone.bind!(value)
+ spawn.bind!(value)
end
def bind!(value)
@@ -195,7 +195,7 @@ def bind!(value)
end
def where(opts, *rest)
- opts.blank? ? self : clone.where!(opts, *rest)
+ opts.blank? ? self : spawn.where!(opts, *rest)
end
def where!(opts, *rest)
@@ -206,7 +206,7 @@ def where!(opts, *rest)
end
def having(opts, *rest)
- opts.blank? ? self : clone.having!(opts, *rest)
+ opts.blank? ? self : spawn.having!(opts, *rest)
end
def having!(opts, *rest)
@@ -217,7 +217,7 @@ def having!(opts, *rest)
end
def limit(value)
- clone.limit!(value)
+ spawn.limit!(value)
end
def limit!(value)
@@ -226,7 +226,7 @@ def limit!(value)
end
def offset(value)
- clone.offset!(value)
+ spawn.offset!(value)
end
def offset!(value)
@@ -235,7 +235,7 @@ def offset!(value)
end
def lock(locks = true)
- clone.lock!(locks)
+ spawn.lock!(locks)
end
def lock!(locks = true)
@@ -283,7 +283,7 @@ def none
end
def readonly(value = true)
- clone.readonly!(value)
+ spawn.readonly!(value)
end
def readonly!(value = true)
@@ -292,7 +292,7 @@ def readonly!(value = true)
end
def create_with(value)
- clone.create_with!(value)
+ spawn.create_with!(value)
end
def create_with!(value)
@@ -301,7 +301,7 @@ def create_with!(value)
end
def from(value)
- clone.from!(value)
+ spawn.from!(value)
end
def from!(value)
@@ -320,7 +320,7 @@ def from!(value)
# User.select(:name).uniq.uniq(false)
# # => You can also remove the uniqueness
def uniq(value = true)
- clone.uniq!(value)
+ spawn.uniq!(value)
end
def uniq!(value = true)
@@ -366,7 +366,7 @@ def uniq!(value = true)
# end
def extending(*modules, &block)
if modules.any? || block
- clone.extending!(*modules, &block)
+ spawn.extending!(*modules, &block)
else
self
end
@@ -382,7 +382,7 @@ def extending!(*modules, &block)
end
def reverse_order
- clone.reverse_order!
+ spawn.reverse_order!
end
def reverse_order!
@@ -5,7 +5,12 @@
module ActiveRecord
module SpawnMethods
-
+
+ # This is overridden by Associations::CollectionProxy
+ def spawn #:nodoc:
+ clone
+ end
+
# Merges in the conditions from <tt>other</tt>, if <tt>other</tt> is an <tt>ActiveRecord::Relation</tt>.
# Returns an array representing the intersection of the resulting records with <tt>other</tt>, if <tt>other</tt> is an array.
#
@@ -23,7 +28,7 @@ def merge(other)
if other.is_a?(Array)
to_a & other
elsif other
- clone.merge!(other)
+ spawn.merge!(other)
else
self
end
@@ -42,7 +47,7 @@ def merge!(other)
# Post.where('id > 10').order('id asc').except(:where) # discards the where condition but keeps the order
#
def except(*skips)
- result = self.class.new(@klass, table, values.except(*skips))
+ result = Relation.new(klass, table, values.except(*skips))
result.default_scoped = default_scoped
result.extend(*extending_values) if extending_values.any?
result
@@ -56,7 +61,7 @@ def except(*skips)
# Post.order('id asc').only(:where, :order) # uses the specified order
#
def only(*onlies)
- result = self.class.new(@klass, table, values.slice(*onlies))
+ result = Relation.new(klass, table, values.slice(*onlies))
result.default_scoped = default_scoped
result.extend(*extending_values) if extending_values.any?
result
@@ -189,7 +189,7 @@ def test_finding_with_includes_on_has_many_association_with_same_include_include
author = assert_queries(3) { Author.scoped(:includes => {:posts_with_comments => :comments}).find(author_id) } # find the author, then find the posts, then find the comments
author.posts_with_comments.each do |post_with_comments|
assert_equal post_with_comments.comments.length, post_with_comments.comments.count
- assert_nil post_with_comments.comments.uniq!
+ assert_nil post_with_comments.comments.to_a.uniq!
end
end
@@ -497,7 +497,7 @@ def test_find_in_association
def test_include_uses_array_include_after_loaded
project = projects(:active_record)
- project.developers.class # force load target
+ project.developers.load_target
developer = project.developers.first
Oops, something went wrong.

0 comments on commit c86a32d

Please sign in to comment.