library: fix sort-name resolution and migration rebuild

Collect every credited artist id when resolving album and song sort keys so
the singular id and plural refs can both be looked up, and add regression
tests for the compilation and stale-sort-name cases.

Drop the FTS tables before the backfill so the old triggers do not index rows
that are about to be discarded, and rely on createSearchIndex's rebuild
instead of rebuilding a second time. Treat a whitespace-only artist sortName
as absent, and use the sort keys in the downloads tree too.
This commit is contained in:
austinried
2026-10-18 05:26:59 +00:00
parent 7d8a2dae9f
commit 0c2e85134b
8 changed files with 118 additions and 29 deletions
@@ -166,7 +166,10 @@ interface DownloadDao {
@Query( @Query(
"SELECT d.sourceId AS sourceId, d.songId AS songId, s.title AS title, s.albumId AS albumId, al.name AS albumName, " + "SELECT d.sourceId AS sourceId, d.songId AS songId, s.title AS title, s.albumId AS albumId, al.name AS albumName, " +
"al.artistId AS artistId, COALESCE(ar.name, al.albumArtist) AS artistName, d.status AS status, " + "al.artistId AS artistId, COALESCE(ar.name, al.albumArtist) AS artistName, d.status AS status, " +
"d.bytes AS bytes, d.total AS total " + "d.bytes AS bytes, d.total AS total, " +
"COALESCE(NULLIF(ar.sortName, ''), NULLIF(al.albumArtistSort, ''), ar.name, al.albumArtist, '') AS artistSort, " +
"COALESCE(NULLIF(al.sortName, ''), al.name, '') AS albumSort, " +
"COALESCE(NULLIF(s.sortName, ''), s.title, '') AS songSort " +
"FROM song_downloads d " + "FROM song_downloads d " +
"JOIN songs s ON s.sourceId = d.sourceId AND s.id = d.songId " + "JOIN songs s ON s.sourceId = d.sourceId AND s.id = d.songId " +
"LEFT JOIN albums al ON al.sourceId = s.sourceId AND al.id = s.albumId " + "LEFT JOIN albums al ON al.sourceId = s.sourceId AND al.id = s.albumId " +
@@ -461,6 +461,9 @@ val MIGRATION_30_31 =
connection.execSQL("ALTER TABLE `albums` ADD COLUMN `albumArtistSort` TEXT NOT NULL DEFAULT '' COLLATE NOCASE") connection.execSQL("ALTER TABLE `albums` ADD COLUMN `albumArtistSort` TEXT NOT NULL DEFAULT '' COLLATE NOCASE")
connection.execSQL("ALTER TABLE `songs` ADD COLUMN `sortName` TEXT NOT NULL DEFAULT '' COLLATE NOCASE") connection.execSQL("ALTER TABLE `songs` ADD COLUMN `sortName` TEXT NOT NULL DEFAULT '' COLLATE NOCASE")
connection.execSQL("ALTER TABLE `songs` ADD COLUMN `artistSort` TEXT NOT NULL DEFAULT '' COLLATE NOCASE") connection.execSQL("ALTER TABLE `songs` ADD COLUMN `artistSort` TEXT NOT NULL DEFAULT '' COLLATE NOCASE")
dropSearchIndex(connection, "artist_search")
dropSearchIndex(connection, "album_search")
dropSearchIndex(connection, "song_search")
connection.execSQL("UPDATE `artists` SET `sortName` = `name`") connection.execSQL("UPDATE `artists` SET `sortName` = `name`")
connection.execSQL("UPDATE `albums` SET `sortName` = `name`") connection.execSQL("UPDATE `albums` SET `sortName` = `name`")
connection.execSQL("UPDATE `songs` SET `sortName` = `title`") connection.execSQL("UPDATE `songs` SET `sortName` = `title`")
@@ -488,15 +491,9 @@ val MIGRATION_30_31 =
ALBUM_INDEXES.forEach { connection.execSQL("DROP INDEX IF EXISTS `$it`") } ALBUM_INDEXES.forEach { connection.execSQL("DROP INDEX IF EXISTS `$it`") }
ARTIST_INDEX_SQL.forEach { connection.execSQL(it) } ARTIST_INDEX_SQL.forEach { connection.execSQL(it) }
ALBUM_INDEX_SQL.forEach { connection.execSQL(it) } ALBUM_INDEX_SQL.forEach { connection.execSQL(it) }
dropSearchIndex(connection, "artist_search")
dropSearchIndex(connection, "album_search")
dropSearchIndex(connection, "song_search")
createSearchIndex(connection, "artist_search", "artists", listOf("name", "sortName")) createSearchIndex(connection, "artist_search", "artists", listOf("name", "sortName"))
createSearchIndex(connection, "album_search", "albums", listOf("name", "albumArtist", "sortName", "albumArtistSort")) createSearchIndex(connection, "album_search", "albums", listOf("name", "albumArtist", "sortName", "albumArtistSort"))
createSearchIndex(connection, "song_search", "songs", listOf("title", "artist", "sortName", "artistSort")) createSearchIndex(connection, "song_search", "songs", listOf("title", "artist", "sortName", "artistSort"))
rebuildSearchIndex(connection, "artist_search")
rebuildSearchIndex(connection, "album_search")
rebuildSearchIndex(connection, "song_search")
} }
} }
@@ -554,13 +551,6 @@ private suspend fun dropSearchIndex(
connection.execSQL("DROP TABLE IF EXISTS `$table`") connection.execSQL("DROP TABLE IF EXISTS `$table`")
} }
private suspend fun rebuildSearchIndex(
connection: SQLiteConnection,
table: String,
) {
connection.execSQL("INSERT INTO `$table`(`$table`) VALUES('rebuild')")
}
// Mirrors the FTS5 table and content-sync triggers Room generates for the @Fts5 entities. // Mirrors the FTS5 table and content-sync triggers Room generates for the @Fts5 entities.
private suspend fun createSearchIndex( private suspend fun createSearchIndex(
connection: SQLiteConnection, connection: SQLiteConnection,
@@ -94,6 +94,9 @@ data class DownloadedSong(
val size: Long = 0, val size: Long = 0,
val bytes: Long = 0, val bytes: Long = 0,
val total: Long = 0, val total: Long = 0,
val artistSort: String = artistName.orEmpty(),
val albumSort: String = albumName.orEmpty(),
val songSort: String = title,
) { ) {
val progress: Float? val progress: Float?
get() = total.takeIf { it > 0 }?.let { (bytes.toFloat() / it).coerceIn(0f, 1f) } get() = total.takeIf { it > 0 }?.let { (bytes.toFloat() / it).coerceIn(0f, 1f) }
@@ -32,7 +32,7 @@ object SubsonicXml {
albumCount = attrs.longAttr("albumCount") ?: 0L, albumCount = attrs.longAttr("albumCount") ?: 0L,
starred = attrs.dateAttr("starred"), starred = attrs.dateAttr("starred"),
coverArt = attrs.attr("coverArt").ifEmpty { null }, coverArt = attrs.attr("coverArt").ifEmpty { null },
sortName = attrs.attr("sortName").ifEmpty { attrs.attr("name") }, sortName = attrs.attr("sortName").takeIf { it.isNotBlank() } ?: attrs.attr("name"),
) )
internal fun albumDraft( internal fun albumDraft(
@@ -190,7 +190,10 @@ class SyncService(
val sortNames = val sortNames =
artistSortNames( artistSortNames(
batch.first().sourceId, batch.first().sourceId,
batch.mapNotNull { it.artistRefs.firstOrNull()?.id }.toSet(), batch
.flatMap { listOfNotNull(it.artistId) + it.artistRefs.map { ref -> ref.id } }
.filter { it.isNotBlank() }
.toSet(),
) )
return batch.map { song -> return batch.map { song ->
val primary = song.artistId ?: song.artists.firstOrNull()?.id val primary = song.artistId ?: song.artists.firstOrNull()?.id
@@ -207,7 +210,10 @@ class SyncService(
val sortNames = val sortNames =
artistSortNames( artistSortNames(
batch.first().sourceId, batch.first().sourceId,
batch.mapNotNull { it.artistId ?: it.artists.firstOrNull()?.id }.toSet(), batch
.flatMap { listOfNotNull(it.artistId) + it.artistRefs.map { ref -> ref.id } }
.filter { it.isNotBlank() }
.toSet(),
) )
return batch.map { album -> return batch.map { album ->
val primary = album.artistId ?: album.artists.firstOrNull()?.id val primary = album.artistId ?: album.artists.firstOrNull()?.id
@@ -31,6 +31,7 @@ data class DownloadArtistNode(
val name: String, val name: String,
val bytes: Long, val bytes: Long,
val albums: List<DownloadAlbumNode>, val albums: List<DownloadAlbumNode>,
val sortName: String = name,
) )
data class DownloadAlbumNode( data class DownloadAlbumNode(
@@ -38,6 +39,7 @@ data class DownloadAlbumNode(
val name: String, val name: String,
val bytes: Long, val bytes: Long,
val songs: List<DownloadedSong>, val songs: List<DownloadedSong>,
val sortName: String = name,
) )
@OptIn(ExperimentalCoroutinesApi::class) @OptIn(ExperimentalCoroutinesApi::class)
@@ -105,6 +107,7 @@ internal fun buildTree(
id = artistKey.first, id = artistKey.first,
name = artistKey.second.ifBlank { unknownArtist }, name = artistKey.second.ifBlank { unknownArtist },
bytes = artistSongs.sumOf { it.size }, bytes = artistSongs.sumOf { it.size },
sortName = artistSongs.first().artistSort.ifBlank { artistKey.second },
albums = albums =
artistSongs artistSongs
.groupBy { it.albumId.orEmpty() to it.albumName.orEmpty() } .groupBy { it.albumId.orEmpty() to it.albumName.orEmpty() }
@@ -113,9 +116,10 @@ internal fun buildTree(
id = albumKey.first, id = albumKey.first,
name = albumKey.second.ifBlank { unknownAlbum }, name = albumKey.second.ifBlank { unknownAlbum },
bytes = albumSongs.sumOf { it.size }, bytes = albumSongs.sumOf { it.size },
songs = albumSongs.sortedBy { it.title.lowercase() }, sortName = albumSongs.first().albumSort.ifBlank { albumKey.second },
songs = albumSongs.sortedBy { it.songSort.lowercase() },
) )
}.sortedBy { it.name.lowercase() }, }.sortedBy { it.sortName.lowercase() },
) )
}.sortedBy { it.name.lowercase() }, }.sortedBy { it.sortName.lowercase() },
) )
@@ -985,31 +985,43 @@ class LibraryDaoTest {
val dao = db.libraryDao() val dao = db.libraryDao()
dao.upsertArtists( dao.upsertArtists(
listOf( listOf(
artist(sourceId, "ar-1", "宇多田ヒカル", albumCount = 1).copy(sortName = "Utada Hikaru"), artist(sourceId, "ar-1", "あいみょん", albumCount = 1).copy(sortName = "Zzz"),
artist(sourceId, "ar-2", "あいみょん", albumCount = 1).copy(sortName = "Aimyon"), artist(sourceId, "ar-2", "宇多田ヒカル", albumCount = 1).copy(sortName = "Aaa"),
), ),
) )
dao.upsertAlbums( dao.upsertAlbums(
listOf( listOf(
album(sourceId, "al-1", "First Love", year = null, starred = null) album(sourceId, "al-1", "Zzz", year = null, starred = null)
.copy(sortName = "First Love", albumArtistSort = "Utada Hikaru"), .copy(sortName = "Aaa", albumArtistSort = "Zzz"),
album(sourceId, "al-2", "Zutto", year = null, starred = null) album(sourceId, "al-2", "Aaa", year = null, starred = null)
.copy(sortName = "Zutto", albumArtistSort = "Aimyon"), .copy(sortName = "Zzz", albumArtistSort = "Aaa"),
), ),
) )
assertEquals( assertEquals(
listOf("あいみょん", "宇多田ヒカル"), listOf("宇多田ヒカル", "あいみょん"),
dao.artistsByName(sourceId, 0, "").page().map { it.name }, dao.artistsByName(sourceId, 0, "").page().map { it.name },
) )
assertEquals( assertEquals(
listOf("First Love", "Zutto"), listOf("あいみょん", "宇多田ヒカル"),
dao.artistsByNameReversed(sourceId, 0, "").page().map { it.name },
)
assertEquals(
listOf("Zzz", "Aaa"),
dao.albumsByName(sourceId, 0, "").page().map { it.name }, dao.albumsByName(sourceId, 0, "").page().map { it.name },
) )
assertEquals( assertEquals(
listOf("Zutto", "First Love"), listOf("Aaa", "Zzz"),
dao.albumsByNameReversed(sourceId, 0, "").page().map { it.name },
)
assertEquals(
listOf("Aaa", "Zzz"),
dao.albumsByArtist(sourceId, 0, "").page().map { it.name }, dao.albumsByArtist(sourceId, 0, "").page().map { it.name },
) )
assertEquals(
listOf("Zzz", "Aaa"),
dao.albumsByArtistReversed(sourceId, 0, "").page().map { it.name },
)
} }
@Test @Test
@@ -993,6 +993,77 @@ class SyncServiceTest {
assertEquals("Utada Hikaru", db.libraryDao().songOnce(1, "s1")!!.artistSort) assertEquals("Utada Hikaru", db.libraryDao().songOnce(1, "s1")!!.artistSort)
} }
@Test
fun songArtistSortUsesTheSingularArtistOverThePluralList() =
runTest {
insertSource()
val source =
FakeMusicSource(
artists =
listOf(
artist("a1", "A").copy(sortName = "Alpha"),
artist("a2", "B").copy(sortName = "Beta"),
),
albums = listOf(album("al1")),
songs =
listOf(
song("s1").copy(
artistId = "a2",
artists = listOf(ArtistRef("a1", "A"), ArtistRef("a2", "B")),
),
),
)
SyncService(db, source).sync()
assertEquals("Beta", db.libraryDao().songOnce(1, "s1")!!.artistSort)
}
@Test
fun albumArtistSortFallsBackToAPluralRefWhenTheSingularArtistIsMissing() =
runTest {
insertSource()
val source =
FakeMusicSource(
artists = listOf(artist("a1", "A").copy(sortName = "Alpha")),
albums =
listOf(
album("al1").copy(
artistId = "missing",
albumArtist = "A",
artists = listOf(ArtistRef("a1", "A")),
),
),
)
SyncService(db, source).sync()
assertEquals("Alpha", db.libraryDao().albumOnce(1, "al1")!!.albumArtistSort)
}
@Test
fun changingAnArtistSortNameUpdatesAlbumAndSongSortKeys() =
runTest {
insertSource()
val source =
FakeMusicSource(
artists = listOf(artist("a1", "A").copy(sortName = "Alpha")),
albums = listOf(album("al1")),
songs = listOf(song("s1")),
)
SyncService(db, source).sync()
assertEquals("Alpha", db.libraryDao().albumOnce(1, "al1")!!.albumArtistSort)
assertEquals("Alpha", db.libraryDao().songOnce(1, "s1")!!.artistSort)
source.artists = listOf(artist("a1", "A").copy(sortName = "Zulu"))
SyncService(db, source).sync()
assertEquals("Zulu", db.libraryDao().albumOnce(1, "al1")!!.albumArtistSort)
assertEquals("Zulu", db.libraryDao().songOnce(1, "s1")!!.artistSort)
}
@Test @Test
fun everyAlbumArtistAndTrackArtistReachesTheSharedAlbum() = fun everyAlbumArtistAndTrackArtistReachesTheSharedAlbum() =
runTest { runTest {