From 61daa2883b91d41e35155f601669536a7cdf0607 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Tue, 10 Nov 2026 10:25:25 -0500 Subject: [PATCH 1/3] Bug 2091444 - TrackingFlags bug_check_can_change_field runs a database query for every custom field on show_bug.cgi, including fields that aren't tracking flags --- extensions/TrackingFlags/Extension.pm | 6 + extensions/TrackingFlags/lib/Flag.pm | 37 +++++ .../TrackingFlags/t/flag-cache-by-name.t | 127 ++++++++++++++++++ 3 files changed, 170 insertions(+) create mode 100644 extensions/TrackingFlags/t/flag-cache-by-name.t diff --git a/extensions/TrackingFlags/Extension.pm b/extensions/TrackingFlags/Extension.pm index 12f577de87..21e7f6d7e9 100644 --- a/extensions/TrackingFlags/Extension.pm +++ b/extensions/TrackingFlags/Extension.pm @@ -589,6 +589,12 @@ sub bug_check_can_change_field { = @$args{qw(bug field old_value new_value priv_results)}; return if $field !~ /^cf_/ or $old_value eq $new_value; + + # Most cf_ fields are not tracking flags. Skip those without a query each. + my $tracking_names = Bugzilla->request_cache->{tracking_flag_name_set} + //= {map { $_ => 1 } Bugzilla->tracking_flag_names}; + return unless $tracking_names->{$field}; + return unless my $flag = Bugzilla::Extension::TrackingFlags::Flag->new({name => $field}); diff --git a/extensions/TrackingFlags/lib/Flag.pm b/extensions/TrackingFlags/lib/Flag.pm index 9322643b88..836a174ed1 100644 --- a/extensions/TrackingFlags/lib/Flag.pm +++ b/extensions/TrackingFlags/lib/Flag.pm @@ -85,9 +85,46 @@ sub new { return $cache->{'tracking_flags'}->{$param}; } + # Lookups by name: reuse a flag already loaded in this request (get_all + # loads all of them) instead of running a query for every lookup. + if ( ref $param eq 'HASH' + && defined $param->{name} + && !exists $param->{condition} + && exists $cache->{'tracking_flags'}) + { + my $flag = _cached_flag_by_name($cache, $param->{name}); + return $flag if $flag; + } + return $class->SUPER::new($param); } +# Returns the flag named $name from the request cache, or undef if it is not +# there. The cache can be partial, so callers must fall back to the database. +sub _cached_flag_by_name { + my ($cache, $name) = @_; + my $flags = $cache->{'tracking_flags'}; + my $index = $cache->{'tracking_flags_by_name'}; + + # Rebuild the name index if the cache was replaced or flags were added or + # removed since it was built. + if (!$index || $index->{flags} != $flags || $index->{count} != keys %$flags) { + $index = $cache->{'tracking_flags_by_name'} = { + flags => $flags, + count => scalar keys %$flags, + by_name => {map { lc($_->name) => $_ } values %$flags}, + }; + } + + my $flag = $index->{by_name}{lc $name} // return undef; + + # update() and remove_from_db() change the cache in place, so only trust the + # index if the flag is still cached under its id and still has this name. + my $current = $flags->{$flag->flag_id} // return undef; + return undef unless $current == $flag && lc($flag->name) eq lc($name); + return $flag; +} + sub new_from_hash { my $class = shift; my $cache = Bugzilla->request_cache->{'tracking_flags'} //= {}; diff --git a/extensions/TrackingFlags/t/flag-cache-by-name.t b/extensions/TrackingFlags/t/flag-cache-by-name.t new file mode 100644 index 0000000000..1565f74163 --- /dev/null +++ b/extensions/TrackingFlags/t/flag-cache-by-name.t @@ -0,0 +1,127 @@ +#!/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; +BEGIN { Bugzilla->extensions } + +use Bugzilla::Constants; +use Bugzilla::Extension::TrackingFlags::Flag; +use Test2::V0; + +# Tracking flag lookups by name should be served from the request cache when +# the flag is already loaded, and fall back to the database otherwise. + +my $FLAG = 'Bugzilla::Extension::TrackingFlags::Flag'; +my @db_lookups; +my $mock_object = mock 'Bugzilla::Object' => ( + override => [ + new => sub { + my ($class, $param) = @_; + push @db_lookups, $param; + return undef; + } + ] +); + +sub flag { bless {id => $_[0], name => $_[1]}, $FLAG } + +sub new_request { + my (%flags) = @_; + Bugzilla->clear_request_cache; + Bugzilla->request_cache->{tracking_flags} + = {map { $_->flag_id => $_ } values %flags} + if %flags; + @db_lookups = (); +} + +subtest 'cached flag found by name without a query' => sub { + my $a = flag(1, 'cf_status_firefox140'); + new_request(a => $a); + is($FLAG->new({name => 'cf_status_firefox140'}), exact_ref($a), + 'cached object'); + is($FLAG->new({name => 'CF_STATUS_FIREFOX140'}), + exact_ref($a), 'case-insensitive, like the database lookup'); + is(scalar @db_lookups, 0, 'no database lookups'); +}; + +subtest 'unknown name falls back to the database' => sub { + new_request(a => flag(1, 'cf_status_firefox140')); + is($FLAG->new({name => 'cf_rank'}), undef, 'not a tracking flag'); + is(\@db_lookups, [{name => 'cf_rank'}], 'one database lookup'); +}; + +subtest 'no request cache: database lookup' => sub { + new_request(); + $FLAG->new({name => 'cf_status_firefox140'}); + is(scalar @db_lookups, 1, 'database lookup'); +}; + +subtest 'index follows changes to the cache' => sub { + my $a = flag(1, 'cf_status_firefox140'); + new_request(a => $a); + $FLAG->new({name => 'cf_status_firefox140'}); # builds the index + + # A flag added to the cache later (e.g. by update()) is found. + my $b = flag(2, 'cf_status_firefox141'); + Bugzilla->request_cache->{tracking_flags}{2} = $b; + is($FLAG->new({name => 'cf_status_firefox141'}), exact_ref($b), 'new flag'); + + # A renamed flag is not returned under its old name. + $a->{name} = 'cf_status_firefox_renamed'; + is($FLAG->new({name => 'cf_status_firefox140'}), undef, 'old name'); + is(scalar @db_lookups, 1, 'old name went to the database'); + + # A flag removed from the cache (remove_from_db()) is not returned. + delete Bugzilla->request_cache->{tracking_flags}{2}; + @db_lookups = (); + is($FLAG->new({name => 'cf_status_firefox141'}), undef, 'removed flag'); + is(scalar @db_lookups, 1, 'removed flag went to the database'); +}; + +subtest 'id lookups are unchanged' => sub { + my $a = flag(1, 'cf_status_firefox140'); + new_request(a => $a); + is($FLAG->new(1), exact_ref($a), 'cached by id'); + is(scalar @db_lookups, 0, 'no database lookups'); +}; + +subtest 'bug_check_can_change_field skips non-tracking fields' => sub { + my $a = flag(1, 'cf_status_firefox140'); + new_request(a => $a); + my $mock_names = mock 'Bugzilla' => + (override => [tracking_flag_names => sub {'cf_status_firefox140'}]); + my $mock_flag = mock $FLAG => (override => [can_set_value => sub {1}]); + + my @results; + my $check = sub { + my ($field) = @_; + Bugzilla::Extension::TrackingFlags->bug_check_can_change_field({ + bug => undef, + field => $field, + old_value => '---', + new_value => 'fixed', + priv_results => \@results, + }); + }; + + $check->('cf_rank'); + is(\@results, [], 'non-tracking field: no result'); + is(\@db_lookups, [], 'non-tracking field: no database lookup'); + + $check->('cf_status_firefox140'); + is(\@results, [{result => PRIVILEGES_REQUIRED_NONE}], 'tracking flag checked'); + is(\@db_lookups, [], 'tracking flag served from the request cache'); +}; + +done_testing; From 52720a9993f02c9709e71c6be0d6cfcc3a860d74 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Tue, 10 Nov 2026 10:49:30 -0500 Subject: [PATCH 2/3] Fix conditional check for tracking flags Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- extensions/TrackingFlags/lib/Flag.pm | 1 + 1 file changed, 1 insertion(+) diff --git a/extensions/TrackingFlags/lib/Flag.pm b/extensions/TrackingFlags/lib/Flag.pm index 836a174ed1..9fa7b2a270 100644 --- a/extensions/TrackingFlags/lib/Flag.pm +++ b/extensions/TrackingFlags/lib/Flag.pm @@ -89,6 +89,7 @@ sub new { # loads all of them) instead of running a query for every lookup. if ( ref $param eq 'HASH' && defined $param->{name} + && !defined $param->{id} && !exists $param->{condition} && exists $cache->{'tracking_flags'}) { From 22ecdb470ba71be81cb5d46a58a6c4ce8f4957c9 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 12 Nov 2026 10:23:47 -0500 Subject: [PATCH 3/3] test: Bug 2091444 - Add test that id lookups take precedence over cached names Addresses review feedback: covers the !defined $param->{id} guard in Flag->new. --- extensions/TrackingFlags/t/flag-cache-by-name.t | 3 +++ 1 file changed, 3 insertions(+) diff --git a/extensions/TrackingFlags/t/flag-cache-by-name.t b/extensions/TrackingFlags/t/flag-cache-by-name.t index 1565f74163..26f8570d18 100644 --- a/extensions/TrackingFlags/t/flag-cache-by-name.t +++ b/extensions/TrackingFlags/t/flag-cache-by-name.t @@ -94,6 +94,9 @@ subtest 'id lookups are unchanged' => sub { new_request(a => $a); is($FLAG->new(1), exact_ref($a), 'cached by id'); is(scalar @db_lookups, 0, 'no database lookups'); + is($FLAG->new({id => 2, name => 'cf_status_firefox140'}), + undef, 'id wins over a cached name'); + is(scalar @db_lookups, 1, 'id lookup went to the database'); }; subtest 'bug_check_can_change_field skips non-tracking fields' => sub {