From 65d74faa1d0f1ad7eea7135a207165cc5d7b7083 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sun, 18 Oct 2026 14:40:16 +0000 Subject: [PATCH] library: count credited albums from album credits too The artist badge now counts albums from album lead ids and album_artists even when an album has no songs, and only counts song-credited albums that still exist, matching what albumsForArtist lists. Preserve the derived albumCount when the server re-upserts an artist row so an identical sync does not rewrite it twice, relax the migration guard to either relation table, and share one album-count order between the plain and downloaded artist lists. --- .../com/subtracks/data/db/LibraryDao.kt | 47 ++++++++---- .../com/subtracks/data/db/Migrations.kt | 45 ++++++++--- .../com/subtracks/data/sync/SyncService.kt | 8 +- .../com/subtracks/data/db/MigrationsTest.kt | 29 ++++++- .../subtracks/data/sync/SyncServiceTest.kt | 76 ++++++++++++++----- 5 files changed, 158 insertions(+), 47 deletions(-) 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 e0933779..4023944e 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt @@ -116,6 +116,25 @@ private const val CREDITED_SONG_ROWS = "ON songs.sourceId = album_artists.sourceId AND songs.albumId = album_artists.albumId " + "WHERE album_artists.sourceId = :sourceId" +private const val CREDITED_ALBUM_ROWS = + "SELECT albums.artistId AS artistId, albums.id AS albumId " + + "FROM albums WHERE albums.sourceId = :sourceId AND albums.artistId IS NOT NULL " + + "UNION " + + "SELECT album_artists.artistId, albums.id " + + "FROM album_artists JOIN albums " + + "ON albums.sourceId = album_artists.sourceId AND albums.id = album_artists.albumId " + + "WHERE album_artists.sourceId = :sourceId " + + "UNION " + + "SELECT songs.artistId, songs.albumId " + + "FROM songs JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + + "WHERE songs.sourceId = :sourceId AND songs.artistId IS NOT NULL " + + "UNION " + + "SELECT song_artists.artistId, songs.albumId " + + "FROM song_artists JOIN songs " + + "ON songs.sourceId = song_artists.sourceId AND songs.id = song_artists.songId " + + "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + + "WHERE song_artists.sourceId = :sourceId" + internal const val ARTISTS_FILTER = "FROM artists WHERE sourceId = :sourceId " + "AND (:starredFilter = 0 OR (:starredFilter = 1 AND starred IS NOT NULL) OR (:starredFilter = 2 AND starred IS NULL)) " + @@ -141,13 +160,8 @@ internal const val ARTISTS_SELECT = internal const val ARTIST_ORDER_BY_NAME = "sortName COLLATE NOCASE, id" internal const val ARTIST_ORDER_BY_NAME_REVERSED = "sortName COLLATE NOCASE DESC, id DESC" -internal const val ARTIST_ORDER_BY_ALBUM_COUNT = "artists.albumCount DESC, sortName COLLATE NOCASE, id" -internal const val ARTIST_ORDER_BY_ALBUM_COUNT_REVERSED = "artists.albumCount ASC, sortName COLLATE NOCASE DESC, id DESC" - -// These order on the projected alias so the downloaded list sorts by the number it shows. -internal const val ARTIST_ORDER_BY_DOWNLOADED_ALBUM_COUNT = "albumCount DESC, sortName COLLATE NOCASE, id" -internal const val ARTIST_ORDER_BY_DOWNLOADED_ALBUM_COUNT_REVERSED = - "albumCount ASC, sortName COLLATE NOCASE DESC, id DESC" +internal const val ARTIST_ORDER_BY_ALBUM_COUNT = "albumCount DESC, sortName COLLATE NOCASE, id" +internal const val ARTIST_ORDER_BY_ALBUM_COUNT_REVERSED = "albumCount ASC, sortName COLLATE NOCASE DESC, id DESC" internal const val ARTIST_ORDER_BY_STARRED = "starred DESC NULLS LAST, sortName COLLATE NOCASE, id" internal const val ARTIST_ORDER_BY_STARRED_REVERSED = "starred ASC NULLS LAST, sortName COLLATE NOCASE DESC, id DESC" internal const val ARTIST_ORDER_BY_FREQUENT = "artists.playCount DESC, sortName COLLATE NOCASE, id" @@ -252,16 +266,20 @@ interface LibraryDao { suspend fun recomputeArtistPlayData(sourceId: Long) // Stored so the download badge query can report an artist's total without walking every song, - // and so the artist badges use the same credit set as the artist page and the shuffle queue. + // and so the artist badges use the same counts as the artist page and the shuffle queue. @Query( - "WITH credited AS ($CREDITED_SONG_ROWS), counts AS (" + - "SELECT artistId AS id, COUNT(DISTINCT songId) AS songCount, " + - "COUNT(DISTINCT albumId) AS albumCount " + + "WITH credited AS ($CREDITED_SONG_ROWS), " + + "credited_albums AS ($CREDITED_ALBUM_ROWS), " + + "counts AS (" + + "SELECT artistId AS id, COUNT(DISTINCT songId) AS songCount " + "FROM credited GROUP BY artistId" + + "), album_counts AS (" + + "SELECT artistId AS id, COUNT(DISTINCT albumId) AS albumCount " + + "FROM credited_albums GROUP BY artistId" + ") " + "UPDATE artists SET songCount = c.songCount, albumCount = c.albumCount, credited = c.credited " + "FROM (SELECT artists.id AS id, COALESCE(counts.songCount, 0) AS songCount, " + - "COALESCE(counts.albumCount, 0) AS albumCount, " + + "COALESCE(album_counts.albumCount, 0) AS albumCount, " + "((NOT EXISTS (SELECT 1 FROM album_artists WHERE sourceId = artists.sourceId) " + "AND NOT EXISTS (SELECT 1 FROM song_artists WHERE sourceId = artists.sourceId)) " + "OR EXISTS (SELECT 1 FROM album_artists aa " + @@ -269,6 +287,7 @@ interface LibraryDao { "OR EXISTS (SELECT 1 FROM song_artists sa " + "WHERE sa.sourceId = artists.sourceId AND sa.artistId = artists.id)) AS credited " + "FROM artists LEFT JOIN counts ON counts.id = artists.id " + + "LEFT JOIN album_counts ON album_counts.id = artists.id " + "WHERE artists.sourceId = :sourceId) c " + "WHERE artists.sourceId = :sourceId AND artists.id = c.id " + "AND (artists.songCount != c.songCount OR artists.albumCount != c.albumCount " + @@ -718,7 +737,7 @@ interface LibraryDao { downloadedFilter: Int = 0, ): PagingSource - @Query("$ARTISTS_SELECT $ARTISTS_FILTER ORDER BY $ARTIST_ORDER_BY_DOWNLOADED_ALBUM_COUNT") + @Query("$ARTISTS_SELECT $ARTISTS_FILTER ORDER BY $ARTIST_ORDER_BY_ALBUM_COUNT") fun artistsByAlbumCount( sourceId: Long, starredFilter: Int, @@ -726,7 +745,7 @@ interface LibraryDao { downloadedFilter: Int = 0, ): PagingSource - @Query("$ARTISTS_SELECT $ARTISTS_FILTER ORDER BY $ARTIST_ORDER_BY_DOWNLOADED_ALBUM_COUNT_REVERSED") + @Query("$ARTISTS_SELECT $ARTISTS_FILTER ORDER BY $ARTIST_ORDER_BY_ALBUM_COUNT_REVERSED") fun artistsByAlbumCountReversed( sourceId: Long, starredFilter: Int, 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 177dc6e3..872fc98a 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt @@ -579,47 +579,70 @@ val MIGRATION_32_33 = } } -// Recomputes the artist totals from the same credit union the sync uses, so badges, the artist +// Recomputes the artist totals from the same credit unions the sync uses, so badges, the artist // page and the shuffle queue agree for libraries that predate the change. val MIGRATION_33_34 = object : Migration(33, 34) { override suspend fun migrate(connection: SQLiteConnection) { connection.execSQL( - "WITH credited AS (" + + "WITH credited_songs AS (" + "SELECT songs.`sourceId` AS sourceId, songs.`artistId` AS artistId, " + - "songs.`id` AS songId, songs.`albumId` AS albumId, " + - "songs.`playCount` AS playCount, songs.`played` AS played " + + "songs.`id` AS songId, songs.`playCount` AS playCount, songs.`played` AS played " + "FROM songs WHERE songs.`artistId` IS NOT NULL " + "UNION " + - "SELECT song_artists.`sourceId`, song_artists.`artistId`, songs.`id`, songs.`albumId`, " + + "SELECT song_artists.`sourceId`, song_artists.`artistId`, songs.`id`, " + "songs.`playCount`, songs.`played` " + "FROM song_artists JOIN songs " + "ON songs.`sourceId` = song_artists.`sourceId` AND songs.`id` = song_artists.`songId` " + "UNION " + - "SELECT albums.`sourceId`, albums.`artistId`, songs.`id`, songs.`albumId`, " + + "SELECT albums.`sourceId`, albums.`artistId`, songs.`id`, " + "songs.`playCount`, songs.`played` " + "FROM albums JOIN songs " + "ON songs.`sourceId` = albums.`sourceId` AND songs.`albumId` = albums.`id` " + "WHERE albums.`artistId` IS NOT NULL " + "UNION " + - "SELECT album_artists.`sourceId`, album_artists.`artistId`, songs.`id`, songs.`albumId`, " + + "SELECT album_artists.`sourceId`, album_artists.`artistId`, songs.`id`, " + "songs.`playCount`, songs.`played` " + "FROM album_artists JOIN songs " + "ON songs.`sourceId` = album_artists.`sourceId` AND songs.`albumId` = album_artists.`albumId`" + + "), credited_albums AS (" + + "SELECT albums.`sourceId` AS sourceId, albums.`artistId` AS artistId, " + + "albums.`id` AS albumId " + + "FROM albums WHERE albums.`artistId` IS NOT NULL " + + "UNION " + + "SELECT album_artists.`sourceId`, album_artists.`artistId`, albums.`id` " + + "FROM album_artists JOIN albums " + + "ON albums.`sourceId` = album_artists.`sourceId` AND albums.`id` = album_artists.`albumId` " + + "UNION " + + "SELECT songs.`sourceId`, songs.`artistId`, songs.`albumId` " + + "FROM songs JOIN albums " + + "ON albums.`sourceId` = songs.`sourceId` AND albums.`id` = songs.`albumId` " + + "WHERE songs.`artistId` IS NOT NULL " + + "UNION " + + "SELECT song_artists.`sourceId`, song_artists.`artistId`, songs.`albumId` " + + "FROM song_artists JOIN songs " + + "ON songs.`sourceId` = song_artists.`sourceId` AND songs.`id` = song_artists.`songId` " + + "JOIN albums ON albums.`sourceId` = songs.`sourceId` AND albums.`id` = songs.`albumId`" + "), totals AS (" + "SELECT sourceId, artistId AS id, COUNT(DISTINCT songId) AS songCount, " + - "COUNT(DISTINCT albumId) AS albumCount, SUM(playCount) AS playCount, MAX(played) AS played " + - "FROM credited GROUP BY sourceId, artistId" + + "SUM(playCount) AS playCount, MAX(played) AS played " + + "FROM credited_songs GROUP BY sourceId, artistId" + + "), album_totals AS (" + + "SELECT sourceId, artistId AS id, COUNT(DISTINCT albumId) AS albumCount " + + "FROM credited_albums GROUP BY sourceId, artistId" + ") " + "UPDATE artists SET songCount = c.songCount, albumCount = c.albumCount, " + "playCount = c.playCount, played = c.played " + "FROM (SELECT artists.`sourceId` AS sourceId, artists.`id` AS id, " + - "COALESCE(totals.songCount, 0) AS songCount, COALESCE(totals.albumCount, 0) AS albumCount, " + + "COALESCE(totals.songCount, 0) AS songCount, " + + "COALESCE(album_totals.albumCount, 0) AS albumCount, " + "COALESCE(totals.playCount, 0) AS playCount, totals.played AS played " + "FROM artists LEFT JOIN totals " + "ON totals.sourceId = artists.`sourceId` AND totals.id = artists.`id` " + + "LEFT JOIN album_totals " + + "ON album_totals.sourceId = artists.`sourceId` AND album_totals.id = artists.`id` " + "WHERE EXISTS (SELECT 1 FROM album_artists WHERE album_artists.`sourceId` = artists.`sourceId`) " + - "AND EXISTS (SELECT 1 FROM song_artists WHERE song_artists.`sourceId` = artists.`sourceId`)" + + "OR EXISTS (SELECT 1 FROM song_artists WHERE song_artists.`sourceId` = artists.`sourceId`)" + ") c " + "WHERE artists.`sourceId` = c.sourceId AND artists.`id` = c.id " + "AND (artists.`songCount` != c.songCount OR artists.`albumCount` != c.albumCount " + 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 fc253531..9fd34505 100644 --- a/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt +++ b/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt @@ -65,7 +65,7 @@ class SyncService( val seen = HashedIds() source.artists().collect { batch -> if (batch.isEmpty()) return@collect - write("artists", artistColumns, primaryKey, batch) { it.values() } + write("artists", artistColumns, primaryKey, batch, preserved = setOf("albumCount")) { it.values() } batch.forEach { seen.add(idHash(it.id)) } artistsSynced += batch.size } @@ -314,11 +314,12 @@ class SyncService( columns: List, key: List, rows: List, + preserved: Set = emptySet(), values: (T) -> List, ) { db.useWriterConnection { connection -> connection.immediateTransaction { - connection.upsertChanged(table, columns, key, rows, values) + connection.upsertChanged(table, columns, key, rows, preserved, values) } } } @@ -613,11 +614,12 @@ private suspend fun PooledConnection.upsertChanged( columns: List, key: List, rows: List, + preserved: Set = emptySet(), values: (T) -> List, ) { if (rows.isEmpty()) return check(values(rows.first()).size == columns.size) { "column/value mismatch for $table" } - val updates = columns.filterNot { it in key } + val updates = columns.filterNot { it in key || it in preserved } val sql = "INSERT INTO $table (${columns.joinToString()}) VALUES (${columns.joinToString { "?" }}) " + "ON CONFLICT(${key.joinToString()}) DO UPDATE SET " + 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 b7ce63c0..8b69dab2 100644 --- a/app/src/test/kotlin/com/subtracks/data/db/MigrationsTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/db/MigrationsTest.kt @@ -283,6 +283,9 @@ class MigrationsTest { migrated.execSQL( "INSERT INTO sources (id, name, address, isActive, createdAt) VALUES (2, 't', 'http://y', 1, 0)", ) + migrated.execSQL( + "INSERT INTO sources (id, name, address, isActive, createdAt) VALUES (3, 'u', 'http://z', 1, 0)", + ) migrated.execSQL( "INSERT INTO artists (sourceId, id, name, albumCount, songCount, playCount, played) " + "VALUES (1, 'a1', 'Album Artist', 1, 0, 0, NULL)", @@ -295,10 +298,18 @@ class MigrationsTest { "INSERT INTO artists (sourceId, id, name, albumCount, songCount, playCount, played) " + "VALUES (2, 'b1', 'Unsynced', 7, 6, 5, 100)", ) + migrated.execSQL( + "INSERT INTO artists (sourceId, id, name, albumCount, songCount, playCount, played) " + + "VALUES (3, 'c1', 'Track Only', 7, 6, 5, 100)", + ) migrated.execSQL( "INSERT INTO albums (sourceId, id, artistId, name, albumArtist, created, songCount) " + "VALUES (1, 'al1', 'a1', 'Album', 'Album Artist', 0, 1)", ) + migrated.execSQL( + "INSERT INTO albums (sourceId, id, artistId, name, albumArtist, created, songCount) " + + "VALUES (1, 'al2', 'a2', 'Empty Album', 'Guest', 0, 0)", + ) migrated.execSQL( "INSERT INTO album_artists (sourceId, albumId, position, artistId, name) " + "VALUES (1, 'al1', 0, 'a1', 'Album Artist')", @@ -315,6 +326,18 @@ class MigrationsTest { "INSERT INTO song_artists (sourceId, songId, position, artistId, name) " + "VALUES (1, 's1', 1, 'a2', 'Guest')", ) + migrated.execSQL( + "INSERT INTO songs (sourceId, id, albumId, artistId, title, playCount, played) " + + "VALUES (3, 's3', NULL, 'c1', 'Three', 2, 50)", + ) + migrated.execSQL( + "INSERT INTO song_artists (sourceId, songId, position, artistId, name) " + + "VALUES (3, 's3', 0, 'c1', 'Track Only')", + ) + migrated.execSQL( + "INSERT INTO songs (sourceId, id, albumId, artistId, title, playCount, played) " + + "VALUES (3, 's4', 'missing', 'c1', 'Four', 3, 60)", + ) MIGRATION_33_34.migrate(migrated) @@ -323,13 +346,17 @@ class MigrationsTest { assertEquals(4L, long(migrated, "SELECT playCount FROM artists WHERE id = 'a1'")) assertEquals(200L, long(migrated, "SELECT played FROM artists WHERE id = 'a1'")) assertEquals(1L, long(migrated, "SELECT songCount FROM artists WHERE id = 'a2'")) - assertEquals(1L, long(migrated, "SELECT albumCount FROM artists WHERE id = 'a2'")) + assertEquals(2L, long(migrated, "SELECT albumCount FROM artists WHERE id = 'a2'")) assertEquals(4L, long(migrated, "SELECT playCount FROM artists WHERE id = 'a2'")) assertEquals(200L, long(migrated, "SELECT played FROM artists WHERE id = 'a2'")) assertEquals(6L, long(migrated, "SELECT songCount FROM artists WHERE id = 'b1'")) assertEquals(7L, long(migrated, "SELECT albumCount FROM artists WHERE id = 'b1'")) assertEquals(5L, long(migrated, "SELECT playCount FROM artists WHERE id = 'b1'")) assertEquals(100L, long(migrated, "SELECT played FROM artists WHERE id = 'b1'")) + assertEquals(2L, long(migrated, "SELECT songCount FROM artists WHERE id = 'c1'")) + assertEquals(0L, long(migrated, "SELECT albumCount FROM artists WHERE id = 'c1'")) + assertEquals(5L, long(migrated, "SELECT playCount FROM artists WHERE id = 'c1'")) + assertEquals(60L, long(migrated, "SELECT played FROM artists WHERE id = 'c1'")) } } 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 5dd16688..35496dbe 100644 --- a/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt @@ -1373,6 +1373,11 @@ class SyncServiceTest { insertSource() val source = FakeMusicSource( + artists = + listOf( + artist("a1", "Artist").copy(albumCount = 9), + artist("a2", "Other").copy(albumCount = 9), + ), albums = listOf( album("al1").copy(artists = listOf(ArtistRef("a1", "Artist"), ArtistRef("a2", "Other"))), @@ -1478,6 +1483,10 @@ class SyncServiceTest { artistId = "a1", artists = listOf(ArtistRef("a1", "Album Artist")), ), + album("al2").copy( + artistId = "a2", + artists = listOf(ArtistRef("a2", "Guest")), + ), ), songs = listOf( @@ -1488,31 +1497,28 @@ class SyncServiceTest { playCount = 3, played = 100, ), + song("s2").copy( + albumId = "al1", + artistId = "a2", + artists = listOf(ArtistRef("a2", "Guest")), + playCount = 4, + played = 200, + ), ), ) SyncService(db, source).sync() val guest = db.libraryDao().artistOnce(1, "a2")!! - assertEquals(1L, guest.albumCount) - assertEquals(1L, guest.songCount) - assertEquals(3L, guest.playCount) - assertEquals(100L, guest.played) + assertEquals(2L, guest.albumCount) + assertEquals(2L, guest.songCount) + assertEquals(7L, guest.playCount) + assertEquals(200L, guest.played) + val albums = db.libraryDao().albumsForArtist(1, "a2").first() + assertEquals(listOf("al1", "al2"), albums.map { it.id }.sorted()) + assertEquals(albums.size.toLong(), guest.albumCount) assertEquals( - listOf("al1"), - db - .libraryDao() - .albumsForArtist(1, "a2") - .first() - .map { it.id }, - ) - assertEquals( - db - .libraryDao() - .albumsForArtist(1, "a2") - .first() - .size - .toLong(), + albums.size.toLong(), db .libraryDao() .artistsByName(1, 0, "") @@ -1520,6 +1526,40 @@ class SyncServiceTest { .first { it.id == "a2" } .albumCount, ) + assertEquals(guest.songCount, db.queueDao().artistLength(1, "a2")) + } + + @Test + fun songsOnMissingAlbumsAreSongsButNotAlbums() = + runTest { + insertSource() + val source = + FakeMusicSource( + artists = listOf(artist("a1", "Artist")), + albums = emptyList(), + songs = + listOf( + song("s1").copy( + albumId = "missing", + artistId = "a1", + artists = listOf(ArtistRef("a1", "Artist")), + ), + ), + ) + + SyncService(db, source).sync() + + val artist = db.libraryDao().artistOnce(1, "a1")!! + assertEquals(1L, artist.songCount) + assertEquals(0L, artist.albumCount) + assertEquals( + emptyList(), + db + .libraryDao() + .albumsForArtist(1, "a1") + .first() + .map { it.id }, + ) } @Test