aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorGeorge Claghorn <george.claghorn@gmail.com>2019-01-17 12:58:12 -0600
committerGitHub <noreply@github.com>2019-01-17 12:58:12 -0600
commit3d7391cb76758c82c8ea7759b6dd8d4402bc2346 (patch)
tree3fa86d268c194afa7e58387731392dc05be14ee3
parent69c963c2e8ac56f4832e767c3c83091b9c6f24dd (diff)
parent5cd733a334846669d6435517f7d9913c4a9b1eb1 (diff)
downloadrails-3d7391cb76758c82c8ea7759b6dd8d4402bc2346.tar.gz
rails-3d7391cb76758c82c8ea7759b6dd8d4402bc2346.tar.bz2
rails-3d7391cb76758c82c8ea7759b6dd8d4402bc2346.zip
Merge pull request #34962 from lifo/am-handle-duplicate-emails
Ensure Action Mailbox processes an email only once
-rw-r--r--actionmailbox/app/models/action_mailbox/inbound_email/message_id.rb22
-rw-r--r--actionmailbox/db/migrate/20180917164000_create_action_mailbox_tables.rb5
-rw-r--r--actionmailbox/test/dummy/db/migrate/20180208205311_create_action_mailroom_tables.rb5
-rw-r--r--actionmailbox/test/dummy/db/schema.rb2
-rw-r--r--actionmailbox/test/unit/inbound_email_test.rb22
-rw-r--r--actionmailbox/test/unit/mailbox/routing_test.rb3
6 files changed, 44 insertions, 15 deletions
diff --git a/actionmailbox/app/models/action_mailbox/inbound_email/message_id.rb b/actionmailbox/app/models/action_mailbox/inbound_email/message_id.rb
index 57b4a2445d..470b93ca20 100644
--- a/actionmailbox/app/models/action_mailbox/inbound_email/message_id.rb
+++ b/actionmailbox/app/models/action_mailbox/inbound_email/message_id.rb
@@ -9,30 +9,30 @@
module ActionMailbox::InboundEmail::MessageId
extend ActiveSupport::Concern
- included do
- before_save :generate_missing_message_id
- end
-
class_methods do
# Create a new +InboundEmail+ from the raw +source+ of the email, which be uploaded as a Active Storage
# attachment called +raw_email+. Before the upload, extract the Message-ID from the +source+ and set
# it as an attribute on the new +InboundEmail+.
def create_and_extract_message_id!(source, **options)
- create! options.merge(message_id: extract_message_id(source)) do |inbound_email|
+ message_checksum = Digest::SHA1.hexdigest(source)
+ message_id = extract_message_id(source) || generate_missing_message_id(message_checksum)
+
+ create! options.merge(message_id: message_id, message_checksum: message_checksum) do |inbound_email|
inbound_email.raw_email.attach io: StringIO.new(source), filename: "message.eml", content_type: "message/rfc822"
end
+ rescue ActiveRecord::RecordNotUnique
+ nil
end
private
def extract_message_id(source)
Mail.from_source(source).message_id rescue nil
end
- end
- private
- def generate_missing_message_id
- self.message_id ||= Mail::MessageIdField.new.message_id.tap do |message_id|
- logger.warn "Message-ID couldn't be parsed or is missing. Generated a new Message-ID: #{message_id}"
+ def generate_missing_message_id(message_checksum)
+ Mail::MessageIdField.new("<#{message_checksum}@#{::Socket.gethostname}.mail>").message_id.tap do |message_id|
+ logger.warn "Message-ID couldn't be parsed or is missing. Generated a new Message-ID: #{message_id}"
+ end
end
- end
+ end
end
diff --git a/actionmailbox/db/migrate/20180917164000_create_action_mailbox_tables.rb b/actionmailbox/db/migrate/20180917164000_create_action_mailbox_tables.rb
index 8cf621d7e3..e697748f82 100644
--- a/actionmailbox/db/migrate/20180917164000_create_action_mailbox_tables.rb
+++ b/actionmailbox/db/migrate/20180917164000_create_action_mailbox_tables.rb
@@ -2,10 +2,13 @@ class CreateActionMailboxTables < ActiveRecord::Migration[6.0]
def change
create_table :action_mailbox_inbound_emails do |t|
t.integer :status, default: 0, null: false
- t.string :message_id
+ t.string :message_id, null: false
+ t.string :message_checksum, null: false
t.datetime :created_at, precision: 6, null: false
t.datetime :updated_at, precision: 6, null: false
+
+ t.index [ :message_id, :message_checksum ], unique: true
end
end
end
diff --git a/actionmailbox/test/dummy/db/migrate/20180208205311_create_action_mailroom_tables.rb b/actionmailbox/test/dummy/db/migrate/20180208205311_create_action_mailroom_tables.rb
index 0124b4b98f..f587f37095 100644
--- a/actionmailbox/test/dummy/db/migrate/20180208205311_create_action_mailroom_tables.rb
+++ b/actionmailbox/test/dummy/db/migrate/20180208205311_create_action_mailroom_tables.rb
@@ -2,10 +2,13 @@ class CreateActionMailboxTables < ActiveRecord::Migration[6.0]
def change
create_table :action_mailbox_inbound_emails do |t|
t.integer :status, default: 0, null: false
- t.string :message_id
+ t.string :message_id, null: false
+ t.string :message_checksum, null: false
t.datetime :created_at, precision: 6
t.datetime :updated_at, precision: 6
+
+ t.index [ :message_id, :message_checksum ], unique: true
end
end
end
diff --git a/actionmailbox/test/dummy/db/schema.rb b/actionmailbox/test/dummy/db/schema.rb
index 6cfe7de765..4aba92bfc6 100644
--- a/actionmailbox/test/dummy/db/schema.rb
+++ b/actionmailbox/test/dummy/db/schema.rb
@@ -15,8 +15,10 @@ ActiveRecord::Schema.define(version: 2018_02_12_164506) do
create_table "action_mailbox_inbound_emails", force: :cascade do |t|
t.integer "status", default: 0, null: false
t.string "message_id"
+ t.string "message_checksum"
t.datetime "created_at", precision: 6
t.datetime "updated_at", precision: 6
+ t.index ["message_id", "message_checksum"], name: "index_action_mailbox_inbound_emails_uniqueness", unique: true
end
create_table "active_storage_attachments", force: :cascade do |t|
diff --git a/actionmailbox/test/unit/inbound_email_test.rb b/actionmailbox/test/unit/inbound_email_test.rb
index 993423406f..76f047eb61 100644
--- a/actionmailbox/test/unit/inbound_email_test.rb
+++ b/actionmailbox/test/unit/inbound_email_test.rb
@@ -11,5 +11,27 @@ module ActionMailbox
test "source returns the contents of the raw email" do
assert_equal file_fixture("welcome.eml").read, create_inbound_email_from_fixture("welcome.eml").source
end
+
+ test "email with message id is processed only once when received multiple times" do
+ mail = Mail.from_source(file_fixture("welcome.eml").read)
+ assert mail.message_id
+
+ inbound_email_1 = create_inbound_email_from_source(mail.to_s)
+ assert inbound_email_1
+
+ inbound_email_2 = create_inbound_email_from_source(mail.to_s)
+ assert_nil inbound_email_2
+ end
+
+ test "email with missing message id is processed only once when received multiple times" do
+ mail = Mail.from_source("Date: Fri, 28 Sep 2018 11:08:55 -0700\r\nTo: a@example.com\r\nMime-Version: 1.0\r\nContent-Type: text/plain\r\nContent-Transfer-Encoding: 7bit\r\n\r\nHello!")
+ assert_nil mail.message_id
+
+ inbound_email_1 = create_inbound_email_from_source(mail.to_s)
+ assert inbound_email_1
+
+ inbound_email_2 = create_inbound_email_from_source(mail.to_s)
+ assert_nil inbound_email_2
+ end
end
end
diff --git a/actionmailbox/test/unit/mailbox/routing_test.rb b/actionmailbox/test/unit/mailbox/routing_test.rb
index d4dad7eafb..d4ba702ac5 100644
--- a/actionmailbox/test/unit/mailbox/routing_test.rb
+++ b/actionmailbox/test/unit/mailbox/routing_test.rb
@@ -15,11 +15,10 @@ end
class ActionMailbox::Base::RoutingTest < ActiveSupport::TestCase
setup do
$processed = false
- @inbound_email = create_inbound_email_from_fixture("welcome.eml")
end
test "string routing" do
- ApplicationMailbox.route @inbound_email
+ ApplicationMailbox.route create_inbound_email_from_fixture("welcome.eml")
assert_equal "Discussion: Let's debate these attachments", $processed
end