From 891406eab9fbe7cf95201c2dfe1a1928d144940f Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sun, 18 Oct 2026 04:14:29 +0000 Subject: [PATCH] library: unify artist credit totals and backfill song counts Use songCount as the total for both artist status queries and stop excluding albumless tracks from download attribution, so the Artists badge and the artist page agree. Backfill song counts in the 29->30 migration for offline upgrades, and recompute only when a song moves albums or its credits change rather than on every play-metadata sync. --- .../com/subtracks/data/db/DownloadDao.kt | 25 ++++--- .../com/subtracks/data/db/LibraryDao.kt | 2 +- .../com/subtracks/data/db/Migrations.kt | 24 +++++++ .../com/subtracks/data/sync/SyncService.kt | 52 ++++++++++++--- .../com/subtracks/data/db/MigrationsTest.kt | 24 +++++++ .../data/repo/DownloadRepositoryTest.kt | 12 +++- .../subtracks/data/sync/SyncServiceTest.kt | 65 +++++++++++++++++++ 7 files changed, 179 insertions(+), 25 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt b/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt index 91d4d3c9..ebc30c9a 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt @@ -15,32 +15,26 @@ import kotlinx.coroutines.flow.Flow // downloads are visible even though they lead no albums of their own. // Starts from songs so status totals include every song the artist is credited on. internal const val ARTIST_DOWNLOAD_ROWS = - "SELECT albums.artistId AS id, songs.id AS songId, song_downloads.status AS status, " + - "song_downloads.downloadedAt AS downloadedAt " + + "SELECT albums.artistId AS id, songs.id AS songId, song_downloads.status AS status " + "FROM songs " + "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "LEFT JOIN song_downloads ON song_downloads.sourceId = songs.sourceId AND song_downloads.songId = songs.id " + "WHERE songs.sourceId = :sourceId AND albums.artistId IS NOT NULL " + "UNION ALL " + - "SELECT album_artists.artistId AS id, songs.id AS songId, song_downloads.status AS status, " + - "song_downloads.downloadedAt AS downloadedAt " + + "SELECT album_artists.artistId AS id, songs.id AS songId, song_downloads.status AS status " + "FROM songs " + "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "JOIN album_artists ON album_artists.sourceId = albums.sourceId AND album_artists.albumId = albums.id " + "LEFT JOIN song_downloads ON song_downloads.sourceId = songs.sourceId AND song_downloads.songId = songs.id " + "WHERE songs.sourceId = :sourceId " + "UNION ALL " + - "SELECT songs.artistId AS id, songs.id AS songId, song_downloads.status AS status, " + - "song_downloads.downloadedAt AS downloadedAt " + + "SELECT songs.artistId AS id, songs.id AS songId, song_downloads.status AS status " + "FROM songs " + - "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "LEFT JOIN song_downloads ON song_downloads.sourceId = songs.sourceId AND song_downloads.songId = songs.id " + "WHERE songs.sourceId = :sourceId AND songs.artistId IS NOT NULL " + "UNION ALL " + - "SELECT song_artists.artistId AS id, songs.id AS songId, song_downloads.status AS status, " + - "song_downloads.downloadedAt AS downloadedAt " + + "SELECT song_artists.artistId AS id, songs.id AS songId, song_downloads.status AS status " + "FROM songs " + - "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "JOIN song_artists ON song_artists.sourceId = songs.sourceId AND song_artists.songId = songs.id " + "LEFT JOIN song_downloads ON song_downloads.sourceId = songs.sourceId AND song_downloads.songId = songs.id " + "WHERE songs.sourceId = :sourceId" @@ -66,14 +60,12 @@ internal const val DOWNLOADED_ARTIST_ROWS = "song_downloads.downloadedAt AS downloadedAt, song_downloads.status AS status " + "FROM song_downloads " + "JOIN songs ON songs.sourceId = song_downloads.sourceId AND songs.id = song_downloads.songId " + - "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "WHERE song_downloads.sourceId = :sourceId AND songs.artistId IS NOT NULL " + "UNION ALL " + "SELECT song_artists.artistId AS id, song_downloads.songId AS songId, " + "song_downloads.downloadedAt AS downloadedAt, song_downloads.status AS status " + "FROM song_downloads " + "JOIN songs ON songs.sourceId = song_downloads.sourceId AND songs.id = song_downloads.songId " + - "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "JOIN song_artists ON song_artists.sourceId = songs.sourceId AND song_artists.songId = songs.id " + "WHERE song_downloads.sourceId = :sourceId" @@ -89,10 +81,15 @@ interface DownloadDao { suspend fun all(): List @Query( - "SELECT COUNT(DISTINCT songId) AS total, " + + "SELECT COALESCE((SELECT songCount FROM artists " + + "WHERE sourceId = :sourceId AND id = :artistId), 0) AS total, " + + "COALESCE(statuses.downloaded, 0) AS downloaded, " + + "COALESCE(statuses.downloading, 0) AS downloading " + + "FROM (SELECT " + "COUNT(DISTINCT CASE WHEN status = 'Completed' THEN songId END) AS downloaded, " + "COUNT(DISTINCT CASE WHEN status IN ('Queued', 'Running') THEN songId END) AS downloading " + - "FROM (" + ARTIST_DOWNLOAD_ROWS + ") WHERE id = :artistId", + "FROM (" + DOWNLOADED_ARTIST_ROWS + ") " + + "WHERE id = :artistId AND status IN ('Completed', 'Queued', 'Running')) statuses", ) fun artistStatus( sourceId: Long, diff --git a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt index b7470dba..76833f0c 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt @@ -108,7 +108,7 @@ internal const val ARTISTS_SELECT = "CASE WHEN :downloadedFilter = 1 THEN (" + "SELECT COUNT(*) FROM ($DOWNLOADED_ARTIST_ALBUMS)" + ") ELSE artists.albumCount END AS albumCount, " + - "artists.starred, artists.coverArt, artists.playCount, artists.played " + "artists.starred, artists.coverArt, artists.playCount, artists.played, artists.songCount " internal const val ARTIST_ORDER_BY_NAME = "name COLLATE NOCASE, id" internal const val ARTIST_ORDER_BY_NAME_REVERSED = "name COLLATE NOCASE DESC, id DESC" diff --git a/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt b/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt index 5fce0e3a..a27b14ac 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt @@ -426,6 +426,30 @@ val MIGRATION_29_30 = object : Migration(29, 30) { override suspend fun migrate(connection: SQLiteConnection) { connection.execSQL("ALTER TABLE `artists` ADD COLUMN `songCount` INTEGER NOT NULL DEFAULT 0") + connection.execSQL( + "WITH credited AS (" + + "SELECT sourceId, artistId, songId FROM song_artists " + + "UNION " + + "SELECT sourceId, artistId, id AS songId FROM songs WHERE artistId IS NOT NULL " + + "UNION " + + "SELECT album_artists.sourceId, album_artists.artistId, songs.id " + + "FROM album_artists JOIN songs " + + "ON songs.sourceId = album_artists.sourceId AND songs.albumId = album_artists.albumId " + + "UNION " + + "SELECT albums.sourceId, albums.artistId, songs.id " + + "FROM albums JOIN songs ON songs.sourceId = albums.sourceId AND songs.albumId = albums.id " + + "WHERE albums.artistId IS NOT NULL" + + "), counts AS (" + + "SELECT sourceId, artistId AS id, COUNT(*) AS songCount FROM credited GROUP BY sourceId, artistId" + + ") " + + "UPDATE artists SET songCount = c.songCount " + + "FROM (SELECT artists.sourceId AS sourceId, artists.id AS id, " + + "COALESCE(counts.songCount, 0) AS songCount " + + "FROM artists LEFT JOIN counts " + + "ON counts.sourceId = artists.sourceId AND counts.id = artists.id) c " + + "WHERE artists.sourceId = c.sourceId AND artists.id = c.id " + + "AND artists.songCount != c.songCount", + ) } } diff --git a/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt b/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt index 79b6042f..b5142c4c 100644 --- a/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt +++ b/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt @@ -31,14 +31,15 @@ class SyncService( private var playlistSongsSynced = 0 private var pruned = 0 + private var creditsChanged = false + suspend fun sync(): SyncSummary { + creditsChanged = false syncArtists() - val libraryBefore = totalChanges() syncAlbums() syncSongs() - val libraryChanged = totalChanges() != libraryBefore optimize() - if (libraryChanged) recomputeArtistSongCounts() + if (creditsChanged) recomputeArtistSongCounts() aggregatePlayData() syncPlaylists() syncPlaylistSongs() @@ -79,7 +80,9 @@ class SyncService( discs.forEach { seenDiscs.add(discHash(it.albumId, it.disc)) } albumsSynced += batch.size } + val prunedBefore = pruned pruned += pruneStaleIds(seenAlbums, { library.albumIdsAfter(source.id, it, PRUNE_PAGE) }, { library.deleteAlbums(source.id, it) }) + if (pruned > prunedBefore) creditsChanged = true pruned += pruneStaleDiscs(seenDiscs, { albumId, disc -> library.discKeysAfter(source.id, albumId, disc, PRUNE_PAGE) }) { deleteStaleDiscs(it) @@ -95,7 +98,9 @@ class SyncService( batch.forEach { seen.add(idHash(it.id)) } songsSynced += batch.size } + val prunedBefore = pruned pruned += pruneStaleIds(seen, { library.songIdsAfter(source.id, it, PRUNE_PAGE) }, { library.deleteSongs(source.id, it) }) + if (pruned > prunedBefore) creditsChanged = true } private suspend fun writeSongs(batch: List) { @@ -110,6 +115,7 @@ class SyncService( } db.useWriterConnection { connection -> connection.immediateTransaction { + if (connection.albumIdsChanged(batch)) creditsChanged = true connection.upsertChanged("songs", songColumns, primaryKey, batch) { it.values() } connection.usePrepared("DELETE FROM song_genres WHERE sourceId = ? AND songId = ? AND position >= ?") { statement -> batch.forEach { song -> @@ -122,6 +128,7 @@ class SyncService( } } connection.upsertChanged("song_genres", songGenreColumns, songGenreKey, genres) { it.values() } + val artistRowsBefore = connection.totalChanges() connection.usePrepared("DELETE FROM song_artists WHERE sourceId = ? AND songId = ? AND position >= ?") { statement -> batch.forEach { song -> statement.bind(1, song.sourceId) @@ -133,6 +140,7 @@ class SyncService( } } connection.upsertChanged("song_artists", songArtistColumns, songArtistKey, artists) { it.values() } + if (connection.totalChanges() != artistRowsBefore) creditsChanged = true } } } @@ -145,13 +153,36 @@ class SyncService( db.libraryDao().recomputeArtistSongCounts(source.id) } - private suspend fun totalChanges(): Long = - db.useWriterConnection { connection -> - connection.usePrepared("SELECT total_changes()") { statement -> - statement.step() - statement.getLong(0) - } + private suspend fun PooledConnection.totalChanges(): Long { + var value = 0L + usePrepared("SELECT total_changes()") { statement -> + statement.step() + value = statement.getLong(0) } + return value + } + + private suspend fun PooledConnection.albumIdsChanged(batch: List): Boolean { + val expected = batch.associate { it.id to it.albumId }.toMutableMap() + if (expected.isEmpty()) return false + var changed = false + batch + .map { it.sourceId to it.id } + .chunked(SQL_VARIABLES) + .forEach { chunk -> + val placeholders = chunk.joinToString(",") { "?" } + usePrepared("SELECT id, albumId FROM songs WHERE sourceId = ? AND id IN ($placeholders)") { statement -> + statement.bindLong(1, chunk.first().first) + chunk.forEachIndexed { index, (_, id) -> statement.bindText(index + 2, id) } + while (statement.step()) { + val id = statement.getText(0) + val albumId = if (statement.isNull(1)) null else statement.getText(1) + if (expected.remove(id) != albumId) changed = true + } + } + } + return changed || expected.isNotEmpty() + } // Without sqlite_stat1 the artist and relation queries can fall back to whole-table scans. private suspend fun optimize() { @@ -213,6 +244,7 @@ class SyncService( connection.immediateTransaction { connection.upsertChanged("albums", albumColumns, primaryKey, albums) { it.values() } connection.upsertChanged("discs", discColumns, discKey, discs) { it.values() } + val artistRowsBefore = connection.totalChanges() connection.usePrepared("DELETE FROM album_artists WHERE sourceId = ? AND albumId = ? AND position >= ?") { statement -> albums.forEach { album -> statement.bind(1, album.sourceId) @@ -224,6 +256,7 @@ class SyncService( } } connection.upsertChanged("album_artists", albumArtistColumns, albumArtistKey, artists) { it.values() } + if (connection.totalChanges() != artistRowsBefore) creditsChanged = true } } } @@ -298,6 +331,7 @@ data class SyncSummary( ) private const val DELETE_CHUNK = 500 +private const val SQL_VARIABLES = 500 private const val PRUNE_PAGE = 500 private const val INITIAL_HASH_CAPACITY = 256 private const val FNV_OFFSET = -3750763034362895579L diff --git a/app/src/test/kotlin/com/subtracks/data/db/MigrationsTest.kt b/app/src/test/kotlin/com/subtracks/data/db/MigrationsTest.kt index e261154a..d05a220a 100644 --- a/app/src/test/kotlin/com/subtracks/data/db/MigrationsTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/db/MigrationsTest.kt @@ -153,6 +153,30 @@ class MigrationsTest { } } + @Test + fun theArtistSongCountBackfillCountsLegacyCredits() = + runTest { + driver.open(":memory:").use { migrated -> + applySchema(migrated, 29) + migrated.execSQL( + "INSERT INTO sources (id, name, address, isActive, createdAt) VALUES (1, 's', 'http://x', 1, 0)", + ) + migrated.execSQL("INSERT INTO artists (sourceId, id, name, albumCount) VALUES (1, 'ar1', 'Album Artist', 1)") + migrated.execSQL("INSERT INTO artists (sourceId, id, name, albumCount) VALUES (1, 'ar2', 'Guest', 0)") + migrated.execSQL( + "INSERT INTO albums (sourceId, id, artistId, name, albumArtist, created, songCount) " + + "VALUES (1, 'al1', 'ar1', 'Album', 'Album Artist', 0, 1)", + ) + migrated.execSQL("INSERT INTO songs (sourceId, id, albumId, artistId, title) VALUES (1, 's1', 'al1', 'ar1', 'One')") + migrated.execSQL("INSERT INTO songs (sourceId, id, albumId, artistId, title) VALUES (1, 's2', NULL, 'ar2', 'Two')") + + MIGRATION_29_30.migrate(migrated) + + assertEquals(1L, long(migrated, "SELECT songCount FROM artists WHERE id = 'ar1'")) + assertEquals(1L, long(migrated, "SELECT songCount FROM artists WHERE id = 'ar2'")) + } + } + private fun text( connection: SQLiteConnection, sql: String, diff --git a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt index 4d9a538e..2b6ee0bd 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt @@ -647,7 +647,17 @@ class DownloadRepositoryTest { ), ) db.libraryDao().upsertArtists( - listOf(Artist(sourceId = 1, id = "ar-guest", name = "Guest", albumCount = 0, starred = null, coverArt = null)), + listOf( + Artist( + sourceId = 1, + id = "ar-guest", + name = "Guest", + albumCount = 0, + starred = null, + coverArt = null, + songCount = 1, + ), + ), ) } diff --git a/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt b/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt index 5babb5f8..06879f96 100644 --- a/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt @@ -999,6 +999,71 @@ class SyncServiceTest { assertEquals(0L, db.libraryDao().artistOnce(1, "a2")!!.songCount) } + @Test + fun artistSongCountsAndDownloadsIncludeAlbumlessTracks() = + runTest { + insertSource() + val source = + FakeMusicSource( + artists = listOf(artist("a1", "A")), + songs = + listOf( + song("s1").copy(albumId = null), + song("s2").copy(albumId = null), + ), + ) + + SyncService(db, source).sync() + + assertEquals(2L, db.libraryDao().artistOnce(1, "a1")!!.songCount) + db.downloadDao().upsert(SongDownload(1, "s1", DownloadStatus.Completed)) + db.downloadDao().upsert(SongDownload(1, "s2", DownloadStatus.Completed)) + + assertEquals( + ListDownloadStatus(total = 2, downloaded = 2), + db.downloadDao().artistStatus(1, "a1").first(), + ) + assertEquals( + 2L, + db + .downloadDao() + .artistStatuses(1) + .first() + .single { it.id == "a1" } + .downloaded, + ) + } + + @Test + fun movingASongBetweenAlbumsUpdatesAlbumArtistCounts() = + runTest { + insertSource() + val source = + FakeMusicSource( + artists = listOf(artist("a1", "A"), artist("a2", "B"), artist("a3", "Track")), + albums = + listOf( + album("al1").copy(artistId = "a1"), + album("al2").copy(artistId = "a2"), + ), + songs = listOf(song("s1").copy(artistId = "a3", albumId = "al1")), + ) + + SyncService(db, source).sync() + + assertEquals(1L, db.libraryDao().artistOnce(1, "a1")!!.songCount) + assertEquals(0L, db.libraryDao().artistOnce(1, "a2")!!.songCount) + assertEquals(1L, db.libraryDao().artistOnce(1, "a3")!!.songCount) + + source.songs = listOf(song("s1").copy(artistId = "a3", albumId = "al2")) + + SyncService(db, source).sync() + + assertEquals(0L, db.libraryDao().artistOnce(1, "a1")!!.songCount) + assertEquals(1L, db.libraryDao().artistOnce(1, "a2")!!.songCount) + assertEquals(1L, db.libraryDao().artistOnce(1, "a3")!!.songCount) + } + @Test fun anIdenticalSyncDoesNotRewriteArtistRows() = runTest {