Bug 992767: backport bug 987032 to bmo (allow memcached to cache bugzilla configuration information)

13 files changed, 237 insertions, 52 deletions

diff --git a/Bugzilla/Classification.pm b/Bugzilla/Classification.pm
index 5d75a6dc6..a989a40f2 100644
--- a/Bugzilla/Classification.pm
+++ b/Bugzilla/Classification.pm
@@ -31,6 +31,8 @@ use base qw(Bugzilla::Field::ChoiceInterface Bugzilla::Object);
 ####    Initialization     ####
 ###############################
 
+use constant IS_CONFIG => 1;
+
 use constant DB_TABLE => 'classifications';
 use constant LIST_ORDER => 'sortkey, name';
 
@@ -75,6 +77,7 @@ sub remove_from_db {
         foreach my $id (@$product_ids) {
             Bugzilla->memcached->clear({ table => 'products', id => $id });
         }
+        Bugzilla->memcached->clear_config();
     }
 
     $self->SUPER::remove_from_db();
diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
index 97d03dc42..3e69d152d 100644
--- a/Bugzilla/Field.pm
+++ b/Bugzilla/Field.pm
@@ -87,6 +87,8 @@ use Scalar::Util qw(blessed);
 ####    Initialization     ####
 ###############################
 
+use constant IS_CONFIG => 1;
+
 use constant DB_TABLE   => 'fielddefs';
 use constant LIST_ORDER => 'sortkey, name';
 
@@ -1056,6 +1058,7 @@ sub create {
         $field->_update_visibility_values();
 
         $dbh->bz_commit_transaction();
+        Bugzilla->memcached->clear_config();
 
         if ($field->custom) {
             my $name = $field->name;
@@ -1080,6 +1083,7 @@ sub create {
               unless $is_obsolete;
 
             Bugzilla->memcached->clear({ table => 'fielddefs', id => $field->id });
+            Bugzilla->memcached->clear_config();
         }
     };
 
@@ -1103,6 +1107,7 @@ sub update {
         $dbh->do("UPDATE " . $self->name . " SET visibility_value_id = NULL");
     }
     $self->_update_visibility_values();
+    Bugzilla->memcached->clear_config();
     return $changes;
 }
 
diff --git a/Bugzilla/Field/Choice.pm b/Bugzilla/Field/Choice.pm
index 773dbd4ce..8d7d61def 100644
--- a/Bugzilla/Field/Choice.pm
+++ b/Bugzilla/Field/Choice.pm
@@ -37,6 +37,8 @@ use Scalar::Util qw(blessed);
 # Initialization #
 ##################
 
