MONGOID-5785 merging the default scope with named scopes is a bug (#5832)

Author: jamis

lib/mongoid/config.rb

@@ -171,6 +171,15 @@ module Config
     # See https://jira.mongodb.org/browse/MONGOID-5658 for more details.
     option :around_callbacks_for_embeds, default: false
 
+    # When this flag is false, named scopes cannot unset a default scope.
+    # This is the traditional (and default) behavior in Mongoid 9 and earlier.
+    #
+    # Setting this flag to true will allow named scopes to unset the default
+    # scope. This will be the default in Mongoid 10.
+    #
+    # See https://jira.mongodb.org/browse/MONGOID-5785 for more details.
+    option :allow_scopes_to_unset_default_scope, default: false
+
     # Returns the Config singleton, for use in the configure DSL.
     #
     # @return [ self ] The Config singleton.

lib/mongoid/scopable.rb

@@ -294,7 +294,13 @@ def define_scope_method(name)
             scope = instance_exec(*args, **kwargs, &scoping[:scope])
             extension = scoping[:extension]
             to_merge = scope || queryable
-            criteria = to_merge.empty_and_chainable? ? to_merge : with_default_scope.merge(to_merge)
+
+            criteria = if Mongoid.allow_scopes_to_unset_default_scope
+                         to_merge
+                       else
+                         to_merge.empty_and_chainable? ? to_merge : with_default_scope.merge(to_merge)
+                       end
+
             criteria.extend(extension)
             criteria
           end

spec/mongoid/scopable_spec.rb

@@ -3,6 +3,19 @@
 
 require "spec_helper"
 
+# Retrieve the singleton class for the given class.
+def singleton_class_for(klass)
+  class <<klass; self; end
+end
+
+# Helper method for removing a declared scope
+def remove_scope(klass, scope)
+  if klass._declared_scopes[scope]
+    singleton_class_for(klass).remove_method(scope)
+    klass._declared_scopes.delete(scope)
+  end
+end
+
 describe Mongoid::Scopable do
 
   describe ".default_scope" do
@@ -349,6 +362,10 @@ def self.default_scope
           Band.create!(name: 'testing')
         end
 
+        after do
+          remove_scope(Band, :tests)
+        end
+
         it 'applies the collation' do
           expect(Band.tests.first['name']).to eq('testing')
         end
@@ -365,10 +382,7 @@ def add_origin
         end
 
         after do
-          class << Band
-            undef_method :active
-          end
-          Band._declared_scopes.clear
+          remove_scope(Band, :active)
         end
 
         let(:scope) do
@@ -390,10 +404,7 @@ class << Band
         end
 
         after do
-          class << Record
-            undef_method :tool
-          end
-          Record._declared_scopes.clear
+          remove_scope(Record, :tool)
         end
 
         context "when calling the scope" do
@@ -423,10 +434,7 @@ class << Record
         end
 
         after do
-          class << Band
-            undef_method :active
-          end
-          Band._declared_scopes.clear
+          remove_scope(Band, :active)
         end
 
         it "adds a method for the scope" do
@@ -461,10 +469,7 @@ class << Band
             end
 
             after do
-              class << Band
-                undef_method :english
-              end
-              Band._declared_scopes.clear
+              remove_scope(Band, :english)
             end
 
             let(:scope) do
@@ -527,10 +532,7 @@ class << Band
           config_override :scope_overwrite_exception, true
 
           after do
-            class << Band
-              undef_method :active
-            end
-            Band._declared_scopes.clear
+            remove_scope(Band, :active)
           end
 
           it "raises an exception" do
@@ -545,10 +547,7 @@ class << Band
           config_override :scope_overwrite_exception, false
 
           after do
-            class << Band
-              undef_method :active
-            end
-            Band._declared_scopes.clear
+            remove_scope(Band, :active)
           end
 
           it "raises no exception" do
@@ -589,10 +588,7 @@ def add_origin
           end
 
           after do
-            class << Band
-              undef_method :active
-            end
-            Band._declared_scopes.clear
+            remove_scope(Band, :active)
           end
 
           let(:scope) do
@@ -652,11 +648,8 @@ class << Band
         end
 
         after do
-          class << Band
-            undef_method :active
-            undef_method :named_by
-          end
-          Band._declared_scopes.clear
+          remove_scope(Band, :active)
+          remove_scope(Band, :named_by)
         end
 
         it "adds a method for the scope" do
@@ -698,10 +691,7 @@ class << Band
             end
 
             after do
-              class << Band
-                undef_method :english
-              end
-              Band._declared_scopes.clear
+              remove_scope(Band, :english)
             end
 
             let(:scope) do
@@ -775,15 +765,9 @@ class << Band
             end
 
             after do
-              class << Article
-                undef_method :is_public
-                undef_method :authored
-              end
-              Article._declared_scopes.clear
-              class << Author
-                undef_method :author
-              end
-              Author._declared_scopes.clear
+              remove_scope(Article, :is_public)
+              remove_scope(Article, :authored)
+              remove_scope(Author, :author)
             end
 
             context "when calling another model's scope from within a scope" do
@@ -816,10 +800,7 @@ class << Author
           config_override :scope_overwrite_exception, true
 
           after do
-            class << Band
-              undef_method :active
-            end
-            Band._declared_scopes.clear
+            remove_scope(Band, :active)
           end
 
           it "raises an exception" do
@@ -834,10 +815,7 @@ class << Band
           config_override :scope_overwrite_exception, false
 
           after do
-            class << Band
-              undef_method :active
-            end
-            Band._declared_scopes.clear
+            remove_scope(Band, :active)
           end
 
           it "raises no exception" do
@@ -867,11 +845,8 @@ class << Band
         end
 
         after do
-          class << Band
-            undef_method :xxx
-            undef_method :yyy
-          end
-          Band._declared_scopes.clear
+          remove_scope(Band, :xxx)
+          remove_scope(Band, :yyy)
         end
 
         let(:criteria) do
@@ -901,15 +876,8 @@ class << Band
       end
 
       after do
-        class << Shape
-          undef_method :located_at
-        end
-        Shape._declared_scopes.clear
-
-        class << Circle
-          undef_method :with_radius
-        end
-        Circle._declared_scopes.clear
+        remove_scope(Shape, :located_at)
+        remove_scope(Circle, :with_radius)
       end
 
       let(:shape_scope_keys) do
@@ -950,16 +918,9 @@ class << Circle
       end
 
       after do
-        class << Shape
-          undef_method :visible
-          undef_method :large
-        end
-        Shape._declared_scopes.clear
-
-        class << Circle
-          undef_method :large
-        end
-        Circle._declared_scopes.clear
+        remove_scope(Shape, :visible)
+        remove_scope(Shape, :large)
+        remove_scope(Circle, :large)
       end
 
       it "uses subclass context for all the other used scopes" do
@@ -1099,9 +1060,7 @@ class U1 < Kaleidoscope; end
           end
 
           after do
-            class << Band
-              undef_method :active
-            end
+            remove_scope(Band, :active)
           end
 
           let(:unscoped) do
@@ -1143,10 +1102,7 @@ class U2 < Band; end
         end
 
         after do
-          class << Band
-            undef_method :skipped
-          end
-          Band._declared_scopes.clear
+          remove_scope(Band, :skipped)
         end
 
         it "does not allow the default scope to be applied" do
@@ -1290,4 +1246,51 @@ class << Band
       end
     end
   end
+
+  describe "scoped queries" do
+    context "with a default scope" do
+      let(:criteria) do
+        Band.where(name: "Depeche Mode")
+      end
+
+      before do
+        Band.default_scope ->{ criteria }
+        Band.scope :unscoped_everyone, -> { unscoped }
+        Band.scope :removed_default, -> { scoped.remove_scoping(all) }
+
+        Band.create name: 'Depeche Mode'
+        Band.create name: 'They Might Be Giants'
+      end
+
+      after do
+        Band.default_scoping = nil
+        remove_scope Band, :unscoped_everyone
+        remove_scope Band, :removed_default
+      end
+
+      context "when allow_scopes_to_unset_default_scope == false" do # default for <= 9
+        config_override :allow_scopes_to_unset_default_scope, false
+
+        it 'merges the default scope into the query with unscoped' do
+          expect(Band.unscoped_everyone.selector).to include('name' => 'Depeche Mode')
+        end
+
+        it 'merges the default scope into the query with remove_scoping' do
+          expect(Band.removed_default.selector).to include('name' => 'Depeche Mode')
+        end
+      end
+
+      context "when allow_scopes_to_unset_default_scope == true" do # default for >= 10
+        config_override :allow_scopes_to_unset_default_scope, true
+
+        it 'does not merge the default scope into the query with unscoped' do
+          expect(Band.unscoped_everyone.selector).not_to include('name' => 'Depeche Mode')
+        end
+
+        it 'does not merge merges the default scope into the query with remove_scoping' do
+          expect(Band.removed_default.selector).not_to include('name' => 'Depeche Mode')
+        end
+      end
+    end
+  end
 end