mirror of
https://github.com/austinried/subtracks.git
synced 2026-11-10 15:11:58 +01:00
library: reconcile artist credits with playback and downloads
Left-join albums in the artist queue queries so albumless credited songs can be played, downloaded and deleted. Add sortName to the starred index, persist a per-source stale-credit marker so an interrupted sync cannot strand artist counts or hide artists, and recompute artist play data in one grouped pass.
This commit is contained in:
+1900
File diff suppressed because it is too large
Load Diff
@@ -93,7 +93,8 @@ private const val DOWNLOADED_ARTIST_ALBUMS =
|
||||
"WHERE song_artists.sourceId = :sourceId AND song_artists.artistId = artists.id " +
|
||||
"AND sd.status = 'Completed' AND songs.albumId IS NOT NULL"
|
||||
|
||||
internal const val ARTIST_CREDITED = "artists.credited = 1"
|
||||
internal const val ARTIST_CREDITED =
|
||||
"(artists.credited = 1 OR COALESCE((SELECT creditsStale FROM sources WHERE id = artists.sourceId), 1) = 1)"
|
||||
|
||||
internal const val ARTISTS_FILTER =
|
||||
"FROM artists WHERE sourceId = :sourceId " +
|
||||
@@ -212,24 +213,25 @@ interface LibraryDao {
|
||||
// The credited albums are the union of the lead id and the relation table, so a shared album
|
||||
// counts once per artist rather than twice.
|
||||
@Query(
|
||||
"UPDATE artists SET " +
|
||||
"playCount = COALESCE((SELECT SUM(a.playCount) FROM albums a " +
|
||||
"WHERE a.sourceId = artists.sourceId AND a.id IN (" +
|
||||
"SELECT albumId FROM album_artists WHERE sourceId = artists.sourceId AND artistId = artists.id " +
|
||||
"UNION SELECT id FROM albums WHERE sourceId = artists.sourceId AND artistId = artists.id)), 0), " +
|
||||
"played = (SELECT MAX(a.played) FROM albums a " +
|
||||
"WHERE a.sourceId = artists.sourceId AND a.id IN (" +
|
||||
"SELECT albumId FROM album_artists WHERE sourceId = artists.sourceId AND artistId = artists.id " +
|
||||
"UNION SELECT id FROM albums WHERE sourceId = artists.sourceId AND artistId = artists.id)) " +
|
||||
"WHERE sourceId = :sourceId AND (" +
|
||||
"playCount IS NOT COALESCE((SELECT SUM(a.playCount) FROM albums a " +
|
||||
"WHERE a.sourceId = artists.sourceId AND a.id IN (" +
|
||||
"SELECT albumId FROM album_artists WHERE sourceId = artists.sourceId AND artistId = artists.id " +
|
||||
"UNION SELECT id FROM albums WHERE sourceId = artists.sourceId AND artistId = artists.id)), 0) " +
|
||||
"OR played IS NOT (SELECT MAX(a.played) FROM albums a " +
|
||||
"WHERE a.sourceId = artists.sourceId AND a.id IN (" +
|
||||
"SELECT albumId FROM album_artists WHERE sourceId = artists.sourceId AND artistId = artists.id " +
|
||||
"UNION SELECT id FROM albums WHERE sourceId = artists.sourceId AND artistId = artists.id)))",
|
||||
"WITH credited AS (" +
|
||||
"SELECT albums.artistId AS artistId, albums.id AS albumId, " +
|
||||
"albums.playCount AS playCount, albums.played AS played " +
|
||||
"FROM albums WHERE albums.sourceId = :sourceId AND albums.artistId IS NOT NULL " +
|
||||
"UNION " +
|
||||
"SELECT album_artists.artistId, albums.id, albums.playCount, albums.played " +
|
||||
"FROM album_artists JOIN albums " +
|
||||
"ON albums.sourceId = album_artists.sourceId AND albums.id = album_artists.albumId " +
|
||||
"WHERE album_artists.sourceId = :sourceId" +
|
||||
"), totals AS (" +
|
||||
"SELECT artistId AS id, SUM(playCount) AS playCount, MAX(played) AS played " +
|
||||
"FROM credited GROUP BY artistId" +
|
||||
") " +
|
||||
"UPDATE artists SET playCount = c.playCount, played = c.played " +
|
||||
"FROM (SELECT artists.id AS id, COALESCE(totals.playCount, 0) AS playCount, totals.played AS played " +
|
||||
"FROM artists LEFT JOIN totals ON totals.id = artists.id " +
|
||||
"WHERE artists.sourceId = :sourceId) c " +
|
||||
"WHERE artists.sourceId = :sourceId AND artists.id = c.id " +
|
||||
"AND (artists.playCount != c.playCount OR artists.played IS NOT c.played)",
|
||||
)
|
||||
suspend fun recomputeArtistPlayData(sourceId: Long)
|
||||
|
||||
@@ -248,7 +250,7 @@ interface LibraryDao {
|
||||
"UPDATE artists SET songCount = c.songCount, credited = c.credited " +
|
||||
"FROM (SELECT artists.id AS id, COALESCE(counts.songCount, 0) AS songCount, " +
|
||||
"((NOT EXISTS (SELECT 1 FROM album_artists WHERE sourceId = artists.sourceId) " +
|
||||
"AND NOT EXISTS (SELECT 1 FROM song_artists WHERE sourceId = artists.sourceId)) " +
|
||||
"OR NOT EXISTS (SELECT 1 FROM song_artists WHERE sourceId = artists.sourceId)) " +
|
||||
"OR EXISTS (SELECT 1 FROM album_artists aa " +
|
||||
"WHERE aa.sourceId = artists.sourceId AND aa.artistId = artists.id) " +
|
||||
"OR EXISTS (SELECT 1 FROM song_artists sa " +
|
||||
|
||||
@@ -557,8 +557,8 @@ val MIGRATION_31_32 =
|
||||
connection.execSQL("ALTER TABLE `artists` ADD COLUMN `credited` INTEGER NOT NULL DEFAULT 1")
|
||||
connection.execSQL(
|
||||
"UPDATE `artists` SET `credited` = 0 " +
|
||||
"WHERE (EXISTS (SELECT 1 FROM `album_artists` WHERE `sourceId` = `artists`.`sourceId`) " +
|
||||
"OR EXISTS (SELECT 1 FROM `song_artists` WHERE `sourceId` = `artists`.`sourceId`)) " +
|
||||
"WHERE EXISTS (SELECT 1 FROM `album_artists` WHERE `sourceId` = `artists`.`sourceId`) " +
|
||||
"AND EXISTS (SELECT 1 FROM `song_artists` WHERE `sourceId` = `artists`.`sourceId`) " +
|
||||
"AND NOT EXISTS (SELECT 1 FROM `album_artists` aa " +
|
||||
"WHERE aa.`sourceId` = `artists`.`sourceId` AND aa.`artistId` = `artists`.`id`) " +
|
||||
"AND NOT EXISTS (SELECT 1 FROM `song_artists` sa " +
|
||||
@@ -567,6 +567,18 @@ val MIGRATION_31_32 =
|
||||
}
|
||||
}
|
||||
|
||||
val MIGRATION_32_33 =
|
||||
object : Migration(32, 33) {
|
||||
override suspend fun migrate(connection: SQLiteConnection) {
|
||||
connection.execSQL("ALTER TABLE `sources` ADD COLUMN `creditsStale` INTEGER NOT NULL DEFAULT 1")
|
||||
connection.execSQL("DROP INDEX IF EXISTS `index_songs_starred`")
|
||||
connection.execSQL(
|
||||
"CREATE INDEX IF NOT EXISTS `index_songs_starred` ON `songs` " +
|
||||
"(`sourceId` ASC, `starred` DESC, `sortName` ASC, `id` ASC)",
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// Mirrors the FTS5 table and content-sync triggers Room generates for the @Fts5 entities.
|
||||
private suspend fun createSearchIndex(
|
||||
connection: SQLiteConnection,
|
||||
@@ -632,4 +644,5 @@ val MIGRATIONS: Array<Migration> =
|
||||
MIGRATION_29_30,
|
||||
MIGRATION_30_31,
|
||||
MIGRATION_31_32,
|
||||
MIGRATION_32_33,
|
||||
)
|
||||
|
||||
@@ -34,7 +34,7 @@ private const val ARTIST_SONG_IDS =
|
||||
|
||||
private const val ARTIST_SONGS_SELECT =
|
||||
"SELECT songs.*, albums.coverArt AS coverArt FROM songs " +
|
||||
"JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " +
|
||||
"LEFT JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " +
|
||||
"WHERE songs.sourceId = :sourceId AND songs.id IN $ARTIST_SONG_IDS"
|
||||
|
||||
private const val ARTIST_SONGS_ORDER =
|
||||
@@ -95,7 +95,7 @@ interface QueueDao {
|
||||
|
||||
@Query(
|
||||
"SELECT COUNT(*) FROM songs " +
|
||||
"JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " +
|
||||
"LEFT JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " +
|
||||
"WHERE songs.sourceId = :sourceId AND songs.id IN $ARTIST_SONG_IDS",
|
||||
)
|
||||
suspend fun artistLength(
|
||||
@@ -203,7 +203,7 @@ interface QueueDao {
|
||||
|
||||
@Query(
|
||||
"SELECT songs.id FROM songs " +
|
||||
"JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " +
|
||||
"LEFT JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " +
|
||||
"WHERE songs.sourceId = :sourceId AND songs.id IN $ARTIST_SONG_IDS" +
|
||||
ARTIST_SONGS_ORDER,
|
||||
)
|
||||
|
||||
@@ -29,6 +29,15 @@ interface SourcesDao {
|
||||
@Query("SELECT id FROM sources WHERE isActive = 1 LIMIT 1")
|
||||
suspend fun activeSourceIdOnce(): Long?
|
||||
|
||||
@Query("SELECT creditsStale FROM sources WHERE id = :sourceId")
|
||||
suspend fun creditsStale(sourceId: Long): Boolean?
|
||||
|
||||
@Query("UPDATE sources SET creditsStale = :stale WHERE id = :sourceId")
|
||||
suspend fun setCreditsStale(
|
||||
sourceId: Long,
|
||||
stale: Boolean,
|
||||
)
|
||||
|
||||
@Insert
|
||||
suspend fun insertSource(source: Source): Long
|
||||
|
||||
|
||||
@@ -54,7 +54,7 @@ import com.subtracks.data.model.UpNextEntry
|
||||
SongDownload::class,
|
||||
PendingAction::class,
|
||||
],
|
||||
version = 32,
|
||||
version = 33,
|
||||
exportSchema = true,
|
||||
)
|
||||
@ColumnTypeConverters(
|
||||
|
||||
@@ -18,6 +18,7 @@ data class Source(
|
||||
val address: String,
|
||||
val isActive: Boolean = false,
|
||||
val createdAt: Long,
|
||||
@ColumnInfo(defaultValue = "1") val creditsStale: Boolean = true,
|
||||
)
|
||||
|
||||
@Entity(
|
||||
@@ -263,8 +264,8 @@ data class PlaylistSong(
|
||||
),
|
||||
Index(
|
||||
name = "index_songs_starred",
|
||||
value = ["sourceId", "starred", "id"],
|
||||
orders = [Index.Order.ASC, Index.Order.DESC, Index.Order.ASC],
|
||||
value = ["sourceId", "starred", "sortName", "id"],
|
||||
orders = [Index.Order.ASC, Index.Order.DESC, Index.Order.ASC, Index.Order.ASC],
|
||||
),
|
||||
],
|
||||
)
|
||||
|
||||
@@ -34,6 +34,8 @@ class SyncService(
|
||||
private var creditsChanged = false
|
||||
|
||||
suspend fun sync(): SyncSummary {
|
||||
val sources = db.sourcesDao()
|
||||
val wasStale = sources.creditsStale(source.id) ?: true
|
||||
creditsChanged = false
|
||||
val artistsBefore = dbTotalChanges()
|
||||
syncArtists()
|
||||
@@ -41,7 +43,10 @@ class SyncService(
|
||||
syncAlbums()
|
||||
syncSongs()
|
||||
optimize()
|
||||
if (creditsChanged || artistsChanged) recomputeArtistSongCounts()
|
||||
if (creditsChanged || artistsChanged || wasStale) {
|
||||
recomputeArtistSongCounts()
|
||||
sources.setCreditsStale(source.id, false)
|
||||
}
|
||||
aggregatePlayData()
|
||||
syncPlaylists()
|
||||
syncPlaylistSongs()
|
||||
@@ -83,7 +88,13 @@ class SyncService(
|
||||
albumsSynced += batch.size
|
||||
}
|
||||
val prunedBefore = pruned
|
||||
pruned += pruneStaleIds(seenAlbums, { library.albumIdsAfter(source.id, it, PRUNE_PAGE) }, { library.deleteAlbums(source.id, it) })
|
||||
pruned +=
|
||||
pruneStaleIds(
|
||||
seenAlbums,
|
||||
{ library.albumIdsAfter(source.id, it, PRUNE_PAGE) },
|
||||
{ library.deleteAlbums(source.id, it) },
|
||||
markStale = { db.sourcesDao().setCreditsStale(source.id, true) },
|
||||
)
|
||||
if (pruned > prunedBefore) creditsChanged = true
|
||||
pruned +=
|
||||
pruneStaleDiscs(seenDiscs, { albumId, disc -> library.discKeysAfter(source.id, albumId, disc, PRUNE_PAGE) }) {
|
||||
@@ -101,7 +112,13 @@ class SyncService(
|
||||
songsSynced += batch.size
|
||||
}
|
||||
val prunedBefore = pruned
|
||||
pruned += pruneStaleIds(seen, { library.songIdsAfter(source.id, it, PRUNE_PAGE) }, { library.deleteSongs(source.id, it) })
|
||||
pruned +=
|
||||
pruneStaleIds(
|
||||
seen,
|
||||
{ library.songIdsAfter(source.id, it, PRUNE_PAGE) },
|
||||
{ library.deleteSongs(source.id, it) },
|
||||
markStale = { db.sourcesDao().setCreditsStale(source.id, true) },
|
||||
)
|
||||
if (pruned > prunedBefore) creditsChanged = true
|
||||
}
|
||||
|
||||
@@ -117,7 +134,8 @@ class SyncService(
|
||||
}
|
||||
db.useWriterConnection { connection ->
|
||||
connection.immediateTransaction {
|
||||
if (connection.albumIdsChanged(batch)) creditsChanged = true
|
||||
val albumChanged = connection.albumIdsChanged(batch)
|
||||
if (albumChanged) creditsChanged = true
|
||||
val resolved = connection.resolveSongSortNames(batch)
|
||||
connection.upsertChanged("songs", songColumns, primaryKey, resolved) { it.values() }
|
||||
connection.usePrepared("DELETE FROM song_genres WHERE sourceId = ? AND songId = ? AND position >= ?") { statement ->
|
||||
@@ -143,7 +161,11 @@ class SyncService(
|
||||
}
|
||||
}
|
||||
connection.upsertChanged("song_artists", songArtistColumns, songArtistKey, artists) { it.values() }
|
||||
if (connection.totalChanges() != artistRowsBefore) creditsChanged = true
|
||||
val artistRowsChanged = connection.totalChanges() != artistRowsBefore
|
||||
if (albumChanged || artistRowsChanged) {
|
||||
connection.markCreditsStale(batch.first().sourceId)
|
||||
creditsChanged = true
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -167,6 +189,13 @@ class SyncService(
|
||||
|
||||
private suspend fun dbTotalChanges(): Long = db.useWriterConnection { it.totalChanges() }
|
||||
|
||||
private suspend fun PooledConnection.markCreditsStale(sourceId: Long) {
|
||||
usePrepared("UPDATE sources SET creditsStale = 1 WHERE id = ? AND creditsStale = 0") { statement ->
|
||||
statement.bindLong(1, sourceId)
|
||||
statement.step()
|
||||
}
|
||||
}
|
||||
|
||||
private suspend fun PooledConnection.albumIdsChanged(batch: List<Song>): Boolean {
|
||||
val expected = batch.associate { it.id to it.albumId }.toMutableMap()
|
||||
if (expected.isEmpty()) return false
|
||||
@@ -319,7 +348,10 @@ class SyncService(
|
||||
}
|
||||
}
|
||||
connection.upsertChanged("album_artists", albumArtistColumns, albumArtistKey, artists) { it.values() }
|
||||
if (connection.totalChanges() != artistRowsBefore) creditsChanged = true
|
||||
if (connection.totalChanges() != artistRowsBefore) {
|
||||
connection.markCreditsStale(albums.first().sourceId)
|
||||
creditsChanged = true
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -347,6 +379,7 @@ class SyncService(
|
||||
seen: HashedIds,
|
||||
pageOf: suspend (afterId: String) -> List<String>,
|
||||
delete: suspend (Collection<String>) -> Unit,
|
||||
markStale: (suspend () -> Unit)? = null,
|
||||
): Int {
|
||||
seen.freeze()
|
||||
var afterId = ""
|
||||
@@ -355,6 +388,7 @@ class SyncService(
|
||||
val ids = pageOf(afterId)
|
||||
if (ids.isEmpty()) return pruned
|
||||
val stale = ids.filterNot { seen.contains(idHash(it)) }
|
||||
if (stale.isNotEmpty()) markStale?.invoke()
|
||||
stale.chunked(DELETE_CHUNK).forEach { delete(it) }
|
||||
pruned += stale.size
|
||||
if (ids.size < PRUNE_PAGE) return pruned
|
||||
|
||||
@@ -64,6 +64,8 @@ class MigrationsTest {
|
||||
assertEquals(1, indexColumn(migrated, "index_albums_starred", "starred").descending)
|
||||
assertEquals(1, indexColumn(migrated, "index_albums_frequent", "playCount").descending)
|
||||
assertEquals(1, indexColumn(migrated, "index_albums_recent", "played").descending)
|
||||
assertEquals("NOCASE", indexColumn(migrated, "index_songs_starred", "sortName").collation)
|
||||
assertEquals(1, indexColumn(migrated, "index_songs_starred", "starred").descending)
|
||||
assertEquals("NOCASE", indexColumn(migrated, "index_artists_name", "sortName").collation)
|
||||
assertEquals(1, indexColumn(migrated, "index_artists_albumCount", "albumCount").descending)
|
||||
assertEquals(1, indexColumn(migrated, "index_artists_starred", "starred").descending)
|
||||
@@ -229,6 +231,13 @@ class MigrationsTest {
|
||||
"INSERT INTO album_artists (sourceId, albumId, position, artistId, name) " +
|
||||
"VALUES (1, 'al1', 0, 'a1', 'Alpha')",
|
||||
)
|
||||
migrated.execSQL(
|
||||
"INSERT INTO songs (sourceId, id, albumId, artistId, title) VALUES (1, 's1', 'al1', 'a1', 'Song')",
|
||||
)
|
||||
migrated.execSQL(
|
||||
"INSERT INTO song_artists (sourceId, songId, position, artistId, name) " +
|
||||
"VALUES (1, 's1', 0, 'a1', 'Alpha')",
|
||||
)
|
||||
|
||||
MIGRATION_31_32.migrate(migrated)
|
||||
|
||||
|
||||
@@ -1167,6 +1167,33 @@ class SyncServiceTest {
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun aStaleCreditMarkerForcesARecomputeOnTheNextSync() =
|
||||
runTest {
|
||||
insertSource()
|
||||
val source =
|
||||
FakeMusicSource(
|
||||
artists = listOf(artist("a1", "A")),
|
||||
albums = listOf(album("al1")),
|
||||
songs = listOf(song("s1")),
|
||||
)
|
||||
|
||||
SyncService(db, source).sync()
|
||||
assertEquals(false, db.sourcesDao().creditsStale(1))
|
||||
|
||||
db.sourcesDao().setCreditsStale(1, true)
|
||||
db.libraryDao().upsertArtists(
|
||||
listOf(db.libraryDao().artistOnce(1, "a1")!!.copy(songCount = 0, credited = false)),
|
||||
)
|
||||
|
||||
SyncService(db, source).sync()
|
||||
|
||||
val artist = db.libraryDao().artistOnce(1, "a1")!!
|
||||
assertEquals(1L, artist.songCount)
|
||||
assertEquals(true, artist.credited)
|
||||
assertEquals(false, db.sourcesDao().creditsStale(1))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun everyAlbumArtistAndTrackArtistReachesTheSharedAlbum() =
|
||||
runTest {
|
||||
@@ -1251,6 +1278,9 @@ class SyncServiceTest {
|
||||
SyncService(db, source).sync()
|
||||
|
||||
assertEquals(2L, db.libraryDao().artistOnce(1, "a1")!!.songCount)
|
||||
assertEquals(listOf("s1", "s2"), db.queueDao().artistSongIds(1, "a1"))
|
||||
assertEquals(2L, db.queueDao().artistLength(1, "a1"))
|
||||
assertEquals(2, db.queueDao().artistSongs(1, "a1", 0, 10).size)
|
||||
db.downloadDao().upsert(SongDownload(1, "s1", DownloadStatus.Completed))
|
||||
db.downloadDao().upsert(SongDownload(1, "s2", DownloadStatus.Completed))
|
||||
|
||||
|
||||
Reference in New Issue
Block a user