Merge pull request #24457 from jeremy/mailer/dont-deliver-later-after-message-is-loaded

Disallow calling `#deliver_later` after local message modifications.

actionmailer/CHANGELOG.md

@@ -1,3 +1,21 @@
+*   Disallow calling `#deliver_later` after making local modifications to
+    the message which would be lost when the delivery job is enqueued.
+
+    Prevents a common, hard-to-find bug like
+
+        message = Notifier.welcome(user, foo)
+        message.message_id = my_generated_message_id
+        message.deliver_later
+
+    The message_id is silently lost! *Only the mailer arguments are passed
+    to the delivery job.*
+
+    This raises an exception now. Make modifications to the message within
+    the mailer method instead, or use a custom Active Job to manage delivery
+    instead of using #deliver_later.
+
+    *Jeremy Daer*
+
 *   Removes `-t` from default Sendmail arguments to match the underlying
     `Mail::Sendmail` setting.
 

actionmailer/lib/action_mailer/message_delivery.rb

@@ -18,6 +18,7 @@ module ActionMailer
       @mailer = mailer
       @mail_method = mail_method
       @args = args
+      @obj = nil
     end
 
     def __getobj__ #:nodoc:
@@ -91,8 +92,19 @@ module ActionMailer
     private
 
       def enqueue_delivery(delivery_method, options={})
-        args = @mailer.name, @mail_method.to_s, delivery_method.to_s, *@args
-        ActionMailer::DeliveryJob.set(options).perform_later(*args)
+        if @obj
+          raise "You've accessed the message before asking to deliver it " \
+            "later, so you may have made local changes that would be " \
+            "silently lost if we enqueued a job to deliver it. Why? Only " \
+            "the mailer method *arguments* are passed with the delivery job! " \
+            "Do not access the message in any way if you mean to deliver it " \
+            "later. Workarounds: 1. don't touch the message before calling " \
+            "#deliver_later, 2. only touch the message *within your mailer " \
+            "method*, or 3. use a custom Active Job instead of #deliver_later."
+        else
+          args = @mailer.name, @mail_method.to_s, delivery_method.to_s, *@args
+          ActionMailer::DeliveryJob.set(options).perform_later(*args)
+        end
       end
   end
 end

actionmailer/test/message_delivery_test.rb

@@ -93,4 +93,12 @@ class MessageDeliveryTest < ActiveSupport::TestCase
       @mail.deliver_later(queue: :another_queue)
     end
   end
+
+  test 'deliver_later after accessing the message is disallowed' do
+    @mail.message # Load the message, which calls the mailer method.
+
+    assert_raise RuntimeError do
+      @mail.deliver_later
+    end
+  end
 end