Adapt artist / audiobook controller for authors and narrators#3570
Adapt artist / audiobook controller for authors and narrators#3570fmunkes wants to merge 31 commits into
Conversation
|
Not sure how common it is that the authors/narrators have actual metadata ? I think it may make sense to quickly browse by narrator or author but not sure if we need to create a whole media type for them, I'm also wondering if we even have metadata. When I checked spotify etc. they did not provide any metadata for authors or narrators except the name |
I like the idea of an artist_type, and then enhancing the artist controller a bit. You are right, the data is usually name and maybe a picture - but since we then have a provider id, we can also ask the provider for the audiobooks belonging to the author/ narrator. Shall I go with the artist_type then? I could also enhance the albums controller for series and introduce another album_type? |
|
I'm not sure how it would work in practice to count authors and narrators as artists but I guess we can say the same for composers (classical music) so it's worth the investigation. would be nice if we can fix it. As for series; can you enlighten me what those are ? |
|
Series would be for audiobooks, not podcast (but why not include it, although I don't know if that exists). A book series would be something like Lord of the rings with multiple books. Attributes would be the series name, picture, and the overall progress. So it is similar to an album (or playlist?) but if I would hit play on a series, my expectation would be that it starts where you left off, (e.g. book no 2) and not with the first one. I guess essentially you can map all of these types to existing MA types with a little bit of extra information. What I can't judge is if it is more future proof to just keep this extra branches on an artist or album in mind, or just separate them completely with their own controller. Or put a controller between base controller and implementation, where both types can inherit from. Maintenance of course being important too. I don't know how well Spotify's integration of audiobooks is, but series is definitely something you find e.g. in audible. Of course, we can also just add them via attributes, and search the db for them. I really don't know what's best - maybe I'm to influenced by abs... |
|
For putting narrators, authors and composers into artists with a dedicated type I'd say yes. For series I'm not sure. Definitely not Album. To me it's more a metadata field which we can use for extra filtering and such. I guess you can compare this to collections within movies |
|
Ok, sounds good. I'll adapt both PRs, and mark them as draft for the time being. For series, what I really would like to have, is, that you can collapse them into a single visual item, instead of multiple Audiobooks. But that's a frontend thing I think, so I'll think about something there. |
|
Ok, I think this is ready for a first review. |
|
Before doing an in-depth review, let's chat about the controller first. It feels a bit strange to have all this mixed logic in the ArtistController (eg Type = ARTIST or Type IN (Narrator , Author) ). @marcelveldt would it be an idea to implement this as a child controller or generic? |
|
A composer, author or writer is a form of Artist. Currently we only support singers but maybe we should broaden it. Can still belong in the Artist controller, just like we already have multiple Album types. It's just a type... |
|
I renamed the default artist type to "singer", that should make it clearer then, that we are talking about music artists and so on. |
| async def _remove_author_narrator_from_library(self, db_id: int, recursive: bool) -> None: | ||
| # recursively also remove author/ narrator audiobooks | ||
| for db_row in await self.mass.music.database.get_rows_from_query( | ||
| f"SELECT album_id FROM {DB_TABLE_AUDIOBOOK_ARTISTS} WHERE artist_id = :artist_id", |
There was a problem hiding this comment.
I think album_id does not exist in the table?
There was a problem hiding this comment.
yeah. Rename issue, I should have checked that more closely...
| return [] | ||
| prov = cast("MusicProvider", prov) | ||
| if ProviderFeature.NARRATOR_AUDIOBOOKS in prov.supported_features: | ||
| return await prov.get_author_audiobooks(item_id) |
There was a problem hiding this comment.
Shouldn't this be prov.get_narrator_audiobooks(item_id) ?
| for provider_mapping in library_artist.provider_mappings: | ||
| if provider_mapping.provider_instance not in unique_providers: | ||
| continue | ||
| provider_audiobooks = await self.get_provider_author_audiobooks( |
There was a problem hiding this comment.
Doesn't this call depend on the artist_type? Ie get_provider_author_audiobooks or get_provider_narrator_audiobooks
There was a problem hiding this comment.
Pull request overview
This PR extends the artist/audiobook controllers to treat audiobook authors and narrators as artist-like entities, adding new provider hooks, database schema support, and controller APIs so audiobook relationships can be queried similarly to music artists.
Changes:
- Added provider-facing author/narrator audiobook APIs and stricter audiobook sync/type handling.
- Added
artist_typesupport plus a newaudiobook_artistsjoin table to model audiobook-to-author/narrator relationships. - Extended artist and audiobook controllers with author/narrator-aware library queries, removal logic, and audiobook lookup endpoints.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
music_assistant/models/music_provider.py |
Adds provider hooks for author/narrator audiobook retrieval and sync-time validation for audiobook contributor types. |
music_assistant/helpers/compare.py |
Prevents artist matching across different artist_type values. |
music_assistant/controllers/music.py |
Bumps DB schema, adds artist_type to artists, and creates the new audiobook_artists table. |
music_assistant/controllers/media/audiobooks.py |
Stores string contributors separately from linked artist records and adds audiobook↔artist mapping writes. |
music_assistant/controllers/media/artists.py |
Adds author/narrator-aware library filtering, removal paths, and audiobook lookup APIs. |
music_assistant/constants.py |
Defines the DB_TABLE_AUDIOBOOK_ARTISTS table name constant. |
Comments suppressed due to low confidence (2)
music_assistant/controllers/media/audiobooks.py:325
- [CRITICAL]
_update_library_itemnow stripsArtistauthors/narrators out of the JSON fields, but it never updatesaudiobook_artists, so overwriting an existing audiobook can drop all author/narrator information from the library item while leaving no replacement links. Update the join table here just like albums updatealbum_artists.
# only serialize str narrators/ authors to db
_update_authors = [author for author in update.authors if isinstance(author, str)]
_update_narrators = [narrator for narrator in update.narrators if isinstance(narrator, str)]
await self.mass.music.database.update(
self.db_table,
{"item_id": db_id},
{
"name": name,
"sort_name": sort_name,
"version": update.version if overwrite else cur_item.version or update.version,
"metadata": serialize_to_json(metadata),
"external_ids": serialize_to_json(
update.external_ids if overwrite else cur_item.external_ids
),
"publisher": cur_item.publisher or update.publisher,
"authors": serialize_to_json(
_update_authors if overwrite else cur_item.authors or _update_authors
),
"narrators": serialize_to_json(
_update_narrators if overwrite else cur_item.narrators or _update_narrators
),
"duration": update.duration if overwrite else cur_item.duration or update.duration,
"search_name": create_safe_string(name, True, True),
"search_sort_name": create_safe_string(sort_name or "", True, True),
"timestamp_added": int(update.date_added.timestamp())
if update.date_added
else UNSET,
},
)
# update/set provider_mappings table
provider_mappings = (
update.provider_mappings
if overwrite
else {*update.provider_mappings, *cur_item.provider_mappings}
)
await self.set_provider_mappings(db_id, provider_mappings, overwrite)
self.logger.debug("updated %s in database: (id %s)", update.name, db_id)
await self._set_playlog(db_id, update)
music_assistant/controllers/media/artists.py:74
- [PROBLEM]
library_items()only appliesalbum_artists_onlyfor singers, butlibrary_count()still filters every artist type throughalbum_artists, so author/narrator result counts can disagree with the actual listing and break pagination. Guard this clause the same way aslibrary_items().
query_parts = [f"artist_type = '{artist_type}'"]
if favorite_only:
query_parts.append("favorite = 1")
if album_artists_only:
query_parts.append(
f"item_id in (select {DB_TABLE_ALBUM_ARTISTS}.artist_id "
f"FROM {DB_TABLE_ALBUM_ARTISTS})"
)
| if (isinstance(lib_author, Artist) and not isinstance(prov_author, Artist)) or ( | ||
| isinstance(lib_narrator, Artist) and not isinstance(prov_narrator, Artist) | ||
| ): | ||
| library_item = await self.mass.music.audiobooks.update_item_in_library( | ||
| library_item.item_id, prov_item | ||
| ) |
|
@fmunkes can you have a look at what I am working on here as there is some overlap in ideas I think. music-assistant/models@main...add-classical |
Yeah, very similar, and much more extensive on your end :). I think your ArtistRole and the ArtistType do in some way describe the same, though I get that an artist can have multiple roles at the same (conductor, composer, etc). Then my question would be, which way we should go - have the same artist potentially multiple times in the database due to different types (e.g. one row for composer, one row for singer), or remove the ArtistType from the Artist completely and add the role of an artist via your credit approach. I personally like the first approach a bit more, not because it's mine, but because I think the downside of having some duplicates, is compensated by the easier use of db methods via sqlite, i.e. by just filtering for the ID of the artist (get me all tracks composed by xy would be a db query, and not in python, which should be faster. What do you think? |
|
Thanks for taking a look. Looking at it more deeply I think these are actually compatible and we don’t need to pick one as they solve different problems. Your Artist.type is the artist’s primary identity (what they mostly are — narrator, author, singer, composer). That’s what your audiobook PR needs, and it’s a useful field to keep. Our Credit with ArtistRole is the per-relationship role — what this artist does on this specific track. Classical music (and could be extended to all music types) needs this because the same person can play multiple roles on the same recording (Karajan is both conductor and main artist on a Beethoven track) and different roles across recordings (some performers also compose). We can’t collapse that to a single per-Artist type without losing information. On the performance point — both approaches are DB-side queries, not Python filtering. Ours filters on track_artists.role, yours filters on artist_id. Both are indexed; neither is slower. Concretely I’d suggest: keep your Artist.type for primary type / audiobook browse, and if mine gets up then I’ll keep Credit for per-track-relationship role. They sit at different layers and serve different queries. |
|
Alright, sounds good |
Adds endpoints for authors/ narrators. Is that sql stuff actually safe?
Same question in reverse to #3569 can be asked here - would it make sense to make authors/ narrators media items, so things like pictures, or more metadata can be added. Also, the provider could be asked directly for their items, instead of searching the db.
I'm willing to do that, but what do you think?
update after discussion below
depends on music-assistant/models#209