Bug 2091196 - fetch_product_versions is not cached for the request: buglist.cgi makes 42 memcached lookups, and a new HTTP request on every call when the product-details endpoint fails

Bugzilla/Util.pm

@@ -1003,14 +1003,20 @@ sub fetch_product_versions {
   my ($product) = @_;
   my $key = "${product}_versions";
 
-  # First check to see if the values are stored in cache. If not
-  # then we will call the end point to get the latest versions.
-  my $versions = Bugzilla->request_cache->{$key}
-    || Bugzilla->memcached->get_data({key => $key});
+  # Look each product up at most once per request, remembering failures too.
+  # Some callers loop over every product channel (e.g. the buglist column
+  # aliases call this 42 times per page), so without this every call went
+  # back to memcached, or to the endpoint again if it was failing.
+  my $request_cache = Bugzilla->request_cache;
+  return $request_cache->{$key} if exists $request_cache->{$key};
+
+  # Then check memcached. If not there, call the end point to get the
+  # latest versions.
+  my $versions = Bugzilla->memcached->get_data({key => $key});
 
   unless ($versions) {
     my $product_endpoint = Bugzilla->params->{product_details_endpoint};
-    return undef if !$product_endpoint;
+    return $request_cache->{$key} = undef if !$product_endpoint;
 
     # Wrap the call in a try block so that we do not block the loading
     # of a bug page if it does not succeed.
@@ -1021,10 +1027,7 @@ sub fetch_product_versions {
       if ($response->is_success) {
         $versions = decode_json($response->body);
 
-        # Cache the values if we need them again in the near term
-        # request_cache is on for a single page load but memcache
-        # will keep the data for a full day before reloading.
-        Bugzilla->request_cache->{$key} = $versions;
+        # Memcached keeps the data for a full day before reloading.
         Bugzilla->memcached->set_data({
           key => $key, value => $versions, expires_in => 86_400,
         });
@@ -1040,7 +1043,7 @@ sub fetch_product_versions {
     };
   }
 
-  return $versions;
+  return $request_cache->{$key} = $versions;
 }
 
 sub mojo_user_agent {

extensions/BMO/Extension.pm

@@ -445,7 +445,10 @@ sub cf_hidden_in_product {
 
   # check Bugzilla's built-in visibility controls first
   if ($bug) {
-    my $field = Bugzilla::Field->new({name => $field_name, cache => 1});
+    # Bugzilla->fields is loaded once per request, so prefer it over a
+    # memcached lookup per field (two round trips each for a lookup by name).
+    my $field = Bugzilla->fields({by_name => 1})->{$field_name}
+      // Bugzilla::Field->new({name => $field_name, cache => 1});
     return 1 if $field && !$field->is_visible_on_bug($bug);
   }
 

t/util-fetch-product-versions.t

@@ -0,0 +1,118 @@
+#!/usr/bin/env perl
+# 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.
+use strict;
+use warnings;
+use 5.10.1;
+use lib qw( . lib local/lib/perl5 );
+
+BEGIN { $ENV{LOG4PERL_CONFIG_FILE} = 'log4perl-t.conf' }
+
+use Bugzilla::Test::MockLocalconfig;
+use Bugzilla;
+use Bugzilla::Memcached;
+use Bugzilla::Util qw(fetch_product_versions);
+use Test2::V0;
+
+# fetch_product_versions should consult memcached and the endpoint at most
+# once per product per request, including when the endpoint is failing.
+
+my $endpoint = 'https://product-details.example';
+my %memcached;
+my ($http_calls, $memcached_gets, $http_ok) = (0, 0, 1);
+
+my $mock_bugzilla = mock 'Bugzilla' =>
+  (override => [params => sub { {product_details_endpoint => $endpoint} }]);
+
+my $mock_memcached = mock 'Bugzilla::Memcached' => (
+  override => [
+    get_data => sub { $memcached_gets++; $memcached{$_[1]{key}} },
+    set_data => sub { $memcached{$_[1]{key}} = $_[1]{value} },
+  ]
+);
+
+my $mock_util = mock 'Bugzilla::Util' => (
+  override => [
+    mojo_user_agent => sub {
+      mock {} => (
+        add => [
+          get => sub {
+            $http_calls++;
+            mock {} => (
+              add => [
+                result => sub {
+                  mock {} => (
+                    add => [
+                      is_success => sub {$http_ok},
+                      body       => sub {'{"LATEST_FIREFOX_VERSION":"140.0"}'},
+                      code       => sub {500},
+                      message    => sub {'Internal Server Error'},
+                    ]
+                  );
+                }
+              ]
+            );
+          }
+        ]
+      );
+    }
+  ]
+);
+
+sub new_request {
+  Bugzilla->clear_request_cache;
+  ($http_calls, $memcached_gets) = (0, 0);
+}
+
+subtest 'endpoint failing: one attempt per request' => sub {
+  new_request();
+  $http_ok = 0;
+  is(fetch_product_versions('firefox'), undef, 'undef on failure') for 1 .. 5;
+  is($http_calls,                       1,     'endpoint called once');
+  is($memcached_gets,                   1,     'memcached checked once');
+
+  new_request();
+  fetch_product_versions('firefox');
+  is($http_calls, 1, 'next request tries again');
+};
+
+subtest 'endpoint ok: fetched once, then served from memcached' => sub {
+  new_request();
+  $http_ok = 1;
+  is(
+    fetch_product_versions('firefox'),
+    {LATEST_FIREFOX_VERSION => '140.0'},
+    'versions returned'
+  ) for 1 .. 5;
+  is($http_calls,     1, 'endpoint called once');
+  is($memcached_gets, 1, 'memcached checked once');
+
+  new_request();
+  fetch_product_versions('firefox') for 1 .. 5;
+  is($http_calls,     0, 'later request does not call the endpoint');
+  is($memcached_gets, 1, 'memcached hit is reused within the request');
+};
+
+subtest 'products are cached separately' => sub {
+  new_request();
+  %memcached = ();
+  fetch_product_versions($_) for qw(firefox thunderbird firefox thunderbird);
+  is($http_calls, 2, 'one fetch per product');
+};
+
+subtest 'no endpoint configured' => sub {
+  new_request();
+  %memcached = ();
+  my $saved = $endpoint;
+  $endpoint = '';
+  is(fetch_product_versions('firefox'), undef, 'undef') for 1 .. 3;
+  is($http_calls,                       0,     'no HTTP call');
+  is($memcached_gets,                   1,     'memcached checked once');
+  $endpoint = $saved;
+};
+
+done_testing;