Repository navigation
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 - #2824
Conversation
…e query for every custom field on show_bug.cgi, including fields that aren't tracking flags
There was a problem hiding this comment.
🟡 Changes recommended
The name-cache fast path can override an explicitly supplied ID, causing the same lookup to return different flags depending on whether the cache is populated.
1 open issue
What changed in this PR
This PR reduces database queries during tracking-flag permission checks on show_bug.cgi, particularly for custom fields that are not tracking flags.
Changes:
- Skip non-tracking custom fields using a request-cached name set.
- Reuse cached tracking flags for name lookups, with database fallback.
- Add tests for cached lookups, cache changes, and permission-check filtering.
| File | Description |
|---|---|
extensions/TrackingFlags/t/flag-cache-by-name.t |
Tests name caching and non-tracking-field filtering. |
extensions/TrackingFlags/lib/Flag.pm |
Adds a request-cache index for flag lookups by name. |
extensions/TrackingFlags/Extension.pm |
Filters non-tracking fields before constructing flag objects. |
Review effort: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| 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'); |
There was a problem hiding this comment.
Copilot's id precedence fix has no test, this fails if the !defined $param->{id} check is removed:
| is(scalar @db_lookups, 0, 'no database lookups'); | |
| 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'); |
There was a problem hiding this comment.
Added in 22ecdb4. The new assertions fail if the !defined $param->{id} guard is removed.
Xzzz
left a comment
Xzzz
left a comment
There was a problem hiding this comment.
LGTM, Thanks!
Just one optional test suggestion in flag-cache-by-name.t
…hed names
Addresses review feedback: covers the !defined $param->{id} guard in Flag->new.
No description provided.