mirror of
https://github.com/austinried/subtracks.git
synced 2026-11-11 15:29:15 +01:00
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.
This commit is contained in:
@@ -266,6 +266,8 @@ val MIGRATION_18_19 =
|
|||||||
"CREATE INDEX IF NOT EXISTS `index_songs_sourceId_albumId_order` ON `songs` " +
|
"CREATE INDEX IF NOT EXISTS `index_songs_sourceId_albumId_order` ON `songs` " +
|
||||||
"(`sourceId`, `albumId`, `disc`, `track`, `id`)",
|
"(`sourceId`, `albumId`, `disc`, `track`, `id`)",
|
||||||
)
|
)
|
||||||
|
// Covered by the prefix of the new index.
|
||||||
|
connection.execSQL("DROP INDEX IF EXISTS `index_songs_sourceId_albumId`")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -125,8 +125,13 @@ interface QueueDao {
|
|||||||
limit: Int,
|
limit: Int,
|
||||||
): List<SongListItem>
|
): List<SongListItem>
|
||||||
|
|
||||||
|
// 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(
|
@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",
|
"ORDER BY songs.disc DESC, songs.track DESC, songs.id DESC LIMIT :limit OFFSET :skip",
|
||||||
)
|
)
|
||||||
suspend fun albumSongsBefore(
|
suspend fun albumSongsBefore(
|
||||||
|
|||||||
@@ -210,7 +210,6 @@ data class PlaylistSong(
|
|||||||
),
|
),
|
||||||
],
|
],
|
||||||
indices = [
|
indices = [
|
||||||
Index("sourceId", "albumId"),
|
|
||||||
Index("sourceId", "artistId"),
|
Index("sourceId", "artistId"),
|
||||||
Index(
|
Index(
|
||||||
name = "index_songs_sourceId_albumId_order",
|
name = "index_songs_sourceId_albumId_order",
|
||||||
|
|||||||
@@ -103,6 +103,37 @@ class QueueKeysetTest {
|
|||||||
assertEquals(listOf("t1a", "t1z"), all)
|
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
|
@Test
|
||||||
fun playlistKeysetMatchesOffsetWindowWithGapsAndGhosts() =
|
fun playlistKeysetMatchesOffsetWindowWithGapsAndGhosts() =
|
||||||
runTest {
|
runTest {
|
||||||
@@ -234,6 +265,7 @@ class QueueKeysetTest {
|
|||||||
listOf(
|
listOf(
|
||||||
album("big", "Big Album"),
|
album("big", "Big Album"),
|
||||||
album("tie", "Tie Album"),
|
album("tie", "Tie Album"),
|
||||||
|
album("mixed", "Mixed Album"),
|
||||||
),
|
),
|
||||||
)
|
)
|
||||||
val big =
|
val big =
|
||||||
@@ -247,7 +279,14 @@ class QueueKeysetTest {
|
|||||||
song(id = "t1z", albumId = "tie", disc = 1, track = 1),
|
song(id = "t1z", albumId = "tie", disc = 1, track = 1),
|
||||||
song(id = "t1a", 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 positions = (0 until 20).map { it * 2L }
|
||||||
val playlistSongs =
|
val playlistSongs =
|
||||||
@@ -281,8 +320,8 @@ class QueueKeysetTest {
|
|||||||
private fun song(
|
private fun song(
|
||||||
id: String,
|
id: String,
|
||||||
albumId: String,
|
albumId: String,
|
||||||
disc: Long,
|
disc: Long? = null,
|
||||||
track: Long,
|
track: Long? = null,
|
||||||
) = Song(
|
) = Song(
|
||||||
sourceId = 1,
|
sourceId = 1,
|
||||||
id = id,
|
id = id,
|
||||||
|
|||||||
Reference in New Issue
Block a user