Skip to content

Fix MONGOID-5136 .touch with custom field is broken for embedded documents #5039

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 1 commit into from
Aug 27, 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
28 changes: 26 additions & 2 deletions lib/mongoid/atomic.rb
Original file line number Diff line number Diff line change
Expand Up @@ -385,11 +385,35 @@ def touch_atomic_updates(field = nil)
updates = atomic_updates
return {} unless atomic_updates.key?("$set")
touches = {}
wanted_keys = %w(updated_at u_at)
# TODO this permits field to be passed as an empty string in which case
# it is ignored, get rid of this behavior.
if field.present?
wanted_keys << field.to_s
end
updates["$set"].each_pair do |key, value|
key_regex = /updated_at|u_at#{"|" + field if field.present?}/
touches.merge!({ key => value }) if key =~ key_regex
if wanted_keys.include?(key.split('.').last)
touches.update(key => value)
end
end
{ "$set" => touches }
end

# Returns the $set atomic updates affecting the specified field.
#
# @param [ String ] field The field name.
#
# @api private
def set_field_atomic_updates(field)
updates = atomic_updates
return {} unless atomic_updates.key?("$set")
sets = {}
updates["$set"].each_pair do |key, value|
if key.split('.').last == field
sets.update(key => value)
end
end
{ "$set" => sets }
end
Copy link
Contributor

@johnnyshields johnnyshields Aug 18, 2021

Choose a reason for hiding this comment

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

Instead of doing this, how about adding atomic_touches as another element in Mongoid::Atomic::UPDATES? Then you can implement the solution in MONGOID-5142 when you resolve the atomic actions at the end, by merging it an existing $set update if one is available, or else creating a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Johnny,

My thinking with this PR and 5142 was to repair the problem using the quicker solution so that, in particular, I could attend to the backlog of PRs (many of which are, as I see, created by you - thank you for that).

If you'd like to create a follow-up to this PR that addresses 5142 I'll appreciate it.

Copy link
Contributor

@johnnyshields johnnyshields Aug 18, 2021

Choose a reason for hiding this comment

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

Sure, I'll take a look at this tomorrow.

My pleasure creating the PRs. Your help to review them is greatly appreciated.

FYI in addition I'm going to look at:

Copy link
Contributor

Choose a reason for hiding this comment

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

Fix for MONGOID-5142 here: #5045, this PR can be closed.

end
end
10 changes: 10 additions & 0 deletions lib/mongoid/touchable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ def touch(field = nil)
# _association.inverse_association.options but inverse_association
# seems to not always/ever be set here. See MONGOID-5014.
_parent.touch

if field
# If we are told to also touch a field, perform a separate write
# for that field. See MONGOID-5136.
# In theory we should combine the writes, which would require
# passing the fields to be updated to the parents - MONGOID-5142.
sets = set_field_atomic_updates(field)
selector = atomic_selector
_root.collection.find(selector).update_one(positionally(selector, sets), session: _session)
end
else
# If the current document is not embedded, it is composition root
# and we need to persist the write here.
Expand Down
18 changes: 18 additions & 0 deletions spec/mongoid/touchable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,24 @@
include_examples 'updates the child'
include_examples 'updates the parent when :touch is true'
include_examples 'updates the parent when :touch is not set'

context 'when also updating an additional field' do
it 'persists the update to the additional field' do
entrance
update_time
entrance.touch(:last_used_at)

entrance.reload
building.reload

# This is the assertion we want.
entrance.last_used_at.should == update_time

# Check other timestamps for good measure.
entrance.updated_at.should == update_time
building.updated_at.should == update_time
end
end
end

context "when the document is referenced" do
Expand Down
2 changes: 2 additions & 0 deletions spec/mongoid/touchable_spec_models.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ class Entrance
include Mongoid::Timestamps

embedded_in :building

field :last_used_at, type: Time
end

class Floor
Expand Down