Defect #39186
closedMissing synchronization between watchers and watcher_users for unsaved objects
0%
Description
(Note: I'm using issues as examples for watched objects throughout this explanation. The bug applies to all other object types too.)
When adding a new watcher to a new (unsaved) issue using add_watcher
, we currently create a new (unsaved) Watcher
object in the watchers
association in Redmine::Acts::Watchable::InstanceMethods#add_watcher
. However, this user is not included in the watcher_users
association, nor its basic watcher_users_id
list.
This can lead to issues when client code attempts to avoid adding the same user multiple times as a watcher as both lists are not synchronized. Consider the following example as a base case. Here, we try to add the same user multiple times as a watcher, which fails with a uniqueness error :
issue = Issue.new(project: Project.first, subject: 'Test', author: User.first, tracker: Project.first.trackers.first)
issue.add_watcher(User.first)
issue.add_watcher(User.first)
issue.save!
# => ActiveRecord::RecordInvalid (Validation failed: Watchers is invalid)
issue.watchers.last.errors.full_messages.to_sentence
# => User has already been taken
This is well and expected, as an object can be watched by a single user only once.
Now however, we can cause unexpected problems if we try to avoid this error by merely checking if the supposed user already watched the issue, e.g.:
issue = Issue.new(project: Project.first, subject: 'Test', author: User.first, tracker: Project.first.trackers.first)
issue.add_watcher(User.first)
issue.add_watcher(User.first) unless issue.watched_by?(User.first)
issue.save!
# => ActiveRecord::RecordInvalid (Validation failed: Watchers is invalid)
As you can see, we have not solved the issue with a (supposedly) suitable check. Let's have a deeper look:
issue = Issue.new(project: Project.first, subject: 'Test', author: User.first, tracker: Project.first.trackers.first)
issue.add_watcher(User.first)
issue.watchers
# => #<ActiveRecord::Associations::CollectionProxy [#<Watcher id: nil, watchable_type: "Issue", watchable_id: 17, user_id: 1>]>
issue.watcher_user_ids
# => []
As you can see, there is a discrepancy here which results in all checks using watcher_user_ids
or watcher_users
to be incorrect for (yet unsaved) watchers added in the current request. This includes checks like the following two examples:
issue.add_watcher(user) unless issue.watched_by?(user)
users = User.active.to_s
users -= issue.watcher_users
users.each do |u|
obj.add_watcher(u)
end
The latter example is adapted from MailHandler#add_watchers
, which is where I detected the bug.
Note that this bug only manifests as long as the issue is not persisted yet. If the issue was already persisted, the watcher_users.reset
line in Redmine::Acts::Watchable::InstanceMethods#add_watcher
works correctly, so that on next access to watcher_users
or watcher_user_ids
, the updated state is loaded from the database.
Now, to fix this behavior for new objects, I propose to adapt Redmine::Acts::Watchable::InstanceMethods#add_watcher
. In case we are dealing with an unsaved object, we will thus not create a Watcher
object directly but append to the watcher_users
list instead. Here, Rails will implicitly create (and remove) the Watcher
object from the (unsaved) watchers
list as required and save them when the underlying object (the issue in this case) is saved.
The underlying problem is rather obscure to reproduce with code plain Redmine, as it manifests itself only when a single unsaved object is passed around and multiple places try to add watchers. At Planio, we observed the issue in multiple stages of mail processing where I suspect, this is more likely to occur with plugins.
Attached to this issue, you will find two patches which attempt to solve this issue:
0001-Fix-watcher-handling-on-unsaved-objects.patch
- This patch solves the bug described above. For persisted objects, the behavior is unchanged.0002-Add-requirement-for-user_preferences-for-watcher_tes.patch
- This (partially unrelated) patch adds the:user_preferences
fixture requirement toWatcherTest
as some tests (at leasttest_watcher_user_ids_should_make_ids_uniq
) rely on the stored user preferences for auto watching in issue create
Files
Updated by Mischa The Evil about 1 year ago
That's some proper research wrapped-up in a great issue!
Holger Just wrote:
As you can see, we have not solved the issue with a (supposedly) suitable check.
[...]
As you can see, there is a discrepancy here which results in all checks using
watcher_user_ids
orwatcher_users
to be incorrect for (yet unsaved) watchers added in the current request. This includes checks like the following two examples:[...]
This seems indeed like bad behavior to me too.
Holger Just wrote:
Now, to fix this behavior for new objects, I propose to adapt
Redmine::Acts::Watchable::InstanceMethods#add_watcher
. In case we are dealing with an unsaved object, we will thus not create aWatcher
object directly but append to thewatcher_users
list instead. Here, Rails will implicitly create (and remove) theWatcher
object from the (unsaved)watchers
list as required and save them when the underlying object (the issue in this case) is saved.
This seems like a proper fix for the given problem.