Evaluate default scopes at the last possible moment in order to avoid problems with default scopes getting included into other scopes and then being unable to remove the default part via unscoped.

10 files changed, 67 insertions, 33 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 64be577624..6b3d408720 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,16 @@
 *Rails 3.1.0 (unreleased)*
 
+* Default scopes are now evaluated at the latest possible moment, to avoid problems where
+  scopes would be created which would implicitly contain the default scope, which would then
+  be impossible to get rid of via Model.unscoped.
+
+  Note that this means that if you are inspecting the internal structure of an
+  ActiveRecord::Relation, it will *not* contain the default scope, though the resulting
+  query will do. You can get a relation containing the default scope by calling
+  ActiveRecord#with_default_scope, though this is not part of the public API.
+
+  [Jon Leighton]
+
 * Deprecated support for passing hashes and relations to 'default_scope'. Please create a class
   method for your scope instead. For example, change this:
 
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index e0fb94c96e..08a41e2d8b 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1110,9 +1110,7 @@ module ActiveRecord #:nodoc:
           # If another Active Record class has been passed in, get its current scope
           scope = scope.current_scope if !scope.is_a?(Relation) && scope.respond_to?(:current_scope)
 
-          # Cannot use self.current_scope, because that could trigger build_default_scope, which
-          # could in turn result in with_scope being called and hence an infinite loop.
-          previous_scope = Thread.current[:"#{self}_current_scope"]
+          previous_scope = self.current_scope
 
           if scope.is_a?(Hash)
             # Dup first and second level of hash (method and params).
@@ -1123,6 +1121,7 @@ module ActiveRecord #:nodoc:
 
             scope.assert_valid_keys([ :find, :create ])
             relation = construct_finder_arel(scope[:find] || {})
+            relation.default_scoped = true unless action == :overwrite
 
             if previous_scope && previous_scope.create_with_value && scope[:create]
               scope_for_create = if action == :merge
@@ -1171,7 +1170,7 @@ MSG
         end
 
         def current_scope #:nodoc:
-          Thread.current[:"#{self}_current_scope"] ||= build_default_scope
+          Thread.current[:"#{self}_current_scope"]
         end
 
         def current_scope=(scope) #:nodoc:
@@ -1227,15 +1226,13 @@ class Post < ActiveRecord::Base
 end
 WARN
 
-          # Reset the current scope as it may contain scopes based on a now-invalid default scope
-          self.current_scope = nil
           self.default_scopes = default_scopes.dup << scope
         end
 
         def build_default_scope #:nodoc:
           if method(:default_scope).owner != Base.singleton_class
-            # Exclusively scope to just the relation, to avoid infinite recursion where the
-            # default scope tries to use the default scope tries to use the default scope...
+            # Use relation.scoping to ensure we ignore whatever the current value of
+            # self.current_scope may be.
             relation.scoping { default_scope }
           elsif default_scopes.any?
             default_scopes.inject(relation) do |default_scope, scope|
diff --git a/activerecord/lib/active_record/named_scope.rb b/activerecord/lib/active_record/named_scope.rb
index a398476dd6..68e3018e22 100644
--- a/activerecord/lib/active_record/named_scope.rb
+++ b/activerecord/lib/active_record/named_scope.rb
@@ -35,7 +35,13 @@ module ActiveRecord
         if options
           scoped.apply_finder_options(options)
         else
-          (current_scope || relation).clone
+          if current_scope
+            current_scope.clone
+          else
+            scope = relation.clone
+            scope.default_scoped = true
+            scope
+          end
         end
       end
 
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index 645ad0fce1..4aa2516cb2 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -6,7 +6,7 @@ module ActiveRecord
     JoinOperation = Struct.new(:relation, :join_class, :on)
     ASSOCIATION_METHODS = [:includes, :eager_load, :preload]
     MULTI_VALUE_METHODS = [:select, :group, :order, :joins, :where, :having, :bind]
-    SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :create_with, :from]
+    SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :create_with, :from, :reorder]
 
     include FinderMethods, Calculations, SpawnMethods, QueryMethods, Batches
 
@@ -15,14 +15,16 @@ module ActiveRecord
     delegate :table_name, :quoted_table_name, :primary_key, :quoted_primary_key, :to => :klass
 
     attr_reader :table, :klass, :loaded
-    attr_accessor :extensions
+    attr_accessor :extensions, :default_scoped
     alias :loaded? :loaded
