This project is archived and is in readonly mode.

#222 ✓stale
Henrik Nyh

named_scope :order on association with :order is unintuitive

Reported by Henrik Nyh | May 19th, 2008 @ 04:58 PM

If I have

class Foo < ActiveRecord::Base
  has_many :bars, :order => 'one'
end

class Bar < ActiveRecord::Base
  named_scope :by_two, :order => 'two'
  named_scope :by_three, :order => 'three'
end

then

>>> a_foo.bars.by_two

will generate SQL containing "ORDER BY one, two".

This is unintuitive to me: I would either expect to override the order entirely (ORDER BY two) or for the last scope to have precedence (ORDER BY two, one).

>>> a_foo.bars.by_two.by_three

will generate the same SQL: "ORDER BY one, two".

This is unintuitive as well; I would expect it to either use the last given order (ORDER by three) or all orders, with right-to-left precedence (ORDER BY three, two, one).

Comments and changes to this ticket

  • josh

    josh July 17th, 2008 @ 01:50 AM

    • Assigned user set to “Pratik”
    • Tag set to activerecord, enhancement, named_scope
  • Henrik Nyh

    Henrik Nyh July 17th, 2008 @ 11:00 PM

    Related issue:

    If you have a named scope with a :select, doing

    Foo.that_named_scope.find(:all, :select => 'something')

    will use the select from the named scope, not the one in the find.

  • Incite
  • Incite

    Incite August 22nd, 2008 @ 09:58 AM

    I've come across similar.. in my case I didn't get a comma separated order statement, just the first order param passed in the scope chain.

    The attached patch seems to address this issue. It specifically deals with :order alongside :includes, :conditions, and merges them with the last ordering being most important.

  • Pratik

    Pratik August 22nd, 2008 @ 01:10 PM

    • State changed from “new” to “incomplete”

    Patch is missing tests.

    Thanks.

  • Incite
  • Henrik Nyh

    Henrik Nyh August 25th, 2008 @ 09:23 AM

    David, thanks for doing this.

    Confirmed that the tests pass.

    It does not, however, seem to handle the case where the association is defined with an order, like "has_many :bars, :order => 'one'" above.

    Attaching a failing test to this effect that goes on top of David's latest patch.

    Not saying the patch should not be accepted without this -- better to fix a little than nothing at all -- but if it's easy to get this, too, that would rock.

  • Incite

    Incite August 25th, 2008 @ 04:07 PM

    Henrik,

    You can get the test working by ordering by 'comments.body', and changing the add_order! function in active_record/base.rb, as attached. It assumes you'll want to apply a named scope on an association..

    This breaks using a finder on a named scope, though.

  • Henrik Nyh

    Henrik Nyh August 25th, 2008 @ 04:23 PM

    David: Thanks! "breaks using a finder on a named scope" only in the sense that the :order from named scopes will always beat the order from associations/finds, right?

    So basically one has to choose whether to let associations/finds or named scopes get priority?

    Seems like one could modify the calls to add_order! (or check the caller, but that's nasty), so we can tell whether the non-scope order is from an association or a find.

    Basically, the add_order! calls from associations.rb could be passed a forth parameter like "association = true", and then association orders could be lower prio than scope orders, but find orders would be higher prio.

    Does this make sense? Is it too complicated? I personally would much prefer moderate complication if it means ActiveRecord behaves more intuitively.

    I'd be happy to try this out and submit a patch, just wanted to check if it makes sense to someone more familiar with the codebase.

  • Incite

    Incite August 25th, 2008 @ 10:30 PM

    Yes, the named scope ordering beats associations/finds with my changes.

    One problem being that in our tests, it's only base.rb#construct_finder_sql that calls add_order! and you don't know much context from there.

    I think you're probably right about the priority being dynamic finder < named scope < association ordering.

  • Wildgoose

    Wildgoose February 6th, 2009 @ 04:13 PM

    I just bumped into this in rails 2.2.2 - sad to see it was never merged

    Personally I think the reason for the complication suggested above is because the order should in fact simply be from left to right. So if you have

    obj.sort_a.sort_b.sort_c

    then you get "order a, b, c"

    This meets all the requirements above. I don't obviously see that it's consistent that scopes should win out over an earlier specified sort order (although I do agree there are occasions that would be nice). If it's desired to force sort overrides then I think this needs an extra syntax, the general rule of scopes, etc, is that they are cumulative from left to right and so should the sort options to be consistent

    Hope to see this in 2.4/3?

  • Andrew Vit

    Andrew Vit March 15th, 2009 @ 06:12 PM

    The current behaviour also prevents us from explicitly setting the order on top of a named scope:

    
    obj.sort_a :order => "b,c"
    #=> "order a"
    

    I would expect that either these be chained for "order a,b,c" or else have the explicit order clobber the one in the named scope.

  • Andrew Vit

    Andrew Vit March 15th, 2009 @ 06:50 PM

    It seems this also applies to default_scope, which is potentially very confusing because it hides the source of the problem.

  • Ryan Bigg

    Ryan Bigg June 21st, 2010 @ 02:03 AM

    Can anybody still confirm this is an issue that needs patching? It's a pretty old ticket.

  • rails

    rails February 26th, 2011 @ 12:00 AM

    • State changed from “incomplete” to “open”
    • Importance changed from “” to “”

    This issue has been automatically marked as stale because it has not been commented on for at least three months.

    The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

    Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

  • rails

    rails February 26th, 2011 @ 12:00 AM

    • State changed from “open” to “stale”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages