Skip to content
This repository was archived by the owner on Mar 2, 2020. It is now read-only.

Wrapping mail generation in rolled-back transaction to prevent database changes from persisting #71

Open
costa wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Wrapping mail generation in rolled-back transaction to prevent database changes from persisting #71

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9023f39
Wrapping the mail generation in a rolled-back transaction to prevent …
costa Mar 30, 2014
9d1d01c
https://github.com/basecamp/mail_view/pull/71#discussion_r17883023
costa Oct 2, 2014
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
10 changes: 8 additions & 2 deletions lib/mail_view.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,14 @@ def not_found(pass = false)
end

def build_mail(name)
mail = send(name)
Mail.inform_interceptors(mail) if defined? Mail
mail = nil
ActiveRecord::Base.transaction do
Copy link
Member

@jeremy jeremy Sep 22, 2014

Choose a reason for hiding this comment

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

How do we know this is the only database connection that's being acted on?

Copy link
Author

@costa costa Oct 2, 2014

Choose a reason for hiding this comment

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

We don't, we cannot account for all side effects, unfortunately, this is just a little something that covers reasonable scenarios, including the case with factories.


mail = send(name)
Mail.inform_interceptors(mail) if defined? Mail

raise ActiveRecord::Rollback
end
mail
Copy link
Member

@jeremy jeremy Sep 22, 2014

Choose a reason for hiding this comment

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

Now we're returning a record that was rolled back, now in inconsistent state. Seems fishy 😁

Copy link
Author

@costa costa Oct 2, 2014

Choose a reason for hiding this comment

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

Which record? Still relevant after the update?

end

Expand Down