diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm index 6958d1cbe0..353630f6e6 100644 --- a/Bugzilla/Util.pm +++ b/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 { diff --git a/extensions/BMO/Extension.pm b/extensions/BMO/Extension.pm index 5caafea963..360259cf0b 100644 --- a/extensions/BMO/Extension.pm +++ b/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); } diff --git a/t/util-fetch-product-versions.t b/t/util-fetch-product-versions.t new file mode 100644 index 0000000000..5180aa2a1f --- /dev/null +++ b/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;