PERF: Move `EmailLog#reply_key` into new `post_reply_keys` table.

This commit is contained in:
Guo Xiang Tan 2018-07-18 16:28:44 +08:00
parent ae8b0a517f
commit fad9c2b971
17 changed files with 320 additions and 100 deletions

View File

@ -18,8 +18,42 @@ class Admin::EmailController < Admin::AdminController
end
def sent
email_logs = filter_logs(EmailLog, params)
render_serialized(email_logs, EmailLogSerializer)
email_logs = EmailLog.sent
.joins("
LEFT JOIN post_reply_keys
ON post_reply_keys.post_id = email_logs.post_id
AND post_reply_keys.user_id = email_logs.user_id
")
email_logs = filter_logs(email_logs, params)
if params[:reply_key].present?
email_logs = email_logs.where(
"post_reply_keys.reply_key ILIKE ?", "%#{params[:reply_key]}%"
)
end
email_logs = email_logs.to_a
tuples = email_logs.map do |email_log|
[email_log.post_id, email_log.user_id]
end
reply_keys = {}
if tuples.present?
PostReplyKey
.where(
"(post_id,user_id) IN (#{(['(?)'] * tuples.size).join(', ')})",
*tuples
)
.pluck(:post_id, :user_id, "reply_key::text")
.each do |post_id, user_id, reply_key|
reply_keys[[post_id, user_id]] = reply_key
end
end
render_serialized(email_logs, EmailLogSerializer, reply_keys: reply_keys)
end
def skipped
@ -149,7 +183,6 @@ class Admin::EmailController < Admin::AdminController
logs = logs.where("users.username ILIKE ?", "%#{params[:user]}%") if params[:user].present?
logs = logs.where("#{table_name}.to_address ILIKE ?", "%#{params[:address]}%") if params[:address].present?
logs = logs.where("#{table_name}.email_type ILIKE ?", "%#{params[:type]}%") if params[:type].present?
logs = logs.where("#{table_name}.reply_key ILIKE ?", "%#{params[:reply_key]}%") if params[:reply_key].present?
logs
end

View File

@ -8,7 +8,7 @@ module Jobs
threshold = SiteSetting.delete_email_logs_after_days.days.ago
EmailLog.where(reply_key: nil)
EmailLog.where("reply_key IS NULL")
.where("created_at < ?", threshold)
.delete_all

View File

@ -1,7 +1,10 @@
require_dependency 'distributed_mutex'
class EmailLog < ActiveRecord::Base
self.ignored_columns = %w{topic_id}
self.ignored_columns = %w{
topic_id
reply_key
}
CRITICAL_EMAIL_TYPES ||= Set.new %w{
account_created
@ -74,10 +77,6 @@ class EmailLog < ActiveRecord::Base
super&.delete('-')
end
def reply_key
super&.delete('-')
end
end
# == Schema Information

View File

@ -0,0 +1,18 @@
class PostReplyKey < ActiveRecord::Base
belongs_to :post
belongs_to :user
before_validation { self.reply_key ||= self.class.generate_reply_key }
validates :post_id, presence: true, uniqueness: { scope: :user_id }
validates :user_id, presence: true
validates :reply_key, presence: true
def reply_key
super&.delete('-')
end
def self.generate_reply_key
SecureRandom.hex(16)
end
end

View File

@ -5,4 +5,13 @@ class EmailLogSerializer < ApplicationSerializer
:bounced
has_one :user, serializer: BasicUserSerializer, embed: :objects
def include_reply_key?
reply_keys = @options[:reply_keys]
reply_keys.present? && reply_keys[[object.post_id, object.user_id]]
end
def reply_key
@options[:reply_keys][[object.post_id, object.user_id]].delete("-")
end
end

View File

@ -0,0 +1,53 @@
require 'migration/column_dropper'
class CreatePostReplyKeys < ActiveRecord::Migration[5.2]
def up
create_table :post_reply_keys do |t|
t.integer :user_id, null: false
t.integer :post_id, null: false
t.uuid :reply_key, null: false
t.timestamps null: false
end
add_index :post_reply_keys, :reply_key, unique: true
Migration::ColumnDropper.mark_readonly(:email_logs, :reply_key)
sql = <<~SQL
DELETE FROM email_logs
WHERE id IN (
SELECT id
FROM (
SELECT
id,
ROW_NUMBER() OVER(PARTITION BY post_id, user_id ORDER BY id DESC) AS row_num
FROM email_logs
) t
WHERE t.row_num > 1
)
SQL
execute(sql)
sql = <<~SQL
INSERT INTO post_reply_keys(
user_id, post_id, reply_key, updated_at, created_at
) SELECT
user_id,
post_id,
reply_key,
updated_at,
created_at
FROM email_logs
WHERE reply_key IS NOT NULL
SQL
execute(sql)
add_index :post_reply_keys, [:user_id, :post_id], unique: true
end
def down
raise ActiveRecord::IrreversibleMigration
end
end

View File

@ -17,6 +17,8 @@ module Email
class MessageBuilder
attr_reader :template_args
ALLOW_REPLY_BY_EMAIL_HEADER = 'X-Discourse-Allow-Reply-By-Email'.freeze
def initialize(to, opts = nil)
@to = to
@opts = opts || {}
@ -147,7 +149,7 @@ module Email
result['X-Auto-Response-Suppress'] = 'All'
if allow_reply_by_email?
result['X-Discourse-Reply-Key'] = reply_key
result[ALLOW_REPLY_BY_EMAIL_HEADER] = true
result['Reply-To'] = reply_by_email_address
else
result['Reply-To'] = from_value
@ -171,10 +173,6 @@ module Email
protected
def reply_key
@reply_key ||= SecureRandom.hex(16)
end
def allow_reply_by_email?
SiteSetting.reply_by_email_enabled? &&
reply_by_email_address.present? &&
@ -196,7 +194,6 @@ module Email
return nil unless SiteSetting.reply_by_email_address.present?
@reply_by_email_address = SiteSetting.reply_by_email_address.dup
@reply_by_email_address.gsub!("%{reply_key}", reply_key)
@reply_by_email_address =
if private_reply?

View File

@ -548,8 +548,8 @@ module Email
if match && match.captures
match.captures.each do |c|
next if c.blank?
email_log = EmailLog.for(c)
return { type: :reply, obj: email_log } if email_log
post_reply_key = PostReplyKey.find_by(reply_key: c)
return { type: :reply, obj: post_reply_key } if post_reply_key
end
end
nil
@ -580,18 +580,18 @@ module Email
skip_validations: user.staged?)
when :reply
email_log = destination[:obj]
post_reply_key = destination[:obj]
if email_log.user_id != user.id && !forwarded_reply_key?(email_log, user)
raise ReplyUserNotMatchingError, "email_log.user_id => #{email_log.user_id.inspect}, user.id => #{user.id.inspect}"
if post_reply_key.user_id != user.id && !forwarded_reply_key?(post_reply_key, user)
raise ReplyUserNotMatchingError, "post_reply_key.user_id => #{post_reply_key.user_id.inspect}, user.id => #{user.id.inspect}"
end
create_reply(user: user,
raw: body,
elided: elided,
hidden_reason_id: hidden_reason_id,
post: email_log.post,
topic: email_log.post.topic,
post: post_reply_key.post,
topic: post_reply_key.post.topic,
skip_validations: user.staged?)
end
end
@ -631,11 +631,11 @@ module Email
end
end
def forwarded_reply_key?(email_log, user)
def forwarded_reply_key?(post_reply_key, user)
incoming_emails = IncomingEmail
.joins(:post)
.where('posts.topic_id = ?', email_log.topic.id)
.addressed_to(email_log.reply_key)
.where('posts.topic_id = ?', post_reply_key.post.topic_id)
.addressed_to(post_reply_key.reply_key)
.addressed_to_user(user)
.pluck(:to_addresses, :cc_addresses)
@ -643,8 +643,8 @@ module Email
next unless contains_email_address_of_user?(to_addresses, user) ||
contains_email_address_of_user?(cc_addresses, user)
return true if contains_reply_by_email_address(to_addresses, email_log.reply_key) ||
contains_reply_by_email_address(cc_addresses, email_log.reply_key)
return true if contains_reply_by_email_address(to_addresses, post_reply_key.reply_key) ||
contains_reply_by_email_address(cc_addresses, post_reply_key.reply_key)
end
false

View File

@ -31,8 +31,7 @@ module Email
return skip(SkippedEmailLog.reason_types[:sender_message_to_blank]) if @message.to.blank?
if SiteSetting.disable_emails == "non-staff"
user = User.find_by_email(to_address)
return unless user && user.staff?
return unless User.find_by_email(to_address)&.staff?
end
if @message.text_part
@ -68,15 +67,20 @@ module Email
@message.parts[0].body = @message.parts[0].body.to_s.gsub(/<img src="(\/uploads\/default\/[^"]+)"([^>]*)>/, '![](' + url_prefix + '\1)')
@message.text_part.content_type = 'text/plain; charset=UTF-8'
user_id = @user&.id
# Set up the email log
email_log = EmailLog.new(email_type: @email_type, to_address: to_address, user_id: @user.try(:id))
email_log = EmailLog.new(
email_type: @email_type,
to_address: to_address,
user_id: user_id
)
host = Email::Sender.host_for(Discourse.base_url)
post_id = header_value('X-Discourse-Post-Id')
topic_id = header_value('X-Discourse-Topic-Id')
reply_key = header_value('X-Discourse-Reply-Key')
reply_key = set_reply_key(post_id, user_id)
# always set a default Message ID from the host
@message.header['Message-ID'] = "<#{SecureRandom.uuid}@#{host}>"
@ -160,12 +164,14 @@ module Email
end
email_log.post_id = post_id if post_id.present?
email_log.reply_key = reply_key if reply_key.present?
# Remove headers we don't need anymore
@message.header['X-Discourse-Topic-Id'] = nil if topic_id.present?
@message.header['X-Discourse-Post-Id'] = nil if post_id.present?
@message.header['X-Discourse-Reply-Key'] = nil if reply_key.present?
if reply_key.present?
@message.header[Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER] = nil
end
# pass the original message_id when using mailjet/mandrill/sparkpost
case ActionMailer::Base.smtp_settings[:address]
@ -251,5 +257,19 @@ module Email
@message.header[name] = data.to_json
end
def set_reply_key(post_id, user_id)
return unless user_id &&
post_id &&
header_value(Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER).present?
reply_key = PostReplyKey.find_or_create_by!(
post_id: post_id,
user_id: user_id
).reply_key
@message.header['Reply-To'] =
header_value('Reply-To').gsub!("%{reply_key}", reply_key)
end
end
end

