Querying through associations doesn't set inverse instance when using extra scopes #5717

Closed
pixeltrix opened this Issue Apr 3, 2012 · 18 comments

Projects

None yet

10 participants

@pixeltrix
Member

Given the following models:

class Manufacturer < ActiveRecord::Base
  has_many :products, :inverse_of => :manufacturer
end

class Product < ActiveRecord::Base
  belongs_to :manufacturer, :inverse_of :products

  def self.by_name
    order(:name)
  end
end

then trying to access the manufacturer of a product queried through the assoication only works without the extra query in a single case:

>> Manufacturer.first.products.to_a.first.manufacturer
SELECT `manufacturers`.* FROM `manufacturers` LIMIT 1
SELECT `products`.* FROM `products` WHERE `products`.`manufacturer_id` = 1

all other cases result in a extra query:

>> Manufacturer.first.products.first.manufacturer
SELECT `manufacturers`.* FROM `manufacturers` LIMIT 1
SELECT `products`.* FROM `products` WHERE `products`.`manufacturer_id` = 1 LIMIT 1
SELECT `manufacturers`.* FROM `manufacturers` WHERE `manufacturers`.`id` = 1 LIMIT 1

This is because the scope created from first, order, etc. has no knowledge of the association. Firstly this is surprising unless you're fairly intimate with AR internals and secondly to workaround the problem you end up making multiple associations, e.g:

class Manufacturer < ActiveRecord::Base
  has_many :products, :inverse_of => :manufacturer
  has_many :sorted_products, :order => 'products.name', :inverse_of => :manufacturer
end

We should be able to pass the association information into the scope and have AR::Relation set the inverse instance in AR::Relation#to_a.

@jonleighton what do you think?

@rafaelfranca
Member

@pixeltrix this issue is stale. Could you try to get more attention for this one in the Campfire room? Or just close it if you think that is better.

@pixeltrix
Member

@rafaelfranca I need to check whether the change to associations in master fixes this - @jonleighton should it have?

@pixeltrix
Member

Okay it's not fixed but the fix appears to be simple enough - I'll push it to a branch so that @jonleighton can review it.

@pixeltrix
Member

Commit 9f607fb has the fix.

@steveklabnik
Member

@pixeltrix are we getting 9f607fb into master any time soon?

@al2o3cr
Contributor
al2o3cr commented Sep 18, 2012

Regarding the comments on 9f607fb, I'd say there are certainly instances where a scoped association should still work much like an association - for instance, inverses should almost certainly be set correctly in code like this:

@some_model.associated_records.where(:foo => 'bar').first_or_create

Seems especially important since that's the recommended form now that find_or_create_by_whatever is going away.

@pixeltrix
Member

@steveklabnik I need to catch up with @jonleighton on this one - I think it still needs addressing but he may want me to take a different approach.

@steveklabnik
Member

Roger. Thanks.

@jonleighton
Member

@pixeltrix I'm on CF today if you want to have a chat. sorry for being slow.

@jonleighton
Member

@pixeltrix thinking about it a bit more, I think your fix could be okay if we check that the record's foreign_key matches the association owner's primary key. (I.e. check that in set_inverse_instance.)

I also might feel a bit better about it if the relation object that has an @association ivar was a subclass of the regular Relation, but that might be over the top. An idea, at least.

@amuino
amuino commented Dec 12, 2012

Any progress on getting this into a release? (I've just tried 3.2.9, problem still exists)

1.9.3p194 :002 > Tournament.find(1).portfolios.includes(:player).map &:tournament
  Tournament Load (0.4ms)  SELECT `tournaments`.* FROM `tournaments` WHERE `tournaments`.`id` = 1 LIMIT 1
  Portfolio Load (0.3ms)  SELECT `portfolios`.* FROM `portfolios` WHERE `portfolios`.`tournament_id` = 1
  Player Load (0.2ms)  SELECT `players`.* FROM `players` WHERE `players`.`id` IN (2, 3)
  Tournament Load (0.2ms)  SELECT `tournaments`.* FROM `tournaments` WHERE `tournaments`.`id` = 1 LIMIT 1
  Tournament Load (0.2ms)  SELECT `tournaments`.* FROM `tournaments` WHERE `tournaments`.`id` = 1 LIMIT 1

We are seeing the additional query when using includes to avoid n+1queries on enrollments… but then we get them back because we are accessing the tournament. Note that the log does not report using the cache for the second portfolio, which would have easied the pain.

@pixeltrix
Member

@amuino it's almost certainly going to be Rails 4 only. I started to re-implement it inline with @jonleighton's recommendations elsewhere but ran into some problems and then work got in the way. It's on my todo list over the next few days.

@bryanlarsen
Contributor

@pixeltrix, any progress? We're currently maintaining an ugly monkey patch similar to your original patch that we'd love to get rid of.

@jonleighton jonleighton was assigned Apr 7, 2013
@neerajdotname
Member

I'm not able to reproduce this issue . I tried both master and 3.2.13. Here is my gist.

https://gist.github.com/neerajdotname/5357445

@jonleighton
Member

I'm planning to take a look at this on Friday. I don't think the issue has gone away but maybe your script doesn't quite pin it down.

@jonleighton jonleighton added a commit that closed this issue May 10, 2013
@jonleighton jonleighton 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! 
d7abe91
@steveklabnik
Member

❤️ ❤️ ❤️

@indirect
Member

:shipit:

@jonleighton jonleighton added a commit that referenced this issue May 10, 2013
@jonleighton jonleighton 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! 
3880a2b
@magnusvk

Found this via Google. Just a note for people like me: I'm still seeing this issue in Rails 4.0.0. Upgrading to the 4-0-stable branch straight from GitHub solved it for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment