This project is archived and is in readonly mode.
named_scope doesn't override default_scope's :order key
Reported by Alexander Podgorbunsky | March 26th, 2009 @ 01:04 PM | in 2.3.10
There's a problem of not overriding deafult_scope's keys that don't merge (:order for instance) by same keys of named_scopes
class Post
default_scope :order => 'posts.created_at asc'
named_scope :recent, :order => 'posts.created_at desc'
end
Post.recent # sorts posts in ascending order
This behavior appeared as a result of resolving #1960
Here's the the patch to fix it Patch also fixes test that was broken too (see comments on #1960)
Added code checks if scoping that was current on creation of Scope object is in scoped_methods stack on execution of scope, and if so - doesn't apply it second time (as it's been already implicitly accounted for during merging named scope's scoping with current_scoped_methods). This eliminates a problem of overriding keys of named scope by default scope
Comments and changes to this ticket
-
Yaroslav Markin March 26th, 2009 @ 04:55 PM
+1 since 2.3 named_scope merging is a problem for all folks using default_scope
-
Eloy Duran March 27th, 2009 @ 11:31 AM
I'm gonna be the party pooper here, but simply saying "+1" does nothing good for the ticket. Please see the contribution guide linked in the help text =>.
The part about commenting:
Reviewing Changes Are you happy with the tests, can you follow what they're testing, is there anything missing Does the documentation still seem right to you Do you like the implementation, can you think of a nicer or faster way to implement a part of their change Once you're happy it's a good change, comment on the lighthouse ticket indicating your approval. Your comment should indicate that you like the change and what you like about it. Something like: I like the way you've restructured that code in generate_finder_sql, much nicer. The tests look good too. If your comment simply says +1, then odds are other reviewers aren't going to take it too seriously. Show that you took the time to review the patch. Once three people have approved it, add the verified tag. This will bring it to the attention of a committer who'll then review the changes looking for the same kinds of things.
-
Yaroslav Markin March 27th, 2009 @ 11:38 AM
@Eloy AFAIR old Rails Trac used to have something like "+1 only if you've tested it yourself", and I think this is the case here — tests pass, works as expected, "works for me".
When someone comes with "something's wrong with this", well, different story.
-
Eloy Duran March 27th, 2009 @ 11:47 AM
@Eloy AFAIR old Rails Trac used to have something like "+1 only if you've tested it yourself", and I think this is the case here — tests pass, works as expected, "works for me".
Well I don't know about the past, I'm just reading what it currently says:
Your comment should indicate that you like the change and what you like about it. Something like: I like the way you've restructured that code in generate_finder_sql, much nicer. The tests look good too. If your comment simply says +1, then odds are other reviewers aren't going to take it too seriously. Show that you took the time to review the patch.
So in both cases (good/problem) only a +/- 1 won't be taken too seriously. Which I can understand as it's basically not constructive criticism. Note that I'm also not claiming it's deconstructive criticism, it's neither :)
-
Pratik March 27th, 2009 @ 11:54 AM
I agree with Eloy. Being the asignee here, I wouldn't take seriously the massive influx of +1s in a very short time.
That is not to say there is anything wrong with the patch itself. Patch looks ok and I'll apply the next time I am applying pending patches.
-
Yaroslav Markin April 15th, 2009 @ 10:04 PM
- Tag changed from 2.3, activerecord, bug, default_scope, named_scope, patch to 2.3, activerecord, bug, default_scope, named_scope, patch, verified
-
Repository May 1st, 2009 @ 10:45 PM
- State changed from new to committed
(from [db0bfe4ede3cdfc2e4ccdb2a89525a914e6d0913]) Default scope :order should be overridden by named scopes.
[#2346 state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net http://github.com/rails/rails/co...
-
Repository May 1st, 2009 @ 10:45 PM
(from [628b4ad679b1971427a20461e8c2332d492e4655]) Default scope :order should be overridden by named scopes.
[#2346 state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net http://github.com/rails/rails/co...
-
Jon June 25th, 2009 @ 09:49 PM
I'm not so sure this bug is fixed. Product.my_named_scope works now, but I'm losing the order key on something like @store.products.my_named_scope. So a scoped query isn't respecting the :order key of a named_scope when a default_scope is present or an order clause of a parent model is present.
I made a gist of some of my app's output here: http://gist.github.com/136140
Probably related to ticket #2253
-
jack dempsey (jackdempsey) July 29th, 2009 @ 04:16 PM
Not working for me as well. Following a couple of the links, starting with Jon's to ticket #2253, lead to what looks like a good solution. Will try the patch later if I can, but for now wanted to give another "ain't working here".
-
Jon July 29th, 2009 @ 04:19 PM
Thanks Jack. In my experimentation, some of the other keys weren't working either. Primarily :select. This is a pretty dangerous bug.
-
Jamie Hill December 5th, 2009 @ 02:25 PM
- Tag changed from 2.3, activerecord, bug, default_scope, named_scope, patch, verified to 2.3, 2.3.5, activerecord, bug, default_scope, named_scope, patch, verified
Sorry to resurrect an old ticket but this is definitely still an issue, default_scope becomes pretty much unusable without being able to override the :order etc. in named scopes.
-
Jeroen June 10th, 2010 @ 03:26 PM
As far as I can see this ticket is still open.
I just ran against this problem and it seems I'm still encountering it (2.3.5 and 2.3.8).
Am I overlooking another ticket on this?
-
Santiago Pastorino June 13th, 2010 @ 02:08 AM
- State changed from committed to open
-
Jan Xie June 23rd, 2010 @ 05:19 AM
- Tag changed from 2.3, 2.3.5, activerecord, bug, default_scope, named_scope, patch, verified to rails 3.0.0.beta4, 2.3, 2.3.5, activerecord, bug, default_scope, named_scope, patch, verified
I just met the same problem today, I'm using edge rails (version string = Rails 3.0.0.beta4). Should I create a different ticket?
-
Neeraj Singh July 29th, 2010 @ 09:21 PM
- Milestone changed from 2.x to 2.3.9
- Importance changed from to
-
Santiago Pastorino July 30th, 2010 @ 04:20 AM
Can someone check if this is still an issue on 2-3-stable and provide a backport please.
Thanks. -
Lars Kuhnt August 3rd, 2010 @ 10:13 AM
I am still having this Problem on 2-3-stable with polymorphic has_many associations.
-
Steve Purcell November 22nd, 2010 @ 02:50 PM
As of the 3.0.3 release, this bug is now present in the latest Rails. I can't comment on the proposed patch, but I will say that I haven't found a tidy way to work around this problem.
-
Neeraj Singh November 22nd, 2010 @ 10:37 PM
@Steve the behavior you are seeing in Rails 3.0.3 is not a bug.
class Car < ActiveRecord::Base default_scope :order => 'id desc' scope :recent, :order => 'name desc' end Car.recent #=> SELECT "cars".* FROM "cars" ORDER BY id desc, name desc
In the above case because of default_scope order has 'id desc' at the first. That's okay.
What you might argue is a bug is that if you try User.unscoped.recent then alos you see 'id desc'. I have documented that. In this case you need to use block form of unscoped.
User.unscoped { recent } #=> SELECT "cars".* FROM "cars" ORDER BY name desc
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>
People watching this ticket
Attachments
Referenced by
- 2349 yet another default_scope bug See discussion in #2346.
- 2257 default_scope with named_scope or scoped merge broken on 2.3 whoops.. the wrong number of ticket.. see #2346
- 1960 Make named_scopes remember the current scope when defined Unfortunately, the test was failing right and then broken...
- 2349 yet another default_scope bug Not quite sure, but that seems to be a consequence of #19...
- 2257 default_scope with named_scope or scoped merge broken on 2.3 This ticket is totally wrong, #2346 is the correct one. W...
- 2346 named_scope doesn't override default_scope's :order key [#2346 state:committed]
- 2346 named_scope doesn't override default_scope's :order key [#2346 state:committed]
- 2253 named_scope and nested order clauses Probably related to this ticket: #2346
- 2923 named_scope through a has_many (or has_many through) generates redundant conditions The reason this is happening is in part due to #1960 and ...
- 6532 default_scope doesn't let override the order This behavior is somewhat similar to ticket #2346, but th...