View File

@ -9,6 +9,7 @@ describe Email::MessageBuilder do
let(:builder) { Email::MessageBuilder.new(to_address, subject: subject, body: body) }
let(:build_args) { builder.build_args }
let(:header_args) { builder.header_args }
let(:allow_reply_header) { described_class::ALLOW_REPLY_BY_EMAIL_HEADER }
it "has the correct to address" do
expect(build_args[:to]).to eq(to_address)
@ -44,7 +45,6 @@ describe Email::MessageBuilder do
context "with allow_reply_by_email" do
let(:reply_by_email_builder) { Email::MessageBuilder.new(to_address, allow_reply_by_email: true) }
let(:reply_key) { reply_by_email_builder.header_args['X-Discourse-Reply-Key'] }
context "With the SiteSetting enabled" do
before do
@ -52,18 +52,22 @@ describe Email::MessageBuilder do
SiteSetting.stubs(:reply_by_email_address).returns("r+%{reply_key}@reply.myforum.com")
end
it "has a X-Discourse-Reply-Key" do
expect(reply_key).to be_present
expect(reply_key.size).to eq(32)
end
it "returns a Reply-To header with the reply key" do
expect(reply_by_email_builder.header_args['Reply-To']).to eq("\"#{SiteSetting.title}\" <r+#{reply_key}@reply.myforum.com>")
expect(reply_by_email_builder.header_args['Reply-To'])
.to eq("\"#{SiteSetting.title}\" <r+%{reply_key}@reply.myforum.com>")
expect(reply_by_email_builder.header_args[allow_reply_header])
.to eq(true)
end
it "cleans up the site title" do
SiteSetting.stubs(:title).returns(">>>Obnoxious Title: Deal, \"With\" It<<<")
expect(reply_by_email_builder.header_args['Reply-To']).to eq("\"Obnoxious Title Deal With It\" <r+#{reply_key}@reply.myforum.com>")
expect(reply_by_email_builder.header_args['Reply-To'])
.to eq("\"Obnoxious Title Deal With It\" <r+%{reply_key}@reply.myforum.com>")
expect(reply_by_email_builder.header_args[allow_reply_header])
.to eq(true)
end
end
@ -72,33 +76,39 @@ describe Email::MessageBuilder do
SiteSetting.stubs(:reply_by_email_enabled?).returns(false)
end
it "has no X-Discourse-Reply-Key" do
expect(reply_key).to be_blank
end
it "returns a Reply-To header that's the same as From" do
expect(header_args['Reply-To']).to eq(build_args[:from])
expect(reply_by_email_builder.header_args['Reply-To'])
.to eq(reply_by_email_builder.build_args[:from])
expect(reply_by_email_builder.header_args[allow_reply_header])
.to eq(nil)
end
end
end
context "with allow_reply_by_email" do
let(:reply_by_email_builder) { Email::MessageBuilder.new(to_address, allow_reply_by_email: true, private_reply: true, from_alias: "Username") }
let(:reply_key) { reply_by_email_builder.header_args['X-Discourse-Reply-Key'] }
let(:reply_by_email_builder) do
Email::MessageBuilder.new(to_address,
allow_reply_by_email: true,
private_reply: true,
from_alias: "Username"
)
end
context "With the SiteSetting enabled" do
before do
SiteSetting.stubs(:reply_by_email_enabled?).returns(true)
SiteSetting.stubs(:reply_by_email_address).returns("r+%{reply_key}@reply.myforum.com")
end
it "has a X-Discourse-Reply-Key" do
expect(reply_key).to be_present
expect(reply_key.size).to eq(32)
SiteSetting.stubs(:reply_by_email_address)
.returns("r+%{reply_key}@reply.myforum.com")
end
it "returns a Reply-To header with the reply key" do
expect(reply_by_email_builder.header_args['Reply-To']).to eq("\"Username\" <r+#{reply_key}@reply.myforum.com>")
expect(reply_by_email_builder.header_args['Reply-To'])
.to eq("\"Username\" <r+%{reply_key}@reply.myforum.com>")
expect(reply_by_email_builder.header_args[allow_reply_header])
.to eq(true)
end
end
@ -107,12 +117,12 @@ describe Email::MessageBuilder do
SiteSetting.stubs(:reply_by_email_enabled?).returns(false)
end
it "has no X-Discourse-Reply-Key" do
expect(reply_key).to be_blank
end
it "returns a Reply-To header that's the same as From" do
expect(header_args['Reply-To']).to eq(build_args[:from])
expect(reply_by_email_builder.header_args['Reply-To'])
.to eq(reply_by_email_builder.build_args[:from])
expect(reply_by_email_builder.header_args[allow_reply_header])
.to eq(nil)
end
end
end

