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.
2026-11-10 15:11:58 +01:00 · 2026-10-19 01:52:12 +00:00
parent ce33a2b6fd
commit 0ac67a124d
4 changed files with 75 additions and 23 deletions
@@ -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 " +
@@ -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<List<String>>

-    @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,
@@ -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 =
@@ -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<Context>()
+            val executions = Collections.synchronizedList(mutableListOf<SqlExecution>())
+            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 ->