From 62d23e976b55ba614dc43b0d8fa83a0aeff04748 Mon Sep 17 00:00:00 2001
From: Haris Amin <aminharis7@gmail.com>
Date: Sun, 4 Mar 2012 07:00:12 -0500
Subject: [PATCH] now passing a proper separate options argument for modifier
 operations instead of forcing it in the keys argument hash

---
 lib/mongo_mapper/plugins/modifiers.rb |  52 ++++---
 test/functional/test_modifiers.rb     | 204 +++++++++++++-------------
 2 files changed, 131 insertions(+), 125 deletions(-)

diff --git a/lib/mongo_mapper/plugins/modifiers.rb b/lib/mongo_mapper/plugins/modifiers.rb
index c7875e5a8..0c024b340 100644
--- a/lib/mongo_mapper/plugins/modifiers.rb
+++ b/lib/mongo_mapper/plugins/modifiers.rb
@@ -65,17 +65,23 @@ def pop(*args)
         private
           def modifier_update(modifier, args)
             criteria, updates, options = criteria_and_keys_from_args(args)
-            if options[:upsert].nil? && options[:safe].nil?
-              collection.update(criteria, {modifier => updates}, :multi => true)
-            else
+            if options
               collection.update(criteria, {modifier => updates}, options.merge(:multi => true))
+            else
+              collection.update(criteria, {modifier => updates}, :multi => true)
             end    
           end
 
           def criteria_and_keys_from_args(args)
             popped_args = args.pop
-            options  = { :upsert => popped_args[:upsert], :safe => popped_args[:safe] }.reject{|k,v| v.nil?}
-            keys     = popped_args.reject{|k,v| [:upsert, :safe, :multi].include?(k)}
+            if popped_args.nil? || (popped_args[:upsert].nil? && popped_args[:safe].nil?)
+              options = nil
+              keys = popped_args.nil? ? args.pop : popped_args
+            else
+              options = { :upsert => popped_args[:upsert], :safe => popped_args[:safe] }.reject{|k,v| v.nil?}
+              keys = args.pop
+            end
+            
             criteria = args[0].is_a?(Hash) ? args[0] : {:id => args}
             [criteria_hash(criteria).to_hash, keys, options]
           end
@@ -85,41 +91,41 @@ def unset(*keys)
         self.class.unset(id, *keys)
       end
 
-      def increment(hash)
-        self.class.increment(id, hash)
+      def increment(hash,options=nil)
+        self.class.increment(id, hash, options)
       end
 
-      def decrement(hash)
-        self.class.decrement(id, hash)
+      def decrement(hash,options=nil)
+        self.class.decrement(id, hash, options)
       end
 
-      def set(hash)
-        self.class.set(id, hash)
+      def set(hash,options=nil)
+        self.class.set(id, hash, options)
       end
 
-      def push(hash)
-        self.class.push(id, hash)
+      def push(hash,options=nil)
+        self.class.push(id, hash, options)
       end
 
-      def push_all(hash)
-        self.class.push_all(id, hash)
+      def push_all(hash,options=nil)
+        self.class.push_all(id, hash, options)
       end
 
-      def pull(hash)
-        self.class.pull(id, hash)
+      def pull(hash,options=nil)
+        self.class.pull(id, hash, options)
       end
 
-      def pull_all(hash)
-        self.class.pull_all(id, hash)
+      def pull_all(hash,options=nil)
+        self.class.pull_all(id, hash, options)
       end
 
-      def add_to_set(hash)
-        self.class.push_uniq(id, hash)
+      def add_to_set(hash,options=nil)
+        self.class.push_uniq(id, hash, options)
       end
       alias push_uniq add_to_set
 
-      def pop(hash)
-        self.class.pop(id, hash)
+      def pop(hash,options=nil)
+        self.class.pop(id, hash, options)
       end
     end
   end
diff --git a/test/functional/test_modifiers.rb b/test/functional/test_modifiers.rb
index ddb233dba..cfe2142a0 100644
--- a/test/functional/test_modifiers.rb
+++ b/test/functional/test_modifiers.rb
@@ -31,292 +31,292 @@ def assert_keys_removed(page, *keys)
         @page  = @page_class.create(:title => 'Home', :tags => %w(foo bar))
         @page2 = @page_class.create(:title => 'Home')
       end
-
+    
       should "work with criteria and keys" do
         @page_class.unset({:title => 'Home'}, :title, :tags)
         assert_keys_removed @page, :title, :tags
         assert_keys_removed @page2, :title, :tags
       end
-
+      
       should "work with ids and keys" do
         @page_class.unset(@page.id, @page2.id, :title, :tags)
         assert_keys_removed @page, :title, :tags
         assert_keys_removed @page2, :title, :tags
       end
     end
-
+    
     context "increment" do
       setup do
         @page  = @page_class.create(:title => 'Home')
         @page2 = @page_class.create(:title => 'Home')
       end
-
+    
       should "work with criteria and modifier hashes" do
         @page_class.increment({:title => 'Home'}, :day_count => 1, :week_count => 2, :month_count => 3)
-
+          
         assert_page_counts @page, 1, 2, 3
         assert_page_counts @page2, 1, 2, 3
       end
-
+    
       should "work with ids and modifier hash" do
         @page_class.increment(@page.id, @page2.id, :day_count => 1, :week_count => 2, :month_count => 3)
-
+    
         assert_page_counts @page, 1, 2, 3
         assert_page_counts @page2, 1, 2, 3
       end
     end
-
+    
     context "decrement" do
       setup do
         @page = @page_class.create(:title => 'Home', :day_count => 1, :week_count => 2, :month_count => 3)
         @page2 = @page_class.create(:title => 'Home', :day_count => 1, :week_count => 2, :month_count => 3)
       end
-
+    
       should "work with criteria and modifier hashes" do
         @page_class.decrement({:title => 'Home'}, :day_count => 1, :week_count => 2, :month_count => 3)
-
+    
         assert_page_counts @page, 0, 0, 0
         assert_page_counts @page2, 0, 0, 0
       end
-
+    
       should "work with ids and modifier hash" do
         @page_class.decrement(@page.id, @page2.id, :day_count => 1, :week_count => 2, :month_count => 3)
-
+    
         assert_page_counts @page, 0, 0, 0
         assert_page_counts @page2, 0, 0, 0
       end
-
+    
       should "decrement with positive or negative numbers" do
         @page_class.decrement(@page.id, @page2.id, :day_count => -1, :week_count => 2, :month_count => -3)
-
+    
         assert_page_counts @page, 0, 0, 0
         assert_page_counts @page2, 0, 0, 0
       end
     end
-
+    
     context "set" do
       setup do
         @page  = @page_class.create(:title => 'Home')
         @page2 = @page_class.create(:title => 'Home')
       end
-
+    
       should "work with criteria and modifier hashes" do
         @page_class.set({:title => 'Home'}, :title => 'Home Revised')
-
+    
         @page.reload
         @page.title.should == 'Home Revised'
-
+    
         @page2.reload
         @page2.title.should == 'Home Revised'
       end
-
+    
       should "work with ids and modifier hash" do
         @page_class.set(@page.id, @page2.id, :title => 'Home Revised')
-
+    
         @page.reload
         @page.title.should == 'Home Revised'
-
+    
         @page2.reload
         @page2.title.should == 'Home Revised'
       end
-
+    
       should "typecast values before querying" do
         @page_class.key :tags, Set
-
+    
         assert_nothing_raised do
           @page_class.set(@page.id, :tags => ['foo', 'bar'].to_set)
           @page.reload
           @page.tags.should == Set.new(['foo', 'bar'])
         end
       end
-
+    
       should "not typecast keys that are not defined in document" do
         assert_raises(BSON::InvalidDocument) do
           @page_class.set(@page.id, :colors => ['red', 'green'].to_set)
         end
       end
-
+    
       should "set keys that are not defined in document" do
         @page_class.set(@page.id, :colors => %w[red green])
         @page.reload
         @page[:colors].should == %w[red green]
       end
     end
-
+    
     context "push" do
       setup do
         @page  = @page_class.create(:title => 'Home')
         @page2 = @page_class.create(:title => 'Home')
       end
-
+    
       should "work with criteria and modifier hashes" do
         @page_class.push({:title => 'Home'}, :tags => 'foo')
-
+    
         @page.reload
         @page.tags.should == %w(foo)
-
+    
         @page2.reload
         @page2.tags.should == %w(foo)
       end
-
+    
       should "work with ids and modifier hash" do
         @page_class.push(@page.id, @page2.id, :tags => 'foo')
-
+    
         @page.reload
         @page.tags.should == %w(foo)
-
+    
         @page2.reload
         @page2.tags.should == %w(foo)
       end
     end
-
+    
     context "push_all" do
       setup do
         @page  = @page_class.create(:title => 'Home')
         @page2 = @page_class.create(:title => 'Home')
         @tags = %w(foo bar)
       end
-
+    
       should "work with criteria and modifier hashes" do
         @page_class.push_all({:title => 'Home'}, :tags => @tags)
-
+    
         @page.reload
         @page.tags.should == @tags
-
+    
         @page2.reload
         @page2.tags.should == @tags
       end
-
+    
       should "work with ids and modifier hash" do
         @page_class.push_all(@page.id, @page2.id, :tags => @tags)
