Permalink
Browse files

Set the inverse when association queries are refined

Suppose Man has_many interests, and inverse_of is used.

Man.first.interests.first.man will correctly execute two queries,
avoiding the need for a third query when Interest#man is called. This is
because CollectionAssociation#first calls set_inverse_instance.

However Man.first.interests.where("1=1").first.man will execute three
queries, even though this is obviously a subset of the records in the
association.

This is because calling where("1=1") spawns a new Relation object from
the CollectionProxy object, and the Relation has no knowledge of the
association, so it cannot set the inverse instance.

This commit solves the problem by making relations spawned from
CollectionProxies return a new Relation subclass called
AssociationRelation, which does know about associations. Records loaded
from this class will get the inverse instance set properly.

Fixes #5717.

Live commit from La Conf! 
  • Loading branch information...
1 parent 0593c00 commit d7abe91cc73a8991033042f4cb7467bba7fa2339 @jonleighton jonleighton committed Apr 25, 2013
@@ -76,6 +76,7 @@ module ActiveRecord
autoload :AutosaveAssociation
autoload :Relation
+ autoload :AssociationRelation
autoload :NullRelation
autoload_under 'relation' do
@@ -0,0 +1,14 @@
+module ActiveRecord
+ class AssociationRelation < Relation
+ def initialize(klass, table, association)
+ super(klass, table)
+ @association = association
+ end
+
+ private
+
+ def exec_queries
+ super.each { |r| @association.set_inverse_instance r }
+ end
+ end
+end
@@ -122,7 +122,11 @@ def klass
# Can be overridden (i.e. in ThroughAssociation) to merge in other scopes (i.e. the
# through association's scope)
def target_scope
- klass.all
+ all = klass.all
+ scope = AssociationRelation.new(klass, klass.arel_table, self)
+ scope.merge! all
+ scope.default_scoped = all.default_scoped?
+ scope
end
# Loads the \target if needed and returns it.
@@ -18,6 +18,8 @@
require 'models/liquid'
require 'models/molecule'
require 'models/electron'
+require 'models/man'
+require 'models/interest'
class AssociationsTest < ActiveRecord::TestCase
fixtures :accounts, :companies, :developers, :projects, :developers_projects,
@@ -242,6 +244,17 @@ def test_scoped_allows_conditions
david = developers(:david)
assert david.projects.equal?(david.projects)
end
+
+ test "inverses get set of subsets of the association" do
+ man = Man.create
+ man.interests.create
+
+ man = Man.find(man.id)
+
+ assert_queries(1) do
+ assert_equal man, man.interests.where("1=1").first.man
+ end
+ end
end
class OverridingAssociationsTest < ActiveRecord::TestCase

5 comments on commit d7abe91

@pixeltrix
Member

@jonleighton ❤️

I'd tried to fix this in a similar way as we'd discussed but kept running into stack errors - I think it was the default_scope that was causing my problems.

@steveklabnik
Member

Yup, during his talk, that's exactly what happened. Jon basically mentioned that the insight came from looking at the second line of the backtrace instead of the first, which points at the overridden #new on relation, and it clicked.

@pixeltrix
Member

Actually, I wonder whether we can extend AssociationRelation further to ensure that later where conditions don't override the association condition (i.e. GitHub email incident). wdyt?

@jonleighton
Member

@pixeltrix I actually tried to write a test which would corrupt the association condition, but wasn't able to get anything that actually worked. If you can create a failing test case then yeah let's definitely try add that extra safety, but in my earlier research (it was a few weeks ago) it didn't seem to be possible (at least not without severely fucking with AR internals).

@pixeltrix
Member

The problem only exists on 3-2-stable because of a fluke of implementation on master and 4-0-stable. The nodes in where_values from the association condition have an @engine instance variable of ActiveRecord::Base whereas any added through where have an @engine instance variable of Model. This causes seen.include?(w.left) to be false even though it's the same table and column.

Please sign in to comment.