S97 — music hierarchy: make the music_* tables authoritative, never write media_items.parent_id for music#570
Merged
Conversation
The music_* tables are the ONE authoritative music hierarchy; media_items.parent_id is NEVER written for artist/album/track. Decision recorded in plan_updates_worklog.md (2026-07-27) and steps/S97.worklog.md; verified read-only on production: artist 4,656 / album 10,966 / track 61,105, and 0 of 76,727 carry a parent_id. Deleted, because making one path authoritative means removing the other: - MusicLibraryScanner::findAdoptableAlbumMediaItemId(): the `OR mi.parent_id = ?` branch and the $artistMediaItemId parameter, plus upsertAlbum()'s now-dead parameter and the flushAlbum() local that fed it. `AND mi.parent_id IS NULL` is KEPT and re-documented as an ENFORCED INVARIANT rather than a no-op filter, with the new bound derived deliberately: correctness needs "the orphan I adopt is interchangeable with a row I would mint now", and under this verdict title + library + unreferenced is already the complete identity of an album media_items row, so the parent check is pure defence that fails SAFE and is the tripwire against a regression re-introducing music parenting. - upsertTrack()'s $albumMediaItemId parameter and its unset() — it was never read; a track is linked by music_tracks.album_id (NOT NULL, enforced FK, ON DELETE CASCADE). - MusicLibraryManager::getArtists()/getAlbums()/getTracks() — the metadata_json readers S99 orphaned; zero production callers. - Dlna\ContentDirectoryService — the six unwired music stubs returning hardcoded 'Unknown Artist' WERE the whole class, and the class had zero references anywhere in the repo (the live one is Dlna\ContentDirectory). Tests: the album-adoption test is re-documented as the invariant test and renamed accordingly, and a new statement-level regression test asserts that no INSERT/UPDATE/REPLACE a scan issues so much as mentions parent_id, while the adoption SELECT must still carry the invariant. The MusicSchemaConnection double now models the narrowed predicate exactly, so deleting it from the SQL still fails a test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The consumers that reached for a music hierarchy were reaching for
media_items.parent_id, which is never written for music. They now read
music_*, which is.
MusicLibraryService gains the hierarchy readers they need
(getArtistMediaItemIds, getArtistsWithMediaItemCount,
getAlbumMediaItemIdsForArtist, getTrackMediaItemIdsForAlbum,
getTrackMediaItemIdsForArtist), all clamped to MAX_EMBEDDED_ROWS for the
same buffered-response reason as the existing batched reads. It is now a
container entry (MediaServicesProvider) instead of being new-ed inline,
since three call sites need it.
- Shuffle (MediaItemController): an album/artist resolves to TRACK
media_items ids — the ids /media/{id}/stream can actually serve.
Previously findByParent() returned nothing and it 404'd; a
parent_id-based hierarchy would have returned unplayable container ids
instead. The parental cap is re-applied to the resolved tracks, so the
new path is not a hole in the cap. A null service keeps the old 404.
- DLNA (LibraryBridge): CATEGORY_TYPES['audio'] narrows from
['music','audio','album','artist','track','audiobook'] to
['artist','audio','audiobook'] — the true top level. The old list
advertised 76,727+ children (countAllByType summed over six types)
while getLibraryItems() could only return getAllByType()'s default page
of 100 per type, so the promised childCount and the delivered list
disagreed by three orders of magnitude, and albums/tracks sat beside
their own artists with no way to descend. Artists are now enumerated
from music_artists (so the count and the listing share one predicate
and orphaned mirror rows are not advertised), and artist -> albums ->
tracks drill-down comes from music_*. Non-music containers still use
parent_id, which is where their hierarchy really lives.
- Search (ItemRepository): artist/album are matched on the authoritative
music_artists.name / music_albums.title and excluded from the
media_items half, so the match no longer runs against an unmaintained
mirror column and orphaned artist/album rows stop being served as hits
that lead nowhere. Two indexed statements rather than one OR-ed query,
because an OR across MATCH..AGAINST and a joined LIKE cannot use the
FULLTEXT index. Failure in the music half degrades to "no music hits".
Also fixes the false comment at MediaItemController's CONTAINER_TYPES,
which claimed music album/artist rows are not scanner-created; they are,
and the production counts prove it. MediaItemController::children() is
deleted — it was never registered on any route, and its three parental
tests are replaced by parental coverage of the new music shuffle path.
CORRECTION, measured read-only on production: the brief's claim that
"only track filenames are searchable; artist and album names are not" is
false in both halves. The scanner stamps artist names and album titles
into media_items.name, a track's name is its tag title, and both
MATCH(name) AGAINST('Eminem') and the LIKE form returned 135 artist +
74 album + 37 track rows. The real defects are the mirror column and the
orphan rows, which is what this change fixes; recorded in the
searchMusicContainers() docblock so the wrong premise is not re-derived.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 7 minor |
| ErrorProne | 2 medium |
| CodeStyle | 26 minor |
| Complexity | 1 medium |
| Comprehensibility | 1 minor |
🟢 Metrics 0 duplication
Metric Results Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #570 +/- ##
============================================
+ Coverage 65.40% 65.81% +0.40%
+ Complexity 21061 21055 -6
============================================
Files 658 657 -1
Lines 66311 66241 -70
============================================
+ Hits 43372 43596 +224
+ Misses 22939 22645 -294 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…livers
BLOCKING 1. getLibraryChildCount('audio') returned an unbounded COUNT(*) from
music_artists (4,656 on prod) while getLibraryItems('audio') stops at
MusicLibraryService::MAX_EMBEDDED_ROWS (2,000), so a renderer was promised 2.3x
what Browse handed it — and two docblocks asserted the two agreed. The count is
now clamped to that same constant, and both claims are replaced with a plain
statement of the residual bound: artists past MAX_EMBEDDED_ROWS are NOT
reachable over DLNA, because getLibraryItems() takes no offset and
ContentDirectory::browseChildren() slices an already-truncated list in PHP. Real
paging is a separate step and is not attempted here.
NB2. ItemRepository::mergeSearchResults() concatenated the music half after the
media_items half and cut at $limit, so a non-music half that filled the page
returned zero artists/albums. The second half now gets a reserved slice of up to
half the page, with the first half keeping whatever it does not use.
NB3. LibraryBridge::getMusicChildren() ran a findById() on EVERY drill-down just
to learn the type, including series/season ones that fell through to
findByParent(). ContentDirectory::browse() has already resolved and cached that
row, so it now hands the type over — one query saved per DLNA browse.
NB4. The audio-root listing could build one IN (...) with 2,000 placeholders.
Id resolution is chunked at 500 (the batch ceiling DuplicateFinder already uses);
findByIds() re-orders to the ids it was given, so the result is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tags once per page NB1: tests/Unit/Dlna/LibraryBridgeTest.php — the assertion message claimed the advertised childCount as a whole never exceeds what getLibraryItems() can return. True for the artist branch the test exercises (the stub zeroes audio/audiobook), false for the non-artist branches, where getLibraryChildCount() still uses the unbounded countAllByType() against getAllByType()'s LIMIT 100 default. Message and docblock now name the ARTIST term explicitly and point the residual defect at S147; the test is not widened and the non-artist branches are untouched. NB2: findByIdsChunked() called findByIds() per chunk, and findByIds() ends in filterItemsByTags(), which issues two profile_tags queries per call when a profile is set — 8 queries for a 2,000-id audio root instead of 2. findByIds() gains an opt-out flag (default true, so all existing call sites are unchanged) and the chunked reader applies filterItemsByTags() once over the concatenated rows. Rows, row order and the 2,000 total bound are identical: the filter is an order-preserving per-item predicate over the same tag sets, so one pass drops exactly what the per-chunk passes dropped. Tests: 7681, Assertions: 49936, Skipped: 129. PHPStan [OK] No errors. phpcs --standard=PSR12 -n src/ clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Decision this implements
Option B — the
music_*tables are authoritative.media_items.parent_idis NEVER written for music.Design settled 2026-07-27; full record in
plan_updates_worklog.md, investigation insteps/S97.worklog.md.Why not the
parent_idchain:media_items.parent_idis a nullableCHAR(36)with a plain index and no FK, whilemusic_tracks.album_id/artist_idareINT UNSIGNED NOT NULLwith enforced FKs + CASCADE. The chain islossier by construction, so it cannot be made authoritative.
music_*⇒parent_idwould be acache, not a source of truth.
INSERTconstraint is unsatisfiable on the adoption path — an adopted orphan alreadyexists with
parent_id IS NULL.?topLevel=1would make albums and tracks vanish fromthe grid/total/A-Z rail, and shuffle would return unplayable album UUIDs.
Premise verified on prod, read-only:
media_itemsartist 4,656 / album 10,966 / track 61,105 — matchingmusic_*exactly — with 0 of 76,727 carrying aparent_id. No migration, no backfill, no rescan, and theS122 skip index is untouched.
Deleted (the non-authoritative path)
findAdoptableAlbumMediaItemId()'sOR mi.parent_id = ?branch + its$artistMediaItemIdparam.🔴
AND mi.parent_id IS NULLis kept and re-derived as an enforced invariant — it now fails safe andacts as the regression tripwire. The "if you ever DO parent music it must be the SAME INSERT" warning is
carried forward in the docblock.
upsertTrack()'s dead$albumMediaItemIdparam +unset().MusicLibraryManager::getArtists()/getAlbums()/getTracks();ContentDirectoryService; unroutedMediaItemController::children().album/artistrows are not scanner-created — they are.Rewired onto
music_*Shuffle (was able to return unplayable album UUIDs), DLNA audio root + drill-down, and music search.
Two premises from the brief that were WRONG and are corrected here
media_items.name. The real defects were amirror column nothing keeps in step with
music_*, and orphaned mirror rows served as hits leadingnowhere. Both fixed; the measurement is pinned in
searchMusicContainers()'s docblock.getAllByType()caps at 100. The actual defect isthat the advertised
childCountand the deliverable list disagreed by three orders of magnitude.Neither re-opens the option-B verdict.
Verification
Tests: 7673(master baseline 7658; +15 fully accounted: −6 deleted, +21 added)[OK] No errors(698/698); phpcs PSR-12 cleancomposer.json, socomposer installnever installs it and the step is wrapped inif [ -f vendor/bin/psalm ]. Its green ismeaningless. Filed separately; not introduced by this PR.
Out of scope
phlix-ui redirect of music libraries to
/app/music(separate repo); S145 (upsertTrack()neverupdating
music_tracks.album_id/artist_id) — deliberately a separate step, behaviour left byte-identical.