Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Delete/destroy methods called on query deletes correct documents #472

Merged
merged 2 commits into from

3 participants

@balexand

This fixes #458. To summarize, if the following is executed:

Doc.where(some_key: "some value").delete_all

The the expected behavior is:

  • Matching documents are deleted
  • Call returns without exception

The observed behavior before this pull-request:

  • All documents are deleted :unamused:
  • NoMethodError is raised with message saying that delete_all cannot be found even though delete_all was found and called :confused:

The problem is that this line was delegating the delete_all call to the model and then the following line was raising the NoMethodError.

This pull-request fixes the problem for delete, delete_all, destroy, and destroy_all. I can also fix other methods, like create and update, but I'm starting with the delete methods since these can result in accidentally destroying data.

@balexand balexand commented on the diff
lib/mongo_mapper/plugins/querying/decorator.rb
@@ -27,8 +43,11 @@ def find!(*ids)
def method_missing(method, *args, &block)
return super unless model.respond_to?(method)
result = model.send(method, *args, &block)
- return super unless result.is_a?(Plucky::Query)
- merge(result)
+ if result.is_a?(Plucky::Query)
+ merge(result)
+ else
+ result

Returning the result even if it's not a Plucky::Query instance is consistent with ActiveRecord. Alternatively, I'd be okay with raising an exception, but I think that a NoMethodError is not the appropriate type of exception.

@jnunemaker Owner

Hmm. Yeah I'm not sure. The instance where you get here is when you attempt to call a method that is not found, right? In which case no method error makes sense. Is there an instance where you would not be doing that?

No, in this instance the method was found, called, and returned successfully. But the return value was of a type other than Plucky::Query. Before this pull-request, a NoMethodError was raised by the call to super. Now, the result is returned.

The previous line, model.send(method, *args, &block), is where a NoMethodError would be raised if the method was not found. I agree that this is correct and this pull-request doesn't change that.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jnunemaker
Owner

Sweet. Sorry for over looking this. Hoping to pull the outstanding issues for plucky like this one this week.

@jnunemaker jnunemaker merged commit 8ae7ce0 into mongomapper:master

1 check failed

Details default The Travis build failed
@balexand

Thanks!

@jnunemaker
Owner
@balexand

No worries about the delay. And it looks like you can close #458 now.

@slowernet

Is it correct to say that there hasn't been a new version pushed out with this fix yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
16 lib/mongo_mapper/plugins/querying.rb
@@ -40,22 +40,6 @@ def update(*args)
end
end
- def delete(*ids)
- query(:_id => ids.flatten).remove
- end
-
- def delete_all(options={})
- query(options).remove
- end
-
- def destroy(*ids)
- find_some!(ids.flatten).each { |doc| doc.destroy }
- end
-
- def destroy_all(options={})
- find_each(options) { |document| document.destroy }
- end
-
# @api private for now
def query(options={})
query = Plucky::Query.new(collection, :transformer => transformer)
View
25 lib/mongo_mapper/plugins/querying/decorator.rb
@@ -2,11 +2,27 @@
module MongoMapper
module Plugins
module Querying
- Methods = Plucky::Methods + [:find!]
+ Methods = Plucky::Methods + [:delete, :delete_all, :destroy, :destroy_all, :find!]
module Decorator
include DynamicQuerying::ClassMethods
+ def delete(*ids)
+ where(:_id => ids.flatten).remove
+ end
+
+ def delete_all(options = {})
+ where(options).remove
+ end
+
+ def destroy(*ids)
+ [find!(*ids.flatten.compact.uniq)].flatten.each { |doc| doc.destroy }
+ end
+
+ def destroy_all(options={})
+ find_each(options) { |document| document.destroy }
+ end
+
def model(model=nil)
return @model if model.nil?
@model = model
@@ -27,8 +43,11 @@ def find!(*ids)
def method_missing(method, *args, &block)
return super unless model.respond_to?(method)
result = model.send(method, *args, &block)
- return super unless result.is_a?(Plucky::Query)
- merge(result)
+ if result.is_a?(Plucky::Query)
+ merge(result)
+ else
+ result

Returning the result even if it's not a Plucky::Query instance is consistent with ActiveRecord. Alternatively, I'd be okay with raising an exception, but I think that a NoMethodError is not the appropriate type of exception.

@jnunemaker Owner

Hmm. Yeah I'm not sure. The instance where you get here is when you attempt to call a method that is not found, right? In which case no method error makes sense. Is there an instance where you would not be doing that?

No, in this instance the method was found, called, and returned successfully. But the return value was of a type other than Plucky::Query. Before this pull-request, a NoMethodError was raised by the call to super. Now, the result is returned.

The previous line, model.send(method, *args, &block), is where a NoMethodError would be raised if the method was not found. I agree that this is correct and this pull-request doesn't change that.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
end
end
end
View
35 test/functional/test_querying.rb
@@ -560,6 +560,41 @@ def setup
should "be chainable" do
@query.sort(:age).first.should == @doc3
end
+
+ context "with methods from MongoMapper::Plugins::Querying" do
+ should "delete" do
+ lambda do
+ @document.where(:first_name => 'Steve').delete(@doc1.id, @doc2.id)
+ end.should change { @document.count }.by(-1)
+ @document.all.should == [@doc1, @doc3]
+ end
+
+ should "delete_all" do
+ lambda do
+ @document.where(:first_name => 'Steph').delete_all(:last_name => "Nunemaker")
+ end.should change { @document.count }.by(-1)
+ @document.all.should == [@doc1, @doc2]
+ end
+
+ should "destroy" do
+ lambda do
+ @document.where(:first_name => 'Steve').destroy(@doc1.id, @doc2.id)
+ end.should raise_error(MongoMapper::DocumentNotFound)
+ @document.count.should == 3
+
+ lambda do
+ @document.where(:last_name => 'Nunemaker').destroy(@doc1.id, @doc3.id)
+ end.should change { @document.count }.by(-2)
+ @document.all.should == [@doc2]
+ end
+
+ should "destroy_all" do
+ lambda do
+ @document.where(:first_name => 'Steph').destroy_all(:last_name => "Nunemaker")
+ end.should change { @document.count }.by(-1)
+ @document.all.should == [@doc1, @doc2]
+ end
+ end
end
context ".fields" do
View
2  test/functional/test_scopes.rb
@@ -130,7 +130,7 @@ def self.young
should "not work if method does not return a query" do
@document.class_eval { def self.age; 20 end }
- lambda { @document.by_name('John').age }.should raise_error(NoMethodError)
+ @document.by_name('John').age.should == 20
end
end
end
Something went wrong with that request. Please try again.