+use constant IS_CONFIG => 1;
+
 use constant DB_COLUMNS => qw(
     id
     value
diff --git a/Bugzilla/Group.pm b/Bugzilla/Group.pm
index 109f06d7f..732de348f 100644
--- a/Bugzilla/Group.pm
+++ b/Bugzilla/Group.pm
@@ -37,6 +37,8 @@ use Bugzilla::Config qw(:admin);
 ##### Module Initialization ###
 ###############################
 
+use constant IS_CONFIG => 1;
+
 use constant DB_COLUMNS => qw(
     groups.id
     groups.name
@@ -231,6 +233,7 @@ sub update {
     Bugzilla::Hook::process('group_end_of_update', 
                             { group => $self, changes => $changes });
     $dbh->bz_commit_transaction();
+    Bugzilla->memcached->clear_config();
     return $changes;
 }
 
@@ -420,6 +423,7 @@ sub create {
 
     Bugzilla::Hook::process('group_end_of_create', { group => $group });
     $dbh->bz_commit_transaction();
+    Bugzilla->memcached->clear_config();
     return $group;
 }
 
diff --git a/Bugzilla/Keyword.pm b/Bugzilla/Keyword.pm
index e2ecc29e5..cda401c59 100644
--- a/Bugzilla/Keyword.pm
+++ b/Bugzilla/Keyword.pm
@@ -27,6 +27,8 @@ use Bugzilla::Util;
 ####    Initialization     ####
 ###############################
 
+use constant IS_CONFIG => 1;
+
 use constant DB_COLUMNS => qw(
    keyworddefs.id
    keyworddefs.name
diff --git a/Bugzilla/Memcached.pm b/Bugzilla/Memcached.pm
index 4bfca2b65..7a9734375 100644
--- a/Bugzilla/Memcached.pm
+++ b/Bugzilla/Memcached.pm
@@ -42,6 +42,10 @@ sub _new {
     return bless($self, $class);
 }
 
+sub enabled {
+    return $_[0]->{memcached} ? 1 : 0;
+}
+
 sub set {
     my ($self, $args) = @_;
     return unless $self->{memcached};
@@ -97,6 +101,32 @@ sub get {
     }
 }
 
+sub set_config {
+    my ($self, $args) = @_;
+    return unless $self->{memcached};
+
+    if (exists $args->{key}) {
+        return $self->_set($self->_config_prefix . ':' . $args->{key}, $args->{data});
+    }
+    else {
+        ThrowCodeError('params_required', { function => "Bugzilla::Memcached::set_config",
+                                            params   => [ 'key' ] });
+    }
+}
+
+sub get_config {
+    my ($self, $args) = @_;
+    return unless $self->{memcached};
+
+    if (exists $args->{key}) {
+        return $self->_get($self->_config_prefix . ':' . $args->{key});
+    }
+    else {
+        ThrowCodeError('params_required', { function => "Bugzilla::Memcached::get_config",
+                                            params   => [ 'key' ] });
+    }
+}
+
 sub clear {
     my ($self, $args) = @_;
     return unless $self->{memcached};
@@ -132,44 +162,66 @@ sub clear {
 
 sub clear_all {
     my ($self) = @_;
-    return unless my $memcached = $self->{memcached};
-    if (!$memcached->incr("prefix", 1)) {
-        $memcached->add("prefix", time());
-    }
+    return unless $self->{memcached};
+    $self->_inc_prefix("global");
+}
 
-    # BMO - log that we've wiped the cache
-    openlog('apache', 'cons,pid', 'local4');
-    syslog('notice', encode_utf8('[memcached] cache cleared'));
-    closelog();
+sub clear_config {
+    my ($self) = @_;
+    return unless $self->{memcached};
+    $self->_inc_prefix("config");
 }
 
 # in order to clear all our keys, we add a prefix to all our keys.  when we
 # need to "clear" all current keys, we increment the prefix.
 sub _prefix {
-    my ($self) = @_;
+    my ($self, $name) = @_;
     # we don't want to change prefixes in the middle of a request
     my $request_cache = Bugzilla->request_cache;
-    if (!$request_cache->{memcached_prefix}) {
+    my $request_cache_key = "memcached_prefix_$name";
+    if (!$request_cache->{$request_cache_key}) {
         my $memcached = $self->{memcached};
-        my $prefix = $memcached->get("prefix");
+        my $prefix = $memcached->get($name);
         if (!$prefix) {
             $prefix = time();
-            if (!$memcached->add("prefix", $prefix)) {
+            if (!$memcached->add($name, $prefix)) {
                 # if this failed, either another process set the prefix, or
                 # memcached is down.  assume we lost the race, and get the new
                 # value.  if that fails, memcached is down so use a dummy
                 # prefix for this request.
-                $prefix = $memcached->get("prefix") || 0;
+                $prefix = $memcached->get($name) || 0;
             }
         }
-        $request_cache->{memcached_prefix} = $prefix;
+        $request_cache->{$request_cache_key} = $prefix;
+    }
+    return $request_cache->{$request_cache_key};
+}
+
+sub _inc_prefix {
+    my ($self, $name) = @_;
+    my $memcached = $self->{memcached};
+    if (!$memcached->incr($name, 1)) {
+        $memcached->add($name, time());
     }
-    return $request_cache->{memcached_prefix};
+    delete Bugzilla->request_cache->{"memcached_prefix_$name"};
+
+    # BMO - log that we've wiped the cache
+    openlog('apache', 'cons,pid', 'local4');
+    syslog('notice', encode_utf8("[memcached] $name cache cleared"));
+    closelog();
+}
+
+sub _global_prefix {
+    return $_[0]->_prefix("global");
+}
+
+sub _config_prefix {
+    return $_[0]->_prefix("config");
 }
 
 sub _encode_key {
     my ($self, $key) = @_;
-    $key = $self->_prefix . ':' . uri_escape_utf8($key);
+    $key = $self->_global_prefix . ':' . uri_escape_utf8($key);
     return length($self->{namespace} . $key) > MAX_KEY_LENGTH
         ? undef
         : $key;
@@ -182,6 +234,7 @@ sub _set {
         ThrowCodeError('param_invalid', { function => "Bugzilla::Memcached::set",
                                           param    => "value" });
     }
+
     $key = $self->_encode_key($key)
         or return;
     return $self->{memcached}->set($key, $value);
@@ -198,10 +251,19 @@ sub _get {
     # detaint returned values
     # hashes and arrays are detainted just one level deep
     if (ref($value) eq 'HASH') {
-        map { defined($_) && trick_taint($_) } values %$value;
+        _detaint_hashref($value);
     }
     elsif (ref($value) eq 'ARRAY') {
-        trick_taint($_) foreach @$value;
+        foreach my $value (@$value) {
+            next unless defined $value;
+            # arrays of hashes are common
+            if (ref($value) eq 'HASH') {
+                _detaint_hashref($value);
+            }
+            elsif (!ref($value)) {
+                trick_taint($value);
+            }
+        }
     }
     elsif (!ref($value)) {
         trick_taint($value);
@@ -209,6 +271,15 @@ sub _get {
     return $value;
 }
 
+sub _detaint_hashref {
+    my ($hashref) = @_;
+    foreach my $value (values %$hashref) {
+        if (defined($value) && !ref($value)) {
+            trick_taint($value);
+        }
+    }
+}
+
 sub _delete {
     my ($self, $key) = @_;
     $key = $self->_encode_key($key)
@@ -273,6 +344,14 @@ L<Bugzilla-E<gt>memcached()|Bugzilla/memcached>.
 
 =head1 METHODS
 
+=over
+
+=item C<enabled>
+
+Returns true if Memcached support is available and enabled.
+
+=back
+
 =head2 Setting
 
 Adds a value to Memcached.
@@ -292,6 +371,13 @@ to C<undef>.
 This is a convenience method which allows cached data to be later retrieved by
 specifying the C<table> and either the C<id> or C<name>.
 
+=item C<set_config({ key =E<gt> $key, data =E<gt> $data })>
+
+Adds the C<data> using the C<key> while identifying the data as part of
+Bugzilla's configuration (such as fields, products, components, groups, etc).
+Values set with C<set_config> are automatically cleared when changes are made
+to Bugzilla's configuration.
+
 =back
 
 =head2 Getting
@@ -313,6 +399,11 @@ Return C<value> with the specified C<table> and C<id>.
 
 Return C<value> with the specified C<table> and C<name>.
 
+=item C<get_config({ key =E<gt> $key })>
+
+Return C<value> with the specified C<key> from the configuration cache.  See
+C<set_config> for more information.
+
 =back
 
 =head2 Clearing
@@ -335,6 +426,11 @@ corresponding C<table> and C<name> entry.
 Removes C<value> with the specified C<table> and C<name>, as well as the
 corresponding C<table> and C<id> entry.
 
+=item C<clear_config>
+
+Removes all configuration related values from the cache.  See C<set_config> for
+more information.
+
 =item C<clear_all>
 
 Removes all values from the cache.
diff --git a/Bugzilla/Milestone.pm b/Bugzilla/Milestone.pm
index a5f3ed383..d8fcd79a5 100644
--- a/Bugzilla/Milestone.pm
+++ b/Bugzilla/Milestone.pm
@@ -108,10 +108,12 @@ sub run_create_validators {
 
 sub update {
     my $self = shift;
+    my $dbh = Bugzilla->dbh;
+
+    $dbh->bz_start_transaction();
     my $changes = $self->SUPER::update(@_);
 
     if (exists $changes->{value}) {
-        my $dbh = Bugzilla->dbh;
         # The milestone value is stored in the bugs table instead of its ID.
         $dbh->do('UPDATE bugs SET target_milestone = ?
                   WHERE target_milestone = ? AND product_id = ?',
@@ -121,8 +123,11 @@ sub update {
         $dbh->do('UPDATE products SET defaultmilestone = ?
                   WHERE id = ? AND defaultmilestone = ?',
                  undef, ($self->name, $self->product_id, $changes->{value}->[0]));
-        Bugzilla->memcached->clear({ table => 'produles', id => $self->product_id });
+        Bugzilla->memcached->clear({ table => 'products', id => $self->product_id });
     }
+    $dbh->bz_commit_transaction();
+    Bugzilla->memcached->clear_config();
+
     return $changes;
 }
 
@@ -130,6 +135,8 @@ sub remove_from_db {
     my $self = shift;
     my $dbh = Bugzilla->dbh;
 
+    $dbh->bz_start_transaction();
+
     # The default milestone cannot be deleted.
     if ($self->name eq $self->product->default_milestone) {
         ThrowUserError('milestone_is_default', { milestone => $self });
@@ -158,8 +165,9 @@ sub remove_from_db {
                              Bugzilla->user->id, $timestamp);
         }
     }
+    $self->SUPER::remove_from_db();
 
-    $dbh->do('DELETE FROM milestones WHERE id = ?', undef, $self->id);
+    $dbh->bz_commit_transaction();
 }
 
 ################################
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 936465b3f..e3e4ca229 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -51,6 +51,11 @@ use constant AUDIT_REMOVES => 1;
 # Memcached.  See documentation in Bugzilla::Memcached for more information.
 use constant USE_MEMCACHED => 1;
 
+# When IS_CONFIG is true, the class is used to track seldom changed
+# configuration objects.  This includes, but is not limited to, fields, field
+# values, keywords, products, classifications, priorities, severities, etc.
+use constant IS_CONFIG => 0;
+
 # This allows the JSON-RPC interface to return Bugzilla::Object instances
 # as though they were hashes. In the future, this may be modified to return
 # less information.
@@ -69,7 +74,7 @@ sub new {
     return $object if $object;
 
     my ($data, $set_memcached);
-    if (Bugzilla->feature('memcached')
+    if (Bugzilla->memcached->enabled
         && $class->USE_MEMCACHED
         && ref($param) eq 'HASH' && $param->{cache})
     {
@@ -380,22 +385,42 @@ sub _do_list_select {
     my $cols  = join(',', $class->_get_db_columns);
     my $order = $class->LIST_ORDER;
 
-    my $sql = "SELECT $cols FROM $table";
-    if (defined $where) {
-        $sql .= " WHERE $where ";
+    # Unconditional requests for configuration data are cacheable.
+    my ($objects, $set_memcached, $memcached_key);
+    if (!defined $where
+        && Bugzilla->memcached->enabled
+        && $class->IS_CONFIG)
+    {
+        $memcached_key = "$class:get_all";
+        $objects = Bugzilla->memcached->get_config({ key => $memcached_key });
+        $set_memcached = $objects ? 0 : 1;
     }
-    $sql .= " ORDER BY $order";
-    
-    $sql .= " $postamble" if $postamble;
-        
-    my $dbh = Bugzilla->dbh;
-    # Sometimes the values are tainted, but we don't want to untaint them
-    # for the caller. So we copy the array. It's safe to untaint because
-    # they're only used in placeholders here.
-    my @untainted = @{ $values || [] };
-    trick_taint($_) foreach @untainted;
-    my $objects = $dbh->selectall_arrayref($sql, {Slice=>{}}, @untainted);
-    $class->_serialisation_keys($objects->[0]) if @$objects;
+
+    if (!$objects) {
+        my $sql = "SELECT $cols FROM $table";
+        if (defined $where) {
+            $sql .= " WHERE $where ";
+        }
+        $sql .= " ORDER BY $order";
+        $sql .= " $postamble" if $postamble;
+
+        my $dbh = Bugzilla->dbh;
+        # Sometimes the values are tainted, but we don't want to untaint them
+        # for the caller. So we copy the array. It's safe to untaint because
+        # they're only used in placeholders here.
+        my @untainted = @{ $values || [] };
+        trick_taint($_) foreach @untainted;
+        $objects = $dbh->selectall_arrayref($sql, {Slice=>{}}, @untainted);
+        $class->_serialisation_keys($objects->[0]) if @$objects;
+    }
+
+    if ($objects && $set_memcached) {
+        Bugzilla->memcached->set_config({
+            key  => $memcached_key,
+            data => $objects
+        });
+    }
+
     foreach my $object (@$objects) {
         $object = $class->new_from_hash($object);
     }
@@ -523,8 +548,11 @@ sub update {
     $self->audit_log(\%changes) if $self->AUDIT_UPDATES;
 
     $dbh->bz_commit_transaction();
-    Bugzilla->memcached->clear({ table => $table, id => $self->id })
-        if $self->USE_MEMCACHED && @values;
+    if ($self->USE_MEMCACHED && @values) {
+        Bugzilla->memcached->clear({ table => $table, id => $self->id });
+        Bugzilla->memcached->clear_config()
+            if $self->IS_CONFIG;
+    }
     $self->_object_cache_remove({ id => $self->id });
     $self->_object_cache_remove({ name => $self->name }) if $self->name;
 
@@ -545,8 +573,11 @@ sub remove_from_db {
     $self->audit_log(AUDIT_REMOVE) if $self->AUDIT_REMOVES;
     $dbh->do("DELETE FROM $table WHERE $id_field = ?", undef, $self->id);
     $dbh->bz_commit_transaction();
-    Bugzilla->memcached->clear({ table => $table, id => $self->id })
-        if $self->USE_MEMCACHED;
+    if ($self->USE_MEMCACHED) {
+        Bugzilla->memcached->clear({ table => $table, id => $self->id });
+        Bugzilla->memcached->clear_config()
+            if $self->IS_CONFIG;
+    }
     $self->_object_cache_remove({ id => $self->id });
     $self->_object_cache_remove({ name => $self->name }) if $self->name;
     undef $self;
@@ -613,6 +644,13 @@ sub create {
     my $object = $class->insert_create_data($field_values);
     $dbh->bz_commit_transaction();
 
+    if (Bugzilla->memcached->enabled
+        && $class->USE_MEMCACHED
+        && $class->IS_CONFIG)
+    {
+        Bugzilla->memcached->clear_config();
+    }
+
     return $object;
 }
 
@@ -708,7 +746,7 @@ sub insert_create_data {
 
 sub get_all {
     my $class = shift;
-    return @{$class->_do_list_select()};
+    return @{ $class->_do_list_select() };
 }
 
 ###############################
diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm
index 452ae90fc..3e74b9afd 100644
--- a/Bugzilla/Product.pm
+++ b/Bugzilla/Product.pm
@@ -41,6 +41,8 @@ use constant DEFAULT_CLASSIFICATION_ID => 1;
 ####    Initialization     ####
 ###############################
 
+use constant IS_CONFIG => 1;
+
 use constant DB_TABLE => 'products';
 
 use constant DB_COLUMNS => qw(
@@ -107,6 +109,7 @@ sub create {
     Bugzilla::Hook::process('product_end_of_create', { product => $product });
 
     $dbh->bz_commit_transaction();
+    Bugzilla->memcached->clear_config();
     return $product;
 }
 
@@ -262,6 +265,7 @@ sub update {
     # Changes have been committed.
     delete $self->{check_group_controls};
     Bugzilla->user->clear_product_cache();
+    Bugzilla->memcached->clear_config();
 
     return $changes;
 }
@@ -320,6 +324,7 @@ sub remove_from_db {
     $dbh->do("DELETE FROM products WHERE id = ?", undef, $self->id);
 
     $dbh->bz_commit_transaction();
+    Bugzilla->memcached->clear_config();
 
     # We have to delete these internal variables, else we get
     # the old lists of products and classifications again.
diff --git a/Bugzilla/Status.pm b/Bugzilla/Status.pm
index ffef600de..4a82f68d0 100644
--- a/Bugzilla/Status.pm
+++ b/Bugzilla/Status.pm
@@ -126,11 +126,21 @@ sub _check_value {
 
 sub BUG_STATE_OPEN {
     my $dbh = Bugzilla->dbh;
-    my $cache = Bugzilla->request_cache;
-    $cache->{status_bug_state_open} ||=
-        $dbh->selectcol_arrayref('SELECT value FROM bug_status 
-                                   WHERE is_open = 1');
-    return @{ $cache->{status_bug_state_open} };
+    my $request_cache = Bugzilla->request_cache;
+    my $cache_key = 'status_bug_state_open';
+    return @{ $request_cache->{$cache_key} }
+        if exists $request_cache->{$cache_key};
+
+    my $rows = Bugzilla->memcached->get_config({ key => $cache_key });
+    if (!$rows) {
+        $rows = $dbh->selectcol_arrayref(
+            'SELECT value FROM bug_status WHERE is_open = 1'
+        );
+        Bugzilla->memcached->set_config({ key => $cache_key, data => $rows });
+    }
+
+    $request_cache->{$cache_key} = $rows;
+    return @$rows;
 }
 
 # Tells you whether or not the argument is a valid "open" state.
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 98aa78e0a..4c0b18250 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -683,13 +683,23 @@ sub groups {
             FROM user_group_map
            WHERE user_id = ? AND isbless = 0}, undef, $self->id);
 
-    my $rows = $dbh->selectall_arrayref(
-        "SELECT DISTINCT grantor_id, member_id
-           FROM group_group_map
-          WHERE grant_type = " . GROUP_MEMBERSHIP);
+    my $cache_key = 'group_grant_type_' . GROUP_MEMBERSHIP;
+    my $membership_rows = Bugzilla->memcached->get_config({
+        key => $cache_key,
+    });
+    if (!$membership_rows) {
+        $membership_rows = $dbh->selectall_arrayref(
+            "SELECT DISTINCT grantor_id, member_id
+               FROM group_group_map
+              WHERE grant_type = " . GROUP_MEMBERSHIP);
+        Bugzilla->memcached->set_config({
+            key  => $cache_key,
+            data => $membership_rows,
+        });
+    }
 
     my %group_membership;
-    foreach my $row (@$rows) {
+    foreach my $row (@$membership_rows) {
         my ($grantor_id, $member_id) = @$row; 
         push (@{ $group_membership{$member_id} }, $grantor_id);
     }
diff --git a/contrib/convert-workflow.pl b/contrib/convert-workflow.pl
index 3dce21f12..e4baf18d6 100755
--- a/contrib/convert-workflow.pl
+++ b/contrib/convert-workflow.pl
@@ -164,6 +164,7 @@ if ($enable_unconfirmed) {
     $dbh->do('UPDATE products SET allows_unconfirmed = 1');
 }
 $dbh->bz_commit_transaction();
+Bugzilla->memcached->clear_all();
 
 print <<END;
 Done. There are some things you may want to fix, now:
diff --git a/editclassifications.cgi b/editclassifications.cgi
index 817c53af7..3ab89f120 100755
--- a/editclassifications.cgi
+++ b/editclassifications.cgi
@@ -232,6 +232,7 @@ if ($action eq 'reclassify') {
     foreach my $name (@names) {
         Bugzilla->memcached->clear({ table => 'products', name => $name });
     }
+    Bugzilla->memcached->clear_config();
 
     LoadTemplate($action);
 }