diff --git a/CHANGELOG.md b/CHANGELOG.md index 909161c0..e2c04ef8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -672,6 +672,65 @@ and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0. ### Fixed +- **DLNA advertised far more children than it could deliver on EVERY root, and none + of them could be paged** (S147). `LibraryBridge::getLibraryChildCount()` counted + with the unbounded `ItemRepository::countAllByType()` while + `getLibraryItems()` listed with `getAllByType()`'s default `LIMIT 100`, and + `ContentDirectory::browseChildren()` then applied the renderer's `StartingIndex` + **in PHP to that already-truncated list** — so any index past the truncation + point returned an empty page while `TotalMatches` still advertised the whole + container. Measured read-only on production 2026-07-27: Video advertised + **10,718** movies and **26,389** episodes against pages of 100 (**107x** and + **264x**), and the Audio root — whose advertised count S97 had already clamped to + 2,000 to stop it lying — left **2,656 of 4,656 artists unreachable by any + navigation**. + + `StartingIndex` is now resolved in **SQL** on every branch: + `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); the `music_*` drill-downs and the `parent_id` drill-down + (`ItemRepository::findByParentPage()`) take one too; and `browseChildren()` no + longer slices — it asks for the requested window and reads `TotalMatches` from a + COUNT that shares the listing's predicate. + + **S97's clamp is removed in the same change**, which is the half that makes this + a fix rather than a swap: a renderer told "2,000" stops at 2,000 however good the + paging underneath is, so the advertised count is the true unbounded total again + and `LibraryBridge::MAX_PAGE_ROWS` is a **page size**, not a ceiling. + + Ordering across page boundaries is the other half. `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 with 400 rows sharing one name, paged 50 at a time: **400 rows + returned, only 372 distinct** — 28 silently repeated, 28 never returned at all. + Both queries now end their `ORDER BY` on the `id` PRIMARY KEY, making the order + total; a real-MySQL test pages a whole container and asserts the concatenated + pages equal the unpaged listing row for row. + + `SortCriteria` is honoured **globally** for any container that fits in one page + (`LibraryBridge::MAX_PAGE_ROWS`, 2,000): the whole container is fetched and sorted + before the window is taken, so the criteria decides *which* rows the page contains. + The first revision of this change sorted only the page it had already fetched, + which on a 20-child Video root with `RequestedCount 5` and `-dc:title` returned the + five alphabetically **lowest** titles reversed instead of the five highest — a + regression against the previous sort-then-slice behaviour, now pinned by a test. + Past 2,000 children the sort still applies within the returned page only; making it + global there needs the sort key in every listing's `ORDER BY` **and** a merge across + a category's types, and is deliberately left out rather than trading pageability + for it. + + One `Browse` now resolves the category counts **once**. Fetching the page and the + advertised total went through two entry points that each recomputed them, so a + single Video-root browse issued `countAllByType()` six times (`movie, series, + video` twice over) — double the COUNTs on an unauthenticated path, and a window in + which a row written between the two evaluations made the advertised total disagree + with the offsets the listing had already spent. The bridge-less `TotalMatches` is + likewise a `countByParent()` now, instead of re-listing the whole container purely + to measure it. + + DLNA remains **off by default and unauthenticated**, so this is a functional gap + closed, not a security change. + - **A retagged music track could NEVER be re-parented, and `rescan` could not reach the tracks that needed it** (S145). Editing a file's `ALBUM` or `ARTIST` tag left `music_tracks.album_id` / `artist_id` pointing at the *old* album permanently. diff --git a/src/Dlna/ContentDirectory.php b/src/Dlna/ContentDirectory.php index a66a1c20..f10d05d3 100644 --- a/src/Dlna/ContentDirectory.php +++ b/src/Dlna/ContentDirectory.php @@ -365,6 +365,52 @@ private function browseMetadata(string $objectId, array $item, string $filter): /** * Browse direct children of a container. * + * ⚠ **`StartingIndex` is resolved in SQL, not by slicing here (S147).** This + * method used to fetch the container's children, count THAT list into + * `TotalMatches`, and `array_slice()` it — but the list it was slicing was + * already truncated upstream (`getAllByType()`'s `LIMIT 100`, or the music + * reader's row cap), so every `StartingIndex` past the truncation point + * returned an empty page while `TotalMatches` still advertised the whole + * container. `getChildren()` now asks for exactly the requested window and + * `TotalMatches` comes from a separate COUNT that shares the listing's + * predicate, so the two agree at every index. + * + * ## What `SortCriteria` is and is NOT guaranteed to do + * + * Measured, not assumed — an earlier revision of this docblock claimed the + * sort behaviour was unchanged by S147 and that was **false for containers + * smaller than the old truncation bound**. Master's order was sort → slice, + * so for a container that fitted inside the pre-S147 fetch the WHOLE + * container was sorted before the window was taken; the first revision of + * S147 took the window first and sorted only that page, which on a 20-child + * Video root with `RequestedCount 5` and `-dc:title` returned the five + * alphabetically LOWEST titles reversed instead of the five highest. + * + * What holds now: + * + * - **Container of at most {@see LibraryBridge::MAX_PAGE_ROWS} children:** + * the sort is GLOBAL. Every child is fetched, sorted, and only then + * windowed, so the criteria decides *which* rows the page contains. That + * is master's semantics restored, and it holds for a strictly wider set of + * containers than master ever managed — master sorted a list already cut + * at `getAllByType()`'s `LIMIT 100` **per type**, so a 150-movie root was + * sorted with 50 movies missing. + * - **Container larger than that:** the sort applies WITHIN the returned + * page only, and `StartingIndex` still selects the rows in the listing's + * own order. Making it global there means carrying the sort key into every + * listing's `ORDER BY` **and** merging across a category's + * {@see LibraryBridge::CATEGORY_TYPES} members, because a root is the + * CONCATENATION of its types — sorting each type's SQL and concatenating + * yields movies-desc-then-series-desc, which is not a globally sorted page. + * That is a distinct piece of work; it is not folded in here, and no + * renderer this server has been driven by sends a `SortCriteria` at all. + * + * ⚠ Cost, stated rather than hidden: a sorted browse of a container LARGER + * than one page resolves the data layer twice — once to learn it does not + * fit, once for the real window. Only that combination pays it; the unsorted + * path (every browse this server has actually served) resolves once, which is + * itself a halving of what it cost before this round. + * * @return array */ private function browseChildren( @@ -374,25 +420,85 @@ private function browseChildren( int $requestedCount, string $sortCriteria ): array { - // Get children based on object ID type - $children = $this->getChildren($objectId); + $startingIndex = max(0, $startingIndex); - // Sort if needed - if (!empty($sortCriteria)) { - $children = $this->sortItems($children, $sortCriteria); + if ($sortCriteria === '') { + // One window, one count, one categoryTypeCounts() — see childPage(). + $page = $this->childPage($objectId, $startingIndex, $requestedCount); + + return $this->childrenResult($page['items'], $page['total']); } - $this->totalMatches = count($children); + // Sorted: take the whole container FIRST when it fits in one page, so the + // criteria selects which rows the window contains rather than merely how + // the window is arranged. + $whole = $this->childPage($objectId, 0, LibraryBridge::MAX_PAGE_ROWS); + if ($whole['total'] <= LibraryBridge::MAX_PAGE_ROWS) { + $sorted = $this->sortItems($whole['items'], $sortCriteria); - // Apply pagination - $resultItems = array_slice($children, $startingIndex, $requestedCount > 0 ? $requestedCount : null); + return $this->childrenResult( + array_slice($sorted, $startingIndex, $requestedCount > 0 ? $requestedCount : null), + $whole['total'] + ); + } - $didl = $this->generateDidl($resultItems); + // Past one page a global sort is not expressible here (see the docblock). + // The container stays PAGEABLE, which matters more: sorting the page is a + // residual wrong, returning empty pages past row 2,000 is the S97 defect. + $page = $this->childPage($objectId, $startingIndex, $requestedCount); + + return $this->childrenResult($this->sortItems($page['items'], $sortCriteria), $page['total']); + } + + /** + * One page of a container together with the container's TOTAL, resolved in a + * single pass over the data layer. + * + * @param string $objectId The container to page. + * @param int $startingIndex DLNA `StartingIndex`. + * @param int $requestedCount DLNA `RequestedCount`; 0 means "as many as you + * can", answered with {@see LibraryBridge::MAX_PAGE_ROWS}. + * @return array{items: array>, total: int} + */ + private function childPage(string $objectId, int $startingIndex, int $requestedCount): array + { + if ($this->libraryBridge !== null) { + // The object's type is handed over rather than re-discovered: + // `browse()` has ALREADY resolved and cached this row in order to + // choose BrowseMetadata vs BrowseDirectChildren. + $resolved = $this->resolveObjectId($objectId); + $resolvedType = is_string($resolved['type'] ?? null) ? $resolved['type'] : null; + + return $this->libraryBridge->browsePage( + $objectId, + $resolvedType, + max(0, $startingIndex), + $requestedCount > 0 ? $requestedCount : LibraryBridge::MAX_PAGE_ROWS + ); + } return [ - 'Result' => $didl, - 'NumberReturned' => count($resultItems), - 'TotalMatches' => $this->totalMatches, + 'items' => $this->getChildren($objectId, $startingIndex, $requestedCount), + 'total' => $this->bridgelessChildCount($objectId), + ]; + } + + /** + * The `Browse` envelope, so the two windowing branches above cannot drift + * apart on what they report. + * + * @param array> $items The page being returned. + * @param int $total The container's total child count. + * @return array + */ + private function childrenResult(array $items, int $total): array + { + $this->totalMatches = $total; + + return [ + 'Result' => $this->generateDidl($items), + 'NumberReturned' => count($items), + 'TotalMatches' => $total, 'UpdateID' => $this->systemUpdateId, ]; } @@ -400,9 +506,13 @@ private function browseChildren( /** * Get children of a container. * + * @param string $objectId The container to list. + * @param int $startingIndex DLNA `StartingIndex`. + * @param int $requestedCount DLNA `RequestedCount`; 0 means "as many as you + * can", which the bridge answers with {@see LibraryBridge::MAX_PAGE_ROWS}. * @return array> */ - private function getChildren(string $objectId): array + private function getChildren(string $objectId, int $startingIndex = 0, int $requestedCount = 0): array { // Use LibraryBridge if available for real data. // @@ -416,7 +526,12 @@ private function getChildren(string $objectId): array $resolved = $this->resolveObjectId($objectId); $resolvedType = is_string($resolved['type'] ?? null) ? $resolved['type'] : null; - return $this->libraryBridge->getContainerChildren($objectId, $resolvedType); + return $this->libraryBridge->getContainerChildren( + $objectId, + $resolvedType, + max(0, $startingIndex), + $requestedCount > 0 ? $requestedCount : LibraryBridge::MAX_PAGE_ROWS + ); } // NOTE: two placeholder branches used to sit here, dispatching to @@ -431,14 +546,61 @@ private function getChildren(string $objectId): array // discard it. Bridge-less callers now fall through to real container // resolution below. - // Handle item-based containers + // Handle item-based containers. This is the BRIDGE-LESS path only — + // tests and any caller that builds a ContentDirectory directly — and it + // keeps the in-PHP window because there is no paging primitive behind it + // and nothing here is production-scale. Behaviour is byte-identical to + // what browseChildren() used to do for it. + $children = $this->bridgelessChildren($objectId); + + return array_slice($children, max(0, $startingIndex), $requestedCount > 0 ? $requestedCount : null); + } + + /** + * Children of a container when NO {@see LibraryBridge} is wired. + * + * @return array> + */ + private function bridgelessChildren(string $objectId): array + { + $parentId = $this->bridgelessParentId($objectId); + + return $parentId === null ? [] : $this->itemRepository->findByParent($parentId); + } + + /** + * `TotalMatches` for the bridge-less path — a COUNT, not the length of a + * second copy of the list. + * + * This used to be `count($this->bridgelessChildren($objectId))`, which + * materialised the whole (unbounded) child list a second time purely to + * measure it, immediately after `getChildren()` had already built it. It + * shares `bridgelessChildren()`'s predicate exactly — same container guard, + * same `parent_id` — so the promise and the deliverable still agree. + */ + private function bridgelessChildCount(string $objectId): int + { + $parentId = $this->bridgelessParentId($objectId); + + return $parentId === null ? 0 : $this->itemRepository->countByParent($parentId); + } + + /** + * The `parent_id` a bridge-less drill-down reads, or NULL when `$objectId` is + * not a container at all. + * + * One definition so the listing and the count above cannot diverge. + */ + private function bridgelessParentId(string $objectId): ?string + { $item = $this->resolveObjectId($objectId); - if ($item !== null && ($item['type'] ?? '') === 'container') { - $parentId = $item['id'] ?? ''; - return $this->itemRepository->findByParent(is_string($parentId) ? $parentId : ''); + if ($item === null || ($item['type'] ?? '') !== 'container') { + return null; } - return []; + $parentId = $item['id'] ?? ''; + + return is_string($parentId) ? $parentId : ''; } /** diff --git a/src/Dlna/LibraryBridge.php b/src/Dlna/LibraryBridge.php index 0a7f5255..613f424a 100644 --- a/src/Dlna/LibraryBridge.php +++ b/src/Dlna/LibraryBridge.php @@ -93,12 +93,14 @@ public function __construct( * 2026-07-27): * * - {@see getLibraryChildCount()} sums {@see ItemRepository::countAllByType()} - * over all six types and advertises **76,727+** children, while - * {@see getLibraryItems()} returns {@see ItemRepository::getAllByType()}'s + * over all six types and advertised **76,727+** children, while + * {@see getLibraryItems()} returned {@see ItemRepository::getAllByType()}'s * default page — **at most 100 per type**. The `` a - * renderer is promised and the list it receives disagree by three orders of - * magnitude. - * - The 100 albums and 100 tracks it does return are an arbitrary + * renderer was promised and the list it received disagreed by three orders + * of magnitude. (S147 closed the disagreement itself — the listing is now + * paged, see {@see getLibraryItems()} — but the flattening below is a + * separate wrong, and this narrowing is still what fixes it.) + * - The 100 albums and 100 tracks it did return were an arbitrary * title-ordered slice with no relation to the 100 artists beside them. * * That could not be fixed by nesting them: {@see getLibraryItems()} calls @@ -143,6 +145,29 @@ public function __construct( */ private const FIND_BY_IDS_CHUNK = 500; + /** + * The most CDS objects one `Browse` response may carry. + * + * This is a PAGE size, not a ceiling on the container. Every browsable + * container in this bridge now answers `StartingIndex` from SQL + * ({@see getLibraryItems()}, {@see getMusicChildren()}, + * {@see ItemRepository::findByParentPage()}), and the count it advertises is + * the true unbounded total, so a renderer reaches row 26,388 of the Video + * root by asking for it — which is precisely what it could NOT do while the + * bound lived in the listing. + * + * UPnP says `RequestedCount = 0` means "as many as you can". This is that + * answer: `NumberReturned` comes back smaller than `TotalMatches` and a + * renderer continues from where it left off, which is the ordinary paging + * contract every CDS client already implements. + * + * 2,000 is deliberately {@see MusicLibraryService::MAX_EMBEDDED_ROWS}'s + * value: `getArtistMediaItemIds()` clamps its own `$limit` to that constant, + * so a larger page here would be silently truncated by it and the arithmetic + * in {@see getLibraryItems()} would start skipping rows. + */ + public const MAX_PAGE_ROWS = MusicLibraryService::MAX_EMBEDDED_ROWS; + /** * Root containers representing the media library categories, with accurate * child counts read from the database. @@ -190,54 +215,80 @@ public function getRootContainers(): array } /** - * Get child count for a library type. - * - * @param string $libraryType The library type (video, audio, images) - * @return int Number of items in the library - * - * @since 0.12.0 + * How many rows each `media_items.type` in a category contributes, in the + * order {@see getLibraryItems()} concatenates them. + * + * ONE source for both the advertised `childCount` and the offset arithmetic + * of the listing. That is the whole mechanism by which "advertised count and + * deliverable list agree at every StartingIndex" holds: the listing walks + * this very map to decide which type a global offset lands in. + * + * ⚠ **Resolve it ONCE per `Browse` — {@see browsePage()} is why that method + * exists.** While `ContentDirectory` fetched the page and the count through + * two separate entry points, this ran twice per request (six `countAllByType()` + * for one Video-root browse, instrumented 2026-07-27) and the two evaluations + * could disagree whenever a row was written between them — not a theoretical + * window on an estate that has a library scan in flight much of the time. + * `browsePage()` spends one map on both, so on the browse path they cannot + * disagree at all. {@see getLibraryChildCount()} and {@see getLibraryItems()} + * remain independent entry points for callers that want only one of the two + * (`getRootContainers()` wants only the count). + * + * @param string $libraryType One of {@see CATEGORY_TYPES}'s keys. + * @return array `media_items.type` => row count, insertion-ordered. */ - private function getLibraryChildCount(string $libraryType): int + private function categoryTypeCounts(string $libraryType): array { $types = self::CATEGORY_TYPES[$libraryType] ?? null; if ($types === null) { - return 0; + return []; } - $total = 0; + $counts = []; foreach ($types as $type) { // The artist count must come from the SAME source as the artist // listing (S97), or the advertised childCount over-counts by every // `media_items[artist]` row that no `music_artists` row points at — // the orphan shape MusicLibraryScanner's adoption path exists to // reclaim. - // - // It is then clamped to MusicLibraryService::MAX_EMBEDDED_ROWS - // because that constant, not the row count, is the hard ceiling on - // what {@see getLibraryItems()} can actually hand over: it calls - // getArtistMediaItemIds(), whose default `$limit` IS that constant. - // Unclamped, a production-shaped library (4,656 artists with a - // `media_items` row, measured read-only 2026-07-27) advertises 4,656 - // and delivers 2,000. - // - // ⚠ RESIDUAL BOUND — the clamp makes the advertised number honest, - // it does NOT make the missing artists reachable. Artists ranked - // past MAX_EMBEDDED_ROWS in `ORDER BY COALESCE(sort_name, name)` are - // not browsable over DLNA at all: getLibraryItems() takes no offset, - // and ContentDirectory::browseChildren() applies `StartingIndex` in - // PHP to the list this bridge already truncated, so a renderer - // cannot page past the cap either. Giving the Audio root a real - // paging path is a separate, filed step — do not paper over it by - // raising the ceiling on one side only. - $total += $type === 'artist' && $this->musicLibrary !== null - ? min( - $this->musicLibrary->getArtistsWithMediaItemCount(), - MusicLibraryService::MAX_EMBEDDED_ROWS - ) + $counts[$type] = $type === 'artist' && $this->musicLibrary !== null + ? $this->musicLibrary->getArtistsWithMediaItemCount() : $this->itemRepository->countAllByType($type); } - return $total; + return $counts; + } + + /** + * Get child count for a library type. + * + * ⚠ **This number is UNBOUNDED, and that is now correct** — read this before + * re-introducing a clamp. S97 clamped the artist term to + * {@see MusicLibraryService::MAX_EMBEDDED_ROWS} because the listing stopped + * dead there: `getLibraryItems()` took no offset and + * `ContentDirectory::browseChildren()` applied `StartingIndex` in PHP to a + * list this bridge had already truncated, so on production's 4,656 artists a + * renderer was promised 4,656, handed 2,000, and could not page to the rest + * by any navigation. The clamp made the promise honest; it did not make the + * artists reachable, and it left the far larger lie on the OTHER roots + * untouched (movies advertised 10,718 against a `LIMIT 100` listing — 107x; + * episodes 26,389 — 264x; all measured read-only 2026-07-27). + * + * S147 removed the bound instead of the honesty: {@see getLibraryItems()} + * takes an offset and resolves it in SQL, {@see MAX_PAGE_ROWS} is a page + * size, and the true total is what a renderer needs in order to know there + * is a second page to ask for. 🛑 Clamping this to the page size again would + * re-create the S97 defect from the other side — the renderer would stop + * after one page believing it had seen everything. + * + * @param string $libraryType The library type (video, audio, images) + * @return int Number of items in the library + * + * @since 0.12.0 + */ + private function getLibraryChildCount(string $libraryType): int + { + return array_sum($this->categoryTypeCounts($libraryType)); } /** @@ -248,6 +299,11 @@ private function getLibraryChildCount(string $libraryType): int * `album` `media_items` row never carries children under `parent_id`, so * `findByParent()` on one returns an empty container and the browse dead-ends. * + * ⚠ **Every branch resolves `$offset`/`$limit` in SQL.** Nothing above this + * method may re-slice the result — {@see ContentDirectory::browseChildren()} + * used to, against a list that was already truncated, which is what made + * `StartingIndex` useless past the first page (S147). + * * @param string $objectId The object ID of the container * @param string|null $objectType The container's `media_items.type`, when the * caller has already resolved the row. {@see ContentDirectory::browse()} @@ -255,32 +311,156 @@ private function getLibraryChildCount(string $libraryType): int * `browseChildren()` — so passing it here saves this class a second * `findById()` on EVERY drill-down, music or not. NULL means "not * known", and the type is looked up as before. + * @param int $offset DLNA `StartingIndex` — rows to skip within the container. + * @param int $limit DLNA `RequestedCount`, clamped to + * `[1, self::MAX_PAGE_ROWS]`. Callers map `RequestedCount = 0` + * ("as many as you can") onto {@see MAX_PAGE_ROWS}. * @return array> Array of child items * * @since 0.12.0 */ - public function getContainerChildren(string $objectId, ?string $objectType = null): array - { - $this->logger?->debug('LibraryBridge: Getting container children', ['object_id' => $objectId]); + public function getContainerChildren( + string $objectId, + ?string $objectType = null, + int $offset = 0, + int $limit = self::MAX_PAGE_ROWS + ): array { + $this->logger?->debug('LibraryBridge: Getting container children', [ + 'object_id' => $objectId, + 'offset' => $offset, + 'limit' => $limit, + ]); + + $offset = max(0, $offset); + $limit = min(max(1, $limit), self::MAX_PAGE_ROWS); // Handle library containers if (strpos($objectId, 'library-') === 0) { $libraryType = substr($objectId, 8); - return $this->getLibraryItems($libraryType); + return $this->getLibraryItems($libraryType, $offset, $limit); } // Music drill-down: artist → albums → tracks, read from `music_*`. - $musicChildren = $this->getMusicChildren($objectId, $objectType); + $musicChildren = $this->getMusicChildren($objectId, $objectType, $offset, $limit); if ($musicChildren !== null) { return $musicChildren; } // Handle regular parent-based children - $items = $this->itemRepository->findByParent($objectId); + $items = $this->itemRepository->findByParentPage($objectId, $limit, $offset); return array_map(fn($item) => $this->itemToCdsObject($item), $items); } + /** + * How many children a container has IN TOTAL — the `TotalMatches` a `Browse` + * response must carry. + * + * Deliberately NOT `count(getContainerChildren(...))`: that is the size of + * one page, and a renderer that is told the page size is the total stops + * after one page. Each branch here shares its listing counterpart's + * predicate exactly, so the promise and the deliverable agree at every + * `StartingIndex`. + * + * @param string $objectId The object ID of the container. + * @param string|null $objectType The container's already-resolved + * `media_items.type`; NULL triggers a lookup, exactly as in + * {@see getContainerChildren()}. + * @return int Total number of children. + */ + public function getContainerChildCount(string $objectId, ?string $objectType = null): int + { + if (strpos($objectId, 'library-') === 0) { + return $this->getLibraryChildCount(substr($objectId, 8)); + } + + if ($this->musicLibrary !== null && $objectId !== '') { + $resolvedType = $this->resolveContainerType($objectId, $objectType); + + if ($resolvedType === 'artist') { + return $this->musicLibrary->countAlbumMediaItemsForArtist($objectId); + } + + if ($resolvedType === 'album') { + return $this->musicLibrary->countTrackMediaItemsForAlbum($objectId); + } + } + + return $this->itemRepository->countByParent($objectId); + } + + /** + * ONE page of a container **and** the container's total, resolved together. + * + * ⚠ **The pairing is the point, not a convenience.** `browseChildren()` used + * to call {@see getContainerChildren()} and {@see getContainerChildCount()} + * separately, and on a root category BOTH of them end in + * {@see categoryTypeCounts()} — so a single `Browse` of the Video root issued + * `countAllByType()` **six** times (movie, series, video, twice over, + * instrumented 2026-07-27) where three do the work. Worse, the two calls + * resolved the counts at two different instants, so a row written between + * them made the advertised `TotalMatches` disagree with the offsets the + * listing had already spent — and on this estate a scan is in flight much of + * the time, which is exactly the window {@see categoryTypeCounts()}'s own + * docblock warns about. Resolving both from one map closes it. + * + * @param string $objectId The container to page. + * @param string|null $objectType Its already-resolved `media_items.type`, if + * the caller has one; NULL costs one `findById()`, exactly as in + * {@see getContainerChildren()}. + * @param int $offset DLNA `StartingIndex`. + * @param int $limit DLNA `RequestedCount`, clamped to `[1, MAX_PAGE_ROWS]`. + * @return array{items: array>, total: int} The + * requested window and the container's TRUE total. + */ + public function browsePage( + string $objectId, + ?string $objectType = null, + int $offset = 0, + int $limit = self::MAX_PAGE_ROWS + ): array { + if (strpos($objectId, 'library-') === 0) { + // Resolved ONCE and spent twice — see the warning above. + $counts = $this->categoryTypeCounts(substr($objectId, 8)); + + return [ + 'items' => $this->itemsFromTypeCounts( + $counts, + max(0, $offset), + min(max(1, $limit), self::MAX_PAGE_ROWS) + ), + 'total' => array_sum($counts), + ]; + } + + // Resolved once here so neither call below pays for a second findById(). + $resolvedType = $objectId === '' ? '' : $this->resolveContainerType($objectId, $objectType); + + return [ + 'items' => $this->getContainerChildren($objectId, $resolvedType, $offset, $limit), + 'total' => $this->getContainerChildCount($objectId, $resolvedType), + ]; + } + + /** + * The container's `media_items.type`, using the caller's value when it has + * one and only then paying for a `findById()`. + * + * @param string $objectId A `media_items` UUID. + * @param string|null $objectType The caller's already-resolved type, if any. + */ + private function resolveContainerType(string $objectId, ?string $objectType): string + { + if ($objectType !== null) { + return $objectType; + } + + $item = $this->itemRepository->findById($objectId); + $type = $item['type'] ?? null; + + return is_string($type) ? $type : ''; + } + /** * Resolves the children of a music container (`artist` → albums, `album` → * tracks) through the authoritative `music_*` tables. @@ -295,25 +475,30 @@ public function getContainerChildren(string $objectId, ?string $objectType = nul * `$objectId` is not a music container — the caller then falls through * to the ordinary `parent_id` drill-down. NULL and `[]` are deliberately * distinct: `[]` means "this artist really has no browsable albums". + * @param int $offset Rows to skip, resolved by SQL `OFFSET` — both listings + * end their `ORDER BY` on a PRIMARY KEY, so the order is TOTAL and a + * paged walk can neither drop nor repeat an album/track. + * @param int $limit Page size, already clamped by the caller. */ - private function getMusicChildren(string $objectId, ?string $objectType = null): ?array - { + private function getMusicChildren( + string $objectId, + ?string $objectType = null, + int $offset = 0, + int $limit = self::MAX_PAGE_ROWS + ): ?array { if ($this->musicLibrary === null || $objectId === '') { return null; } - if ($objectType === null) { - $item = $this->itemRepository->findById($objectId); - $objectType = is_string($item['type'] ?? null) ? $item['type'] : ''; - } + $objectType = $this->resolveContainerType($objectId, $objectType); if (!in_array($objectType, self::MUSIC_CONTAINER_TYPES, true)) { return null; } $childIds = $objectType === 'artist' - ? $this->musicLibrary->getAlbumMediaItemIdsForArtist($objectId) - : $this->musicLibrary->getTrackMediaItemIdsForAlbum($objectId); + ? $this->musicLibrary->getAlbumMediaItemIdsForArtist($objectId, $limit, $offset) + : $this->musicLibrary->getTrackMediaItemIdsForAlbum($objectId, $limit, $offset); return array_map( fn(array $child): array => $this->itemToCdsObject($child), @@ -360,38 +545,101 @@ private function findByIdsChunked(array $ids): array } /** - * Get media items from a specific library type. + * One page of a root category's children. + * + * A category is the CONCATENATION of its {@see CATEGORY_TYPES} members in a + * fixed order (`video` = movies, then series, then loose videos), so + * `StartingIndex` is a global index into that concatenation, not into any one + * type. The walk below spends the offset against + * {@see categoryTypeCounts()} — the same counts the container advertises — + * skipping whole types until it lands inside one, then hands the remainder to + * that type's own SQL `OFFSET`. Once a type is entered, `$skip` is zeroed and + * later types start from their first row, which is what makes a page that + * straddles a type boundary contiguous. + * + * ⚠ **Why this had to be SQL and not `array_slice()`.** Before S147 this + * method took no offset at all: it returned `getAllByType()`'s default + * `LIMIT 100` per type, and `ContentDirectory::browseChildren()` then applied + * `StartingIndex` in PHP to that already-truncated list. On production's + * 26,389 episodes a renderer was promised 26,389 and could reach 100 of them + * — asking for `StartingIndex = 5000` returned nothing at all rather than the + * 5,001st episode. + * + * **Order is preserved across page boundaries** because every per-type + * listing sorts on a TOTAL order: `getAllByType()` ends its `ORDER BY` on the + * `id` PRIMARY KEY, and `getArtistMediaItemIds()` on `music_artists.id`. + * Without that, two rows sharing a sort key have no defined relative order + * and MySQL may return them one way for one `OFFSET` and the other way for + * the next — the classic paged-walk row loss. + * + * `$remaining` is decremented by what was REQUESTED, not by what came back, + * so the offset arithmetic stays aligned with the counts even if the profile + * tag filter drops rows from a music page. A short page is then honest + * (`NumberReturned` < requested) instead of silently shifting every + * subsequent page. * * @param string $libraryType The library type (video, audio, images) + * @param int $offset DLNA `StartingIndex` into the whole category. + * @param int $limit Page size, clamped by the caller to + * `[1, self::MAX_PAGE_ROWS]`. * @return array> Array of media items converted to CDS objects * * @since 0.12.0 */ - private function getLibraryItems(string $libraryType): array + private function getLibraryItems(string $libraryType, int $offset = 0, int $limit = self::MAX_PAGE_ROWS): array { - $types = self::CATEGORY_TYPES[$libraryType] ?? null; - if ($types === null) { - return []; - } + return $this->itemsFromTypeCounts( + $this->categoryTypeCounts($libraryType), + max(0, $offset), + min(max(1, $limit), self::MAX_PAGE_ROWS) + ); + } + /** + * The offset walk of {@see getLibraryItems()}, driven by a counts map the + * caller already has. + * + * Split out so {@see browsePage()} can spend ONE {@see categoryTypeCounts()} + * result on both the listing and the advertised total instead of resolving + * the counts twice per `Browse`. + * + * @param array $counts `media_items.type` => row count, in the + * order the category concatenates them. + * @param int $skip Rows to skip, already clamped to `>= 0`. + * @param int $remaining Page size, already clamped to `[1, MAX_PAGE_ROWS]`. + * @return array> + */ + private function itemsFromTypeCounts(array $counts, int $skip, int $remaining): array + { $objects = []; - foreach ($types as $type) { + foreach ($counts as $type => $available) { + if ($remaining <= 0) { + break; + } + + if ($skip >= $available) { + $skip -= $available; + continue; + } + + $take = min($remaining, $available - $skip); + $items = $type === 'artist' && $this->musicLibrary !== null // Enumerated from `music_artists` (S97), so the root shows exactly // the artists that can actually be drilled into — an orphaned // `media_items[artist]` row is not one of them. // - // Bounded twice over: getArtistMediaItemIds() stops at - // MusicLibraryService::MAX_EMBEDDED_ROWS (the same ceiling - // getLibraryChildCount() clamps the advertised count to), and the - // id resolution is chunked so no single `IN (…)` carries 2,000 - // placeholders. See self::FIND_BY_IDS_CHUNK. - ? $this->findByIdsChunked($this->musicLibrary->getArtistMediaItemIds()) - : $this->itemRepository->getAllByType($type); + // The id resolution is chunked so no single `IN (…)` carries + // MAX_PAGE_ROWS placeholders. See self::FIND_BY_IDS_CHUNK. + ? $this->findByIdsChunked($this->musicLibrary->getArtistMediaItemIds($take, $skip)) + : $this->itemRepository->getAllByType($type, $take, $skip); foreach ($items as $item) { $objects[] = $this->itemToCdsObject($item); } + + $remaining -= $take; + $skip = 0; } return $objects; diff --git a/src/Media/Library/ItemRepository.php b/src/Media/Library/ItemRepository.php index eef89f50..bee8b7ca 100644 --- a/src/Media/Library/ItemRepository.php +++ b/src/Media/Library/ItemRepository.php @@ -751,6 +751,62 @@ public function findByParent(string $parentId): array return $this->hydrateRows($results); } + /** + * One page of a parent's children, in a TOTAL order so paging is stable. + * + * {@see findByParent()} is deliberately left unbounded — a dozen callers + * (metadata matching, marker detection, series merging) want every child and + * two test doubles override its signature. This is the paging twin used by + * the DLNA `BrowseDirectChildren` path, which must answer a `StartingIndex` + * from SQL rather than by truncating in PHP. + * + * ⚠ The `id` tiebreak is load-bearing, not cosmetic. `findByParent()` orders + * by `name` alone; two children sharing a name (two "Episode 1" rows under + * different seasons of a merged series, say) have NO defined relative order, + * and MySQL is free to return them differently on two executions of the same + * statement with different `OFFSET`s. That is exactly how an offset-paged + * walk drops one row and repeats another. `id` is the PRIMARY KEY, so + * `(name, id)` is unique and the order is total. + * + * @param string $parentId The parent media item's unique identifier. + * @param int $limit Page size, clamped to `[1, self::ABSOLUTE_ROW_CEILING]`. + * @param int $offset Rows to skip (negative offsets normalize to 0). + * @return array> Hydrated children, `name` then `id`. + */ + public function findByParentPage(string $parentId, int $limit, int $offset = 0): array + { + $results = $this->db->query( + "SELECT * FROM media_items WHERE parent_id = ? ORDER BY name, id LIMIT ? OFFSET ?", + [ + $parentId, + min(max(1, $limit), self::ABSOLUTE_ROW_CEILING), + $this->normalizeOffset($offset), + ] + ); + + return $this->hydrateRows($results); + } + + /** + * Counts a parent's direct children. + * + * Pairs with {@see findByParentPage()}: the DLNA `TotalMatches` a renderer is + * promised must be the TRUE total, not the size of the page it was handed, + * or it cannot know to ask for a second page. + * + * @param string $parentId The parent media item's unique identifier. + * @return int Number of direct children. + */ + public function countByParent(string $parentId): int + { + $result = $this->db->query( + "SELECT COUNT(*) as count FROM media_items WHERE parent_id = ?", + [$parentId] + ); + + return $this->extractCount($result); + } + /** * Finds all child items for multiple parent media items in a single query. * @@ -867,9 +923,22 @@ public function getByType( */ public function getAllByType(string $type, int $limit = 100, int $offset = 0): array { + // ⚠ The trailing `id` is what makes OFFSET paging safe here, and it is + // NOT redundant with titleOrder()'s `sort_title, name`. Two rows can + // share BOTH (a remake and its original are both "Dune" with the same + // sort key), and their relative order is then undefined — MySQL may + // legitimately return them in one order for `LIMIT 100 OFFSET 0` and the + // other for `LIMIT 100 OFFSET 100`, which drops one row from the paged + // walk and repeats the other. `id` is the PRIMARY KEY, so appending it + // makes the order TOTAL and every page boundary reproducible. + // + // This costs nothing in plan terms: `WHERE type = ?` resolves through + // the single-column `idx_type` and the ORDER BY already filesorts (there + // is no `(type, sort_title, …)` index — the composite from migration 050 + // leads with `library_id`), so the sort was happening either way. $results = $this->db->query( - "SELECT * FROM media_items WHERE type = ? ORDER BY " . self::titleOrder() . " LIMIT ? OFFSET ?", - [$type, $limit, $offset] + "SELECT * FROM media_items WHERE type = ? ORDER BY " . self::titleOrder() . ", id ASC LIMIT ? OFFSET ?", + [$type, min(max(1, $limit), self::ABSOLUTE_ROW_CEILING), $this->normalizeOffset($offset)] ); return $this->hydrateRows($results); diff --git a/src/Media/Music/MusicLibraryService.php b/src/Media/Music/MusicLibraryService.php index 052a4076..3249a876 100644 --- a/src/Media/Music/MusicLibraryService.php +++ b/src/Media/Music/MusicLibraryService.php @@ -1179,12 +1179,19 @@ public function getTracksCount(): int * reachable through S96(e)), and a renderer that is promised N children and * handed N-1 is a renderer that shows an error. * - * ⚠ This count is UNBOUNDED and is therefore not, on its own, the number a - * renderer may be promised: {@see getArtistMediaItemIds()} stops at - * {@see MAX_EMBEDDED_ROWS}. {@see \Phlix\Dlna\LibraryBridge} clamps this value - * to that constant before advertising it, and documents the artists beyond it - * as unreachable over DLNA. Any other caller that pairs this count with a - * bounded listing must clamp it the same way. + * ⚠ This count is UNBOUNDED, and since S147 that is exactly what a renderer + * is promised: {@see \Phlix\Dlna\LibraryBridge} advertises it verbatim and no + * longer clamps it. The clamp was correct only while the Audio root had no + * offset path — S97 added it so the advertised number stopped lying about a + * listing that stopped dead at {@see MAX_EMBEDDED_ROWS}. S147 removed the + * bound instead: {@see getArtistMediaItemIds()} takes an `$offset`, + * {@see \Phlix\Dlna\LibraryBridge::getLibraryItems()} threads DLNA's + * `StartingIndex` into it, and every artist is reachable by paging. + * {@see MAX_EMBEDDED_ROWS} is now a PAGE size, not a ceiling on the set. + * + * A caller that pairs this count with a listing it cannot page (one that + * truncates in PHP, say) still owes the reader an explanation — but there is + * no such caller today. * * @return int Number of artists a `media_items`-backed browse can show. */ @@ -1226,12 +1233,16 @@ public function getArtistMediaItemIds(int $limit = self::MAX_EMBEDDED_ROWS, int * * @param string $artistMediaItemId The artist's `media_items` UUID. * @param int $limit Clamped to `[1, self::MAX_EMBEDDED_ROWS]`. + * @param int $offset Page offset (negative offsets are clamped to 0). The + * `ORDER BY` ends in `al.id`, the PRIMARY KEY, so it is a TOTAL order + * and paging with this offset can neither drop nor repeat a row. * @return list Album `media_items` UUIDs (albums whose own mint failed * are absent — there is no `media_items` row to browse to). */ public function getAlbumMediaItemIdsForArtist( string $artistMediaItemId, - int $limit = self::MAX_EMBEDDED_ROWS + int $limit = self::MAX_EMBEDDED_ROWS, + int $offset = 0 ): array { if ($artistMediaItemId === '') { return []; @@ -1243,8 +1254,34 @@ public function getAlbumMediaItemIdsForArtist( JOIN music_artists ar ON ar.id = al.artist_id WHERE ar.media_item_id = ? AND al.media_item_id IS NOT NULL ORDER BY al.year IS NULL, al.year, COALESCE(al.sort_title, al.title), al.id - LIMIT ?", - [$artistMediaItemId, $this->clampRowLimit($limit)] + LIMIT ? OFFSET ?", + [$artistMediaItemId, $this->clampRowLimit($limit), max(0, $offset)] + )); + } + + /** + * Counts one artist's browsable albums — the {@see getAlbumMediaItemIdsForArtist()} + * predicate, unbounded. + * + * The DLNA drill-down advertises this as `TotalMatches`, so it MUST share the + * listing's predicate exactly (`al.media_item_id IS NOT NULL`); an album whose + * `media_items` mint failed is not browsable and must not be promised. + * + * @param string $artistMediaItemId The artist's `media_items` UUID. + * @return int Number of albums a `media_items`-backed browse can show. + */ + public function countAlbumMediaItemsForArtist(string $artistMediaItemId): int + { + if ($artistMediaItemId === '') { + return 0; + } + + return $this->firstCount($this->db->query( + "SELECT COUNT(*) AS cnt + FROM music_albums al + JOIN music_artists ar ON ar.id = al.artist_id + WHERE ar.media_item_id = ? AND al.media_item_id IS NOT NULL", + [$artistMediaItemId] )); } @@ -1257,11 +1294,15 @@ public function getAlbumMediaItemIdsForArtist( * * @param string $albumMediaItemId The album's `media_items` UUID. * @param int $limit Clamped to `[1, self::MAX_EMBEDDED_ROWS]`. + * @param int $offset Page offset (negative offsets are clamped to 0). The + * `ORDER BY` ends in `t.id`, the PRIMARY KEY, so it is a TOTAL order + * and paging with this offset can neither drop nor repeat a row. * @return list Track `media_items` UUIDs in playback order. */ public function getTrackMediaItemIdsForAlbum( string $albumMediaItemId, - int $limit = self::MAX_EMBEDDED_ROWS + int $limit = self::MAX_EMBEDDED_ROWS, + int $offset = 0 ): array { if ($albumMediaItemId === '') { return []; @@ -1273,8 +1314,30 @@ public function getTrackMediaItemIdsForAlbum( JOIN music_albums al ON al.id = t.album_id WHERE al.media_item_id = ? ORDER BY t.disc_number, t.track_number, t.id - LIMIT ?", - [$albumMediaItemId, $this->clampRowLimit($limit)] + LIMIT ? OFFSET ?", + [$albumMediaItemId, $this->clampRowLimit($limit), max(0, $offset)] + )); + } + + /** + * Counts one album's tracks — the {@see getTrackMediaItemIdsForAlbum()} + * predicate, unbounded. + * + * @param string $albumMediaItemId The album's `media_items` UUID. + * @return int Number of tracks on the album. + */ + public function countTrackMediaItemsForAlbum(string $albumMediaItemId): int + { + if ($albumMediaItemId === '') { + return 0; + } + + return $this->firstCount($this->db->query( + "SELECT COUNT(*) AS cnt + FROM music_tracks t + JOIN music_albums al ON al.id = t.album_id + WHERE al.media_item_id = ?", + [$albumMediaItemId] )); } diff --git a/tests/Integration/Dlna/DlnaBrowsePagingIntegrationTest.php b/tests/Integration/Dlna/DlnaBrowsePagingIntegrationTest.php new file mode 100644 index 00000000..7f61d30f --- /dev/null +++ b/tests/Integration/Dlna/DlnaBrowsePagingIntegrationTest.php @@ -0,0 +1,343 @@ + Every media_items id this test created. */ + private array $seededIds = []; + + protected function setUp(): void + { + parent::setUp(); + + $host = getenv('DB_HOST') ?: '127.0.0.1'; + $port = (int) (getenv('DB_PORT') ?: 3306); + + if (!$this->isMysqlReachable($host, $port)) { + $this->markTestSkipped( + sprintf('No MySQL on %s:%d — skipping the S147 DLNA paging proof. Runs in CI.', $host, $port), + ); + } + + try { + ConnectionPool::init(dirname(__DIR__, 3) . '/config/database.php'); + $this->db = ConnectionPool::getConnection('mysql'); + } catch (Throwable $e) { + $this->markTestSkipped('Could not connect to MySQL: ' . $e->getMessage()); + } + + $db = $this->db(); + $this->libraryId = $this->uuid(); + $db->query( + 'INSERT INTO libraries (id, name, type, paths) VALUES (?, ?, ?, ?)', + [$this->libraryId, 'S147 DLNA Paging Lib', 'movie', json_encode(['/tmp/phlix-s147-paging'])], + ); + + $repo = new ItemRepository($db); + + // The tie block: one name for every row, so `sort_title, name` cannot + // order them and only the `id` tiebreak can. + for ($i = 0; $i < self::TIED_ROWS; $i++) { + $this->seededIds[] = $repo->create([ + 'library_id' => $this->libraryId, + 'name' => 'Dune', + 'type' => 'photo', + 'path' => sprintf('/tmp/phlix-s147-paging/tied-%04d.jpg', $i), + ]); + } + + // Ordinary distinct rows either side of the tie block. + for ($i = 0; $i < self::DISTINCT_ROWS; $i++) { + $this->seededIds[] = $repo->create([ + 'library_id' => $this->libraryId, + 'name' => sprintf('%s S147 Distinct %03d', chr(ord('A') + ($i % 26)), $i), + 'type' => 'photo', + 'path' => sprintf('/tmp/phlix-s147-paging/distinct-%04d.jpg', $i), + ]); + } + + $db->query('ANALYZE TABLE media_items'); + } + + protected function tearDown(): void + { + if ($this->db !== null && $this->libraryId !== '') { + // ON DELETE CASCADE takes the media_items rows with the library. + $this->db->query('DELETE FROM libraries WHERE id = ?', [$this->libraryId]); + } + parent::tearDown(); + } + + /** + * 🔴 THE DECISIVE ONE: paging `getAllByType()` reproduces the unpaged listing + * EXACTLY — same rows, same order, nothing dropped, nothing repeated. + * + * Asserted against the unpaged listing rather than against the seeded set, + * because `getAllByType()` has no library filter: any `photo` row another + * fixture left behind is legitimately part of both listings, and comparing + * the two makes the assertion independent of what else is in the schema. + * + * Mutation target: delete `, id ASC` from `ItemRepository::getAllByType()`'s + * `ORDER BY` and this goes RED on a real MySQL while every unit test stays + * green. + */ + public function testPagingByTypeReproducesTheUnpagedOrderExactly(): void + { + $repo = new ItemRepository($this->db()); + + $total = $repo->countAllByType('photo'); + $this->assertGreaterThanOrEqual( + self::TIED_ROWS + self::DISTINCT_ROWS, + $total, + 'the fixture must be present', + ); + + $unpaged = array_column($repo->getAllByType('photo', $total + 10, 0), 'id'); + $this->assertCount($total, $unpaged, 'the unpaged listing is the reference order'); + + $paged = []; + for ($offset = 0; $offset < $total; $offset += self::PAGE_SIZE) { + foreach ($repo->getAllByType('photo', self::PAGE_SIZE, $offset) as $row) { + $paged[] = $row['id']; + } + } + + $this->assertSame( + count($paged), + count(array_unique($paged)), + sprintf( + 'the paged walk repeated %d row(s). With ties on (sort_title, name) and no `id` ' + . 'tiebreak, MySQL arranges the tied rows differently per LIMIT+OFFSET, so pages ' + . 'overlap and other rows are never returned at all.', + count($paged) - count(array_unique($paged)), + ), + ); + + $this->assertSame( + $unpaged, + $paged, + 'paging must tile the listing exactly: the concatenated pages must equal the unpaged ' + . 'order, row for row', + ); + + foreach ($this->seededIds as $id) { + $this->assertContains($id, $paged, 'every seeded row must be reachable by paging'); + } + } + + /** + * The acceptance criterion, end to end through the public `Browse` action: a + * renderer is told a total, walks it with `StartingIndex`, and reaches the + * LAST row. + * + * Before S147 this was impossible for any container past the first page — + * `getLibraryItems()` took no offset and `browseChildren()` applied + * `StartingIndex` in PHP to a list already cut at `LIMIT 100`, so a + * `StartingIndex` beyond 100 returned an empty page while `TotalMatches` + * still advertised the whole container. + */ + public function testABrowseCanBePagedToTheLastRowOfTheContainer(): void + { + $repo = new ItemRepository($this->db()); + $bridge = new LibraryBridge($repo, $this->createMock(HlsStreamer::class)); + $cd = new ContentDirectory($repo); + $cd->setLibraryBridge($bridge); + + $first = $cd->browse('library-photos', 'BrowseDirectChildren', '*', 0, self::PAGE_SIZE, ''); + $total = $first['TotalMatches'] ?? 0; + + $this->assertIsInt($total); + $this->assertGreaterThan( + self::PAGE_SIZE, + $total, + 'the fixture must exceed one page or this proves nothing', + ); + $this->assertSame( + $repo->countAllByType('photo'), + $total, + 'TotalMatches must be the container total, not the size of the page just returned', + ); + + $walked = []; + $numberReturned = 0; + for ($offset = 0; $offset < $total; $offset += self::PAGE_SIZE) { + $page = $cd->browse('library-photos', 'BrowseDirectChildren', '*', $offset, self::PAGE_SIZE, ''); + + $this->assertGreaterThan( + 0, + $page['NumberReturned'] ?? 0, + sprintf('StartingIndex %d returned an empty page — the container is not pageable', $offset), + ); + $numberReturned += is_int($page['NumberReturned'] ?? null) ? $page['NumberReturned'] : 0; + + $didl = is_string($page['Result'] ?? null) ? $page['Result'] : ''; + preg_match_all('/ $itemId) { + $walked[] = $itemId !== '' ? $itemId : $matches[2][$index]; + } + } + + $this->assertSame($total, $numberReturned, 'the pages must sum to the advertised total'); + $this->assertSame($total, count($walked), 'every advertised child must appear in some page'); + $this->assertSame(count($walked), count(array_unique($walked)), 'no row may appear in two pages'); + + $lastAdvertised = array_column($repo->getAllByType('photo', $total + 10, 0), 'id'); + $this->assertSame( + end($lastAdvertised), + end($walked), + 'the LAST child of the container must be reachable — the whole point of S147', + ); + } + + /** + * `findByParentPage()` pages a `parent_id` drill-down with the same + * guarantee, so the series/season branch of the browse is not left behind. + */ + public function testAParentDrillDownPagesWithoutDroppingOrRepeatingARow(): void + { + $repo = new ItemRepository($this->db()); + + $parentId = $repo->create([ + 'library_id' => $this->libraryId, + 'name' => 'S147 Season', + 'type' => 'season', + 'path' => '/tmp/phlix-s147-paging/season', + ]); + $this->seededIds[] = $parentId; + + // Same-name children: `ORDER BY name` alone cannot order these either. + for ($i = 0; $i < 120; $i++) { + $this->seededIds[] = $repo->create([ + 'library_id' => $this->libraryId, + 'parent_id' => $parentId, + 'name' => 'Episode', + 'type' => 'episode', + 'path' => sprintf('/tmp/phlix-s147-paging/ep-%04d.mkv', $i), + ]); + } + + $total = $repo->countByParent($parentId); + $this->assertSame(120, $total); + + $walked = []; + for ($offset = 0; $offset < $total; $offset += 25) { + foreach ($repo->findByParentPage($parentId, 25, $offset) as $row) { + $walked[] = $row['id']; + } + } + + $this->assertCount($total, $walked); + $this->assertSame(count($walked), count(array_unique($walked)), 'no episode may be paged twice'); + $this->assertSame( + array_column($repo->findByParentPage($parentId, $total, 0), 'id'), + $walked, + 'the paged walk must equal the single-page listing exactly', + ); + } + + private function db(): Connection + { + if ($this->db === null) { + $this->fail('No database connection'); + } + + return $this->db; + } + + private function isMysqlReachable(string $host, int $port): bool + { + $socket = @fsockopen($host, $port, $errno, $errstr, 1.0); + if ($socket === false) { + return false; + } + fclose($socket); + + return true; + } + + private function uuid(): string + { + return sprintf( + '%04x%04x-%04x-%04x-%04x-%04x%04x%04x', + random_int(0, 0xffff), + random_int(0, 0xffff), + random_int(0, 0xffff), + random_int(0, 0x0fff) | 0x4000, + random_int(0, 0x3fff) | 0x8000, + random_int(0, 0xffff), + random_int(0, 0xffff), + random_int(0, 0xffff), + ); + } +} diff --git a/tests/Integration/Dlna/DlnaMusicDrillDownCountIntegrationTest.php b/tests/Integration/Dlna/DlnaMusicDrillDownCountIntegrationTest.php new file mode 100644 index 00000000..38e711aa --- /dev/null +++ b/tests/Integration/Dlna/DlnaMusicDrillDownCountIntegrationTest.php @@ -0,0 +1,496 @@ + Seeded `music_artists` ids — cascade-delete albums + tracks. */ + private array $artistIds = []; + + /** @var list Seeded `media_items` ids. */ + private array $mediaIds = []; + + /** The artist under test, by `media_items` UUID. */ + private string $artistMediaItemId = ''; + + /** A second artist, by `media_items` UUID. */ + private string $otherArtistMediaItemId = ''; + + /** The album under test, by `media_items` UUID. */ + private string $albumMediaItemId = ''; + + protected function setUp(): void + { + parent::setUp(); + + $host = getenv('DB_HOST') ?: '127.0.0.1'; + $port = (int) (getenv('DB_PORT') ?: 3306); + + if (!$this->isMysqlReachable($host, $port)) { + $this->markTestSkipped(sprintf( + 'No MySQL on %s:%d — skipping the S147 music drill-down COUNT proof. Runs in CI.', + $host, + $port, + )); + } + + try { + ConnectionPool::init(dirname(__DIR__, 3) . '/config/database.php'); + $db = ConnectionPool::getConnection('mysql'); + // Forces the lazily-leased pooled connection to open for real. + $db->query('SELECT 1'); + $this->db = $db; + } catch (Throwable $e) { + $this->markTestSkipped('Could not connect to MySQL: ' . $e->getMessage()); + } + + $this->libraryId = Uuid::v4(); + $this->prefix = '!S147-' . substr(Uuid::v4(), 0, 8) . '-'; + + $this->seedFixtures(); + } + + protected function tearDown(): void + { + $this->purgeFixtures(); + parent::tearDown(); + } + + /** + * 🔴 MUTANT A: drop `AND al.media_item_id IS NOT NULL` from + * {@see MusicLibraryService::countAlbumMediaItemsForArtist()} and this goes + * RED — the artist would advertise 5 albums against a listing that can only + * ever return 4. + * + * Asserted as a THREE-way identity (count === listing size === the number + * actually minted), because that is the invariant the DLNA drill-down needs: + * a `TotalMatches` a renderer cannot walk to is the S97 defect. + */ + public function testTheAlbumCountSharesTheListingPredicateExactly(): void + { + $service = $this->service(); + + $listed = $service->getAlbumMediaItemIdsForArtist($this->artistMediaItemId); + $counted = $service->countAlbumMediaItemsForArtist($this->artistMediaItemId); + + $this->assertSame( + self::MINTED_ALBUMS, + count($listed), + 'the listing must skip the album whose media_items mint failed — there is no row to browse to', + ); + $this->assertSame( + self::MINTED_ALBUMS, + $counted, + sprintf( + 'the COUNT must carry `al.media_item_id IS NOT NULL` too. Without it this returns %d — ' + . 'an artist advertising more albums than its container can deliver, which is the S97 ' + . 'defect one level down.', + self::MINTED_ALBUMS + self::UNMINTED_ALBUMS, + ), + ); + $this->assertSame( + count($listed), + $counted, + 'advertised count and deliverable list must agree — the S147 acceptance criterion', + ); + } + + /** + * The album COUNT is scoped to ITS artist: dropping `ar.media_item_id = ?` + * (or joining the wrong way) makes this RED, because the fixture deliberately + * seeds a second artist with a different number of albums. + */ + public function testTheAlbumCountIsScopedToTheArtistItWasAskedAbout(): void + { + $service = $this->service(); + + $this->assertSame( + self::MINTED_ALBUMS, + $service->countAlbumMediaItemsForArtist($this->artistMediaItemId), + ); + $this->assertSame( + self::OTHER_ARTIST_ALBUMS, + $service->countAlbumMediaItemsForArtist($this->otherArtistMediaItemId), + 'a second artist in the same library must count only ITS OWN albums', + ); + $this->assertSame( + 0, + $service->countAlbumMediaItemsForArtist(Uuid::v4()), + 'an artist that does not exist has no albums, not all of them', + ); + } + + /** + * 🔴 MUTANT B: rewrite {@see MusicLibraryService::countTrackMediaItemsForAlbum()}'s + * `WHERE` as `al.media_item_id = ? OR 1 = 1` — counting every track in the + * library — and this goes RED. The fixture seeds strictly more tracks + * elsewhere than on the album under test, so a library-wide count cannot + * coincide with the right answer. + */ + public function testTheTrackCountIsScopedToTheAlbumItWasAskedAbout(): void + { + $service = $this->service(); + + $listed = $service->getTrackMediaItemIdsForAlbum($this->albumMediaItemId); + $counted = $service->countTrackMediaItemsForAlbum($this->albumMediaItemId); + + $this->assertSame(self::TRACKS_ON_ALBUM, count($listed), 'the listing returns the album\'s tracks'); + $this->assertSame( + self::TRACKS_ON_ALBUM, + $counted, + sprintf( + 'the COUNT must be scoped to this album. A predicate that leaks (or is dropped) counts ' + . 'the fixture\'s other %d tracks too, and the album advertises a total it cannot deliver.', + self::TRACKS_ELSEWHERE, + ), + ); + $this->assertSame(count($listed), $counted, 'advertised count and deliverable list must agree'); + $this->assertSame( + 0, + $service->countTrackMediaItemsForAlbum(Uuid::v4()), + 'an album that does not exist has no tracks, not the whole library\'s', + ); + } + + /** + * The same walk-with-`OFFSET` proof the video root got, on both music + * drill-downs: the pages must tile the container exactly — the concatenation + * equals the unpaged listing, nothing dropped, nothing repeated, and the walk + * ends exactly at the advertised COUNT. + * + * The unit doubles cannot prove this half: they model `LIMIT`/`OFFSET` with + * `array_slice()`, which is a total order by construction, so they pass even + * against an `ORDER BY` with no PRIMARY KEY tiebreak at all. + */ + public function testBothMusicDrillDownsPageExactlyToTheirAdvertisedCount(): void + { + $service = $this->service(); + + $albums = $service->getAlbumMediaItemIdsForArtist($this->artistMediaItemId); + $walkedAlbums = []; + for ($offset = 0; $offset < $service->countAlbumMediaItemsForArtist($this->artistMediaItemId); $offset += 2) { + foreach ($service->getAlbumMediaItemIdsForArtist($this->artistMediaItemId, 2, $offset) as $id) { + $walkedAlbums[] = $id; + } + } + + $this->assertSame($albums, $walkedAlbums, 'the paged album walk must equal the unpaged listing exactly'); + $this->assertSame( + count($walkedAlbums), + count(array_unique($walkedAlbums)), + 'no album may appear on two pages', + ); + + $tracks = $service->getTrackMediaItemIdsForAlbum($this->albumMediaItemId); + $walkedTracks = []; + for ($offset = 0; $offset < $service->countTrackMediaItemsForAlbum($this->albumMediaItemId); $offset += 3) { + foreach ($service->getTrackMediaItemIdsForAlbum($this->albumMediaItemId, 3, $offset) as $id) { + $walkedTracks[] = $id; + } + } + + $this->assertSame($tracks, $walkedTracks, 'the paged track walk must equal the unpaged listing exactly'); + $this->assertSame( + count($walkedTracks), + count(array_unique($walkedTracks)), + 'no track may appear on two pages', + ); + } + + /** + * End to end through the DLNA bridge against real MySQL: what the drill-down + * ADVERTISES equals what it DELIVERS, for both music container types. + * + * A service-level assertion alone would pass even if the bridge advertised + * something else, so the acceptance criterion is asserted where a renderer + * would actually read it. + */ + public function testTheDlnaDrillDownAdvertisesWhatItCanDeliver(): void + { + $bridge = new LibraryBridge( + new ItemRepository($this->db()), + $this->createMock(HlsStreamer::class), + null, + $this->service(), + ); + + $albums = $bridge->getContainerChildren($this->artistMediaItemId, 'artist'); + $this->assertSame( + self::MINTED_ALBUMS, + $bridge->getContainerChildCount($this->artistMediaItemId, 'artist'), + 'the artist container advertises only the albums it can browse to', + ); + $this->assertCount(self::MINTED_ALBUMS, $albums, 'and delivers exactly that many'); + + $tracks = $bridge->getContainerChildren($this->albumMediaItemId, 'album'); + $this->assertSame( + self::TRACKS_ON_ALBUM, + $bridge->getContainerChildCount($this->albumMediaItemId, 'album'), + 'the album container advertises its own track count', + ); + $this->assertCount(self::TRACKS_ON_ALBUM, $tracks, 'and delivers exactly that many'); + } + + private function service(): MusicLibraryService + { + return new MusicLibraryService($this->db(), $this->createMock(MusicLibraryScanner::class)); + } + + private function seedFixtures(): void + { + $db = $this->db(); + + $db->query( + "INSERT INTO libraries (id, name, type, paths) VALUES (?, ?, 'music', '[]')", + [$this->libraryId, 'S147 Music Drill-Down Lib'], + ); + + $this->artistMediaItemId = $this->mediaItem('artist-under-test', 'artist'); + $artistId = $this->artist('Artist Under Test', $this->artistMediaItemId); + + $this->otherArtistMediaItemId = $this->mediaItem('other-artist', 'artist'); + $otherArtistId = $this->artist('Other Artist', $this->otherArtistMediaItemId); + + // The album under test, plus three more minted albums for the same artist. + $this->albumMediaItemId = $this->mediaItem('album-under-test', 'album'); + $albumId = $this->album($artistId, 'Album Under Test', $this->albumMediaItemId, 1994); + + $mintedElsewhere = []; + for ($i = 1; $i < self::MINTED_ALBUMS; $i++) { + $mintedElsewhere[] = $this->album( + $artistId, + sprintf('Minted Album %d', $i), + $this->mediaItem(sprintf('minted-album-%d', $i), 'album'), + 2000 + $i, + ); + } + + // ⚠ The whole point of the fixture: an album whose `media_items` mint + // failed. Schema-permitted (migration 065 leaves the column NULL-able), + // and invisible to the LISTING — so it must be invisible to the COUNT. + for ($i = 0; $i < self::UNMINTED_ALBUMS; $i++) { + $mintedElsewhere[] = $this->album($artistId, sprintf('Unminted Album %d', $i), null, 2010 + $i); + } + + for ($i = 0; $i < self::OTHER_ARTIST_ALBUMS; $i++) { + $this->album( + $otherArtistId, + sprintf('Other Album %d', $i), + $this->mediaItem(sprintf('other-album-%d', $i), 'album'), + 1980 + $i, + ); + } + + for ($i = 0; $i < self::TRACKS_ON_ALBUM; $i++) { + $this->track($albumId, $artistId, sprintf('Track %d', $i), $i + 1); + } + + // Tracks that are NOT on the album under test, spread over its siblings — + // strictly more of them than there are on it, so a count that forgets its + // `WHERE` cannot accidentally agree. + for ($i = 0; $i < self::TRACKS_ELSEWHERE; $i++) { + $this->track( + $mintedElsewhere[$i % count($mintedElsewhere)], + $artistId, + sprintf('Elsewhere Track %d', $i), + $i + 1, + ); + } + } + + /** + * `media_items.type` is a 13-member ENUM; `artist`, `album` and `track` are + * all members (migrations 001 + 034). + */ + private function mediaItem(string $name, string $type): string + { + $id = Uuid::v4(); + $this->db()->query( + 'INSERT INTO media_items (id, library_id, name, type, path) VALUES (?, ?, ?, ?, ?)', + [$id, $this->libraryId, $this->prefix . $name, $type, '/s147-music-drilldown/' . $id . '.flac'], + ); + $this->mediaIds[] = $id; + + return $id; + } + + private function artist(string $name, string $mediaItemId): int + { + $db = $this->db(); + $db->query( + 'INSERT INTO music_artists (media_item_id, name) VALUES (?, ?)', + [$mediaItemId, $this->prefix . $name], + ); + $id = $this->idOf('music_artists', 'media_item_id', $mediaItemId); + $this->artistIds[] = $id; + + return $id; + } + + private function album(int $artistId, string $title, ?string $mediaItemId, int $year): int + { + $db = $this->db(); + $title = $this->prefix . $title; + + $db->query( + 'INSERT INTO music_albums (media_item_id, artist_id, title, year, total_tracks) VALUES (?, ?, ?, ?, 0)', + [$mediaItemId, $artistId, $title, $year], + ); + + return $mediaItemId === null + ? $this->idOf('music_albums', 'title', $title) + : $this->idOf('music_albums', 'media_item_id', $mediaItemId); + } + + private function track(int $albumId, int $artistId, string $title, int $trackNumber): void + { + $this->db()->query( + 'INSERT INTO music_tracks (media_item_id, album_id, artist_id, title, track_number, disc_number,' + . ' duration_secs) VALUES (?, ?, ?, ?, ?, 1, 180)', + [ + $this->mediaItem(strtolower(str_replace(' ', '-', $title)), 'track'), + $albumId, + $artistId, + $this->prefix . $title, + $trackNumber, + ], + ); + } + + /** + * @param 'music_artists'|'music_albums' $table + * @param 'media_item_id'|'title' $column + */ + private function idOf(string $table, string $column, string $value): int + { + // Both identifiers come from the closed literal sets in the signature — + // no caller-supplied identifier is ever interpolated here. + $rows = $this->db()->query( + sprintf('SELECT id FROM %s WHERE %s = ?', $table, $column), + [$value], + ); + + $this->assertIsArray($rows); + $this->assertArrayHasKey(0, $rows, sprintf('the seeded %s row must exist', $table)); + $row = $rows[0]; + $this->assertIsArray($row); + + return (int) $row['id']; + } + + private function purgeFixtures(): void + { + $db = $this->db; + if ($db === null) { + return; + } + + // music_albums / music_tracks cascade off music_artists. + foreach ($this->artistIds as $artistId) { + $db->query('DELETE FROM music_artists WHERE id = ?', [$artistId]); + } + foreach ($this->mediaIds as $id) { + $db->query('DELETE FROM media_items WHERE id = ?', [$id]); + } + if ($this->libraryId !== '') { + $db->query('DELETE FROM libraries WHERE id = ?', [$this->libraryId]); + } + } + + private function db(): Connection + { + if ($this->db === null) { + $this->fail('No database connection'); + } + + return $this->db; + } + + private function isMysqlReachable(string $host, int $port): bool + { + $socket = @fsockopen($host, $port, $errno, $errstr, 1.0); + if ($socket === false) { + return false; + } + fclose($socket); + + return true; + } +} diff --git a/tests/Unit/Dlna/ContentDirectoryTest.php b/tests/Unit/Dlna/ContentDirectoryTest.php index be703673..290017d6 100644 --- a/tests/Unit/Dlna/ContentDirectoryTest.php +++ b/tests/Unit/Dlna/ContentDirectoryTest.php @@ -274,4 +274,40 @@ public function testBrowseWithOffset(): void $this->assertArrayHasKey('Result', $result); } + + /** + * S147 fix round — the bridge-less `TotalMatches` is a COUNT, not a second + * copy of the child list. + * + * It used to be `count($this->bridgelessChildren($objectId))`, evaluated a few + * statements after `getChildren()` had already materialised the very same + * (unbounded) list — so this path fetched every child TWICE, once to show and + * once to measure. `findByParent()` is therefore expected exactly ONCE, and + * the advertised total comes from `countByParent()`. + */ + public function testTheBridgelessChildCountIsACountNotASecondFetch(): void + { + $children = []; + for ($i = 0; $i < 3; $i++) { + $children[] = ['id' => 'child-' . $i, 'name' => 'Child ' . $i, 'type' => 'episode']; + } + + $this->itemRepositoryMock->expects($this->once()) + ->method('findByParent') + ->with('library-video') + ->willReturn($children); + $this->itemRepositoryMock->expects($this->once()) + ->method('countByParent') + ->with('library-video') + ->willReturn(9999); + + $result = $this->contentDirectory->browse('library-video', 'BrowseDirectChildren', '*', 0, 3, ''); + + $this->assertSame(3, $result['NumberReturned']); + $this->assertSame( + 9999, + $result['TotalMatches'], + 'TotalMatches must come from countByParent(), not from re-listing the container' + ); + } } diff --git a/tests/Unit/Dlna/LibraryBridgeTest.php b/tests/Unit/Dlna/LibraryBridgeTest.php index 090ac347..2320a2da 100644 --- a/tests/Unit/Dlna/LibraryBridgeTest.php +++ b/tests/Unit/Dlna/LibraryBridgeTest.php @@ -178,8 +178,8 @@ public function testGetContainerChildrenUsesItemRepository(): void $this->itemRepositoryMock ->expects($this->once()) - ->method('findByParent') - ->with($parentId) + ->method('findByParentPage') + ->with($parentId, LibraryBridge::MAX_PAGE_ROWS, 0) ->willReturn($expectedItems); $children = $this->bridge->getContainerChildren($parentId); @@ -519,30 +519,27 @@ public function testAudioRootCountsArtistsButNotAlbumsOrTracks(): void } /** - * With the `music_*` reader wired, the advertised artist count comes from - * `music_artists` — the same source the LISTING is enumerated from — and is - * then clamped to the ceiling that listing actually stops at, - * {@see MusicLibraryService::MAX_EMBEDDED_ROWS}. + * S147 — the advertised artist count is the TRUE, UNBOUNDED total again, and + * it is honest because the listing can now be paged all the way to it. * - * On the production shape (4,656 artists with a `media_items` row) the - * unclamped count advertises 4,656 while `getLibraryItems()` can only hand - * over 2,000, because it calls `getArtistMediaItemIds()` whose default limit - * IS that constant. The renderer is then promised 2.3x what it receives. + * This test replaces S97's `…ClampsTheAdvertisedArtistCountToWhatItCanDeliver`, + * and the reversal is deliberate rather than a relaxation. S97 clamped the + * artist term to {@see MusicLibraryService::MAX_EMBEDDED_ROWS} because the + * listing genuinely stopped there — production advertised 4,656 artists, + * handed over 2,000, and no `StartingIndex` could reach the remaining 2,656. + * Clamping made the promise true by shrinking it. S147 makes it true by + * keeping the promise and delivering on it: `getLibraryItems()` takes an + * offset, resolves it in SQL, and {@see LibraryBridge::MAX_PAGE_ROWS} is a + * page size. * - * ⚠ This test pins the ADVERTISED number only. It does not claim the other - * 2,656 artists are reachable — they are not, and - * {@see LibraryBridge::getLibraryChildCount()} says so: there is no offset - * path into the audio root, so a renderer cannot page past the cap. Real - * DLNA paging is a separate step. - * - * ⚠ Scope: the ARTIST term only. The stub zeroes `audio`/`audiobook`, so the - * asserted `child_count` is that term alone. Every NON-artist term is still - * counted with the unbounded `ItemRepository::countAllByType()` while - * `getLibraryItems()` lists it with `getAllByType()`, whose default is - * `LIMIT 100` — so the Video root still advertises more than it delivers. - * That is pre-existing on `master`, untouched by S97, and filed as S147. + * 🛑 **Re-introducing the clamp would be a regression, not a safety net.** A + * renderer that is told 2,000 asks for 2,000 and stops; the 2,656 artists past + * the page are then unreachable *because of the number*, even though the + * paging works. The count and the page size are different quantities and must + * stay different. {@see testEveryArtistPastThePageSizeIsReachableByPaging()} + * is the other half of this pair — it walks to the last artist. */ - public function testAudioRootClampsTheAdvertisedArtistCountToWhatItCanDeliver(): void + public function testTheAudioRootAdvertisesTheTrueUnboundedArtistCount(): void { $this->stubCounts(['artist' => 4658, 'audio' => 0, 'audiobook' => 0]); $music = $this->createMock(MusicLibraryService::class); @@ -558,11 +555,201 @@ public function testAudioRootClampsTheAdvertisedArtistCountToWhatItCanDeliver(): $byId = array_column($bridge->getRootContainers(), 'child_count', 'id'); $this->assertSame( - MusicLibraryService::MAX_EMBEDDED_ROWS, + 4656, $byId['library-audio'] ?? null, - 'the ARTIST term of the advertised childCount must not exceed the number of artists ' - . 'getLibraryItems() can hand over (this stub zeroes audio/audiobook, so the whole ' - . 'count is that term); the non-artist terms are still unbounded — S147' + 'the advertised childCount must be the TRUE total (this stub zeroes audio/audiobook, so ' + . 'the whole count is the artist term), NOT MAX_PAGE_ROWS — a renderer needs the total to ' + . 'know a second page exists' + ); + $this->assertGreaterThan( + LibraryBridge::MAX_PAGE_ROWS, + $byId['library-audio'] ?? 0, + 'the fixture must sit ABOVE the page size, or this test cannot tell a clamp from its absence' + ); + } + + /** + * S147 — the Video root advertises its true total too. + * + * The audio root was the SMALLEST instance of this defect, not the only one: + * `countAllByType()` was already unbounded here while `getAllByType()` listed + * with its default `LIMIT 100`, so production advertised 10,718 movies and + * 26,389 episodes against pages of 100 — 107x and 264x, against the 2.1x that + * was escalated. The count was always right; it is the LISTING that S147 + * fixed, so this asserts the count stayed unbounded while the delivery caught + * up. + */ + public function testTheVideoRootAdvertisesTheTrueUnboundedCountAcrossAllItsTypes(): void + { + $this->stubCounts(['movie' => 10718, 'series' => 434, 'video' => 6]); + + $byId = array_column($this->bridge->getRootContainers(), 'child_count', 'id'); + + $this->assertSame(10718 + 434 + 6, $byId['library-video'] ?? null); + } + + /** + * S147 — 🔴 THE ACCEPTANCE CRITERION: a renderer can reach the LAST artist in + * a library larger than one page. + * + * Before S147 this was impossible by any navigation. `getLibraryItems()` took + * no offset, so it always returned the first {@see MusicLibraryService::MAX_EMBEDDED_ROWS} + * artists, and `ContentDirectory::browseChildren()` applied `StartingIndex` in + * PHP to *that* list — so `StartingIndex = 4000` sliced past the end of a + * 2,000-row array and returned NOTHING. The walk below is the proof, not a + * spot check: it pages the entire 4,656-artist container and asserts the + * concatenation of the pages equals the unpaged order exactly — same rows, + * same order, none dropped, none repeated. + */ + public function testEveryArtistPastThePageSizeIsReachableByPaging(): void + { + $total = 4656; + $all = []; + for ($i = 0; $i < $total; $i++) { + $all[] = sprintf('artist-%05d', $i); + } + + $this->stubCounts(['artist' => $total, 'audio' => 0, 'audiobook' => 0]); + $this->itemRepositoryMock->method('findByIds')->willReturnCallback( + static fn (array $batch): array => array_map( + static fn (string $id): array => ['id' => $id, 'name' => $id, 'type' => 'artist', 'path' => ''], + $batch + ) + ); + $this->itemRepositoryMock->method('filterItemsByTags')->willReturnArgument(0); + + $music = $this->createMock(MusicLibraryService::class); + $music->method('getArtistsWithMediaItemCount')->willReturn($total); + // The real query is `ORDER BY COALESCE(sort_name, name), id LIMIT ? OFFSET ?` + // — a TOTAL order, so this slice models it exactly. + $music->method('getArtistMediaItemIds')->willReturnCallback( + static fn (int $limit, int $offset): array => array_values(array_slice($all, $offset, $limit)) + ); + + $bridge = new LibraryBridge($this->itemRepositoryMock, $this->hlsStreamerMock, null, $music); + + $advertised = array_column($bridge->getRootContainers(), 'child_count', 'id')['library-audio'] ?? 0; + + $walked = []; + $pages = 0; + for ($index = 0; $index < $advertised; $index += LibraryBridge::MAX_PAGE_ROWS) { + $page = $bridge->getContainerChildren('library-audio', null, $index, LibraryBridge::MAX_PAGE_ROWS); + $this->assertNotSame([], $page, sprintf('StartingIndex %d must not return an empty page', $index)); + foreach (array_column($page, 'id') as $id) { + $walked[] = $id; + } + $pages++; + } + + $this->assertGreaterThan(1, $pages, 'the fixture must need more than one page or it proves nothing'); + $this->assertSame( + $all, + $walked, + 'paging the whole container must reproduce the unpaged order EXACTLY — no dropped row, no ' + . 'repeated row, no reordering across a page boundary' + ); + $this->assertSame( + 'artist-04655', + end($walked), + 'the LAST artist must be reachable — the acceptance criterion S147 exists for' + ); + } + + /** + * S147 — the same walk for a root whose children span SEVERAL + * `media_items.type`s, which is where the offset arithmetic can actually go + * wrong. + * + * `video` is the concatenation `movie → series → video`, so `StartingIndex` + * is a global index into that concatenation and a page can straddle a type + * boundary. The walk asserts the concatenation of pages equals + * movies-then-series-then-videos exactly, which pins BOTH that whole types are + * skipped correctly and that the remainder is handed to the right type's SQL + * `OFFSET`. + */ + public function testPagingTheVideoRootSpansItsTypesWithoutDroppingOrRepeatingARow(): void + { + $rows = [ + 'movie' => 2500, + 'series' => 434, + 'video' => 6, + ]; + + $expected = []; + foreach ($rows as $type => $count) { + for ($i = 0; $i < $count; $i++) { + $expected[] = sprintf('%s-%05d', $type, $i); + } + } + + $this->stubCounts($rows); + $this->itemRepositoryMock->method('getAllByType')->willReturnCallback( + static function (string $type, int $limit, int $offset) use ($rows): array { + $items = []; + for ($i = $offset; $i < min($offset + $limit, $rows[$type] ?? 0); $i++) { + $items[] = ['id' => sprintf('%s-%05d', $type, $i), 'name' => 'x', 'type' => $type, 'path' => '']; + } + + return $items; + } + ); + + $advertised = array_column($this->bridge->getRootContainers(), 'child_count', 'id')['library-video'] ?? 0; + $this->assertSame(count($expected), $advertised); + + $walked = []; + for ($index = 0; $index < $advertised; $index += LibraryBridge::MAX_PAGE_ROWS) { + $page = $this->bridge->getContainerChildren('library-video', null, $index, LibraryBridge::MAX_PAGE_ROWS); + foreach (array_column($page, 'id') as $id) { + $walked[] = $id; + } + } + + $this->assertSame( + $expected, + $walked, + 'a page that straddles the movie/series boundary must be contiguous: the concatenated pages ' + . 'must equal movies-then-series-then-videos with nothing dropped or repeated' + ); + } + + /** + * S147 — `TotalMatches` on a `Browse` is the container's TRUE total, not the + * size of the page just handed over. + * + * `browseChildren()` used to set `TotalMatches = count($children)` where + * `$children` was the truncated list it then sliced. A renderer reading + * `NumberReturned == TotalMatches` concludes it has seen everything and stops + * — so even a correct paging layer underneath would never be asked for page + * two. Asserted through the PUBLIC browse path, because a bridge-level test + * would pass even if `ContentDirectory` ignored the new count entirely. + */ + public function testBrowseReportsTheContainerTotalNotThePageSize(): void + { + $this->stubCounts(['movie' => 10718, 'series' => 0, 'video' => 0]); + $this->itemRepositoryMock->method('getAllByType')->willReturnCallback( + static function (string $type, int $limit, int $offset): array { + $items = []; + for ($i = $offset; $i < min($offset + $limit, 10718); $i++) { + $items[] = ['id' => 'movie-' . $i, 'name' => 'M' . $i, 'type' => 'movie', 'path' => '']; + } + + return $items; + } + ); + + $cd = new \Phlix\Dlna\ContentDirectory($this->itemRepositoryMock); + $cd->setLibraryBridge($this->bridge); + + $result = $cd->browse('library-video', 'BrowseDirectChildren', '*', 4000, 5, ''); + + $this->assertSame(10718, $result['TotalMatches'] ?? null, 'TotalMatches must be the container total'); + $this->assertSame(5, $result['NumberReturned'] ?? null); + $this->assertStringContainsString( + 'movie-4000', + is_string($result['Result'] ?? null) ? $result['Result'] : '', + 'StartingIndex 4000 must return the 4,001st movie — before S147 it returned an empty page, ' + . 'because the PHP slice ran against a list already truncated at LIMIT 100' ); } @@ -607,7 +794,7 @@ public function testDrillingIntoAnArtistReturnsItsAlbumsFromTheMusicTables(): vo $this->itemRepositoryMock->method('findById')->willReturn([ 'id' => 'artist-1', 'name' => 'An Artist', 'type' => 'artist', 'path' => '', ]); - $this->itemRepositoryMock->expects($this->never())->method('findByParent'); + $this->itemRepositoryMock->expects($this->never())->method('findByParentPage'); $this->itemRepositoryMock->method('findByIds')->with(['album-1', 'album-2'])->willReturn([ ['id' => 'album-1', 'name' => 'First', 'type' => 'album', 'path' => ''], ['id' => 'album-2', 'name' => 'Second', 'type' => 'album', 'path' => ''], @@ -635,7 +822,7 @@ public function testDrillingIntoAnAlbumReturnsItsTracksFromTheMusicTables(): voi $this->itemRepositoryMock->method('findById')->willReturn([ 'id' => 'album-1', 'name' => 'First', 'type' => 'album', 'path' => '', ]); - $this->itemRepositoryMock->expects($this->never())->method('findByParent'); + $this->itemRepositoryMock->expects($this->never())->method('findByParentPage'); $this->itemRepositoryMock->method('findByIds')->with(['track-1'])->willReturn([ ['id' => 'track-1', 'name' => 'A Song', 'type' => 'track', 'path' => '/music/a.mp3'], ]); @@ -660,9 +847,10 @@ public function testDrillingIntoASeriesStillUsesTheParentIdHierarchy(): void $this->itemRepositoryMock->method('findById')->willReturn([ 'id' => 'series-1', 'name' => 'A Show', 'type' => 'series', 'path' => '', ]); - $this->itemRepositoryMock->expects($this->once())->method('findByParent')->with('series-1')->willReturn([ - ['id' => 'season-1', 'name' => 'S1', 'type' => 'season', 'path' => ''], - ]); + $this->itemRepositoryMock->expects($this->once())->method('findByParentPage') + ->with('series-1', LibraryBridge::MAX_PAGE_ROWS, 0)->willReturn([ + ['id' => 'season-1', 'name' => 'S1', 'type' => 'season', 'path' => ''], + ]); $music = $this->createMock(MusicLibraryService::class); $music->expects($this->never())->method('getAlbumMediaItemIdsForArtist'); @@ -685,9 +873,10 @@ public function testDrillingIntoASeriesStillUsesTheParentIdHierarchy(): void public function testAKnownContainerTypeSpareTheBridgeASecondLookup(): void { $this->itemRepositoryMock->expects($this->never())->method('findById'); - $this->itemRepositoryMock->expects($this->once())->method('findByParent')->with('series-1')->willReturn([ - ['id' => 'season-1', 'name' => 'S1', 'type' => 'season', 'path' => ''], - ]); + $this->itemRepositoryMock->expects($this->once())->method('findByParentPage') + ->with('series-1', LibraryBridge::MAX_PAGE_ROWS, 0)->willReturn([ + ['id' => 'season-1', 'name' => 'S1', 'type' => 'season', 'path' => ''], + ]); $music = $this->createMock(MusicLibraryService::class); $bridge = new LibraryBridge($this->itemRepositoryMock, $this->hlsStreamerMock, null, $music); @@ -705,7 +894,7 @@ public function testAKnownContainerTypeSpareTheBridgeASecondLookup(): void public function testAKnownArtistTypeGoesStraightToTheMusicTables(): void { $this->itemRepositoryMock->expects($this->never())->method('findById'); - $this->itemRepositoryMock->expects($this->never())->method('findByParent'); + $this->itemRepositoryMock->expects($this->never())->method('findByParentPage'); $this->itemRepositoryMock->method('findByIds')->with(['album-1'])->willReturn([ ['id' => 'album-1', 'name' => 'First', 'type' => 'album', 'path' => ''], ]); @@ -734,13 +923,16 @@ public function testContentDirectoryHandsTheResolvedTypeToTheBridge(): void $this->itemRepositoryMock->expects($this->once())->method('findById')->with('artist-1')->willReturn([ 'id' => 'artist-1', 'name' => 'An Artist', 'type' => 'artist', 'path' => '', ]); - $this->itemRepositoryMock->expects($this->never())->method('findByParent'); + $this->itemRepositoryMock->expects($this->never())->method('findByParentPage'); $this->itemRepositoryMock->method('findByIds')->with(['album-1'])->willReturn([ ['id' => 'album-1', 'name' => 'First', 'type' => 'album', 'path' => ''], ]); $music = $this->createMock(MusicLibraryService::class); $music->method('getAlbumMediaItemIdsForArtist')->willReturn(['album-1']); + // S147: TotalMatches is a COUNT that shares the listing's predicate, not + // the size of the page — so the drill-down needs it stubbed too. + $music->method('countAlbumMediaItemsForArtist')->willReturn(1); $bridge = new LibraryBridge($this->itemRepositoryMock, $this->hlsStreamerMock, null, $music); $cd = new \Phlix\Dlna\ContentDirectory($this->itemRepositoryMock); @@ -752,6 +944,93 @@ public function testContentDirectoryHandsTheResolvedTypeToTheBridge(): void $this->assertStringContainsString('album-1', is_string($result['Result'] ?? null) ? $result['Result'] : ''); } + /** + * S147 — a MUSIC drill-down resolves `StartingIndex` in SQL too, so a long + * discography or a boxed set is pageable rather than truncated. + * + * The offset has to reach the `music_*` query itself: these readers order on + * `(year, sort_title, al.id)` / `(disc, track, t.id)`, which the bridge cannot + * reproduce after the fact, and slicing the returned ids in PHP would just + * move the S97 defect one layer down. + */ + public function testAMusicDrillDownPassesTheBrowseOffsetIntoTheMusicQuery(): void + { + $captured = []; + + $music = $this->createMock(MusicLibraryService::class); + $music->method('getAlbumMediaItemIdsForArtist')->willReturnCallback( + function (string $id, int $limit, int $offset) use (&$captured): array { + $captured = ['id' => $id, 'limit' => $limit, 'offset' => $offset]; + + return ['album-page-2']; + } + ); + $music->method('countAlbumMediaItemsForArtist')->willReturn(3400); + + $this->itemRepositoryMock->method('findByIds')->willReturn([ + ['id' => 'album-page-2', 'name' => 'Later', 'type' => 'album', 'path' => ''], + ]); + + $bridge = new LibraryBridge($this->itemRepositoryMock, $this->hlsStreamerMock, null, $music); + + $children = $bridge->getContainerChildren('artist-1', 'artist', 2500, 40); + + $this->assertSame(['album-page-2'], array_column($children, 'id')); + $this->assertSame( + ['id' => 'artist-1', 'limit' => 40, 'offset' => 2500], + $captured, + 'the browse StartingIndex and RequestedCount must arrive at the music_* query unmodified' + ); + $this->assertSame( + 3400, + $bridge->getContainerChildCount('artist-1', 'artist'), + 'the drill-down advertises the artist\'s TRUE album count, not the size of the page' + ); + } + + /** + * The same for an album: its tracks are paged in SQL and its `TotalMatches` + * is a COUNT, not `count($page)`. + */ + public function testAnAlbumDrillDownPagesItsTracksAndCountsThemSeparately(): void + { + $captured = []; + + $music = $this->createMock(MusicLibraryService::class); + $music->method('getTrackMediaItemIdsForAlbum')->willReturnCallback( + function (string $id, int $limit, int $offset) use (&$captured): array { + $captured = ['id' => $id, 'limit' => $limit, 'offset' => $offset]; + + return ['track-9']; + } + ); + $music->method('countTrackMediaItemsForAlbum')->willReturn(212); + + $this->itemRepositoryMock->method('findByIds')->willReturn([ + ['id' => 'track-9', 'name' => 'Nine', 'type' => 'track', 'path' => '/m/9.mp3'], + ]); + + $bridge = new LibraryBridge($this->itemRepositoryMock, $this->hlsStreamerMock, null, $music); + + $this->assertSame( + ['track-9'], + array_column($bridge->getContainerChildren('album-1', 'album', 200, 10), 'id') + ); + $this->assertSame(['id' => 'album-1', 'limit' => 10, 'offset' => 200], $captured); + $this->assertSame(212, $bridge->getContainerChildCount('album-1', 'album')); + } + + /** + * A `parent_id` container advertises its own COUNT — the series/season branch + * must not regress to "the page I just built is the whole container". + */ + public function testAParentIdContainerAdvertisesItsCountedTotal(): void + { + $this->itemRepositoryMock->method('countByParent')->with('season-1')->willReturn(1234); + + $this->assertSame(1234, $this->bridge->getContainerChildCount('season-1', 'season')); + } + /** * The audio root resolves its artist ids in BOUNDED batches. * @@ -820,4 +1099,151 @@ function ( 'each batch must skip findByIds() own tag filter so profile_tags is not queried per chunk' ); } + + /** + * S147 fix round — 🔴 REGRESSION PIN: `SortCriteria` must select WHICH rows + * the page contains, not merely how the page is arranged. + * + * The first revision of S147 replaced master's sort → slice with slice → + * sort. Measured on exactly this fixture (20-child Video root, + * `StartingIndex 0`, `RequestedCount 5`, `-dc:title`) it returned + * `movie-04, 03, 02, 01, 00` — the five alphabetically LOWEST titles, + * reversed — where master returned the five highest. The container is far + * smaller than {@see LibraryBridge::MAX_PAGE_ROWS}, so the whole of it must be + * sorted before the window is taken. + */ + public function testASortedBrowseWindowsTheSortedContainerNotTheSortedWindow(): void + { + $cd = $this->videoRootBrowser(20); + + $result = $cd->browse('library-video', 'BrowseDirectChildren', '*', 0, 5, '-dc:title'); + + $this->assertSame( + ['movie-19', 'movie-18', 'movie-17', 'movie-16', 'movie-15'], + $this->idsFromDidl($result), + 'a descending title sort must return the five HIGHEST titles. Sorting only the page ' + . 'returns the five lowest, reversed — which is what this browse did before the fix.' + ); + $this->assertSame(20, $result['TotalMatches'] ?? null); + $this->assertSame(5, $result['NumberReturned'] ?? null); + } + + /** + * The same guarantee on a LATER page: `StartingIndex` must index into the + * SORTED container, so consecutive sorted pages tile it in sorted order. + */ + public function testASortedBrowseIsPagedInSortedOrder(): void + { + $cd = $this->videoRootBrowser(20); + + $this->assertSame( + ['movie-14', 'movie-13', 'movie-12', 'movie-11', 'movie-10'], + $this->idsFromDidl($cd->browse('library-video', 'BrowseDirectChildren', '*', 5, 5, '-dc:title')), + 'page two of a descending sort continues the descending order' + ); + $this->assertSame( + ['movie-15', 'movie-16', 'movie-17', 'movie-18', 'movie-19'], + $this->idsFromDidl($cd->browse('library-video', 'BrowseDirectChildren', '*', 15, 5, '+dc:title')), + 'and an ascending sort reaches the end of the container' + ); + } + + /** + * ⚠ The residual, pinned so it cannot silently become something else: past + * {@see LibraryBridge::MAX_PAGE_ROWS} the sort is page-local — but the + * container stays PAGEABLE, which is the property S147 exists to deliver. A + * "global sort" implemented by refusing to look past row 2,000 would re-create + * the S97 defect for every renderer that sends a `SortCriteria`. + */ + public function testASortedBrowseOfAHugeContainerStaysPageable(): void + { + $cd = $this->videoRootBrowser(6000); + + $result = $cd->browse('library-video', 'BrowseDirectChildren', '*', 5000, 5, '-dc:title'); + + $this->assertSame(6000, $result['TotalMatches'] ?? null); + $this->assertSame(5, $result['NumberReturned'] ?? null); + $this->assertSame( + ['movie-5004', 'movie-5003', 'movie-5002', 'movie-5001', 'movie-5000'], + $this->idsFromDidl($result), + 'StartingIndex 5000 must still reach the 5,001st row; the sort applies within the page' + ); + } + + /** + * S147 fix round — one `Browse` resolves the category counts ONCE. + * + * Instrumented before the fix, a single Browse of the Video root ran + * `countAllByType()` six times — `movie, series, video, movie, series, video` + * — because `ContentDirectory` fetched the page and the total through two + * entry points that each ended in `categoryTypeCounts()`. That is double the + * COUNTs on an unauthenticated path, and it opens a TOCTOU window between the + * number advertised and the offsets the listing spent. + */ + public function testOneBrowseResolvesTheCategoryCountsOnce(): void + { + $seen = []; + $this->itemRepositoryMock->method('countAllByType') + ->willReturnCallback(static function (string $type) use (&$seen): int { + $seen[] = $type; + + return $type === 'movie' ? 40 : 0; + }); + $this->itemRepositoryMock->method('getAllByType')->willReturn([]); + + $cd = new \Phlix\Dlna\ContentDirectory($this->itemRepositoryMock); + $cd->setLibraryBridge($this->bridge); + + $cd->browse('library-video', 'BrowseDirectChildren', '*', 0, 5, ''); + + $this->assertSame( + ['movie', 'series', 'video'], + $seen, + 'the page and the advertised total must be spent from ONE counts map — six COUNTs here ' + . 'means the map was resolved twice, at two different instants' + ); + } + + /** + * A ContentDirectory driving a Video root of `$rows` movies, with the listing + * resolved in SQL exactly as the real repository does it. + */ + private function videoRootBrowser(int $rows): \Phlix\Dlna\ContentDirectory + { + $this->stubCounts(['movie' => $rows, 'series' => 0, 'video' => 0]); + $this->itemRepositoryMock->method('getAllByType')->willReturnCallback( + static function (string $type, int $limit, int $offset) use ($rows): array { + $items = []; + for ($i = $offset; $i < min($offset + $limit, $rows); $i++) { + $items[] = [ + 'id' => 'movie-' . $i, + // Zero-padded so the lexical order the DLNA sorter uses + // agrees with the numeric order the ids read as. + 'name' => sprintf('Title %05d', $i), + 'type' => 'movie', + 'path' => '', + ]; + } + + return $items; + } + ); + + $cd = new \Phlix\Dlna\ContentDirectory($this->itemRepositoryMock); + $cd->setLibraryBridge($this->bridge); + + return $cd; + } + + /** + * @param array $result A `Browse` response. + * @return list Object ids, in the order the DIDL lists them. + */ + private function idsFromDidl(array $result): array + { + $didl = is_string($result['Result'] ?? null) ? $result['Result'] : ''; + preg_match_all('/<(?:item|container) id="([^"]+)"/', $didl, $matches); + + return $matches[1]; + } } diff --git a/tests/Unit/Media/Music/MusicLibraryServiceTest.php b/tests/Unit/Media/Music/MusicLibraryServiceTest.php index 26e975d7..f8e1774b 100644 --- a/tests/Unit/Media/Music/MusicLibraryServiceTest.php +++ b/tests/Unit/Media/Music/MusicLibraryServiceTest.php @@ -368,7 +368,11 @@ public function testGetTrackMediaItemIdsForAlbumJoinsTheAlbumOnItsMediaItemId(): ); $this->assertStringContainsString('WHERE al.media_item_id = ?', $captured['sql']); $this->assertStringContainsString('ORDER BY t.disc_number, t.track_number', $captured['sql']); - $this->assertSame(['album-uuid', MusicLibraryService::MAX_EMBEDDED_ROWS], $captured['params']); + $this->assertSame( + ['album-uuid', MusicLibraryService::MAX_EMBEDDED_ROWS, 0], + $captured['params'], + 'S147 — the third bind is the OFFSET that makes a long album pageable over DLNA' + ); } /** @@ -401,7 +405,7 @@ public function testGetAlbumMediaItemIdsForArtistSkipsAlbumsWithoutAMediaItem(): $this->assertStringContainsString('ar.media_item_id = ?', $captured['sql']); $this->assertStringContainsString('al.media_item_id IS NOT NULL', $captured['sql']); - $this->assertSame(['artist-uuid', 10], $captured['params']); + $this->assertSame(['artist-uuid', 10, 0], $captured['params']); } /** @@ -432,7 +436,7 @@ public function testTheHierarchyReadersClampTheirLimitAndShortCircuitOnAnEmptyId $captured = $this->captureQuery( fn(MusicLibraryService $s): mixed => $s->getTrackMediaItemIdsForAlbum('album-uuid', 10_000) ); - $this->assertSame(['album-uuid', MusicLibraryService::MAX_EMBEDDED_ROWS], $captured['params']); + $this->assertSame(['album-uuid', MusicLibraryService::MAX_EMBEDDED_ROWS, 0], $captured['params']); $db = $this->createMock(Connection::class); $db->expects($this->never())->method('query');