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 {