Skip to content

MONGOID-5206 fix bug where embedded document is not re-embedded #5115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions lib/mongoid/association/embedded/embeds_one/proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,29 @@ def substitute(replacement)
# The associated object will be replaced by the below update if non-nil, so only
# run the callbacks and state-changing code by passing persist: false in that case.
_target.destroy(persist: !replacement) if persistable?

# A little explanation on why this is needed... Say we have three assignments:
#
# canvas.palette = palette
# canvas.palette = nil
# canvas.palette = palette
# Where canvas embeds_one palette.
#
# Previously, what was happening was, on the first assignment,
# palette was considered a "new record" (new_record?=true) and
# thus palette was being inserted into the database. However,
# on the third assignment, we're trying to reassign the palette,
# palette is no longer considered a new record, because it had
# been inserted previously. This is not exactly accurate,
# because the second assignment ultimately removed the palette
# from the database, so it needs to be reinserted. Since the
# palette's new_record is false, Mongoid ends up "updating" the
# document, which doesn't reinsert it into the database.
#
# The change I introduce here, respecifies palette as a "new
# record" when it gets removed from the database, so if it is
# reassigned, it will be reinserted into the database.
_target.new_record = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this should be inside the destroy?

Copy link
Contributor Author

@neilshweky neilshweky Dec 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little explanation on why this fixes the bug... Say we have three assignments:

canvas.palette = palette
canvas.palette = nil 
canvas.palette = palette

Where canvas embeds_one palette.

Previously, what was happening was, on the first assignment, palette was considered a "new record" (new_record?=true) and thus palette was being inserted into the database. However, on the third assignment, where trying to reassign the palette, palette is no longer considered a new record, because it had been inserted previously. This is not exactly accurate, because the second assignment ultimately removed the palette from the database, so it needs to be reinserted. Since the palette's new_record is false, Mongoid ends up "updating" the document, which doesn't reinsert it into the database.

The change I introduce here, respecifies palette as a "new record" when it gets removed from the database, so if it is reassigned, it will be reinserted into the database.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great explanation, can you please copy it into a comment in the source so that we can refer to it in the future when someone looks at this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

end
unbind_one
return nil unless replacement
Expand Down
13 changes: 13 additions & 0 deletions spec/mongoid/association/accessors_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,19 @@
end
end
end

context "when the association is set to nil first" do

let!(:name) do
person.build_name
end

it "returns true" do
person.name = nil
person.name = name
expect(person).to have_name
end
end
end

context "when the association is an embeds many" do
Expand Down
23 changes: 23 additions & 0 deletions spec/mongoid/reloadable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,29 @@
end
end

context "when embedded documents are unasssigned and reassigned" do

let(:palette) do
Palette.new
end

let(:canvas) do
Canvas.create!
end

before do
canvas.palette = palette
canvas.palette = nil
canvas.palette = palette
canvas.save!
canvas.reload
end

it "reloads the embedded document correctly" do
expect(canvas.palette).to eq(palette)
end
end

context "with relational associations" do

let(:person) do
Expand Down