S147 — give DLNA a real offset path, and remove S97's clamp in the same commit - #574
Merged
Conversation
…me commit `LibraryBridge::getLibraryChildCount()` counted with the unbounded `countAllByType()` while `getLibraryItems()` listed with `getAllByType()`'s default `LIMIT 100`, and `ContentDirectory::browseChildren()` applied the renderer's `StartingIndex` in PHP to that already-truncated list. Any index past the truncation point returned an empty page while `TotalMatches` still advertised the whole container. Measured read-only on prod 2026-07-27: Video advertised 10,718 movies and 26,389 episodes against pages of 100 (107x, 264x); Audio left 2,656 of 4,656 artists unreachable by any navigation. `StartingIndex` is now resolved in SQL on every branch: - `LibraryBridge::getLibraryItems()` takes an offset and spends it against the same per-type counts the container advertises, so a page straddling the movie -> series boundary is contiguous. - `MusicLibraryService::getAlbumMediaItemIdsForArtist()` / `getTrackMediaItemIdsForAlbum()` take an `$offset`, and gained COUNT twins. - `ItemRepository::findByParentPage()` / `countByParent()` do the same for the `parent_id` drill-down. - `browseChildren()` no longer slices; it asks for the requested window and reads `TotalMatches` from a COUNT sharing the listing's predicate. S97's clamp is removed HERE and not later: a renderer told "2,000" stops at 2,000 however good the paging beneath it is. The advertised count is the true unbounded total again and `LibraryBridge::MAX_PAGE_ROWS` is a PAGE size. All three docblocks S97 wrote asserting the bound are rewritten in this commit (`LibraryBridge::getLibraryChildCount()`, `MusicLibraryService::getArtistsWithMediaItemCount()`, and the `LibraryBridgeTest` test that pinned the clamp). Order across page boundaries: `getAllByType()` ordered by `sort_title, name` and `findByParent()` by `name`; neither is unique, and with ties MySQL arranges the tied rows differently per LIMIT+OFFSET. Measured on MySQL 8.0.46, 400 rows sharing one name paged 50 at a time: 400 returned, only 372 distinct. Both queries now end their ORDER BY on the `id` PRIMARY KEY. A real-MySQL test pages a whole container and asserts the concatenated pages equal the unpaged listing row for row -- a unit double that models LIMIT/OFFSET with array_slice() is a total order by construction and could not have caught this. DLNA stays off by default and unauthenticated; no change there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 1 medium |
| BestPractice | 3 medium 1 minor |
| Documentation | 18 minor |
| ErrorProne | 8 medium |
| CodeStyle | 65 minor |
| Comprehensibility | 4 minor |
🟢 Metrics 157 complexity · 0 duplication
Metric Results Complexity 157 Duplication 0
🟢 Coverage 92.47% diff coverage · +0.08% coverage variation
Metric Results Coverage variation ✅ +0.08% coverage variation (-1.00%) Diff coverage ✅ 92.47% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (1bf710b) 62199 40438 65.01% Head commit (0ff8be8) 62297 (+98) 40551 (+113) 65.09% (+0.08%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#574) 146 135 92.47% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #574 +/- ##
============================================
+ Coverage 65.82% 65.90% +0.07%
- Complexity 21069 21101 +32
============================================
Files 657 657
Lines 66262 66366 +104
============================================
+ Hits 43620 43741 +121
+ Misses 22642 22625 -17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…nt once Four findings from the S147 per-step review, nothing else. 1. TWO SURVIVING MUTANTS. Neither `countAlbumMediaItemsForArtist()` nor `countTrackMediaItemsForAlbum()` was pinned by any test — both appear in the suite only through `createMock(MusicLibraryService::class)`. Dropping `AND al.media_item_id IS NOT NULL` from the album count left the ENTIRE 7,702-test suite green against real MySQL, and that mutant is the S97 defect one level down: the artist drill-down would advertise more albums than it can deliver. New real-MySQL test `DlnaMusicDrillDownCountIntegrationTest` seeds an artist with one album whose `media_items` mint failed, a second artist, and more tracks off the album under test than on it, then asserts count === listing === minted for both drill-downs, pages both to their advertised count, and re-asserts the whole thing through `LibraryBridge`. The in-memory doubles cannot express a JOIN or a NULL column, so this had to be a real database. 2. `SortCriteria` REGRESSION, and a docblock that stated something false. Master sorted then sliced; the first S147 revision sliced then sorted, so a container UNDER the old bound got the wrong ROWS, not merely the wrong arrangement — a 20-child Video root with `RequestedCount 5` / `-dc:title` returned the five lowest titles reversed. `browseChildren()` now fetches and sorts the whole container before windowing it whenever it fits in one page, which is master's semantics restored over a strictly wider set of containers (master's pre-slice list was itself cut at `LIMIT 100` per type). Past one page the sort stays page-local — a global sort there needs the key in every listing's ORDER BY plus a merge across a category's types — and the docblock now says exactly that instead of "it did before too". 3. `categoryTypeCounts()` ran TWICE per Browse (instrumented: six `countAllByType()` for one Video root), doubling the COUNTs and opening the TOCTOU window its own docblock acknowledges. New `LibraryBridge::browsePage()` resolves the map once and spends it on both the listing and the advertised total. 4. The bridge-less `getChildCount()` re-materialised the entire child list to measure it. It is a `countByParent()` now, sharing the listing's predicate through one `bridgelessParentId()`. Out of scope and untouched, per the brief: finding 5 (proven unreachable — `ON DELETE SET NULL` FKs plus no DLNA profile), the `(type, sort_title, name, id)` index migration, and anything in the music scanner or path lookups. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
S147 — give DLNA a real offset path, and remove S97's clamp in the same commit
A DLNA renderer could not reach past the first page of a large container:
ContentDirectoryadvertised aTotalMatchesit then refused to serve, becauseStartingIndexwas applied by slicing a list that had already been clamped. Three layers had to move together — moving fewer re-creates the bug from the other side.Changes
src/Dlna/LibraryBridge.php— newMAX_PAGE_ROWS,categoryTypeCounts(),getContainerChildCount(),resolveContainerType(); listing/drill-down now take(offset, limit). S97's clamp removed.src/Dlna/ContentDirectory.php—browseChildren()no longer slices;TotalMatchescomes from a real COUNT.src/Media/Library/ItemRepository.php—idtiebreak ongetAllByType(); newfindByParentPage()+countByParent().src/Media/Music/MusicLibraryService.php—$offseton both drill-down readers plus two COUNT twins.The advertised count and the listing are now derived from one
categoryTypeCounts()map, so they cannot disagree.The ordering landmine, reproduced on real MySQL
Paging without a unique tiebreak silently drops and duplicates rows across page boundaries. MySQL 8.0.46, 400 rows sharing one name, walked 50 at a time:
ORDER BYsort_title, namesort_title, name, idAt page sizes 100 and 137 the same fixture returned 400/400 even while broken, so the test walks at 50 deliberately and says why in a comment.
Why the integration test is load-bearing
With the tiebreak deleted (mutation M1) the entire 7,505-test Unit suite still passes while the integration test is RED. The unit doubles page with
array_slice(), which is a total order by construction, so they are structurally incapable of expressing this failure. Same family as the S145 M10 survivor.Mutation matrix — 10 written, 10 RED, no survivors
M1
getAllByTypetiebreak · M2findByParentPagetiebreak · M3 re-instate the S97 clamp (7 RED) · M4TotalMatches = page· M5 per-type offset → 0 · M6StartingIndexnot forwarded · M7 music drill-down offset dropped · M8 album reader OFFSET bind removed · M9 re-add the PHP slice · M10$skipnot reset across a type boundary.M7 and M10 had no covering test; both were written before the run, not in response to a survivor. Committed before mutating; every restore by md5-verified file copy.
Deep OFFSET — measured, not assumed
Prod
EXPLAIN(read-only):key: idx_type,Extra: Using filesort— there is no(type, sort_title, …)index, so the sort is paid on page 1 too. Timed at prod scale (26,389 episodes):LIMIT 100 OFFSET 0= 319 ms,LIMIT 100 OFFSET 26289= 500 ms — ~1.6×, not orders of magnitude.countAllByType= 6.3 ms.Follow-up measured but deliberately not taken (needs a migration, out of scope):
INDEX (type, sort_title, name, id)removes the filesort — deepest page 500 → 104 ms, page 1 319 → 2.3 ms.Notes
findByParent()was left alone on purpose: two test doubles override its signature (AdminMergeControllerTest:464,SeriesMergerTest:570) and widening the parent's parameter list would fatal them.SortCriteriare-orders within the page only. It did before too (it ran on the already-truncated list), so this is not a regression — but it is now stated inbrowseChildren()'s docblock instead of being left implicit.Gates
phpcs0 errors ·phpstan --level=9[OK] No errors·psalmNo errors found!, no baseline ·phpunit7702 tests / 49995 assertions (no DB) and 7702 / 58848 / 7 skipped against a throwaway MySQL.🤖 Generated with Claude Code