Permalink
Browse files

Use $pull with id match instead of $pullAll on embeds_many nested

attributes delete. Fixes #1928.
  • Loading branch information...
1 parent cddc424 commit 0113ccd38ddf5216f239c4940ed975268ec77035 @durran durran committed Apr 23, 2012
View
6 CHANGELOG.md
@@ -15,6 +15,12 @@ For instructions on upgrading to newer versions, visit
* \#1930 Ensure complex criteria are expanded in all where clauses.
(Hans Hasselberg)
+* \#1928 Deletion of embedded documents via nested attributes now performs
+ a $pull with id match criteria instead of a $pullAll to cover all cases.
+ Previously newly added defaults to documents that had already persisted
+ could not be deleted in this matter since the doc did not match what was
+ in the database.
+
* \#1924/\#1917 Fix pushing to embedded relations with default scopes not
scoping on the new document. (Hans Hasselberg)
View
19 lib/mongoid/atomic.rb
@@ -160,15 +160,18 @@ def atomic_position
#
# @since 2.2.0
def atomic_pulls
- delayed_atomic_pulls.inject({}) do |pulls, (name, docs)|
- pulls.tap do |pull|
- docs.each do |doc|
- (pull[doc.atomic_path] ||= []).push(doc.as_document)
- doc.destroyed = true
- doc.flagged_for_destroy = false
- end
+ pulls = {}
+ delayed_atomic_pulls.each_pair do |_, docs|
+ path = nil
+ ids = docs.map do |doc|
+ path ||= doc.atomic_path
+ doc.destroyed = true
+ doc.flagged_for_destroy = false
+ doc.id
end
+ pulls[path] = { "_id" => { "$in" => ids }} and path = nil
end
+ pulls
end
# Get all the push attributes that need to occur.
@@ -273,7 +276,7 @@ def generate_atomic_updates(mods, doc)
mods.push(doc.atomic_pushes)
mods.push(doc.atomic_array_pushes)
mods.add_to_set(doc.atomic_array_add_to_sets)
- mods.pull(doc.atomic_array_pulls)
+ mods.pull_all(doc.atomic_array_pulls)
end
end
end
View
45 lib/mongoid/atomic/modifiers.rb
@@ -26,17 +26,32 @@ def add_to_set(modifications)
end
end
- # Adds pull modifiers to the modifiers hash.
+ # Adds pull all modifiers to the modifiers hash.
#
- # @example Add pull operations.
- # modifiers.pull({ "addresses" => { "street" => "Bond" }})
+ # @example Add pull all operations.
+ # modifiers.pull_all({ "addresses" => { "street" => "Bond" }})
#
- # @param [ Hash ] modifications The pull modifiers.
+ # @param [ Hash ] modifications The pull all modifiers.
#
- # @since 2.2.0
+ # @since 3.0.0
+ def pull_all(modifications)
+ modifications.each_pair do |field, value|
+ add_operation(pull_alls, field, value)
+ pull_fields << field.split(".", 2)[0]
+ end
+ end
+
+ # Adds pull all modifiers to the modifiers hash.
+ #
+ # @example Add pull all operations.
+ # modifiers.pull({ "addresses" => { "_id" => { "$in" => [ 1, 2, 3 ]}}})
+ #
+ # @param [ Hash ] modifications The pull all modifiers.
+ #
+ # @since 3.0.0
def pull(modifications)
modifications.each_pair do |field, value|
- add_operation(pulls, field, value)
+ pulls[field] = value
pull_fields << field.split(".", 2)[0]
end
end
@@ -226,15 +241,27 @@ def set_fields
# Get the $pullAll operations or intialize a new one.
#
# @example Get the $pullAll operations.
- # modifiers.pulls
+ # modifiers.pull_alls
#
# @return [ Hash ] The $pullAll operations.
#
- # @since 2.1.0
- def pulls
+ # @since 3.0.0
+ def pull_alls
self["$pullAll"] ||= {}
end
+ # Get the $pull operations or intialize a new one.
+ #
+ # @example Get the $pull operations.
+ # modifiers.pulls
+ #
+ # @return [ Hash ] The $pull operations.
+ #
+ # @since 3.0.0
+ def pulls
+ self["$pull"] ||= {}
+ end
+
# Get the $pushAll operations or intialize a new one.
#
# @example Get the $pushAll operations.
View
1 spec/app/models/appointment.rb
@@ -1,6 +1,7 @@
class Appointment
include Mongoid::Document
field :active, :type => Boolean, :default => true
+ field :timed, :type => Boolean, :default => true
embedded_in :person
default_scope where(:active => true)
end
View
55 spec/functional/mongoid/nested_attributes_spec.rb
@@ -1149,6 +1149,61 @@
context "when allow_destroy is true" do
+ context "when the child has defaults" do
+
+ before(:all) do
+ Person.accepts_nested_attributes_for :appointments, allow_destroy: true
+ end
+
+ after(:all) do
+ Person.send(:undef_method, :appointments_attributes=)
+ end
+
+ context "when the parent is persisted" do
+
+ let!(:persisted) do
+ Person.create
+ end
+
+ context "when only 1 child has the default persisted" do
+
+ let!(:app_one) do
+ persisted.appointments.create
+ end
+
+ let!(:app_two) do
+ persisted.appointments.create.tap do |app|
+ app.unset(:timed)
+ end
+ end
+
+ context "when destroying both children" do
+
+ let(:from_db) do
+ Person.find(persisted.id)
+ end
+
+ before do
+ from_db.appointments_attributes =
+ {
+ "bar" => { "id" => app_one.id, "_destroy" => true },
+ "foo" => { "id" => app_two.id, "_destroy" => true }
+ }
+ from_db.save
+ end
+
+ it "destroys both children" do
+ from_db.appointments.should be_empty
+ end
+
+ it "persists the deletes" do
+ from_db.reload.appointments.should be_empty
+ end
+ end
+ end
+ end
+ end
+
context "when the child is not paranoid" do
before(:all) do
View
66 spec/unit/mongoid/atomic/modifiers_spec.rb
@@ -79,7 +79,7 @@
context "when adding a single pull" do
let(:pulls) do
- { "addresses" => [{ "_id" => "one" }] }
+ { "addresses" => { "_id" => { "$in" => [ "one" ]}} }
end
before do
@@ -88,6 +88,62 @@
it "adds the push all modifiers" do
modifiers.should eq(
+ { "$pull" => { "addresses" => { "_id" => { "$in" => [ "one" ]}}}}
+ )
+ end
+ end
+
+ context "when adding to an existing pull" do
+
+ let(:pull_one) do
+ { "addresses" => { "_id" => { "$in" => [ "one" ]}} }
+ end
+
+ let(:pull_two) do
+ { "addresses" => { "_id" => { "$in" => [ "two" ]}} }
+ end
+
+ before do
+ modifiers.pull(pull_one)
+ modifiers.pull(pull_two)
+ end
+
+ it "overwrites the previous pulls" do
+ modifiers.should eq(
+ { "$pull" => { "addresses" => { "_id" => { "$in" => [ "two" ]}}}}
+ )
+ end
+ end
+ end
+ end
+
+ describe "#pull_all" do
+
+ context "when the pulls are empty" do
+
+ before do
+ modifiers.pull_all({})
+ end
+
+ it "does not contain any pull operations" do
+ modifiers.should eq({})
+ end
+ end
+
+ context "when no conflicting modifications are present" do
+
+ context "when adding a single pull" do
+
+ let(:pulls) do
+ { "addresses" => [{ "_id" => "one" }] }
+ end
+
+ before do
+ modifiers.pull_all(pulls)
+ end
+
+ it "adds the push all modifiers" do
+ modifiers.should eq(
{ "$pullAll" =>
{ "addresses" => [
{ "_id" => "one" }
@@ -109,8 +165,8 @@
end
before do
- modifiers.pull(pull_one)
- modifiers.pull(pull_two)
+ modifiers.pull_all(pull_one)
+ modifiers.pull_all(pull_two)
end
it "adds the pull all modifiers" do
@@ -236,7 +292,7 @@
end
before do
- modifiers.pull(pulls)
+ modifiers.pull_all(pulls)
modifiers.push(pushes)
end
@@ -316,7 +372,7 @@
end
before do
- modifiers.pull(pulls)
+ modifiers.pull_all(pulls)
modifiers.set(sets)
end

0 comments on commit 0113ccd

Please sign in to comment.