Bug 910565: backport bug 877078 to bmo (shift bugmail generation to the jobqueue)

6 files changed, 200 insertions, 67 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 6f3107d5c..0b17a02e6 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -313,21 +313,11 @@ use constant EXTRA_REQUIRED_FIELDS => qw(creation_ts target_milestone cc qa_cont
 
 #####################################################################
 
-# This and "new" catch every single way of creating a bug, so that we
-# can call _create_cf_accessors.
-sub _do_list_select {
-    my $invocant = shift;
-    $invocant->_create_cf_accessors();
-    return $invocant->SUPER::_do_list_select(@_);
-}
-
 sub new {
     my $invocant = shift;
     my $class = ref($invocant) || $invocant;
     my $param = shift;
 
-    $class->_create_cf_accessors();
-
     # Remove leading "#" mark if we've just been passed an id.
     if (!ref $param && $param =~ /^#(\d+)$/) {
         $param = $1;
@@ -376,6 +366,10 @@ sub new {
     return $self;
 }
 
+sub initialize {
+    $_[0]->_create_cf_accessors();
+}
+
 sub cache_key {
     my $class = shift;
     my $key = $class->SUPER::cache_key(@_)
diff --git a/Bugzilla/BugMail.pm b/Bugzilla/BugMail.pm
index 3d3bd2dbf..a96989056 100644
--- a/Bugzilla/BugMail.pm
+++ b/Bugzilla/BugMail.pm
@@ -49,6 +49,7 @@ use Date::Format;
 use Scalar::Util qw(blessed);
 use List::MoreUtils qw(uniq firstidx);
 use Sys::Hostname;
+use Storable qw(dclone);
 
 use constant BIT_DIRECT    => 1;
 use constant BIT_WATCHING  => 2;
@@ -418,30 +419,76 @@ sub sendMail {
     $bugmailtype = "dep_changed" if $dep_only;
 
     my $vars = {
-        date => $date,
-        to_user => $user,
-        bug => $bug,
-        reasons => \@reasons,
-        reasons_watch => \@reasons_watch,
-        reasonsheader => join(" ", @headerrel),
+        date               => $date,
+        to_user            => $user,
+        bug                => $bug,
+        reasons            => \@reasons,
+        reasons_watch      => \@reasons_watch,
+        reasonsheader      => join(" ", @headerrel),
         reasonswatchheader => join(" ", @watchingrel),
-        changer => $changer,
-        diffs => \@display_diffs,
-        changedfields => \@changedfields,
-        changedfieldnames => \@changedfieldnames,
-        new_comments => \@send_comments,
-        threadingmarker => build_thread_marker($bug->id, $user->id, !$bug->lastdiffed),
-        referenced_bugs => $referenced_bugs,
-        bugmailtype => $bugmailtype
+        changer            => $changer,
+        diffs              => \@display_diffs,
+        changedfields      => \@changedfields,
+        changedfieldnames  => \@changedfieldnames,
+        new_comments       => \@send_comments,
+        threadingmarker    => build_thread_marker($bug->id, $user->id, !$bug->lastdiffed),
+        referenced_bugs    => $referenced_bugs,
+        bugmailtype        => $bugmailtype,
     };
-    my $msg =  _generate_bugmail($user, $vars);
-    MessageToMTA($msg);
+
+    if (Bugzilla->params->{'use_mailer_queue'}) {
+        enqueue($vars);
+    } else {
+        MessageToMTA(_generate_bugmail($vars));
+    }
 
     return 1;
 }
 
+sub enqueue {
+    my ($vars) = @_;
+    # we need to flatten all objects to a hash before pushing to the job queue.
+    # the hashes need to be inflated in the dequeue method.
+    $vars->{bug}          = _flatten_object($vars->{bug});
+    $vars->{to_user}      = $vars->{to_user}->flatten_to_hash;
+    $vars->{changer}      = _flatten_object($vars->{changer});
+    $vars->{new_comments} = [ map { _flatten_object($_) } @{ $vars->{new_comments} } ];
+    foreach my $diff (@{ $vars->{diffs} }) {
+        $diff->{who} = _flatten_object($diff->{who});
+    }
+    Bugzilla->job_queue->insert('bug_mail', { vars => $vars });
+}
+
+sub dequeue {
+    my ($payload) = @_;
+    # clone the payload so we can modify it without impacting TheSchwartz's
+    # ability to process the job when we've finished
+    my $vars = dclone($payload);
+    # inflate objects
+    $vars->{bug}          = Bugzilla::Bug->new_from_hash($vars->{bug});
+    $vars->{to_user}      = Bugzilla::User->new_from_hash($vars->{to_user});
+    $vars->{changer}      = Bugzilla::User->new_from_hash($vars->{changer});
+    $vars->{new_comments} = [ map { Bugzilla::Comment->new_from_hash($_) } @{ $vars->{new_comments} } ];
+    foreach my $diff (@{ $vars->{diffs} }) {
+        $diff->{who} = Bugzilla::User->new_from_hash($diff->{who});
+    }
+    # generate bugmail and send
+    MessageToMTA(_generate_bugmail($vars), 1);
+}
+
+sub _flatten_object {
+    my ($object) = @_;
+    # nothing to do if it's already flattened
+    return $object unless blessed($object);
+    # the same objects are used for each recipient, so cache the flattened hash
+    my $cache = Bugzilla->request_cache->{bugmail_flat_objects} ||= {};
+    my $key = blessed($object) . '-' . $object->id;
+    return $cache->{$key} ||= $object->flatten_to_hash;
+}
+
 sub _generate_bugmail {
-    my ($user, $vars) = @_;
+    my ($vars) = @_;
+    my $user = $vars->{to_user};
     my $template = Bugzilla->template_inner($user->setting('lang'));
     my ($msg_text, $msg_html, $msg_header);
 
diff --git a/Bugzilla/Job/BugMail.pm b/Bugzilla/Job/BugMail.pm
new file mode 100644
index 000000000..9c176b005
--- /dev/null
+++ b/Bugzilla/Job/BugMail.pm
@@ -0,0 +1,31 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+
+package Bugzilla::Job::BugMail;
+
+use 5.10.1;
+use strict;
+
+use Bugzilla::BugMail;
+BEGIN { eval "use parent qw(Bugzilla::Job::Mailer)"; }
+
+sub work {
+    my ($class, $job) = @_;
+    my $success = eval {
+        Bugzilla::BugMail::dequeue($job->arg->{vars});
+        1;
+    };
+    if (!$success) {
+        $job->failed($@);
+        undef $@;
+    }
+    else {
+        $job->completed;
+    }
+}
+
+1;
diff --git a/Bugzilla/JobQueue.pm b/Bugzilla/JobQueue.pm
index 053928dd0..a94598b87 100644
--- a/Bugzilla/JobQueue.pm
+++ b/Bugzilla/JobQueue.pm
@@ -35,6 +35,7 @@ use fields qw(_worker_pidfile);
 # If you add new types of jobs, you should add a mapping here.
 use constant JOB_MAP => {
     send_mail => 'Bugzilla::Job::Mailer',
+    bug_mail  => 'Bugzilla::Job::BugMail',
 };
 
 # Without a driver cache TheSchwartz opens a new database connection
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 34134a69f..b1bc229bf 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -59,10 +59,13 @@ sub TO_JSON { return { %{ $_[0] } }; }
 sub new {
     my $invocant = shift;
     my $class    = ref($invocant) || $invocant;
-    my $object   = $class->_init(@_);
-    bless($object, $class) if $object;
+    my $param    = shift;
 
-    Bugzilla::Hook::process('object_end_of_new', { object => $object });
+    my $object = $class->_cache_get($param);
+    return $object if $object;
+
+    $object = $class->new_from_hash($class->_load_from_db($param));
+    $class->_cache_set($param, $object);
 
     return $object;
 }
@@ -72,11 +75,9 @@ sub new {
 # Bugzilla::Object, make sure that you modify bz_setup_database
 # in Bugzilla::DB::Pg appropriately, to add the right LOWER
 # index. You can see examples already there.
-sub _init {
+sub _load_from_db {
     my $class = shift;
     my ($param) = @_;
-    my $object = $class->_cache_get($param);
-    return $object if $object;
     my $dbh = Bugzilla->dbh;
     my $columns = join(',', $class->_get_db_columns);
     my $table   = $class->DB_TABLE;
@@ -88,17 +89,18 @@ sub _init {
         $id = $param->{id};
     }
 
+    my $object_data;
     if (defined $id) {
         # We special-case if somebody specifies an ID, so that we can
         # validate it as numeric.
         detaint_natural($id)
           || ThrowCodeError('param_must_be_numeric',
-                            {function => $class . '::_init'});
+                            {function => $class . '::_load_from_db'});
 
         # Too large integers make PostgreSQL crash.
         return if $id > MAX_INT_32;
 
-        $object = $dbh->selectrow_hashref(qq{
+        $object_data = $dbh->selectrow_hashref(qq{
             SELECT $columns FROM $table
              WHERE $id_field = ?}, undef, $id);
     } else {
@@ -125,12 +127,47 @@ sub _init {
         }
 
         map { trick_taint($_) } @values;
-        $object = $dbh->selectrow_hashref(
+        $object_data = $dbh->selectrow_hashref(
             "SELECT $columns FROM $table WHERE $condition", undef, @values);
     }
+    return $object_data;
+}
 
-    $class->_cache_set($param, $object) if $object;
-    return $object;
+sub new_from_list {
+    my $invocant = shift;
+    my $class = ref($invocant) || $invocant;
+    my ($id_list) = @_;
+    my $id_field = $class->ID_FIELD;
+
+    my @detainted_ids;
+    foreach my $id (@$id_list) {
+        detaint_natural($id) ||
+            ThrowCodeError('param_must_be_numeric',
+                          {function => $class . '::new_from_list'});
+        # Too large integers make PostgreSQL crash.
+        next if $id > MAX_INT_32;
+        push(@detainted_ids, $id);
+    }
+
+    # We don't do $invocant->match because some classes have
+    # their own implementation of match which is not compatible
+    # with this one. However, match() still needs to have the right $invocant
+    # in order to do $class->DB_TABLE and so on.
+    return match($invocant, { $id_field => \@detainted_ids });
+}
+
+sub new_from_hash {
+    my $invocant = shift;
+    my $class = ref($invocant) || $invocant;
+    my $object_data = shift || return;
+    $class->_serialisation_keys($object_data);
+    bless($object_data, $class);
+    $object_data->initialize();
+    return $object_data;
+}
+
+sub initialize {
+    # abstract
 }
 
 # Provides a mechanism for objects to be cached in the request_cahce
@@ -171,6 +208,15 @@ sub cache_key {
     }
 }
 
+# To support serialisation, we need to capture the keys in an object's default
+# hashref.
+sub _serialisation_keys {
+    my ($class, $object) = @_;
+    my $cache = Bugzilla->request_cache->{serialisation_keys} ||= {};
+    $cache->{$class} = [ keys %$object ] if $object && !exists $cache->{$class};
+    return @{ $cache->{$class} };
+}
+
 sub check {
     my ($invocant, $param) = @_;
     my $class = ref($invocant) || $invocant;
@@ -204,28 +250,6 @@ sub check {
     return $obj;
 }
 
-sub new_from_list {
-    my $invocant = shift;
-    my $class = ref($invocant) || $invocant;
-    my ($id_list) = @_;
-    my $id_field = $class->ID_FIELD;
-
-    my @detainted_ids;
-    foreach my $id (@$id_list) {
-        detaint_natural($id) ||
-            ThrowCodeError('param_must_be_numeric',
-                          {function => $class . '::new_from_list'});
-        # Too large integers make PostgreSQL crash.
-        next if $id > MAX_INT_32;
-        push(@detainted_ids, $id);
-    }
-    # We don't do $invocant->match because some classes have
-    # their own implementation of match which is not compatible
-    # with this one. However, match() still needs to have the right $invocant
-    # in order to do $class->DB_TABLE and so on.
-    return match($invocant, { $id_field => \@detainted_ids });
-}
-
 # Note: Future extensions to this could be:
 #  * Add a MATCH_JOIN constant so that we can join against
 #    certain other tables for the WHERE criteria.
@@ -324,8 +348,11 @@ sub _do_list_select {
     my @untainted = @{ $values || [] };
     trick_taint($_) foreach @untainted;
     my $objects = $dbh->selectall_arrayref($sql, {Slice=>{}}, @untainted);
-    bless ($_, $class) foreach @$objects;
-    return $objects
+    $class->_serialisation_keys($objects->[0]) if @$objects;
+    foreach my $object (@$objects) {
+        $object = $class->new_from_hash($object);
+    }
+    return $objects;
 }
 
 ###############################
@@ -503,6 +530,13 @@ sub audit_log {
     }
 }
 
+sub flatten_to_hash {
+    my $self = shift;
+    my $class = blessed($self);
+    my %hash = map { $_ => $self->{$_} } $class->_serialisation_keys;
+    return \%hash;
+}
+
 ###############################
 ####      Subroutines    ######
 ###############################
@@ -1030,6 +1064,17 @@ database matching the parameters you passed in.
 
 =back
 
+=item C<initialize>
+
+=over
+
+=item B<Description>
+
+Abstract method to allow subclasses to perform initialization tasks after an
+object has been created.
+
+=back
+
 =item C<check>
 
 =over
diff --git a/extensions/BMO/lib/Reports/EmailQueue.pm b/extensions/BMO/lib/Reports/EmailQueue.pm
index 1bf2ca003..f1383aac7 100644
--- a/extensions/BMO/lib/Reports/EmailQueue.pm
+++ b/extensions/BMO/lib/Reports/EmailQueue.pm
@@ -39,11 +39,26 @@ sub report {
     $vars->{'jobs'} = $dbh->selectall_arrayref($query, { Slice => {} });
     foreach my $job (@{ $vars->{'jobs'} }) {
         eval {
-            my $msg = _cond_thaw(delete $job->{'arg'})->{msg};
-            if (ref($msg) && blessed($msg) eq 'Email::MIME') {
-                $job->{'subject'} = $msg->header('subject');
-            } else {
-                ($job->{'subject'}) = $msg =~ /\nSubject: ([^\n]+)/;
+            my ($recipient, $description);
+            my $arg = _cond_thaw(delete $job->{arg});
+
+            if (exists $arg->{vars}) {
+                my $vars = $arg->{vars};
+                $recipient = $vars->{to_user}->{login_name};
+                $description = '[Bug ' . $vars->{bug}->{bug_id} . '] ' . $vars->{bug}->{short_desc};
+            } elsif (exists $arg->{msg}) {
+                my $msg = $arg->{msg};
+                if (ref($msg) && blessed($msg) eq 'Email::MIME') {
+                    $recipient = $msg->header('to');
+                    $description = $msg->header('subject');
+                } else {
+                    ($recipient) = $msg =~ /\nTo: ([^\n]+)/;
+                    ($description) = $msg =~ /\nSubject: ([^\n]+)/;
+                }
+            }
+
+            if ($recipient) {
+                $job->{subject} = "<$recipient> $description";
             }
         };
     }