Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

now passing a proper separate options argument for modifier operation…

…s instead of forcing it in the keys argument hash
  • Loading branch information...
commit 62d23e976b55ba614dc43b0d8fa83a0aeff04748 1 parent f4ef5fb
@hamin authored
Showing with 131 additions and 125 deletions.
  1. +29 −23 lib/mongo_mapper/plugins/modifiers.rb
  2. +102 −102 test/functional/test_modifiers.rb
View
52 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?)

Hmm. Not a fan of explicitly allowing some keys. This would mean each time the driver updates we have to update this spot here. Also means each should be tested. Would be nice to instead somehow just work if we were dealing with options instead of criteria.

Any thoughts on how to make that work? It is difficult since we don't require passing the criteria.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ 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)

Looking good. A few nitpicky things and I'll pull. The coding style of the rest of MM has a space between parameters: hash, options=nil.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ self.class.pop(id, hash, options)
end
end
end
View
204 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
@jnunemaker

Looking good. A few nitpicky things and I'll pull. The coding style of the rest of MM has a space between parameters: hash, options=nil.

@jnunemaker

Hmm. Not a fan of explicitly allowing some keys. This would mean each time the driver updates we have to update this spot here. Also means each should be tested. Would be nice to instead somehow just work if we were dealing with options instead of criteria.

Any thoughts on how to make that work? It is difficult since we don't require passing the criteria.

Please sign in to comment.
Something went wrong with that request. Please try again.