Order clause dropped from has_many :through #2083

Closed
pixeltrix opened this Issue Jul 15, 2011 · 12 comments

Comments

Projects
None yet
8 participants
Owner

pixeltrix commented Jul 15, 2011

Given the following model:

class Menu < ActiveRecord::Base
  belongs_to :parent, :class_name => 'Link'
  has_many :links, :order => 'links.name'
end

class Link < ActiveRecord::Base
  belongs_to :menu
  has_one :child, :class_name => 'Menu', :foreign_key => 'parent_id'
  has_many :child_links, :through => :child, :source => :links
end

When you try and fetch the child_links association then the order clause from the links association in Link is ignored, e.g:

>> link.child_links.scoped.to_sql
=> "SELECT links.* FROM links INNER JOIN menus ON links.menu_id = menus.id WHERE menus.parent_id = 1

This isn't a regression - Rails 3.0.9 has the same behaviour, but I'm wondering whether it should be changed for Rails 3.2. What do you think @jonleighton?

@ghost ghost assigned pixeltrix Jul 15, 2011

Owner

pixeltrix commented Jul 15, 2011

Just to add - the :conditions option isn't dropped but the :order option is which is unexpected behaviour in my opinion.

Member

jonleighton commented Jul 15, 2011

Unsure on first glance. Seems reasonable, but I don't know whether it might throw up issues. Two related questions would be: 1) what happens if the assoc and the source both have an order? and 2) what happens with nested associations? (They are more or less the same question actually.)

I'd say feel free to try implementing and see if you encounter issues.

Owner

pixeltrix commented Jul 15, 2011

Already thought of that. Since I'm going to be adding a :reorder option to associations (as we discussed in #1660) then the :order options would merge (like :conditions do) and you could use :reorder to overwrite. However we would want to deprecate it first so initially in Rails 3.2 any :order on the has_many :through definition would overwrite any :order option on the join model with a deprecation warning and in Rails 3.3 we would change to merging.

Member

steveklabnik commented Apr 28, 2012

Are you still working up a patch, @pixeltrix, or should this be closed?

Owner

pixeltrix commented Apr 28, 2012

@steveklabnik I'll address this when I add the :reorder option mentioned in #1660

Member

steveklabnik commented Apr 28, 2012

Sounds good!

Member

steveklabnik commented Sep 14, 2012

@pixeltrix since it's been a few months and you haven't changed this behavior, and this is sorta a feature request more than a bug, can we close this?

Member

schneems commented Oct 19, 2012

1 month ping, any luck on implementation?

Contributor

mdespuits commented Feb 13, 2013

Another ping. Any updates on this?

I think this has been fixed, @neerajdotname was that your patch, can you confirm please?

Member

neerajdotname commented Apr 9, 2013

@carlosantoniodasilva I just checked master and it is fixed there. It was fixed in #10087 .

@neerajdotname awesome, thanks!

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