This project is archived and is in readonly mode.

#6297 ✓stale
2kan

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

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

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.

Comments and changes to this ticket

  • 2kan

    2kan January 17th, 2011 @ 01:21 AM

    • 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.

  • Adam Wróbel

    Adam Wróbel January 25th, 2011 @ 01:39 PM

    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_...

  • 2kan

    2kan January 25th, 2011 @ 02:02 PM

    @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?

  • Adam Wróbel

    Adam Wróbel January 25th, 2011 @ 02:07 PM

    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.

  • rails

    rails April 26th, 2011 @ 01:00 AM

    • 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.

  • rails

    rails April 26th, 2011 @ 01: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>

People watching this ticket

Attachments

Tags

Referenced by

Pages