-
+    
         @page.reload
         @page.tags.should == @tags
-
+    
         @page2.reload
         @page2.tags.should == @tags
       end
     end
-
+    
     context "pull" do
       setup do
         @page  = @page_class.create(:title => 'Home', :tags => %w(foo bar))
         @page2 = @page_class.create(:title => 'Home', :tags => %w(foo bar))
       end
-
+    
       should "work with criteria and modifier hashes" do
         @page_class.pull({:title => 'Home'}, :tags => 'foo')
-
+    
         @page.reload
         @page.tags.should == %w(bar)
-
+    
         @page2.reload
         @page2.tags.should == %w(bar)
       end
-
+    
       should "be able to pull with ids and modifier hash" do
         @page_class.pull(@page.id, @page2.id, :tags => 'foo')
-
+    
         @page.reload
         @page.tags.should == %w(bar)
-
+    
         @page2.reload
         @page2.tags.should == %w(bar)
       end
     end
-
+    
     context "pull_all" do
       setup do
         @page  = @page_class.create(:title => 'Home', :tags => %w(foo bar baz))
         @page2 = @page_class.create(:title => 'Home', :tags => %w(foo bar baz))
       end
-
+    
       should "work with criteria and modifier hashes" do
         @page_class.pull_all({:title => 'Home'}, :tags => %w(foo bar))
-
+    
         @page.reload
         @page.tags.should == %w(baz)
-
+    
         @page2.reload
         @page2.tags.should == %w(baz)
       end
-
+    
       should "work with ids and modifier hash" do
         @page_class.pull_all(@page.id, @page2.id, :tags => %w(foo bar))
-
+    
         @page.reload
         @page.tags.should == %w(baz)
-
+    
         @page2.reload
         @page2.tags.should == %w(baz)
       end
     end
-
+    
     context "add_to_set" do
       setup do
         @page  = @page_class.create(:title => 'Home', :tags => 'foo')
         @page2 = @page_class.create(:title => 'Home')
       end
-
+    
       should "be able to add to set with criteria and modifier hash" do
         @page_class.add_to_set({:title => 'Home'}, :tags => 'foo')
-
+    
         @page.reload
         @page.tags.should == %w(foo)
-
+    
         @page2.reload
         @page2.tags.should == %w(foo)
       end
-
+    
       should "be able to add to set with ids and modifier hash" do
         @page_class.add_to_set(@page.id, @page2.id, :tags => 'foo')
-
+    
         @page.reload
         @page.tags.should == %w(foo)
-
+    
         @page2.reload
         @page2.tags.should == %w(foo)
       end
     end
-
+    
     context "push_uniq" do
       setup do
         @page  = @page_class.create(:title => 'Home', :tags => 'foo')
         @page2 = @page_class.create(:title => 'Home')
       end
-
+    
       should "be able to push uniq with criteria and modifier hash" do
         @page_class.push_uniq({:title => 'Home'}, :tags => 'foo')
-
+    
         @page.reload
         @page.tags.should == %w(foo)
-
+    
         @page2.reload
         @page2.tags.should == %w(foo)
       end
-
+    
       should "be able to push uniq with ids and modifier hash" do
         @page_class.push_uniq(@page.id, @page2.id, :tags => 'foo')
-
+    
         @page.reload
         @page.tags.should == %w(foo)
-
+    
         @page2.reload
         @page2.tags.should == %w(foo)
       end
     end
-
+    
     context "pop" do
       setup do
         @page  = @page_class.create(:title => 'Home', :tags => %w(foo bar))
       end
-
+    
       should "be able to remove the last element the array" do
         @page_class.pop(@page.id, :tags => 1)
         @page.reload
         @page.tags.should == %w(foo)
       end
-
+    
       should "be able to remove the first element of the array" do
         @page_class.pop(@page.id, :tags => -1)
         @page.reload
@@ -327,7 +327,7 @@ def assert_keys_removed(page, *keys)
     context "additional options (upsert & safe)" do
       should "be able to pass upsert option" do
         new_key_value = DateTime.now.to_s
-        @page_class.increment({:title => new_key_value}, :day_count => 1, :upsert => true)
+        @page_class.increment({:title => new_key_value}, {:day_count => 1}, {:upsert => true})
         @page_class.count(:title => new_key_value).should == 1
         @page_class.first(:title => new_key_value).day_count.should == 1
       end
@@ -337,13 +337,13 @@ def assert_keys_removed(page, *keys)
         
         # We are trying to increment a key of type string here which should fail
         assert_raises(Mongo::OperationFailure) do