+    alias :default_scoped? :default_scoped
 
     def initialize(klass, table)
       @klass, @table = klass, table
 
       @implicit_readonly = nil
       @loaded            = false
+      @default_scoped    = false
 
       SINGLE_VALUE_METHODS.each {|v| instance_variable_set(:"@#{v}_value", nil)}
       (ASSOCIATION_METHODS + MULTI_VALUE_METHODS).each {|v| instance_variable_set(:"@#{v}_values", [])}
@@ -372,7 +374,7 @@ module ActiveRecord
     end
 
     def where_values_hash
-      equalities = @where_values.grep(Arel::Nodes::Equality).find_all { |node|
+      equalities = with_default_scope.where_values.grep(Arel::Nodes::Equality).find_all { |node|
         node.left.relation.name == table_name
       }
 
@@ -400,6 +402,15 @@ module ActiveRecord
       to_a.inspect
     end
 
+    def with_default_scope #:nodoc:
+      if default_scoped?
+        default_scope = @klass.send(:build_default_scope)
+        default_scope ? default_scope.merge(self) : self
+      else
+        self
+      end
+    end
+
     protected
 
     def method_missing(method, *args, &block)
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 02b7056989..94aa999715 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -8,7 +8,8 @@ module ActiveRecord
     attr_accessor :includes_values, :eager_load_values, :preload_values,
                   :select_values, :group_values, :order_values, :joins_values,
                   :where_values, :having_values, :bind_values,
-                  :limit_value, :offset_value, :lock_value, :readonly_value, :create_with_value, :from_value
+                  :limit_value, :offset_value, :lock_value, :readonly_value, :create_with_value,
+                  :from_value, :reorder_value
 
     def includes(*args)
       args.reject! {|a| a.blank? }
@@ -63,7 +64,11 @@ module ActiveRecord
     end
 
     def reorder(*args)
-      except(:order).order(args)
+      return self if args.blank?
+
+      relation = clone
+      relation.reorder_value = args.flatten
+      relation
     end
 
     def joins(*args)
@@ -163,7 +168,7 @@ module ActiveRecord
     end
 
     def arel
-      @arel ||= build_arel
+      @arel ||= with_default_scope.build_arel
     end
 
     def build_arel
@@ -180,7 +185,8 @@ module ActiveRecord
 
       arel.group(*@group_values.uniq.reject{|g| g.blank?}) unless @group_values.empty?
 
-      arel.order(*@order_values.uniq.reject{|o| o.blank?}) unless @order_values.empty?
+      order = @reorder_value ? @reorder_value : @order_values
+      arel.order(*order.uniq.reject{|o| o.blank?}) unless order.empty?
 
       build_select(arel, @select_values.uniq)
 
diff --git a/activerecord/lib/active_record/relation/spawn_methods.rb b/activerecord/lib/active_record/relation/spawn_methods.rb
index 128e0fbd86..69706b5ead 100644
--- a/activerecord/lib/active_record/relation/spawn_methods.rb
+++ b/activerecord/lib/active_record/relation/spawn_methods.rb
@@ -8,6 +8,8 @@ module ActiveRecord
 
       merged_relation = clone
 
+      r = r.with_default_scope if r.default_scoped? && r.klass != klass
+
       Relation::ASSOCIATION_METHODS.each do |method|
         value = r.send(:"#{method}_values")
 
@@ -70,6 +72,7 @@ module ActiveRecord
     #
     def except(*skips)
       result = self.class.new(@klass, table)
+      result.default_scoped = default_scoped
 
       ((Relation::ASSOCIATION_METHODS + Relation::MULTI_VALUE_METHODS) - skips).each do |method|
         result.send(:"#{method}_values=", send(:"#{method}_values"))
@@ -94,6 +97,7 @@ module ActiveRecord
     #
     def only(*onlies)
       result = self.class.new(@klass, table)
+      result.default_scoped = default_scoped
 
       ((Relation::ASSOCIATION_METHODS + Relation::MULTI_VALUE_METHODS) & onlies).each do |method|
         result.send(:"#{method}_values=", send(:"#{method}_values"))
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index 73887f9b49..e57c5b3b87 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -1626,17 +1626,9 @@ class BasicsTest < ActiveRecord::TestCase
     assert_not_equal c1, c2
   end
 
-  def test_default_scope_is_reset
+  def test_current_scope_is_reset
     Object.const_set :UnloadablePost, Class.new(ActiveRecord::Base)
-    UnloadablePost.table_name = 'posts'
-    UnloadablePost.class_eval do
-      class << self
-        def default_scope
-          order('posts.comments_count ASC')
-        end
-      end
-    end
-    UnloadablePost.scoped # make Thread.current[:UnloadablePost_scoped_methods] not nil
+    UnloadablePost.send(:current_scope=, UnloadablePost.scoped)
 
     UnloadablePost.unloadable
     assert_not_nil Thread.current[:UnloadablePost_current_scope]
diff --git a/activerecord/test/cases/relation_scoping_test.rb b/activerecord/test/cases/relation_scoping_test.rb
index a91072b94b..5079aec9ba 100644
--- a/activerecord/test/cases/relation_scoping_test.rb
+++ b/activerecord/test/cases/relation_scoping_test.rb
@@ -329,7 +329,7 @@ class DefaultScopingTest < ActiveRecord::TestCase
 
   def test_default_scoping_with_threads
     2.times do
-      Thread.new { assert_equal ['salary DESC'], DeveloperOrderedBySalary.scoped.order_values }.join
+      Thread.new { assert DeveloperOrderedBySalary.scoped.to_sql.include?('salary DESC') }.join
     end
   end
 
@@ -427,6 +427,13 @@ class DefaultScopingTest < ActiveRecord::TestCase
     jamis = PoorDeveloperCalledJamis.create_with(:name => 'Aaron').create_with(nil).new
     assert_equal 'Jamis', jamis.name
   end
+
+  def test_unscoped_with_named_scope_should_not_have_default_scope
+    assert_equal [DeveloperCalledJamis.find(developers(:poor_jamis).id)], DeveloperCalledJamis.poor
+
+    assert DeveloperCalledJamis.unscoped.poor.include?(developers(:david).becomes(DeveloperCalledJamis))
+    assert_equal 10, DeveloperCalledJamis.unscoped.poor.length
+  end
 end
 
 class DeprecatedDefaultScopingTest < ActiveRecord::TestCase
@@ -459,7 +466,7 @@ class DeprecatedDefaultScopingTest < ActiveRecord::TestCase
 
   def test_default_scoping_with_threads
     2.times do
-      Thread.new { assert_equal ['salary DESC'], DeprecatedDeveloperOrderedBySalary.scoped.order_values }.join
+      Thread.new { assert DeprecatedDeveloperOrderedBySalary.scoped.to_sql.include?('salary DESC') }.join
     end
   end
 
@@ -469,12 +476,10 @@ class DeprecatedDefaultScopingTest < ActiveRecord::TestCase
     ActiveSupport::Deprecation.silence { klass.send :default_scope, :limit => 1 }
 
     # Scopes added on children should append to parent scope
-    assert_equal 1,               klass.scoped.limit_value
-    assert_equal ['salary DESC'], klass.scoped.order_values
+    assert_equal [developers(:jamis).id], klass.all.map(&:id)
 
     # Parent should still have the original scope
-    assert_nil DeprecatedDeveloperOrderedBySalary.scoped.limit_value
-    assert_equal ['salary DESC'], DeprecatedDeveloperOrderedBySalary.scoped.order_values
+    assert_equal Developer.order('salary DESC').map(&:id), DeprecatedDeveloperOrderedBySalary.all.map(&:id)
   end
 
   def test_default_scope_called_twice_merges_conditions
diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb
index 7bdbd773b6..6874bd18f8 100644
--- a/activerecord/test/cases/relation_test.rb
+++ b/activerecord/test/cases/relation_test.rb
@@ -20,7 +20,7 @@ module ActiveRecord
     end
 
     def test_single_values
-      assert_equal [:limit, :offset, :lock, :readonly, :create_with, :from].map(&:to_s).sort,
+      assert_equal [:limit, :offset, :lock, :readonly, :create_with, :from, :reorder].map(&:to_s).sort,
         Relation::SINGLE_VALUE_METHODS.map(&:to_s).sort
     end
 
diff --git a/activerecord/test/models/developer.rb b/activerecord/test/models/developer.rb
index 28b31caf7b..10385ba899 100644
--- a/activerecord/test/models/developer.rb
+++ b/activerecord/test/models/developer.rb
@@ -114,6 +114,8 @@ class DeveloperCalledJamis < ActiveRecord::Base
   def self.default_scope
     where :name => 'Jamis'
   end
+
+  scope :poor, where('salary < 150000')
 end
 
 class AbstractDeveloperCalledJamis < ActiveRecord::Base