This project is archived and is in readonly mode.

#1812 ✓resolved
Brian Mitchell

default_scope can't take procs

Reported by Brian Mitchell | January 28th, 2009 @ 10:24 PM | in 3.1

The primary problem I have with the current implementation is that it only supports a hash as the argument. I'd like to be able to pass in a proc as well similar to how named and anonymous scopes currently work.

My current work-around involves pushing a lambda into default_scoping and implementing a simple argument selector to emulate [] on a hash but there are other problems later if named scopes are used in conjunction (I have to convert to a hash at that point). I've browsed through the code and not found a quick fix. I'll search more later but if someone else has an idea for fixing this I'd be grateful.

Comments and changes to this ticket

  • Nate

    Nate February 11th, 2009 @ 10:13 PM

    +1 on this. i was definitely surprised by this behavior

  • Stephan Kaag
  • Brandon
  • Adrian Mugnolo

    Adrian Mugnolo February 20th, 2009 @ 06:21 PM

    +1

    I've found that this would be very useful for simple model i18n like in:

    
    default_scope :ordered, lambda { {:order => column_translated(:name)} }
    
  • Adrian Mugnolo

    Adrian Mugnolo February 20th, 2009 @ 06:22 PM

    Er, sorry. I meant:

    
    default_scope lambda { {:order => column_translated(:name)} }
    
  • Peter Wagenet

    Peter Wagenet March 16th, 2009 @ 09:15 PM

    This surprised me too. From looking at the code it looks like the quick and easy solution to adding a default scope was taken. I'll poke around a bit and see if I can't figure something out.

  • Josh N. Abbott

    Josh N. Abbott March 16th, 2009 @ 09:18 PM

    Agreed. If you look at the implementation of default_scope vs. named_scope, you'll see a lot more time and thought went into named_scope.

    I think that if we're going to have a default scope option, we should at least consider having it accept the same options as named_scope (such as Proc).

  • Peter Wagenet

    Peter Wagenet March 17th, 2009 @ 02:29 AM

    Attached is a patch that adds support for setting a block for default scope as such:

    
    default_scope do
      { :conditions => ["created_at < ?", Time.now] }
    end
    

    Other than this it behaves almost the same as the previous implementation. In the few places I had to change it, I tried to mimic the behavior of named_scopes. The most notable of these is that the order param now overwrites previous ones. For instance

    
    class Person < ActiveRecord::Base
      default_scope :order => "name DESC"
      named_scope :by_age, :order => "age DESC"
    end
    Person.by_age
    

    will produce the following order clause: "age DESC" where previously it would have produced "name DESC, age DESC".

    Also, defining multiple default_scopes will stack effects in the same way that multiple nested_scopes would. This way you could define a sitewide default_scope and add on to it in subclasses.

    Another small caveat is that, while the condition will be used in the create scope if it is a hash, it will be ignored if it is a string. So { :name => "Peter" } would be used whereas "name = 'Peter'" would not be. Personally, I don't think default scope should affect the create scope, but since that was already there I was hesitant to remove that functionality entirely.

    Finally, to completely override the effects of default_scope, use #with_exclusive_scope.

    Please let me know if you have any questions or suggestions.

  • Peter Wagenet

    Peter Wagenet March 17th, 2009 @ 02:30 AM

    • Tag changed from 2.3, activerecord, default, scope to 2.3, activerecord, default, patch, scope, tested

    Sorry about the formatting. Not sure what happened there. You should still be able to make sense of it.

  • Peter Wagenet

    Peter Wagenet March 17th, 2009 @ 03:09 PM

    • Tag changed from 2.3, activerecord, default, patch, scope, tested to 2.3, activerecord, default_scope, enhancement, named_scope, patch, scope, tested

    Updated the patch with a slightly cleaner version.

  • Peter Wagenet

    Peter Wagenet March 18th, 2009 @ 03:28 PM

    Here's an even better version. Default scope isn't merged in until #scope is actually called which means that you can override the default scope from within a #with_scope block. I also added a #without_default_scope wrapper that temporarily disables all default_scopes for that class.

    
    Person.with_scope(:find => { :conditions => { :active => true }}) do
      Person.without_default_scope do
        Person.find(:all) # => Returns all active people, ignoring any default_scope
      end
    end
    
  • Peter Wagenet

    Peter Wagenet March 18th, 2009 @ 03:34 PM

    Also, if you want to use this functionality in a Rails 2.3 app without patching, check out http://gist.github.com/81187

  • Peter Wagenet
  • matthewcford

    matthewcford April 3rd, 2009 @ 03:21 PM

    I've applied Peters patch and it works for me, thanks.

  • Pratik

    Pratik May 18th, 2009 @ 08:56 PM

    Hey Peter,

    Any reason to go with block instead of procs ? Why not the following :

    default_scope proc { { :conditions => ["created_at < ?", Time.now] } }
    
  • Peter Wagenet

    Peter Wagenet May 18th, 2009 @ 09:25 PM

    Pratik,

    I thought blocks looked nicer, but using procs would be more consistent with named_scope.

  • Pratik

    Pratik May 18th, 2009 @ 09:29 PM

    Yeah. I think it should be consistent with named_scopes. Rest looks nice. Thanks for working on this !

  • runa

    runa August 3rd, 2009 @ 08:33 PM

    • Tag changed from 2.3, activerecord, default_scope, enhancement, named_scope, patch, scope, tested to 2.3, activerecord, bug, default_scope, enhancement, named_scope, patch, scope, tested

    Guys, I'm using the monkeypatch from "March 18th, 2009 @ 03:34 PM" and found the following problem: if you find() and object and include it's relationships, the related table default_scope is not set:

    @@@ruby Loading development environment (Rails 2.3.2)

    Employer.send(:default_scope){{:conditions => {:id => 33}}} job=Job.find(:first,:include => [:employer]) => #

    And runs: 
    @@@ruby
    DEBUG      Job Load (0.3ms)   SELECT * FROM jobs LIMIT 1
    DEBUG      Employer Load (0.4ms)   SELECT * FROM employers WHERE (employers.id = 2424)
    
    (see, no scope in Employer)

    if I now reload job's data and ask for the related employer object:
    @@@ruby

    job.reload DEBUG Job Load (0.4ms) SELECT * FROM jobs WHERE (jobs.id = 56)
    job.employer DEBUG Employer Load (0.4ms) SELECT * FROM employers WHERE (employers.id = 2424) AND (employers.id = 33)

    (see the scope applied to Employer)
    
  • runa

    runa August 3rd, 2009 @ 08:35 PM

    Ok, I screwed the formatting before (no preview button? :)

    Loading development environment (Rails 2.3.2)
    Employer.send(:default_scope){{:conditions => {:id => 33}}} job=Job.find(:first,:include => [:employer]) => #
    

    And runs:

    DEBUG      Job Load (0.3ms)   SELECT * FROM jobs LIMIT 1
    DEBUG      Employer Load (0.4ms)   SELECT * FROM employers WHERE (employers.id = 2424)
    

    (see, no scope in Employer)

    if I now reload job's data and ask for the related employer object: @@@ruby

    Job.reload
    DEBUG Job Load (0.4ms) SELECT * FROM jobs WHERE (jobs.id = 56) job.employer 
    DEBUG Employer Load (0.4ms) SELECT * FROM employers WHERE (employers.id = 2424) AND (employers.id = 33)
    

    (see the scope applied to Employer)

  • Brian Mitchell

    Brian Mitchell August 3rd, 2009 @ 08:41 PM

    Right. I actually don't use that patch. Instead I create an object that poses as a Hash which seems to work well (a hash with a proc passed is not enough since you can't merge or iterate properly -- I basically have a proc subclass that generates proxied new hashes on demand).

  • runa

    runa August 3rd, 2009 @ 09:26 PM

    Brian: could you give me an example? I would love to see a simpler solution!

  • Brian Mitchell

    Brian Mitchell August 3rd, 2009 @ 09:38 PM

    I use something like this code: http://gist.github.com/160809

    A hash poser can take defaults or a block (default is used when the block returns nothing) which allows you to conditionally provide something dynamic.

    Let me know if you have trouble using it or not but it has proven to work consistently enough for one of my projects.

  • Steffen Bartsch

    Steffen Bartsch October 2nd, 2009 @ 09:42 AM

    Brian, is there any reason not to use the default-by-block function of Ruby's Hash implementation? I.e.

    Hash.new {|hash, key| key == :conditions ? {:a_column => "a_value"} : nil }
    

    In my case, I needed to directly set self.default_scoping as default_scope copies the :condition value for the :create scope. Then, it worked perfectly.

  • Brian Mitchell

    Brian Mitchell October 2nd, 2009 @ 04:28 PM

    @Steffen There are issues that still won't work on that method. Specifically creation scopes (it uses a has_key? rather than a nil check last time I looked) as well as proper scope merging as they use other keys and/or require iteration.

    So if it is working for you, it is probably limited to very specific cases. I would not recommend trying to use that method in more general code.

  • Jeremy Kemper

    Jeremy Kemper May 4th, 2010 @ 06:48 PM

    • Milestone changed from 2.x to 3.x
  • Neeraj Singh

    Neeraj Singh June 29th, 2010 @ 05:19 AM

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

    I am looking at rails3 code and looks like I am stuck.

    def construct_finder_arel(options = {}, scope = nil)
    +  options = options.call if options.is_a?(Proc) 
      relation = options.is_a?(Hash) ? unscoped.apply_finder_options(options) : unscoped.merge(options)
      relation = scope.merge(relation) if scope
      relation
    end
    

    Notice the line with + sign. I added that line and now default_scope can take proc. However since the default_scope finally builds a relation object, the lazy loading behavior of default_scope is lost. It means if you define

    default_scope proc { { :conditions => ["created_at < ?", Time.now] } }
    

    then every single time you will get the same value for Time.now which defeats the whole point of having a proc.

    Please correct me but to the extent I know relation object doest not support laziness the way named_scope does.

    I am marking this ticket as open so that we can get more eyes on this ticket.

  • Neeraj Singh

    Neeraj Singh August 13th, 2010 @ 02:25 PM

    • Tag changed from 2.3, activerecord, bug, default_scope, enhancement, named_scope, patch, scope, tested to activerecord, default_scope, enhancement, named_scope, scope
  • Neeraj Singh

    Neeraj Singh August 13th, 2010 @ 02:26 PM

    • Tag changed from activerecord, default_scope, enhancement, named_scope, scope to rails 3, activerecord, default_scope, enhancement, named_scope, scope
  • Tanel Suurhans

    Tanel Suurhans September 24th, 2010 @ 08:35 AM

    The issue is that default scopes should be stored as procs and evaluated on the last moment. Evaluating the proc when appending the scope will not work as expected.
    Its not too complicated to get the default_scope to accept procs with hash conditions, but supporting arel conditions is a bit more complex.
    I managed to get some concept code working, supporting all possible default_scope conditions with the exception of arel conditions needing a unscoped prefix.

    As the scopes are merged and evaluated at the last possible moment, i.e the current_scoped_methods method, and arel conditions are delegated to the scoped method, it will end up in an endless loop (as scoped will access the current_scoped_methods method, which in turn then makes a call to scoped etc).

      def default_scope(scope_options = {})
        # wrap the scope into a lambda, evaluating the actual given conditions first
        self.default_scoping << lambda { construct_finder_arel(scope_options.is_a?(Proc) ? scope_options.call : scope_options) }
      end
    
      def current_scoped_methods 
        # merge all scopes 
        scoped_methods.inject(relation) do |relation, scope|            
          relation.merge(scope.call)
        end
      end
    
      # model
    
      class Category < ActiveRecord::Base
        default_scope order(:name)
        default_scope lambda { { :conditions => ["created_at < ?", Time.now] } }
        default_scope lambda { where("created_at < ?", Time.now) }
      end
    

    Not sure if these changes would yield any side effects, haven't had time to run the test suite just yet. Plus the endless call chain would still also need solving.

  • Tim Morgan

    Tim Morgan October 15th, 2010 @ 04:56 AM

    This seems to make it work:

        def current_scoped_methods #:nodoc:
          if m = scoped_methods.last and m.is_a?(Proc)
            unscoped { m.call }
          else
            scoped_methods.last
          end
        end
    

    The commit is available (with unit test) in my fork here: http://github.com/seven1m/rails/commit/d0251fd1adac2dc2d20a7e4b4f0c...

    Patch is also attached.

    Let me know what you think!

    -Tim

  • Ryan Bigg

    Ryan Bigg October 19th, 2010 @ 08:32 AM

    • Tag cleared.

    Automatic cleanup of spam.

  • Tim Morgan

    Tim Morgan October 19th, 2010 @ 03:38 PM

    I rebased my fork, so the commit is now: http://github.com/seven1m/rails/commit/282f5bb6a6dd89d810e67f7bb864...

    Can someone take a look at this?

  • Neeraj Singh

    Neeraj Singh October 19th, 2010 @ 05:33 PM

    Looks good to me.

    I would replace "and" with "&&" in line if m = scoped_methods.last and m.is_a?(Proc).

  • Tim Morgan

    Tim Morgan October 19th, 2010 @ 05:41 PM

    Replacing and with && would cause m to = true inside the conditional. "and" has lower operator precedence than = in Ruby.

  • Jon Leighton

    Jon Leighton October 19th, 2010 @ 05:49 PM

    Nice one for doing a patch.

    Personally I prefer:

        def current_scoped_methods #:nodoc:
          last = scoped_methods.last
          if last.is_a?(Proc)
            unscoped(&last)
          else
            last
          end
        end
    

    I dislike assignment in conditional tests, and I think you can use the & operator to pass the block directly to unscoped. (Or did you discover some reason why this shouldn't be done?)

  • Tim Morgan

    Tim Morgan October 19th, 2010 @ 05:56 PM

    I just forgot about the & operator for passing the block -- that should work. I'll be happy to remove the assignment from the conditional if you think it will get this accepted.

  • Neeraj Singh

    Neeraj Singh October 19th, 2010 @ 06:00 PM

    Building on what Jon suggested, it can be reduced to two lines.

    def current_scoped_methods #:nodoc:
      last = scoped_methods.last
      last.is_a?(Proc) ? unscoped(&last) : last
    end
    
  • Jon Leighton

    Jon Leighton October 19th, 2010 @ 06:05 PM

    Heh, we're really grasping at straws now!

    I have my head in my own patch at the moment, but will try to run the tests on this later and add a +1.

  • Tim Morgan
  • Aaron Patterson

    Aaron Patterson October 19th, 2010 @ 11:30 PM

    • State changed from “open” to “committed”
    • Milestone changed from 3.x to 3.1
    • Tag set to rails 3.1

    I've pushed this to master. As it's a new feature, I think it should go in the 3.1.0 release.

  • Tim Morgan
  • Adam Wróbel

    Adam Wróbel October 27th, 2010 @ 08:18 PM

    Hey guys,

    I'm a little confused. I've just cloned rails/master and built a gem only to find out this feature still isn't working. Tim's patch with new current_scoped_methods and test cases is merged, but Tanel's modification of default_scope method is not. The lambda is immediately passed to construct_finder_arel and produces a familiar error.

    Also, Tim, ActiveRecord::Relation responds to :call so maybe current_scoped_methods should use is_a?(Proc) like in Neeraj's version.

    I've just hopped into this conversation, so a huge thanks to you guys for looking into this issue.

    Adam

  • Tim Morgan

    Tim Morgan October 27th, 2010 @ 08:35 PM

    Adam, I know the tests on edge rails pass for me as of Oct 20th (haven't tested since then), but I haven't tried it in a real app yet.

    Since I am trying to upgrade a 2.x app, I am still working in 3.0 land, and the following monkeypatch gets me what I need for now:

        module ActiveRecord
          class Base
            class << self
              protected
                def current_scoped_methods #:nodoc:
                  method = scoped_methods.last
                  if method.respond_to?(:call)
                    unscoped(&method)
                  else
                    method
                  end
                end
            end
          end
        end
    
  • Draiken

    Draiken October 29th, 2010 @ 04:53 PM

    Hi !
    I need to set a conditional default_scope and after implementing Tim Morgan's solution on an initializer I still did not manage to pass a Proc to default_scope

    I recieve a

    NoMethodError: undefined methodincludes_values' for #<Proc:0xb5e7b214>

    on the merge method in the SpawnMethods module

    Is there any other way to set a conditional default_scope other than passing a Proc? I really need this :/

  • Adam Wróbel

    Adam Wróbel October 29th, 2010 @ 05:19 PM

    Hi Draiken,

    That's exactly the same error I was getting. Just like I described in my last post I had to merge Tim's and Tanel's modifications. Attached is my git diff patch. Good luck!

  • Tim Morgan

    Tim Morgan October 29th, 2010 @ 05:24 PM

    Are you using latest checkout of 3-0-stable or the 3.0.1 gem?

    I believe my patch will only work with 3-0-stable, i.e. upcoming 3.0.2 gem.

    In your Gemfile, you can specify the following:

    gem 'rails', :git => 'git://github.com/rails/rails.git', :branch => '3-0-stable'
    

    Let me know if that works.

  • Draiken

    Draiken October 29th, 2010 @ 06:29 PM

    Adam Wróbel's patch worked perfectly.

    I had only implemented Tim Morgan's snippet and not the previous one.
    If anyone else have this problem, that patch works :]

    Thank you all very much.

  • Tanel Suurhans

    Tanel Suurhans November 24th, 2010 @ 08:37 PM

    It seems that with Tim Morgans patch the proc for the default scope is not evaluated properly. In production mode the value inside the proc (Time.now for example) stays the same for every invocation.

  • Tim Morgan

    Tim Morgan November 24th, 2010 @ 09:13 PM

    I just updated to Rails 3.0.3 and tested this in production mode. It works for me.

    # irb
    require 'rubygems'
    require 'active_record'
    require 'logger'
    
    ActiveRecord::Base.establish_connection(:adapter => 'sqlite3', :database => ':memory:')
    ActiveRecord::Migration.create_table :widgets do |t|
      t.string :name
      t.timestamps
    end
    
    class Widget < ActiveRecord::Base
      default_scope lambda { where(['created_at >= ?', 1.minute.ago]) }
    end
    
    module ActiveRecord
      class Base
        class << self
          protected
            def current_scoped_methods #:nodoc:
              method = scoped_methods.last
              if method.respond_to?(:call)
                unscoped(&method)
              else
                method
              end
            end
        end
      end
    end
    
    Widget.create!(:name => 'foo')
    # => #<Widget id: 1, name: "foo", created_at: "2010-11-24 15:05:39", updated_at: "2010-11-24 15:05:39"> 
    
    ActiveRecord::Base.logger = Logger.new(STDOUT)
    
    Widget.all
    # Widget Load (0.4ms)  SELECT "widgets".* FROM "widgets" WHERE (created_at >= '2010-11-24 15:05:10.456719')
    
    Widget.all
    # Widget Load (0.3ms)  SELECT "widgets".* FROM "widgets" WHERE (created_at >= '2010-11-24 15:05:16.256514')
    
    Widget.all
    # Widget Load (0.3ms)  SELECT "widgets".* FROM "widgets" WHERE (created_at >= '2010-11-24 15:05:17.373986')
    
    Widget.all
    # Widget Load (0.4ms)  SELECT "widgets".* FROM "widgets" WHERE (created_at >= '2010-11-24 15:05:20.403725')
    
  • Adam Wróbel

    Adam Wróbel January 19th, 2011 @ 07:17 PM

    Tim, sorry for late reply. I've tried your example and it indeed worked. But when I switched my project to 3.0.3 gem and applied your patch I was still getting errors. After some digging I've found out that the problem is with models that use multiple calls to default_scope. This is allowed by rails and the exceptions raise when default_scope tries to merge scopes provided with multiple calls.

    To see those errors change the model definition in your example to:

    class Widget < ActiveRecord::Base
      default_scope order(:created_at)
      default_scope lambda { where(['created_at >= ?', 1.minute.ago]) }
    end
    

    This looks stupid, but something similar happens when you monkey patch foreign models to add your own default_scope in addition to the one they already use.

    The patch I've uploaded previously was a diff of ActiveRecord source code. A monkey patch that gets me going in my project is:

    module ActiveRecord
      class Base

    class &lt;&lt; self
      def default_scope(options = {})
        reset_scoped_methods
        self.default_scoping &lt;&lt; lambda {
          construct_finder_arel(options.is_a?(Proc) ? options.call : options)
        }
      end
    
      protected
        def current_scoped_methods
          method = scoped_methods.last
          if method.respond_to?(:call)
            unscoped(&amp;method)
          else
            method
          end
        end
    end
    
    
    
    
    end end

    Cheers

  • Adam Wróbel
  • Adam Wróbel

    Adam Wróbel January 20th, 2011 @ 12:29 AM

    Actually, I've just found out that my previous patch and all other patches in this ticket were just ignoring the previous default_scope definitions instead of merging them with the current.

    I've updated my gist to take previous default_scope declarations into account. This is consistent with how default_scope works for non-lambda arguments. This patch allows you to have declarations like this one:

    class Widget < ActiveRecord::Base
      default_scope order(:created_at)
      default_scope limit(5)
      default_scope lambda { where(['created_at >= ?', 10.minute.ago]) }
      default_scope lambda { where(['created_at <= ?', 1.minute.ago]) }
    end
    

    The resulting query for Widget.all looks like this:

    SELECT "widgets".* FROM "widgets" WHERE (created_at >= '2011-01-19 23:40:11.181327') AND (created_at <= '2011-01-19 23:49:11.181152') ORDER BY created_at LIMIT 5
    

    Of course usually you wouldn't put those declarations next to each other, but rather in different monkey patches for the some engine.

    A monkey patch for 3.0.x
    https://gist.github.com/786674

    I've also created a pull request at github. Hopefully we'll see this in 3.1:
    https://github.com/rails/rails/pull/169

  • Adam Wróbel

    Adam Wróbel January 20th, 2011 @ 12:35 AM

    Patch for 3.1 for those who prefer it over github fork.

  • Adam Wróbel

    Adam Wróbel January 20th, 2011 @ 05:27 PM

    I was reminded to use respond_to?(:call) instead of kind_of?(Proc) on github. Here's a corrected patch.

  • Tim Morgan

    Tim Morgan January 20th, 2011 @ 08:07 PM

    Adam, thanks for your work on this. I had noticed a problem in the console, whenever a model using default_scope would get reloaded, this bug would cause an exception.

    I think I worked around it my app with some ugly hack, but your fix looks like the right way to handle this.

    I tested the monkeypatch against my 3.0.3 system and it works as advertised. So a +1 on that.

    I did NOT test your patch against 3.1 though.

    Hopefully we can get @tenderlove back on here so he can commit.

  • Julien Guimont

    Julien Guimont January 22nd, 2011 @ 03:35 PM

    Hello,

    The monkey patch for 3.0.x from adamwrobel works for most of the cases, but it seems it doesn't scope the validation for uniqueness. You need to add it to each validates.

    Any pointers as to why?

    Thanks,
    Julien.

  • Adam Wróbel

    Adam Wróbel January 22nd, 2011 @ 04:24 PM

    This is unrelated to the lambda support. Uniqueness validator just ignores the default_scope regardless of whether it's lambda or arel finder.

    activerecord/lib/active_record/validations/uniqueness.rb, line 24:

    relation = finder_class.unscoped.where(sql, *params)
    

    If you feel this is a bug you can open a new ticket. But supporting it would be a bit tricky. Uniqueness validator expects column names which it compares with simple = sql operator. Named and default scopes are much more powerful. Beside allowing all the order(), limit(), offset() etc. finders they allow more complex where() conditions.

    You might need to add the :scope option explicitly to your validations or if that's not possible you can do something similar to what I did in a project at my company, but I have to warn you - it's a really ugly hack:
    https://gist.github.com/791214

    This fits my project, but you might want to modify this to read scope column names from model's class variables.

  • Aaron Patterson

    Aaron Patterson January 22nd, 2011 @ 09:59 PM

    • State changed from “committed” to “open”
    • Assigned user set to “Aaron Patterson”

    reopening and assigning to myself.

  • Ivan Ukhov

    Ivan Ukhov January 28th, 2011 @ 06:58 PM

    Adam Wróbel,

    about your patch, try to change the order of the default_scope calls in the model like:

    class Widget < ActiveRecord::Base
      default_scope lambda { where(['created_at >= ?', 10.minute.ago]) }
      default_scope lambda { where(['created_at <= ?', 1.minute.ago]) }
      default_scope order(:created_at)
      default_scope limit(5)
    end
    

    Internally order and limit (as well as where and others) are delegated to scoped that calls current_scope_methods, therefore, the last two calls will calculate lambdas immediately, save the result, and keep appending it to the final query (thanks to construct_finder_arel). If one of the first two default_scope lambdas passes a hash to where like where(:locale => I18n.locale.to_s), the last two scopes will completely overwrite the locale attribute in the final query.

  • Ivan Ukhov

    Ivan Ukhov January 28th, 2011 @ 07:30 PM

    Actually, this is a problem with all defined scopes in the model using scope and new arel syntax. All regular scopes will overwrite default ones.

  • Adam Wróbel

    Adam Wróbel January 29th, 2011 @ 02:05 PM

    Thanks for finding this issue.

    You're right that those latter calls calculate previously defined lambdas and append their results to the final query. A test with where() other than equality condition given in hash:

    class Widget < ActiveRecord::Base
      default_scope lambda { where(['created_at >= ?', 10.minute.ago]) }
      default_scope order(:created_at)
      default_scope where(:name => "new")
      default_scope limit(5)
    end
    
    Widget.all
    
    #SELECT "widgets".* FROM "widgets" WHERE
    #(created_at >= '2011-01-28 19:03:40.789990') AND
    #(created_at >= '2011-01-28 19:03:40.787447') AND
    #(created_at >= '2011-01-28 19:03:40.788207') AND
    #(created_at >= '2011-01-28 19:03:40.788917') ORDER BY created_at LIMIT 5
    

    The lambda was called four times - once with each non-lambda default_scope call after it was defined and once during the final fetch. Results of all those calls were merged in the final query.

    Test with varying where({}):

    class Widget < ActiveRecord::Base
       default_scope lambda do 
        tmpGlobal ||= "a"
        $tmpGlobal.next!
        where(:name => $tmpGlobal.dup )
      end
      default_scope order(:created_at)
      default_scope limit(5)
    end
    
    Widget.all
    
    #  Widget Load (0.2ms)  SELECT "widgets".* FROM "widgets" WHERE "widgets"."name" = 'c' ORDER BY created_at LIMIT 5
    
    Widget.all
    
    #  Widget Load (0.2ms)  SELECT "widgets".* FROM "widgets" WHERE "widgets"."name" = 'c' ORDER BY created_at LIMIT 5
    

    The lambda value that is stored inside the last default_scope call overwrites what is generated later.

    Although this is an issue that should be remedied the current implementation is still better than the current state of master branch where one just gets exceptions.

    If you have a control over the last calls to default_scope (like when inheriting a class from gem or monkey patching it in your application) then you can invoke your call like this:

    default_scope unscoped.limit(5)

    This will produce correct results. Of course it's not "the Rails way". I'll try to find a workaround, but from my investigation so far it doesn't look easy. Maybe Aaron can help.

    I've got a feeling that if we find a solution to this, then we could also make calls like this work:
    Post.unscoped.published

    Currently it's the same as:
    Post.published

  • Ivan Ukhov

    Ivan Ukhov January 29th, 2011 @ 02:20 PM

    Please also try with regular scopes + where, order, etc. Again they will just overwrite default scopes. My workaround is a module called DynamicDefaultScoping, I include it in classes where I need dynamic default scopes after all calls to regular default_scope and scope and then user already dynamic default_scope.

    In that module I've also implemented the possibility to name default scopes like:

    default_scope :language, lambda { |*args|
      locale = (args[0].presence || I18n.locale).to_s
      case locale = (args[0].presence || I18n.locale).to_s
      when 'any', 'all'
        where :locale => I18n.available_locales.map(&:to_s)
      else
        where :locale => locale
      end
    }
    

    ... and the possibility to later overwrite the default value by calling those named scopes. It works with different combinations with other scopes:

    Post.recent
    Post.language(:en).recent
    Post.recent.language(:en)
    

    I can share if somebody's interested.

  • Ivan Ukhov
  • Adam Wróbel

    Adam Wróbel January 31st, 2011 @ 06:07 AM

    • Tag changed from rails 3.1 to rails 3.1, default_scope, named_scope, patch

    This problem can also surface when using named scopes.

    class Widget < ActiveRecord::Base
      default_scope lambda { where(['created_at >= ?', 10.minute.ago]) }
      scope :red, order(:name => "red")
    end
    

    That call to order will evaluate default scope and save it's current state in :red named scope. The same thing causes the Post.unscoped.published == Post.published absurd. Also calling multiple named scopes like Post.published.recent.by_author("Mike") pollutes the final query with multiple default_scope nodes.

    I see three options for working around this:

    .1. Change scope and default_scope syntax to require blocks.

    Like this:

    default_scope { where(:name => :red) } # or
    default_scope do { :conditions => {...} } end # or
    scope :published { where( :published => 1 ) }
    

    This way we can execute them inside unscoped.scoping {} and apply clean default/named scopes when necessary. Cons: API change; application code is a bit ugly; scope could no longer accept extensions like it does now - through blocks.

    .2. Remove definitions from existing default scope from relations passed to default_scope and scope methods.

    It's a general idea, but one specific implementation I thought about is: With each created ActiveRecord::Relation keep it's second copy that would be free of definitions from default scope. Let's say that the copy is called r.without_default. Relations returned from default scope would have the without_default cleared to unscoped. Then every scope merge or modification would affect both copies equally. Named scopes would be merged into current relation using their without_default copy.

    Cons: rails code gets a bit ugly; we're wasting resources

    .3. Keep the default scope separate from the current scope stack and only merge it into the current relation when we're about to hit database.

    Cons: also some ugly rails code; not as simple as the above; might brake 3rd party gems that use methods like where_values, order_values, because those wouldn't contain data from default scope.

    I've started implementing 3 and then thought 2 would be much quicker. You can find it on my pull request on github. I'm also attaching the patch.

    I don't like that code, but it works. Some testing follows:

    class Widget < ActiveRecord::Base
      default_scope lambda {
        $tmpGlobal ||= "a";
        $tmpGlobal.next!
        where(:name => $tmpGlobal.dup )
      }
      default_scope order(:created_at)
      default_scope limit(5)
      scope :red, where(:name => "red")
      scope :blue, where(:name => "blue")
    end
    Widget.all
    #SELECT "widgets".* FROM "widgets" WHERE "widgets"."name" = 'k' ORDER BY created_at LIMIT 5
    Widget.all
    #SELECT "widgets".* FROM "widgets" WHERE "widgets"."name" = 'm' ORDER BY created_at LIMIT 5
    Widget.red
    #SELECT "widgets".* FROM "widgets" WHERE "widgets"."name" = 'red' ORDER BY created_at LIMIT 5
    Widget.unscoped.blue
    #SELECT "widgets".* FROM "widgets" WHERE "widgets"."name" = 'blue'
    
  • 2kan

    2kan February 2nd, 2011 @ 02:40 PM

    I think that Adam Wróbel is correct in his general idea: AR::Relation should be refactored because there are some problems with current code. I think we should find some new way to store and apply default_scopes.

    Known issues: this ticket, ticket #6011 (which as I understand difficult to fix in edge without ugly fixes) also we have problems with except (ticket #6290, hey can anybody review it???). Anything else?

    I think that the best solution for now is to prevent merging default_scopes with procs. It looks very standoffish for all users and especially for new users to Rails3 and Rails at all when rails raises some internal exception like

    undefined method merge' for #&lt;Proc:0x108af8058&gt;</code>
    </pre>
    
    
    or
    undefined method `includes_values' for #<Proc:0x108af63c0>
    
    
    
    
    
    I think that it is not acceptable. So right now we can raise an exception (see my patch at https://rails.lighthouseapp.com/projects/8994/tickets/6297, also I will attach it here) and start working on refactoring this code. I think we should find as many issues about AR::Realtion as possible for now to design new great solution.
  • Adam Wróbel

    Adam Wróbel March 17th, 2011 @ 01:17 PM

    I've started a discussion on core with suggestion to change scope syntax. I think the syntax change is certainly a better way of resolving this issue than a hacky work around like the one I've posted here.

    http://groups.google.com/group/rubyonrails-core/browse_thread/threa...

  • Jon Leighton

    Jon Leighton April 18th, 2011 @ 11:39 PM

    • State changed from “open” to “resolved”

    Hi,

    I'm closing this ticket because I think the issue is resolved now, see:

    https://github.com/rails/rails/commit/8572ae6671c6ec7c2524f327cee82...

    If anyone disagrees please shout.

    Jon

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>

Referenced by

Pages