-          @page_class.increment({:title => "Better Be Safe than Sorry"}, :title => 1, :safe => true)
+          @page_class.increment({:title => "Better Be Safe than Sorry"}, {:title => 1}, {:safe => true})
         end
       end
       
       should "be able to pass both safe and upsert options" do
         new_key_value = DateTime.now.to_s
-        @page_class.increment({:title => new_key_value}, :day_count => 1, :upsert => true, :safe => true)
+        @page_class.increment({:title => new_key_value}, {:day_count => 1}, {:upsert => true, :safe => true})
         @page_class.count(:title => new_key_value).should == 1
         @page_class.first(:title => new_key_value).day_count.should == 1
       end
@@ -356,107 +356,107 @@ def assert_keys_removed(page, *keys)
       page.unset(:title, :tags)
       assert_keys_removed page, :title, :tags
     end
-
+  
     should "be able to increment with modifier hashes" do
       page = @page_class.create
       page.increment(:day_count => 1, :week_count => 2, :month_count => 3)
-
+  
       assert_page_counts page, 1, 2, 3
     end
-
+  
     should "be able to decrement with modifier hashes" do
       page = @page_class.create(:day_count => 1, :week_count => 2, :month_count => 3)
       page.decrement(:day_count => 1, :week_count => 2, :month_count => 3)
-
+      
       assert_page_counts page, 0, 0, 0
     end
-
+      
     should "always decrement when decrement is called whether number is positive or negative" do
       page = @page_class.create(:day_count => 1, :week_count => 2, :month_count => 3)
       page.decrement(:day_count => -1, :week_count => 2, :month_count => -3)
-
+      
       assert_page_counts page, 0, 0, 0
     end
-
+      
     should "be able to set with modifier hashes" do
       page  = @page_class.create(:title => 'Home')
       page.set(:title => 'Home Revised')
-
+      
       page.reload
       page.title.should == 'Home Revised'
     end
-
+      
     should "be able to push with modifier hashes" do
       page = @page_class.create
       page.push(:tags => 'foo')
-
+      
       page.reload
       page.tags.should == %w(foo)
     end
-
+      
     should "be able to push_all with modifier hashes" do
       page = @page_class.create
       page.push_all(:tags => %w(foo bar))
-
+      
       page.reload
       page.tags.should == %w(foo bar)
     end
-
+      
     should "be able to pull with criteria and modifier hashes" do
       page = @page_class.create(:tags => %w(foo bar))
       page.pull(:tags => 'foo')
-
+      
       page.reload
       page.tags.should == %w(bar)
     end
-
+      
     should "be able to pull_all with criteria and modifier hashes" do
       page = @page_class.create(:tags => %w(foo bar baz))
       page.pull_all(:tags => %w(foo bar))
-
+      
       page.reload
       page.tags.should == %w(baz)
     end
-
+      
     should "be able to add_to_set with criteria and modifier hash" do
       page  = @page_class.create(:tags => 'foo')
       page2 = @page_class.create
-
+      
       page.add_to_set(:tags => 'foo')
       page2.add_to_set(:tags => 'foo')
-
+      
       page.reload
       page.tags.should == %w(foo)
-
+      
       page2.reload
       page2.tags.should == %w(foo)
     end
-
+      
     should "be able to push uniq with criteria and modifier hash" do
       page  = @page_class.create(:tags => 'foo')
       page2 = @page_class.create
-
+      
       page.push_uniq(:tags => 'foo')
       page2.push_uniq(:tags => 'foo')
-
+      
       page.reload
       page.tags.should == %w(foo)
-
+      
       page2.reload
       page2.tags.should == %w(foo)
     end
-
+      
     should "be able to pop with modifier hashes" do
       page = @page_class.create(:tags => %w(foo bar))
       page.pop(:tags => 1)
-
+      
       page.reload
       page.tags.should == %w(foo)
     end
     
     should "be able to pass upsert option" do
       page = @page_class.create(:title => "Upsert Page")
-      page.increment(:new_count => 1, :upsert => true)
+      page.increment({:new_count => 1}, {:upsert => true})
       
       page.reload
       page.new_count.should == 1
@@ -467,13 +467,13 @@ def assert_keys_removed(page, *keys)
       
       # We are trying to increment a key of type string here which should fail
       assert_raises(Mongo::OperationFailure) do
-        page.increment(:title => 1, :safe => true)
+        page.increment({:title => 1}, {:safe => true})
       end
     end
     
     should "be able to pass upsert and safe options" do
       page = @page_class.create(:title => "Upsert and Safe Page")
-      page.increment(:another_count => 1, :upsert => true, :safe => true)
+      page.increment({:another_count => 1}, {:upsert => true, :safe => true})
       
       page.reload
       page.another_count.should == 1