View File

@ -63,12 +63,12 @@ describe Email::Receiver do
it "doesn't raise an InactiveUserError when the sender is staged" do
user = Fabricate(:user, email: "staged@bar.com", active: false, staged: true)
post = Fabricate(:post)
email_log = Fabricate(:email_log,
to_address: 'reply+aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa@bar.com',
reply_key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa',
post_reply_key = Fabricate(:post_reply_key,
user: user,
post: Fabricate(:post)
post: post,
reply_key: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
)
expect { process(:staged_sender) }.not_to raise_error
@ -153,7 +153,14 @@ describe Email::Receiver do
let(:user) { Fabricate(:user, email: "discourse@bar.com") }
let(:topic) { create_topic(category: category, user: user) }
let(:post) { create_post(topic: topic, user: user) }
let!(:email_log) { Fabricate(:email_log, reply_key: reply_key, user: user, topic: topic, post: post) }
let!(:post_reply_key) do
Fabricate(:post_reply_key,
reply_key: reply_key,
user: user,
post: post
)
end
it "uses MD5 of 'mail_string' there is no message_id" do
mail_string = email(:missing_message_id)
@ -814,12 +821,15 @@ describe Email::Receiver do
context "with a valid reply" do
it "returns the destination when the key is valid" do
Fabricate(:email_log, reply_key: '4f97315cc828096c9cb34c6f1a0d6fe8')
post_reply_key = Fabricate(:post_reply_key,
reply_key: '4f97315cc828096c9cb34c6f1a0d6fe8'
)
dest = Email::Receiver.check_address('foo+4f97315cc828096c9cb34c6f1a0d6fe8@bar.com')
expect(dest).to be_present
expect(dest[:type]).to eq(:reply)
expect(dest[:obj]).to be_present
expect(dest[:obj]).to eq(post_reply_key)
end
end
end
@ -925,7 +935,10 @@ describe Email::Receiver do
let(:user) { Fabricate(:user, email: "discourse@bar.com") }
let(:topic) { create_topic(category: category, user: user) }
let(:post) { create_post(topic: topic, user: user) }
let!(:email_log) { Fabricate(:email_log, reply_key: reply_key, user: user, topic: topic, post: post) }
let!(:post_reply_key) do
Fabricate(:post_reply_key, reply_key: reply_key, user: user, post: post)
end
context "when the email address isn't matching the one we sent the notification to" do
include_examples "no staged users", :reply_user_not_matching, Email::Receiver::ReplyUserNotMatchingError

View File

@ -2,6 +2,7 @@ require 'rails_helper'
require 'email/sender'
describe Email::Sender do
let(:post) { Fabricate(:post) }
context "disable_emails is enabled" do
let(:user) { Fabricate(:user) }
@ -292,19 +293,19 @@ describe Email::Sender do
let(:email_log) { EmailLog.last }
it 'should create the right log' do
email_sender.send
expect do
email_sender.send
end.to_not change { PostReplyKey.count }
expect(email_log).to be_present
expect(email_log.email_type).to eq('valid_type')
expect(email_log.to_address).to eq('eviltrout@test.domain')
expect(email_log.reply_key).to be_blank
expect(email_log.user_id).to be_blank
end
end
context "email log with a post id and topic id" do
let(:topic) { Fabricate(:topic) }
let(:post) { Fabricate(:post, topic: topic) }
let(:topic) { post.topic }
before do
message.header['X-Discourse-Post-Id'] = post.id
@ -320,19 +321,6 @@ describe Email::Sender do
end
end
context "email log with a reply key" do
before do
message.header['X-Discourse-Reply-Key'] = reply_key
end
let(:email_log) { EmailLog.last }
it 'should create the right log' do
email_sender.send
expect(email_log.reply_key).to eq(reply_key)
end
end
context 'email parts' do
it 'should contain the right message' do
email_sender.send
@ -364,6 +352,42 @@ describe Email::Sender do
expect(@email_log.user_id).to eq(user.id)
end
describe "post reply keys" do
let(:post) { Fabricate(:post) }
before do
message.header['X-Discourse-Post-Id'] = post.id
message.header['Reply-To'] = "test-%{reply_key}@test.com"
end
describe 'when allow reply by email header is not present' do
it 'should not create a post reply key' do
expect { email_sender.send }.to_not change { PostReplyKey.count }
end
end
describe 'when allow reply by email header is present' do
let(:header) { Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER }
before do
message.header[header] = "test-%{reply_key}@test.com"
end
it 'should create a post reply key' do
expect { email_sender.send }.to change { PostReplyKey.count }.by(1)
post_reply_key = PostReplyKey.last
expect(message.header['Reply-To'].value).to eq(
"test-#{post_reply_key.reply_key}@test.com"
)
expect(message.header[header]).to eq(nil)
expect(post_reply_key.user_id).to eq(user.id)
expect(post_reply_key.post_id).to eq(post.id)
expect { email_sender.send }.to change { PostReplyKey.count }.by(0)
end
end
end
end
end

View File

@ -0,0 +1,5 @@
Fabricator(:post_reply_key) do
user
post
reply_key { PostReplyKey.generate_reply_key }
end

View File

@ -3,7 +3,6 @@ require 'rails_helper'
describe Jobs::CleanUpEmailLogs do
before do
Fabricate(:email_log, created_at: 2.years.ago, reply_key: SecureRandom.hex)
Fabricate(:email_log, created_at: 2.years.ago)
Fabricate(:email_log, created_at: 2.weeks.ago)
Fabricate(:email_log, created_at: 2.days.ago)
@ -14,14 +13,14 @@ describe Jobs::CleanUpEmailLogs do
it "removes old email logs without a reply_key" do
Jobs::CleanUpEmailLogs.new.execute({})
expect(EmailLog.count).to eq(3)
expect(EmailLog.count).to eq(2)
expect(SkippedEmailLog.count).to eq(1)
end
it "does not remove old email logs when delete_email_logs_after_days is 0" do
SiteSetting.delete_email_logs_after_days = 0
Jobs::CleanUpEmailLogs.new.execute({})
expect(EmailLog.count).to eq(4)
expect(EmailLog.count).to eq(3)
expect(SkippedEmailLog.count).to eq(2)
end

View File

@ -101,20 +101,18 @@ describe EmailLog do
end
end
%w{reply_key bounce_key}.each do |key|
describe "##{key}" do
it "should format the #{key} correctly" do
hex = SecureRandom.hex
email_log = Fabricate(:email_log, user: user, "#{key}": hex)
describe "#bounce_key" do
it "should format the bounce_key correctly" do
hex = SecureRandom.hex
email_log = Fabricate(:email_log, user: user, bounce_key: hex)
raw_key = EmailLog.where(id: email_log.id)
.pluck("#{key}::text")
.first
raw_key = EmailLog.where(id: email_log.id)
.pluck("bounce_key::text")
.first
expect(raw_key).to_not eq(hex)
expect(raw_key.delete('-')).to eq(hex)
expect(EmailLog.find(email_log.id).send(key)).to eq(hex)
end
expect(raw_key).to_not eq(hex)
expect(raw_key.delete('-')).to eq(hex)
expect(EmailLog.find(email_log.id).bounce_key).to eq(hex)
end
end
end

View File

@ -0,0 +1,20 @@
require 'rails_helper'
RSpec.describe PostReplyKey do
describe "#reply_key" do
it "should format the reply_key correctly" do
hex = SecureRandom.hex
post_reply_key = Fabricate(:post_reply_key,
reply_key: hex
)
raw_key = PostReplyKey.where(id: post_reply_key.id)
.pluck("reply_key::text")
.first
expect(raw_key).to_not eq(hex)
expect(raw_key.delete('-')).to eq(hex)
expect(PostReplyKey.find(post_reply_key.id).reply_key).to eq(hex)
end
end
end

View File

@ -2,6 +2,7 @@ require 'rails_helper'
describe Admin::EmailController do
let(:admin) { Fabricate(:admin) }
let(:email_log) { Fabricate(:email_log) }
before do
sign_in(admin)
@ -32,9 +33,30 @@ describe Admin::EmailController do
end
describe '#sent' do
it "succeeds" do
let(:post) { Fabricate(:post) }
let(:email_log) { Fabricate(:email_log, post: post) }
let(:post_reply_key) do
Fabricate(:post_reply_key, post: post, user: email_log.user)
end
it "should return the right response" do
email_log
get "/admin/email/sent.json"
expect(response.status).to eq(200)
log = JSON.parse(response.body).first
expect(log["id"]).to eq(email_log.id)
expect(log["reply_key"]).to eq(nil)
post_reply_key
get "/admin/email/sent.json"
expect(response.status).to eq(200)
log = JSON.parse(response.body).first
expect(log["id"]).to eq(email_log.id)
expect(log["reply_key"]).to eq(post_reply_key.reply_key)
end
end