Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

[PATCH] Default scopes can't be merged with something that responds_to call

#6297

When we have something like this:

class A < ActiveRecord::Base
 default_scope where('salary >= 10000')
 default_scope lambda { order('salary DESC') }
end

or

class A < ActiveRecord::Base
 default_scope lambda { order('salary DESC') }
 default_scope where('salary >= 10000')
end

an internal exception will be thrown:

undefined method `includes_values' for #<Proc:0x108af63c0>

or

undefined method `merge' for #<Proc:0x108af8058>

I think that we can't really merge here:
1) we can't merge them because it is wrong to call default_scope argument when default_scope is defined (as we can't for scopes, for example there could be Time.now) and should call when we apply default_scope.
2) we can't merge them when we apply default_scope because when we have

default_scope lambda {...}
default_scope where(...)

when 'where' executes it is executes through the 'scoped' method where we merge relation with 'current_scoped_methods' which now is the first default_scope. But we should not execute first default_scope and merge here because we are working with the default_scope argument. This mean that we can't separate situations where we are defining the default_scope and when we are applying the default_scope.

So I think we should raise an exception here.

Reported by 2kan · January 17th, 2011 @ 01:13 AM

State: stale
Milestone: none
Assigned to: Aaron Patterson Aaron Patterson
Importance: none

Activity

  1. 2kan
    2kan
    • Tag changed from rails edge, active_record, default_scope to rails edge, active_record, bug, default_scope, patch

    Here is my patch (with test) that raises exception in such situations.

    January 17th, 2011 @ 01:21 AM

  2. Adam Wróbel
    Adam Wróbel

    There is an open ticket to enable proc handling for default scope. It contains a patch that enables lambda default scopes instead of raising an exception on receiving one.

    https://rails.lighthouseapp.com/projects/8994/tickets/1812-default_...

    January 25th, 2011 @ 01:39 PM

  3. 2kan
    2kan

    @Adam, Looks like are familiar with those ticket and patches. What are the current status? Are they working for edge rails? I mean that as I see you can define default_scope with lambda now and it works correct. But what about merging two default_scopes where one uses lambda and one hash or AR::Relation? Does it work correct and lambda evaluates when we apply default_scopes but not when we are define them?

    January 25th, 2011 @ 02:02 PM

  4. Adam Wróbel
    Adam Wróbel

    It's currently waiting to be reviewed by Aaron and hopefully merged into edge for a release in 3.1. And yes - that code allows one to call default_scope multiple times and merges finders and lambdas calling the latter at the time of database fetch rather than default_scope definition.

    January 25th, 2011 @ 02:07 PM

  5. rails
    rails
    • State changed from new to open

    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.

    April 26th, 2011 @ 01:00 AM

  6. rails
    rails
    • State changed from open to stale

    April 26th, 2011 @ 01:00 AM