From 649528e3384ce85cb69943c287ce49c262e5d81b Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sun, 27 Sep 2026 07:24:55 +0000 Subject: [PATCH] address review: NULL disc/track in backward album seeks; drop redundant index - `albumSongsBefore` used the null-intolerant row-value `<`, so an album with NULL `disc`/`track` (untagged files) dropped exactly the rows a backward window needs, returning the wrong track at a position. Use a NULL-aware predicate (NULL sorts first, so it belongs to any `before` set); `albumSongsFrom` is unaffected. - The new `(sourceId, albumId, disc, track, id)` index has the old `(sourceId, albumId)` index as a prefix, so drop the latter in the migration and from the entity. - Tests: mixed NULL disc/track album for the before-window and a full backward scan; both fail on the previous commit. --- .../com/subtracks/data/db/Migrations.kt | 2 + .../kotlin/com/subtracks/data/db/QueueDao.kt | 7 ++- .../kotlin/com/subtracks/data/model/Models.kt | 1 - .../subtracks/data/repo/QueueKeysetTest.kt | 45 +++++++++++++++++-- 4 files changed, 50 insertions(+), 5 deletions(-) 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 531e7eb9..5813b903 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt @@ -266,6 +266,8 @@ val MIGRATION_18_19 = "CREATE INDEX IF NOT EXISTS `index_songs_sourceId_albumId_order` ON `songs` " + "(`sourceId`, `albumId`, `disc`, `track`, `id`)", ) + // Covered by the prefix of the new index. + connection.execSQL("DROP INDEX IF EXISTS `index_songs_sourceId_albumId`") } } diff --git a/app/src/main/kotlin/com/subtracks/data/db/QueueDao.kt b/app/src/main/kotlin/com/subtracks/data/db/QueueDao.kt index 2b3a9ce1..916ee019 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/QueueDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/QueueDao.kt @@ -125,8 +125,13 @@ interface QueueDao { limit: Int, ): List + // Row-value `<` is null-intolerant, and NULL disc/track sort first, so a plain + // `(disc, track, id) < (...)` would drop exactly the untagged tracks a backward window needs. @Query( - "$ALBUM_SONGS_SELECT AND (songs.disc, songs.track, songs.id) < (:disc, :track, :id) " + + "$ALBUM_SONGS_SELECT AND (" + + "songs.disc IS NULL OR songs.disc < :disc OR " + + "(songs.disc = :disc AND (songs.track IS NULL OR songs.track < :track OR " + + "(songs.track = :track AND songs.id < :id)))) " + "ORDER BY songs.disc DESC, songs.track DESC, songs.id DESC LIMIT :limit OFFSET :skip", ) suspend fun albumSongsBefore( diff --git a/app/src/main/kotlin/com/subtracks/data/model/Models.kt b/app/src/main/kotlin/com/subtracks/data/model/Models.kt index 99ad39ae..e1dfac05 100644 --- a/app/src/main/kotlin/com/subtracks/data/model/Models.kt +++ b/app/src/main/kotlin/com/subtracks/data/model/Models.kt @@ -210,7 +210,6 @@ data class PlaylistSong( ), ], indices = [ - Index("sourceId", "albumId"), Index("sourceId", "artistId"), Index( name = "index_songs_sourceId_albumId_order", diff --git a/app/src/test/kotlin/com/subtracks/data/repo/QueueKeysetTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/QueueKeysetTest.kt index c22cb771..723d2cf8 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/QueueKeysetTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/QueueKeysetTest.kt @@ -103,6 +103,37 @@ class QueueKeysetTest { assertEquals(listOf("t1a", "t1z"), all) } + @Test + fun albumKeysetBeforeIncludesNullDiscAndTrack() = + runTest { + seedLibrary() + val all = allAlbumRows("mixed").map { it.song.id } + assertEquals(listOf("n1", "n2", "s1", "s2"), all) + + val actual = + db + .queueDao() + .albumSongsBefore(1, "mixed", disc = 1, track = 2, id = "s2", skip = 0, limit = 2) + .asReversed() + .map { it.song.id } + + assertEquals(listOf("n2", "s1"), actual) + } + + @Test + fun scanningAnAlbumWithNullDiscAndTrackBackwardsResolvesEveryRow() = + runTest { + seedLibrary() + repository.replace(listOf(repository.albumEntry(1, "mixed"))) + val snapshot = repository.snapshot() + val expected = allAlbumRows("mixed").map { it.song.id } + + val resolved = + (snapshot.size - 1 downTo 0).map { repository.itemAt(snapshot, it)?.song?.id }.asReversed() + + assertEquals(expected, resolved) + } + @Test fun playlistKeysetMatchesOffsetWindowWithGapsAndGhosts() = runTest { @@ -234,6 +265,7 @@ class QueueKeysetTest { listOf( album("big", "Big Album"), album("tie", "Tie Album"), + album("mixed", "Mixed Album"), ), ) val big = @@ -247,7 +279,14 @@ class QueueKeysetTest { song(id = "t1z", albumId = "tie", disc = 1, track = 1), song(id = "t1a", albumId = "tie", disc = 1, track = 1), ) - db.libraryDao().upsertSongs(big + tie) + val mixed = + listOf( + song(id = "n1", albumId = "mixed"), + song(id = "n2", albumId = "mixed", track = 3), + song(id = "s1", albumId = "mixed", disc = 1, track = 1), + song(id = "s2", albumId = "mixed", disc = 1, track = 2), + ) + db.libraryDao().upsertSongs(big + tie + mixed) val positions = (0 until 20).map { it * 2L } val playlistSongs = @@ -281,8 +320,8 @@ class QueueKeysetTest { private fun song( id: String, albumId: String, - disc: Long, - track: Long, + disc: Long? = null, + track: Long? = null, ) = Song( sourceId = 1, id = id,