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
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 1 commit
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
6 changes: 5 additions & 1 deletion lib/mail_view.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,11 @@ def not_found(pass = false)
end

def build_mail(name)
mail = send(name)
mail = nil
ActiveRecord::Base.transaction do
Copy link
Member

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

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)
raise ActiveRecord::Rollback
end
Mail.inform_interceptors(mail) if defined? Mail
Copy link
Member

Choose a reason for hiding this comment

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

This would need to be within the transaction, before rollback.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks

mail
Copy link
Member

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

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