From 0ac67a124dcb74a6f6397e075175382ab2928fc3 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Mon, 19 Oct 2026 01:52:12 +0000 Subject: [PATCH] library: tighten credit query coverage Share the artist page SQL with its plan test and assert the artist seeks, so the test fails if the predicate push-down regresses. Add a query-log test that an identical sync skips the aggregate, stop the downloaded album badge from counting a song whose album row is missing, and correct comments that overclaimed the remaining hand-written credit paths. --- .../com/subtracks/data/db/DownloadDao.kt | 2 +- .../com/subtracks/data/db/LibraryDao.kt | 21 ++++---- .../com/subtracks/data/db/LibraryDaoTest.kt | 24 ++++----- .../subtracks/data/sync/SyncServiceTest.kt | 51 +++++++++++++++++++ 4 files changed, 75 insertions(+), 23 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 906269ee..1ca7d61b 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt @@ -21,7 +21,7 @@ internal const val ARTIST_DOWNLOAD_ROWS = "AND song_downloads.songId = credits.songId" // Starts from downloads so lists and filters over partially downloaded libraries do not scan songs. -// This is the one hand-written copy of the credit paths, kept for that query plan. +// Kept hand-written for that plan. internal const val DOWNLOADED_ARTIST_ROWS = "SELECT albums.artistId AS id, song_downloads.songId AS songId, " + "song_downloads.downloadedAt AS downloadedAt, song_downloads.status AS status " + 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 e7b9f886..be480107 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt @@ -114,6 +114,11 @@ internal const val CREDITED_ALBUM_ROWS = // Download visibility follows the credit path: an album credit includes the album when any song is // downloaded, while a track credit includes it only when the credited track is downloaded. +internal const val ALBUMS_FOR_ARTIST = + "SELECT albums.* FROM (SELECT DISTINCT albumId FROM ($CREDITED_ALBUM_ROWS) WHERE artistId = :artistId) credits " + + "JOIN albums ON albums.sourceId = :sourceId AND albums.id = credits.albumId " + + "ORDER BY year DESC, sortName COLLATE NOCASE, id" + private const val DOWNLOADED_ARTIST_ALBUMS = "SELECT albums.id FROM albums " + "WHERE albums.sourceId = :sourceId AND albums.artistId = artists.id " + @@ -129,15 +134,17 @@ private const val DOWNLOADED_ARTIST_ALBUMS = "WHERE songs.sourceId = albums.sourceId AND songs.albumId = albums.id AND sd.status = 'Completed') " + "UNION " + "SELECT songs.albumId FROM songs " + + "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "JOIN song_downloads sd ON sd.sourceId = songs.sourceId AND sd.songId = songs.id " + "WHERE songs.sourceId = :sourceId AND songs.artistId = artists.id " + - "AND sd.status = 'Completed' AND songs.albumId IS NOT NULL " + + "AND sd.status = 'Completed' " + "UNION " + "SELECT 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 " + "JOIN song_downloads sd ON sd.sourceId = songs.sourceId AND sd.songId = songs.id " + "WHERE song_artists.sourceId = :sourceId AND song_artists.artistId = artists.id " + - "AND sd.status = 'Completed' AND songs.albumId IS NOT NULL" + "AND sd.status = 'Completed'" internal const val ARTISTS_FILTER = "FROM artists WHERE sourceId = :sourceId " + @@ -253,8 +260,8 @@ interface LibraryDao { ) suspend fun recomputeAlbumPlayData(sourceId: Long) - // Counts, plays and the credited flag come from the same credit unions, so the artist badges, - // page, shuffle and downloads cannot disagree. + // Counts, plays and the credited flag come from the shared credit unions. The download-aware + // queries are separate because their visibility follows the credit path. @Query( "WITH credited AS (SELECT DISTINCT * FROM ($CREDITED_SONG_ROWS)), " + "credited_albums AS ($CREDITED_ALBUM_ROWS), " + @@ -1052,11 +1059,7 @@ interface LibraryDao { songId: String, ): Flow> - @Query( - "SELECT albums.* FROM (SELECT DISTINCT albumId FROM ($CREDITED_ALBUM_ROWS) WHERE artistId = :artistId) credits " + - "JOIN albums ON albums.sourceId = :sourceId AND albums.id = credits.albumId " + - "ORDER BY year DESC, sortName COLLATE NOCASE, id", - ) + @Query(ALBUMS_FOR_ARTIST) fun albumsForArtist( sourceId: Long, artistId: String, diff --git a/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt b/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt index 55beba27..79db2354 100644 --- a/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt @@ -859,20 +859,18 @@ class LibraryDaoTest { @Test fun theArtistPageAlbumQueryFindsArtistCreditsThroughArtistIndexes() = runTest { - val plan = - plan( - "SELECT albums.* FROM (SELECT DISTINCT albumId FROM ($CREDITED_ALBUM_ROWS) " + - "WHERE artistId = :artistId) credits " + - "JOIN albums ON albums.sourceId = :sourceId AND albums.id = credits.albumId " + - "ORDER BY year DESC, sortName COLLATE NOCASE, id", - ) + val plan = plan(ALBUMS_FOR_ARTIST) - assertTrue(plan, plan.contains("index_albums_sourceId_artistId")) - assertTrue(plan, plan.contains("index_album_artists_sourceId_artistId")) - assertTrue(plan, plan.contains("index_songs_sourceId_artistId")) - assertTrue(plan, plan.contains("index_song_artists_sourceId_artistId")) - assertFalse(plan, plan.contains("SCAN albums")) - assertFalse(plan, plan.contains("SCAN songs")) + assertTrue(plan, plan.contains("USING INDEX index_albums_sourceId_artistId (sourceId=? AND artistId=?)")) + assertTrue( + plan, + plan.contains("USING INDEX index_album_artists_sourceId_artistId (sourceId=? AND artistId=?)"), + ) + assertTrue(plan, plan.contains("USING INDEX index_songs_sourceId_artistId (sourceId=? AND artistId=?)")) + assertTrue( + plan, + plan.contains("USING INDEX index_song_artists_sourceId_artistId (sourceId=? AND artistId=?)"), + ) } private suspend fun plan(sql: String): String = 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 349aad6d..79e18d78 100644 --- a/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt @@ -7,6 +7,8 @@ import androidx.room3.useWriterConnection import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.subtracks.data.db.LoggingSQLiteDriver +import com.subtracks.data.db.SqlExecution import com.subtracks.data.db.SubtracksDatabase import com.subtracks.data.db.artistsDefaultQuery import com.subtracks.data.model.Album @@ -29,9 +31,11 @@ import kotlinx.coroutines.flow.flow import kotlinx.coroutines.test.runTest import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import java.util.Collections @RunWith(AndroidJUnit4::class) class SyncServiceTest { @@ -1678,6 +1682,7 @@ class SyncServiceTest { SyncService(db, source).sync() db.downloadDao().upsert(SongDownload(1, "s1", DownloadStatus.Completed)) db.downloadDao().upsert(SongDownload(1, "s3", DownloadStatus.Completed)) + db.downloadDao().upsert(SongDownload(1, "s4", DownloadStatus.Completed)) for (artistId in listOf("a1", "a2", "a3")) { val artist = db.libraryDao().artistOnce(1, artistId)!! @@ -1696,6 +1701,20 @@ class SyncServiceTest { assertEquals("$artistId total", songIds.size.toLong(), status.total) assertEquals("$artistId downloaded", downloaded.toLong(), status.downloaded) } + + val downloadedArtists = db.libraryDao().artistsByName(1, 0, "", downloadedFilter = 1).allRows() + for (artist in downloadedArtists) { + assertEquals( + "${artist.id} downloaded albums", + db + .libraryDao() + .albumsForArtistDownloaded(1, artist.id) + .first() + .size + .toLong(), + artist.albumCount, + ) + } } @Test @@ -1745,6 +1764,38 @@ class SyncServiceTest { assertEquals(1L, db.libraryDao().artistOnce(1, "a2")!!.albumCount) } + @Test + fun anIdenticalSyncSkipsTheArtistAggregateQuery() = + runTest { + val context = ApplicationProvider.getApplicationContext() + val executions = Collections.synchronizedList(mutableListOf()) + val loggingDb = + Room + .inMemoryDatabaseBuilder(context, SubtracksDatabase::class.java) + .setDriver(LoggingSQLiteDriver(BundledSQLiteDriver()) { executions += it }) + .build() + try { + loggingDb.sourcesDao().upsertSource( + Source(id = 1, name = "test", address = "http://localhost", isActive = true, createdAt = 0), + ) + val source = + FakeMusicSource( + artists = listOf(artist("a1", "Artist")), + albums = listOf(album("al1")), + songs = listOf(song("s1")), + ) + + SyncService(loggingDb, source).sync() + synchronized(executions) { executions.clear() } + SyncService(loggingDb, source).sync() + + val secondSyncExecutions = synchronized(executions) { executions.toList() } + assertTrue(secondSyncExecutions.none { it.sql.contains("credited_albums") }) + } finally { + loggingDb.close() + } + } + private suspend fun totalChanges(): Long = db.useWriterConnection { connection -> connection.usePrepared("SELECT total_changes()") { statement ->