[2.x] fix(tags): restore subquery for permitted tag IDs (regression from #4502)#4649
Conversation
… cache miss PR #4502 replaced the SQL subquery in Tag::scopeWhereHasPermission with a PHP-side filter cached on the actor User instance via WeakMap. The intent was to avoid re-running a correlated subquery per visibility- scope application. In practice the actor User instance is recreated several times during a single request (e.g. when something does User::find($actor->id)), so the WeakMap missed each time and ran: SELECT permission FROM group_permission WHERE group_id IN (?) SELECT id, is_restricted FROM tags on every cache-miss path. On a real forum with ~20 discussions across many distinct authors and groups, this manifested as ~30 redundant group_permission lookups and ~6 redundant tag enumerations per /api/discussions request. See #4605 for the full diagnosis. Restore the SQL subquery approach (`tags.id IN (SELECT id FROM tags AS perm_tags WHERE ...)`) that 1.x used. The DB does the filtering inline, no PHP-side cache to miss, and the subquery is cheap regardless of how many distinct User instances appear in the response. Drop flushPermittedTagCache() — it was only ever scaffolding for the removed WeakMap (introduced in #4502) and the only caller was the test that needed to clear it between cases. Both go. Add PermissionScopeQueryCountTest as a regression guard: asserts the discussion list does not enumerate the tags table more than once per request, even with 10 distinct non-admin authors with varied group memberships. Failed with 4 enumerations on the un-fixed code, passes with 0–1 after the fix. Fixes #4605.
2094497 to
f3e1799
Compare
…-tag interaction The existing DiscussionVisibilityTest covers the parent/child case implicitly via the multi-tag fixtures (discussion 5 with tags [6,7,8], discussion 6 with tags [12,13]) — these are excluded from the user's view because not all their tags are permitted. But the test set doesn't isolate the property I want pinned for the perf-fix in #4649: that an explicit permission on a restricted CHILD tag does not bypass the parent tag's restriction. Add three focused tests: - permission_on_child_does_not_grant_visibility_when_parent_is_off_limits Single-tag discussion where the user has perm on the restricted child but not the restricted parent — must be hidden. - permission_on_child_grants_visibility_when_parent_is_unrestricted Single-tag discussion where the user has perm on the restricted child and the parent is unrestricted — must be visible (the parent_id branch hits the unrestricted-via-global-perm path). - root_restricted_tag_with_explicit_permission_is_visible Single-tag discussion on a restricted ROOT tag (no parent) with explicit permission — must be visible (the orWhereNull('parent_id') branch). These pin the contract of the parent/child clause in Tag::scopeWhereHasPermission so any future refactor that breaks it fails loudly.
|
Update from offline review: did two extra confidence-raising checks before recommending merge. 1. Side-by-side query traceCaptured the visibility-relevant SQL emitted by `/api/discussions` as a non-admin user (with explicit perm on restricted tags 5, 8, 11) on (a) the un-fixed WeakMap code and (b) the fix branch, against an identical fixture. Un-fixed (current 2.x): -- final discussion query embeds the PHP-resolved permitted IDs inline: Fixed (this PR): The two queries define the same permitted set:
These are mathematically identical. The outer parent/child clause structure (`tags.id IN ... AND (tags.parent_id IN ... OR parent_id IS NULL)`) is byte-identical between the two implementations. The fix is structurally a faithful port of the WeakMap path back into SQL. 2. Explicit parent/child visibility testsThe existing visibility tests covered the parent/child case implicitly via multi-tag discussions 5 and 6 in the fixture (excluded because not all their tags are permitted). I added three focused tests that isolate the property at single-tag granularity:
All three pass on both the un-fixed and fix branches, confirming they describe properties that hold under the existing implementation and the new one. |
Summary
Fixes #4605. `Tag::scopeWhereHasPermission` regressed in #4502 from a SQL subquery to a PHP-side filter cached in a `WeakMap<User, …>`. When the actor User instance is recreated mid-request (which happens in several places), the cache misses and re-runs:
```sql
SELECT permission FROM group_permission WHERE group_id IN (?)
SELECT id, is_restricted FROM tags
```
On a real forum (~20 discussions, multiple authors with varied groups) this produced ~30 redundant `group_permission` lookups and ~6 redundant tag enumerations per `/api/discussions` request, contributing measurably to page-render latency (1.27s on 2.0-rc.1 vs 0.38s on 1.8.16 on identical hardware).
Diagnosis
The reporter (#4605) instrumented production traffic and identified two redundant query templates dominating the regression:
Both come from the cache-miss path in `resolvePermittedTagIds`. The WeakMap is keyed by the actor User object, so any flow that re-loads the actor (or compares against a fresh instance) defeats the cache.
Fix
Restore the SQL subquery approach that 1.x used:
```php
// tags.id IN (SELECT id FROM tags AS perm_tags WHERE …)
```
The DB does the filtering inline. No PHP-side cache, no enumeration of the entire tags table, no per-User-instance miss path. The subquery is cheap (a small filtered select on the tags table) and runs once per visibility-scope application — exactly what 1.x did.
`flushPermittedTagCache()` is dropped — it was scaffolding for the WeakMap (introduced in #4502) and the only caller was the test cleaning up after it.
Tests
Regression guard — `PermissionScopeQueryCountTest` exercises the exact bug shape: 10 distinct non-admin authors with varied group memberships each authoring a discussion, then a non-admin actor lists the discussions. Asserts the unconstrained `select id, is_restricted from tags` query — the signature of #4605 — runs at most once per request.
Visibility correctness — Added 3 focused tests in `DiscussionVisibilityTest` to pin the parent/child clause property at single-tag granularity:
All three pass on both the un-fixed and fix branches, confirming they describe properties that hold under the existing implementation as well as the new one.
Query-trace equivalence — Captured the visibility SQL emitted by `/api/discussions` against an identical fixture on (a) the un-fixed code and (b) this fix. The outer parent/child predicate structure is byte-identical, and the permitted-set definitions (`unrestricted ∪ explicit-perm restricted` via PHP filter on un-fixed, vs `is_restricted = false ∪ (is_restricted = true AND id IN explicit-perms)` via SQL subquery on fixed) are mathematically equivalent. Full traces in the analysis comment.
Suite results — 144/144 tags integration tests pass (1 pre-existing unrelated failure on `ShowTest::can_show_tag_with_url_decoded_utf8_slug` is present on `2.x` baseline). 673/673 core integration tests pass. PHPStan clean.
Performance impact
The reporter's measurements suggest restoring 1.x's query count drops `/forum/` HTML render from 1.27s to ~0.85s on their forum (independent of an unrelated polls regression). The remaining headroom belongs to other work outside this PR.