Commit 5f27e26b authored by Sean McGivern's avatar Sean McGivern

Only generate repository push email once

The repository push email can be very expensive to generate, especially
with syntax-highlighted diffs. Instead of generating the email for each
recipient, generate one email object and reset the Message-Id and To
headers for each recipient. (Cloning would also be expensive in the case
of large emails, although probably not as bad as generating from
scratch.)
parent 48c80fdf
...@@ -21,6 +21,7 @@ v 8.8.0 (unreleased) ...@@ -21,6 +21,7 @@ v 8.8.0 (unreleased)
- Fix error when visiting commit builds page before build was updated - Fix error when visiting commit builds page before build was updated
- Add 'l' shortcut to open Label dropdown on issuables and 'i' to create new issue on a project - Add 'l' shortcut to open Label dropdown on issuables and 'i' to create new issue on a project
- Update SVG sanitizer to conform to SVG 1.1 - Update SVG sanitizer to conform to SVG 1.1
- Speed up push emails with multiple recipients by only generating the email once
- Updated search UI - Updated search UI
- Display informative message when new milestone is created - Display informative message when new milestone is created
- Sanitize milestones and labels titles - Sanitize milestones and labels titles
......
...@@ -59,9 +59,9 @@ module Emails ...@@ -59,9 +59,9 @@ module Emails
subject: subject("Project was moved")) subject: subject("Project was moved"))
end end
def repository_push_email(project_id, recipient, opts = {}) def repository_push_email(project_id, opts = {})
@message = @message =
Gitlab::Email::Message::RepositoryPush.new(self, project_id, recipient, opts) Gitlab::Email::Message::RepositoryPush.new(self, project_id, opts)
# used in notify layout # used in notify layout
@target_url = @message.target_url @target_url = @message.target_url
...@@ -72,7 +72,6 @@ module Emails ...@@ -72,7 +72,6 @@ module Emails
mail(from: sender(@message.author_id, @message.send_from_committer_email?), mail(from: sender(@message.author_id, @message.send_from_committer_email?),
reply_to: @message.reply_to, reply_to: @message.reply_to,
to: @message.recipient,
subject: @message.subject) subject: @message.subject)
end end
end end
......
class EmailsOnPushWorker class EmailsOnPushWorker
include Sidekiq::Worker include Sidekiq::Worker
attr_reader :email, :skip_premailer
def perform(project_id, recipients, push_data, options = {}) def perform(project_id, recipients, push_data, options = {})
options.symbolize_keys! options.symbolize_keys!
options.reverse_merge!( options.reverse_merge!(
...@@ -41,11 +43,11 @@ class EmailsOnPushWorker ...@@ -41,11 +43,11 @@ class EmailsOnPushWorker
end end
end end
recipients.split(" ").each do |recipient| recipients.split.each do |recipient|
begin begin
Notify.repository_push_email( send_email(
project_id,
recipient, recipient,
project_id,
author_id: author_id, author_id: author_id,
ref: ref, ref: ref,
action: action, action: action,
...@@ -53,14 +55,29 @@ class EmailsOnPushWorker ...@@ -53,14 +55,29 @@ class EmailsOnPushWorker
reverse_compare: reverse_compare, reverse_compare: reverse_compare,
send_from_committer_email: send_from_committer_email, send_from_committer_email: send_from_committer_email,
disable_diffs: disable_diffs disable_diffs: disable_diffs
).deliver_now )
# These are input errors and won't be corrected even if Sidekiq retries # These are input errors and won't be corrected even if Sidekiq retries
rescue Net::SMTPFatalError, Net::SMTPSyntaxError => e rescue Net::SMTPFatalError, Net::SMTPSyntaxError => e
logger.info("Failed to send e-mail for project '#{project.name_with_namespace}' to #{recipient}: #{e}") logger.info("Failed to send e-mail for project '#{project.name_with_namespace}' to #{recipient}: #{e}")
end end
end end
ensure ensure
@email = nil
compare = nil compare = nil
GC.start GC.start
end end
private
def send_email(recipient, project_id, options)
# Generating the body of this email can be expensive, so only do it once
@skip_premailer ||= email.present?
@email ||= Notify.repository_push_email(project_id, options)
email.to = recipient
email.add_message_id
email.header[:skip_premailer] = true if skip_premailer
email.deliver_now
end
end end
...@@ -2,7 +2,6 @@ module Gitlab ...@@ -2,7 +2,6 @@ module Gitlab
module Email module Email
module Message module Message
class RepositoryPush class RepositoryPush
attr_accessor :recipient
attr_reader :author_id, :ref, :action attr_reader :author_id, :ref, :action
include Gitlab::Routing.url_helpers include Gitlab::Routing.url_helpers
...@@ -11,13 +10,12 @@ module Gitlab ...@@ -11,13 +10,12 @@ module Gitlab
delegate :name, to: :author, prefix: :author delegate :name, to: :author, prefix: :author
delegate :username, to: :author, prefix: :author delegate :username, to: :author, prefix: :author
def initialize(notify, project_id, recipient, opts = {}) def initialize(notify, project_id, opts = {})
raise ArgumentError, 'Missing options: author_id, ref, action' unless raise ArgumentError, 'Missing options: author_id, ref, action' unless
opts[:author_id] && opts[:ref] && opts[:action] opts[:author_id] && opts[:ref] && opts[:action]
@notify = notify @notify = notify
@project_id = project_id @project_id = project_id
@recipient = recipient
@opts = opts.dup @opts = opts.dup
@author_id = @opts.delete(:author_id) @author_id = @opts.delete(:author_id)
......
...@@ -8,7 +8,7 @@ describe Gitlab::Email::Message::RepositoryPush do ...@@ -8,7 +8,7 @@ describe Gitlab::Email::Message::RepositoryPush do
let!(:author) { create(:author, name: 'Author') } let!(:author) { create(:author, name: 'Author') }
let(:message) do let(:message) do
described_class.new(Notify, project.id, 'recipient@example.com', opts) described_class.new(Notify, project.id, opts)
end end
context 'new commits have been pushed to repository' do context 'new commits have been pushed to repository' do
......
...@@ -593,7 +593,7 @@ describe Notify do ...@@ -593,7 +593,7 @@ describe Notify do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:tree_path) { namespace_project_tree_path(project.namespace, project, "master") } let(:tree_path) { namespace_project_tree_path(project.namespace, project, "master") }
subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :create) } subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :create) }
it_behaves_like 'it should not have Gmail Actions links' it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link" it_behaves_like "a user cannot unsubscribe through footer link"
...@@ -606,10 +606,6 @@ describe Notify do ...@@ -606,10 +606,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender) expect(sender.address).to eq(gitlab_sender)
end end
it 'is sent to recipient' do
is_expected.to deliver_to 'devs@company.name'
end
it 'has the correct subject' do it 'has the correct subject' do
is_expected.to have_subject /Pushed new branch master/ is_expected.to have_subject /Pushed new branch master/
end end
...@@ -624,7 +620,7 @@ describe Notify do ...@@ -624,7 +620,7 @@ describe Notify do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:tree_path) { namespace_project_tree_path(project.namespace, project, "v1.0") } let(:tree_path) { namespace_project_tree_path(project.namespace, project, "v1.0") }
subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/tags/v1.0', action: :create) } subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/tags/v1.0', action: :create) }
it_behaves_like 'it should not have Gmail Actions links' it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link" it_behaves_like "a user cannot unsubscribe through footer link"
...@@ -637,10 +633,6 @@ describe Notify do ...@@ -637,10 +633,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender) expect(sender.address).to eq(gitlab_sender)
end end
it 'is sent to recipient' do
is_expected.to deliver_to 'devs@company.name'
end
it 'has the correct subject' do it 'has the correct subject' do
is_expected.to have_subject /Pushed new tag v1\.0/ is_expected.to have_subject /Pushed new tag v1\.0/
end end
...@@ -654,7 +646,7 @@ describe Notify do ...@@ -654,7 +646,7 @@ describe Notify do
let(:example_site_path) { root_path } let(:example_site_path) { root_path }
let(:user) { create(:user) } let(:user) { create(:user) }
subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :delete) } subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :delete) }
it_behaves_like 'it should not have Gmail Actions links' it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link" it_behaves_like "a user cannot unsubscribe through footer link"
...@@ -667,10 +659,6 @@ describe Notify do ...@@ -667,10 +659,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender) expect(sender.address).to eq(gitlab_sender)
end end
it 'is sent to recipient' do
is_expected.to deliver_to 'devs@company.name'
end
it 'has the correct subject' do it 'has the correct subject' do
is_expected.to have_subject /Deleted branch master/ is_expected.to have_subject /Deleted branch master/
end end
...@@ -680,7 +668,7 @@ describe Notify do ...@@ -680,7 +668,7 @@ describe Notify do
let(:example_site_path) { root_path } let(:example_site_path) { root_path }
let(:user) { create(:user) } let(:user) { create(:user) }
subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/tags/v1.0', action: :delete) } subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/tags/v1.0', action: :delete) }
it_behaves_like 'it should not have Gmail Actions links' it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link" it_behaves_like "a user cannot unsubscribe through footer link"
...@@ -693,10 +681,6 @@ describe Notify do ...@@ -693,10 +681,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender) expect(sender.address).to eq(gitlab_sender)
end end
it 'is sent to recipient' do
is_expected.to deliver_to 'devs@company.name'
end
it 'has the correct subject' do it 'has the correct subject' do
is_expected.to have_subject /Deleted tag v1\.0/ is_expected.to have_subject /Deleted tag v1\.0/
end end
...@@ -710,7 +694,7 @@ describe Notify do ...@@ -710,7 +694,7 @@ describe Notify do
let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base, project), to: Commit.new(compare.head, project)) } let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base, project), to: Commit.new(compare.head, project)) }
let(:send_from_committer_email) { false } let(:send_from_committer_email) { false }
subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, send_from_committer_email: send_from_committer_email) } subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, send_from_committer_email: send_from_committer_email) }
it_behaves_like 'it should not have Gmail Actions links' it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link" it_behaves_like "a user cannot unsubscribe through footer link"
...@@ -723,10 +707,6 @@ describe Notify do ...@@ -723,10 +707,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender) expect(sender.address).to eq(gitlab_sender)
end end
it 'is sent to recipient' do
is_expected.to deliver_to 'devs@company.name'
end
it 'has the correct subject' do it 'has the correct subject' do
is_expected.to have_subject /\[#{project.path_with_namespace}\]\[master\] #{commits.length} commits:/ is_expected.to have_subject /\[#{project.path_with_namespace}\]\[master\] #{commits.length} commits:/
end end
...@@ -818,7 +798,7 @@ describe Notify do ...@@ -818,7 +798,7 @@ describe Notify do
let(:commits) { Commit.decorate(compare.commits, nil) } let(:commits) { Commit.decorate(compare.commits, nil) }
let(:diff_path) { namespace_project_commit_path(project.namespace, project, commits.first) } let(:diff_path) { namespace_project_commit_path(project.namespace, project, commits.first) }
subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) } subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) }
it_behaves_like 'it should show Gmail Actions View Commit link' it_behaves_like 'it should show Gmail Actions View Commit link'
it_behaves_like "a user cannot unsubscribe through footer link" it_behaves_like "a user cannot unsubscribe through footer link"
...@@ -831,10 +811,6 @@ describe Notify do ...@@ -831,10 +811,6 @@ describe Notify do
expect(sender.address).to eq(gitlab_sender) expect(sender.address).to eq(gitlab_sender)
end end
it 'is sent to recipient' do
is_expected.to deliver_to 'devs@company.name'
end
it 'has the correct subject' do it 'has the correct subject' do
is_expected.to have_subject /#{commits.first.title}/ is_expected.to have_subject /#{commits.first.title}/
end end
......
...@@ -6,29 +6,66 @@ describe EmailsOnPushWorker do ...@@ -6,29 +6,66 @@ describe EmailsOnPushWorker do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:data) { Gitlab::PushDataBuilder.build_sample(project, user) } let(:data) { Gitlab::PushDataBuilder.build_sample(project, user) }
let(:recipients) { user.email }
let(:perform) { subject.perform(project.id, recipients, data.stringify_keys) }
subject { EmailsOnPushWorker.new } subject { EmailsOnPushWorker.new }
before do
allow(Project).to receive(:find).and_return(project)
end
describe "#perform" do describe "#perform" do
it "sends mail" do context "when there are no errors in sending" do
subject.perform(project.id, user.email, data.stringify_keys) let(:email) { ActionMailer::Base.deliveries.last }
before { perform }
email = ActionMailer::Base.deliveries.last it "sends a mail with the correct subject" do
expect(email.subject).to include('Change some files') expect(email.subject).to include('Change some files')
end
it "sends the mail to the correct recipient" do
expect(email.to).to eq([user.email]) expect(email.to).to eq([user.email])
end end
end
it "gracefully handles an input SMTP error" do context "when there is an SMTP error" do
before do
ActionMailer::Base.deliveries.clear ActionMailer::Base.deliveries.clear
allow(Notify).to receive(:repository_push_email).and_raise(Net::SMTPFatalError) allow(Notify).to receive(:repository_push_email).and_raise(Net::SMTPFatalError)
perform
end
subject.perform(project.id, user.email, data.stringify_keys) it "gracefully handles an input SMTP error" do
expect(ActionMailer::Base.deliveries.count).to eq(0) expect(ActionMailer::Base.deliveries.count).to eq(0)
end end
end end
context "when there are multiple recipients" do
let(:recipients) do
1.upto(5).map { |i| user.email.sub('@', "+#{i}@") }.join("\n")
end
before do
# This is a hack because we modify the mail object before sending, for efficency,
# but the TestMailer adapter just appends the objects to an array. To clone a mail
# object, create a new one!
# https://github.com/mikel/mail/issues/314#issuecomment-12750108
allow_any_instance_of(Mail::TestMailer).to receive(:deliver!).and_wrap_original do |original, mail|
original.call(Mail.new(mail.encoded))
end
ActionMailer::Base.deliveries.clear
end
it "sends the mail to each of the recipients" do
perform
expect(ActionMailer::Base.deliveries.count).to eq(5)
expect(ActionMailer::Base.deliveries.map(&:to).flatten).to contain_exactly(*recipients.split)
end
it "only generates the mail once" do
expect(Notify).to receive(:repository_push_email).once.and_call_original
expect(Premailer::Rails::CustomizedPremailer).to receive(:new).once.and_call_original
perform
end
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment