diff --git a/internal/api/admin_duplicates.go b/internal/api/admin_duplicates.go index be595999..0bc48c42 100644 --- a/internal/api/admin_duplicates.go +++ b/internal/api/admin_duplicates.go @@ -331,15 +331,17 @@ func (h *handlers) handleRunDuplicateSweep(w http.ResponseWriter, _ *http.Reques // handleDismissDuplicateGroup implements POST // /api/admin/library/duplicates/{id}/dismiss: "these are not duplicates". The -// sweep keeps the dismissal and will not propose that set of tracks again. 404 -// duplicate_group_not_pending when the group was already resolved or is gone. +// sweep keeps the dismissal and will not propose that set of tracks again. On a +// cross-release group it also undoes the song link (M498 #5438): the copies +// stop sharing likes from here on. 404 duplicate_group_not_pending when the +// group was already resolved or is gone. func (h *handlers) handleDismissDuplicateGroup(w http.ResponseWriter, r *http.Request) { id, ok := parseUUID(chi.URLParam(r, "id")) if !ok { writeAdminJSONErr(w, http.StatusBadRequest, "invalid_id") return } - n, err := dbq.New(h.pool).DismissDuplicateGroup(r.Context(), id) + n, err := h.dismissDuplicateGroup(r.Context(), id) if err != nil { h.logger.Error("admin: dismiss duplicate group", "err", err) writeAdminJSONErr(w, http.StatusInternalServerError, "server_error") @@ -352,6 +354,25 @@ func (h *handlers) handleDismissDuplicateGroup(w http.ResponseWriter, r *http.Re writeJSON(w, http.StatusOK, map[string]string{"status": "dismissed"}) } +// dismissDuplicateGroup dismisses the group and unlinks its copies as one song, +// together. +func (h *handlers) dismissDuplicateGroup(ctx context.Context, id pgtype.UUID) (int64, error) { + tx, err := h.pool.Begin(ctx) + if err != nil { + return 0, err + } + defer func() { _ = tx.Rollback(ctx) }() + tq := dbq.New(tx) + n, err := tq.DismissDuplicateGroup(ctx, id) + if err != nil || n == 0 { + return n, err + } + if err := tq.UnlinkDuplicateGroupSongs(ctx, id); err != nil { + return 0, err + } + return n, tx.Commit(ctx) +} + // mergeDuplicateRequest chooses the copy to keep. An empty survivor_track_id // keeps the report's proposal. type mergeDuplicateRequest struct { diff --git a/internal/api/likes.go b/internal/api/likes.go index b36dfcd7..aad2a663 100644 --- a/internal/api/likes.go +++ b/internal/api/likes.go @@ -3,6 +3,7 @@ package api import ( "errors" "net/http" + "slices" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" @@ -50,20 +51,36 @@ func (h *handlers) handleLikeTrack(w http.ResponseWriter, r *http.Request) { writeErr(w, apierror.InternalMsg("lookup failed", err)) return } - rows, err := q.LikeTrack(r.Context(), dbq.LikeTrackParams{UserID: user.ID, TrackID: id}) + liked, err := q.LikeTrack(r.Context(), dbq.LikeTrackParams{UserID: user.ID, TrackID: id}) if err != nil { h.logger.Error("api: like track insert", "err", err) writeErr(w, apierror.InternalMsg("insert failed", err)) return } - if rows == 1 { + if slices.Contains(liked, id) { _ = playevents.CaptureContextualLikeIfPlaying(r.Context(), q, user.ID, id, h.logger) } - h.logLikeChange(r, syncpkg.EntityLikeTrack, user.ID, id, syncpkg.OpUpsert) - h.publishLikeEvent(user.ID, id, "track", true) + // The like is on the song (M498 #5438): every copy it reached changes on + // the client too. + for _, t := range withTrack(id, liked) { + h.logLikeChange(r, syncpkg.EntityLikeTrack, user.ID, t, syncpkg.OpUpsert) + h.publishLikeEvent(user.ID, t, "track", true) + } w.WriteHeader(http.StatusNoContent) } +// withTrack is the tracks a like or unlike changed, led by the one asked for, +// which is reported even when it was already in that state. +func withTrack(id pgtype.UUID, changed []pgtype.UUID) []pgtype.UUID { + out := []pgtype.UUID{id} + for _, t := range changed { + if t != id { + out = append(out, t) + } + } + return out +} + func (h *handlers) handleUnlikeTrack(w http.ResponseWriter, r *http.Request) { user, ok := requireUser(w, r) if !ok { @@ -74,7 +91,8 @@ func (h *handlers) handleUnlikeTrack(w http.ResponseWriter, r *http.Request) { return } q := dbq.New(h.pool) - if err := q.UnlikeTrack(r.Context(), dbq.UnlikeTrackParams{UserID: user.ID, TrackID: id}); err != nil { + unliked, err := q.UnlikeTrack(r.Context(), dbq.UnlikeTrackParams{UserID: user.ID, TrackID: id}) + if err != nil { h.logger.Error("api: unlike track", "err", err) writeErr(w, apierror.InternalMsg("delete failed", err)) return @@ -83,8 +101,10 @@ func (h *handlers) handleUnlikeTrack(w http.ResponseWriter, r *http.Request) { h.logger.Error("api: soft-delete contextual_likes", "err", err) // Don't fail the response — soft-delete is best-effort. } - h.logLikeChange(r, syncpkg.EntityLikeTrack, user.ID, id, syncpkg.OpDelete) - h.publishLikeEvent(user.ID, id, "track", false) + for _, t := range withTrack(id, unliked) { + h.logLikeChange(r, syncpkg.EntityLikeTrack, user.ID, t, syncpkg.OpDelete) + h.publishLikeEvent(user.ID, t, "track", false) + } w.WriteHeader(http.StatusNoContent) } diff --git a/internal/db/dbq/events.sql.go b/internal/db/dbq/events.sql.go index d626f477..bbd3673f 100644 --- a/internal/db/dbq/events.sql.go +++ b/internal/db/dbq/events.sql.go @@ -261,7 +261,7 @@ func (q *Queries) InsertSkipEvent(ctx context.Context, arg InsertSkipEventParams } const listRecentSessionTracks = `-- name: ListRecentSessionTracks :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source FROM tracks t +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, t.song_id, t.song_key FROM tracks t JOIN play_events pe ON pe.track_id = t.id WHERE pe.session_id = $1 AND pe.started_at < $2 @@ -308,6 +308,8 @@ func (q *Queries) ListRecentSessionTracks(ctx context.Context, arg ListRecentSes &i.TagReadVersion, &i.MissingSince, &i.MbidSource, + &i.SongID, + &i.SongKey, ); err != nil { return nil, err } diff --git a/internal/db/dbq/history.sql.go b/internal/db/dbq/history.sql.go index 0d0a4efa..7a493b16 100644 --- a/internal/db/dbq/history.sql.go +++ b/internal/db/dbq/history.sql.go @@ -14,7 +14,7 @@ import ( const listUserHistory = `-- name: ListUserHistory :many SELECT pe.id AS event_id, pe.started_at, - t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, + t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, t.song_id, t.song_key, albums.title AS album_title, artists.name AS artist_name FROM play_events pe @@ -82,6 +82,8 @@ func (q *Queries) ListUserHistory(ctx context.Context, arg ListUserHistoryParams &i.Track.TagReadVersion, &i.Track.MissingSince, &i.Track.MbidSource, + &i.Track.SongID, + &i.Track.SongKey, &i.AlbumTitle, &i.ArtistName, ); err != nil { diff --git a/internal/db/dbq/likes.sql.go b/internal/db/dbq/likes.sql.go index b85e1d49..f5d73817 100644 --- a/internal/db/dbq/likes.sql.go +++ b/internal/db/dbq/likes.sql.go @@ -34,9 +34,12 @@ func (q *Queries) CountLikedArtists(ctx context.Context, userID pgtype.UUID) (in } const countLikedTracks = `-- name: CountLikedTracks :one -SELECT count(*) FROM general_likes WHERE user_id = $1 +SELECT count(DISTINCT t.song_key) FROM general_likes l + JOIN tracks t ON t.id = l.track_id + WHERE l.user_id = $1 ` +// Liked songs, each counted once however many releases hold it. func (q *Queries) CountLikedTracks(ctx context.Context, userID pgtype.UUID) (int64, error) { row := q.db.QueryRow(ctx, countLikedTracks, userID) var count int64 @@ -76,10 +79,13 @@ func (q *Queries) LikeArtist(ctx context.Context, arg LikeArtistParams) error { return err } -const likeTrack = `-- name: LikeTrack :execrows +const likeTrack = `-- name: LikeTrack :many INSERT INTO general_likes (user_id, track_id) -VALUES ($1, $2) +SELECT $1::uuid, t.id + FROM tracks t + WHERE t.song_key = (SELECT s.song_key FROM tracks s WHERE s.id = $2::uuid) ON CONFLICT (user_id, track_id) DO NOTHING +RETURNING track_id ` type LikeTrackParams struct { @@ -87,12 +93,27 @@ type LikeTrackParams struct { TrackID pgtype.UUID } -func (q *Queries) LikeTrack(ctx context.Context, arg LikeTrackParams) (int64, error) { - result, err := q.db.Exec(ctx, likeTrack, arg.UserID, arg.TrackID) +// A like is on the song (M498 #5438): every copy of it sharing the track's +// song_key is liked. Returns the tracks newly liked, for the sync log; empty +// when the song was already liked. +func (q *Queries) LikeTrack(ctx context.Context, arg LikeTrackParams) ([]pgtype.UUID, error) { + rows, err := q.db.Query(ctx, likeTrack, arg.UserID, arg.TrackID) if err != nil { - return 0, err + return nil, err } - return result.RowsAffected(), nil + defer rows.Close() + var items []pgtype.UUID + for rows.Next() { + var track_id pgtype.UUID + if err := rows.Scan(&track_id); err != nil { + return nil, err + } + items = append(items, track_id) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil } const listLikedAlbumIDs = `-- name: ListLikedAlbumIDs :many @@ -260,10 +281,14 @@ func (q *Queries) ListLikedTrackIDs(ctx context.Context, userID pgtype.UUID) ([] } const listLikedTrackRows = `-- name: ListLikedTrackRows :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source FROM tracks t -JOIN general_likes l ON l.track_id = t.id -WHERE l.user_id = $1 -ORDER BY l.liked_at DESC +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, t.song_id, t.song_key FROM tracks t +JOIN (SELECT DISTINCT ON (lt.song_key) l.track_id, l.liked_at + FROM general_likes l + JOIN tracks lt ON lt.id = l.track_id + WHERE l.user_id = $1 + ORDER BY lt.song_key, (lt.missing_since IS NOT NULL), l.liked_at, lt.id) l + ON l.track_id = t.id +ORDER BY l.liked_at DESC, t.id LIMIT $2 OFFSET $3 ` @@ -273,6 +298,8 @@ type ListLikedTrackRowsParams struct { Offset int32 } +// One row per liked song: of its copies, the one present on disk, then the +// one liked first. func (q *Queries) ListLikedTrackRows(ctx context.Context, arg ListLikedTrackRowsParams) ([]Track, error) { rows, err := q.db.Query(ctx, listLikedTrackRows, arg.UserID, arg.Limit, arg.Offset) if err != nil { @@ -303,6 +330,8 @@ func (q *Queries) ListLikedTrackRows(ctx context.Context, arg ListLikedTrackRows &i.TagReadVersion, &i.MissingSince, &i.MbidSource, + &i.SongID, + &i.SongKey, ); err != nil { return nil, err } @@ -342,8 +371,13 @@ func (q *Queries) UnlikeArtist(ctx context.Context, arg UnlikeArtistParams) erro return err } -const unlikeTrack = `-- name: UnlikeTrack :exec -DELETE FROM general_likes WHERE user_id = $1 AND track_id = $2 +const unlikeTrack = `-- name: UnlikeTrack :many +DELETE FROM general_likes + WHERE user_id = $1::uuid + AND (track_id = $2::uuid + OR track_id IN (SELECT t.id FROM tracks t + WHERE t.song_key = (SELECT s.song_key FROM tracks s WHERE s.id = $2::uuid))) +RETURNING track_id ` type UnlikeTrackParams struct { @@ -351,7 +385,24 @@ type UnlikeTrackParams struct { TrackID pgtype.UUID } -func (q *Queries) UnlikeTrack(ctx context.Context, arg UnlikeTrackParams) error { - _, err := q.db.Exec(ctx, unlikeTrack, arg.UserID, arg.TrackID) - return err +// Unlikes the song: every copy sharing the track's song_key. Returns the +// tracks unliked, for the sync log. +func (q *Queries) UnlikeTrack(ctx context.Context, arg UnlikeTrackParams) ([]pgtype.UUID, error) { + rows, err := q.db.Query(ctx, unlikeTrack, arg.UserID, arg.TrackID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []pgtype.UUID + for rows.Next() { + var track_id pgtype.UUID + if err := rows.Scan(&track_id); err != nil { + return nil, err + } + items = append(items, track_id) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil } diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index b80a0111..88ce634a 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -754,6 +754,8 @@ type Track struct { TagReadVersion int16 MissingSince pgtype.Timestamptz MbidSource *string + SongID pgtype.UUID + SongKey pgtype.UUID } type TrackAcoustidLookup struct { diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index bab29ed0..b0938825 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/internal/db/dbq/recommendation.sql.go @@ -208,7 +208,7 @@ WITH plays AS ( WHERE user_id = $2 AND was_skipped = false GROUP BY track_id ) -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, t.song_id, t.song_key, albums.title AS album_title, artists.name AS artist_name FROM plays p @@ -271,6 +271,8 @@ func (q *Queries) ListMostPlayedTracksForArtist(ctx context.Context, arg ListMos &i.Track.TagReadVersion, &i.Track.MissingSince, &i.Track.MbidSource, + &i.Track.SongID, + &i.Track.SongKey, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -291,7 +293,7 @@ WITH plays AS ( WHERE user_id = $1 AND was_skipped = false GROUP BY track_id ) -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, t.song_id, t.song_key, albums.title AS album_title, artists.name AS artist_name FROM plays p @@ -356,6 +358,8 @@ func (q *Queries) ListMostPlayedTracksForUser(ctx context.Context, arg ListMostP &i.Track.TagReadVersion, &i.Track.MissingSince, &i.Track.MbidSource, + &i.Track.SongID, + &i.Track.SongKey, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -695,7 +699,7 @@ func (q *Queries) ListRediscoverArtistsForUser(ctx context.Context, arg ListRedi const loadRadioCandidates = `-- name: LoadRadioCandidates :many SELECT - t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, + t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, t.song_id, t.song_key, (l.user_id IS NOT NULL)::bool AS is_liked, pe.last_played_at::timestamptz AS last_played_at, pe.play_count, @@ -777,6 +781,8 @@ func (q *Queries) LoadRadioCandidates(ctx context.Context, arg LoadRadioCandidat &i.Track.TagReadVersion, &i.Track.MissingSince, &i.Track.MbidSource, + &i.Track.SongID, + &i.Track.SongKey, &i.IsLiked, &i.LastPlayedAt, &i.PlayCount, @@ -925,7 +931,7 @@ random_fill AS ( LIMIT $9 ) SELECT - t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, + t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, t.song_id, t.song_key, (l.user_id IS NOT NULL)::bool AS is_liked, pe.last_played_at::timestamptz AS last_played_at, pe.play_count, @@ -1053,6 +1059,8 @@ func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandid &i.Track.TagReadVersion, &i.Track.MissingSince, &i.Track.MbidSource, + &i.Track.SongID, + &i.Track.SongKey, &i.IsLiked, &i.LastPlayedAt, &i.PlayCount, diff --git a/internal/db/dbq/songs.sql.go b/internal/db/dbq/songs.sql.go new file mode 100644 index 00000000..fc19bd00 --- /dev/null +++ b/internal/db/dbq/songs.sql.go @@ -0,0 +1,150 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: songs.sql + +package dbq + +import ( + "context" + + "github.com/jackc/pgx/v5/pgtype" +) + +const linkTracksAsSong = `-- name: LinkTracksAsSong :one + +WITH keys AS ( + SELECT DISTINCT song_key FROM tracks WHERE id = ANY($1::uuid[]) +), target AS ( + SELECT song_key FROM keys ORDER BY song_key LIMIT 1 +), moved AS ( + UPDATE tracks t + SET song_id = (SELECT song_key FROM target) + WHERE t.song_key IN (SELECT song_key FROM keys) + AND t.song_key <> (SELECT song_key FROM target) + RETURNING t.id +) +SELECT (SELECT song_key FROM target)::uuid AS song_key, + (SELECT count(*) FROM moved)::bigint AS moved +` + +type LinkTracksAsSongRow struct { + SongKey pgtype.UUID + Moved int64 +} + +// Song links (M498 #5438): copies of one song on different releases share a +// song_key (migration 0076). +// Links the tracks into one song, joining any songs they already belong to. +// The song keeps the lowest key among them, so linking the same set again +// changes nothing. Returns that key and how many tracks moved to it. +func (q *Queries) LinkTracksAsSong(ctx context.Context, trackIds []pgtype.UUID) (LinkTracksAsSongRow, error) { + row := q.db.QueryRow(ctx, linkTracksAsSong, trackIds) + var i LinkTracksAsSongRow + err := row.Scan(&i.SongKey, &i.Moved) + return i, err +} + +const listTrackSongKeys = `-- name: ListTrackSongKeys :many +SELECT id, song_key FROM tracks WHERE id = ANY($1::uuid[]) +` + +type ListTrackSongKeysRow struct { + ID pgtype.UUID + SongKey pgtype.UUID +} + +func (q *Queries) ListTrackSongKeys(ctx context.Context, ids []pgtype.UUID) ([]ListTrackSongKeysRow, error) { + rows, err := q.db.Query(ctx, listTrackSongKeys, ids) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListTrackSongKeysRow + for rows.Next() { + var i ListTrackSongKeysRow + if err := rows.Scan(&i.ID, &i.SongKey); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const mergeInheritSongLink = `-- name: MergeInheritSongLink :exec +UPDATE tracks s + SET song_id = l.song_key + FROM tracks l + WHERE s.id = $1::uuid + AND l.id = $2::uuid + AND s.song_id IS NULL + AND l.song_key <> l.id +` + +type MergeInheritSongLinkParams struct { + SurvivorID pgtype.UUID + LoserID pgtype.UUID +} + +// A merge keeps the removed copy's song link: when the copy being removed was +// linked to copies on other releases and the survivor was not, the survivor +// joins that song. +func (q *Queries) MergeInheritSongLink(ctx context.Context, arg MergeInheritSongLinkParams) error { + _, err := q.db.Exec(ctx, mergeInheritSongLink, arg.SurvivorID, arg.LoserID) + return err +} + +const shareSongLikes = `-- name: ShareSongLikes :many +INSERT INTO general_likes (user_id, track_id, liked_at) +SELECT l.user_id, t.id, min(l.liked_at) + FROM general_likes l + JOIN tracks lt ON lt.id = l.track_id + JOIN tracks t ON t.song_key = lt.song_key + WHERE lt.song_key = $1::uuid + GROUP BY l.user_id, t.id +ON CONFLICT (user_id, track_id) DO NOTHING +RETURNING user_id, track_id +` + +type ShareSongLikesRow struct { + UserID pgtype.UUID + TrackID pgtype.UUID +} + +// A like on any copy of the song becomes a like on every copy, dated the +// earliest. Returns the likes added, for the sync log. +func (q *Queries) ShareSongLikes(ctx context.Context, songKey pgtype.UUID) ([]ShareSongLikesRow, error) { + rows, err := q.db.Query(ctx, shareSongLikes, songKey) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ShareSongLikesRow + for rows.Next() { + var i ShareSongLikesRow + if err := rows.Scan(&i.UserID, &i.TrackID); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const unlinkDuplicateGroupSongs = `-- name: UnlinkDuplicateGroupSongs :exec +UPDATE tracks SET song_id = NULL + WHERE song_id IS NOT NULL + AND id IN (SELECT track_id FROM duplicate_group_members WHERE group_id = $1) +` + +// "Not the same song": the group's copies stop sharing a song. Likes already +// shared stay where they are. +func (q *Queries) UnlinkDuplicateGroupSongs(ctx context.Context, groupID pgtype.UUID) error { + _, err := q.db.Exec(ctx, unlinkDuplicateGroupSongs, groupID) + return err +} diff --git a/internal/db/dbq/tracks.sql.go b/internal/db/dbq/tracks.sql.go index bbdf7f65..a1210c55 100644 --- a/internal/db/dbq/tracks.sql.go +++ b/internal/db/dbq/tracks.sql.go @@ -247,7 +247,7 @@ func (q *Queries) FindMissingTrackByMbid(ctx context.Context, mbid string) ([]Fi } const getTrackByID = `-- name: GetTrackByID :one -SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source FROM tracks WHERE id = $1 +SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source, song_id, song_key FROM tracks WHERE id = $1 ` func (q *Queries) GetTrackByID(ctx context.Context, id pgtype.UUID) (Track, error) { @@ -274,12 +274,14 @@ func (q *Queries) GetTrackByID(ctx context.Context, id pgtype.UUID) (Track, erro &i.TagReadVersion, &i.MissingSince, &i.MbidSource, + &i.SongID, + &i.SongKey, ) return i, err } const getTrackByPath = `-- name: GetTrackByPath :one -SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source FROM tracks WHERE file_path = $1 +SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source, song_id, song_key FROM tracks WHERE file_path = $1 ` func (q *Queries) GetTrackByPath(ctx context.Context, filePath string) (Track, error) { @@ -306,12 +308,14 @@ func (q *Queries) GetTrackByPath(ctx context.Context, filePath string) (Track, e &i.TagReadVersion, &i.MissingSince, &i.MbidSource, + &i.SongID, + &i.SongKey, ) return i, err } const getTracksByIDs = `-- name: GetTracksByIDs :many -SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source FROM tracks WHERE id = ANY($1::uuid[]) +SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source, song_id, song_key FROM tracks WHERE id = ANY($1::uuid[]) ` // Batched lookup used by /api/library/sync to hydrate upsert payloads @@ -346,6 +350,8 @@ func (q *Queries) GetTracksByIDs(ctx context.Context, dollar_1 []pgtype.UUID) ([ &i.TagReadVersion, &i.MissingSince, &i.MbidSource, + &i.SongID, + &i.SongKey, ); err != nil { return nil, err } @@ -358,7 +364,7 @@ func (q *Queries) GetTracksByIDs(ctx context.Context, dollar_1 []pgtype.UUID) ([ } const listArtistTracksForUser = `-- name: ListArtistTracksForUser :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, t.song_id, t.song_key, albums.title AS album_title, artists.name AS artist_name FROM tracks t @@ -419,6 +425,8 @@ func (q *Queries) ListArtistTracksForUser(ctx context.Context, arg ListArtistTra &i.Track.TagReadVersion, &i.Track.MissingSince, &i.Track.MbidSource, + &i.Track.SongID, + &i.Track.SongKey, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -519,7 +527,7 @@ func (q *Queries) ListMissingTracks(ctx context.Context, arg ListMissingTracksPa } const listRandomTracksForUser = `-- name: ListRandomTracksForUser :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, t.missing_since, t.mbid_source, t.song_id, t.song_key, albums.title AS album_title, artists.name AS artist_name FROM tracks t @@ -577,6 +585,8 @@ func (q *Queries) ListRandomTracksForUser(ctx context.Context, arg ListRandomTra &i.Track.TagReadVersion, &i.Track.MissingSince, &i.Track.MbidSource, + &i.Track.SongID, + &i.Track.SongKey, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -709,7 +719,7 @@ func (q *Queries) ListTrackPathsForReconcile(ctx context.Context) ([]ListTrackPa } const listTracksByAlbum = `-- name: ListTracksByAlbum :many -SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source FROM tracks +SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source, song_id, song_key FROM tracks WHERE album_id = $1 AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -756,6 +766,8 @@ func (q *Queries) ListTracksByAlbum(ctx context.Context, arg ListTracksByAlbumPa &i.TagReadVersion, &i.MissingSince, &i.MbidSource, + &i.SongID, + &i.SongKey, ); err != nil { return nil, err } @@ -829,7 +841,7 @@ func (q *Queries) MarkTracksMissing(ctx context.Context, ids []pgtype.UUID) (int } const searchTracks = `-- name: SearchTracks :many -SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source FROM tracks +SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source, song_id, song_key FROM tracks WHERE title ILIKE '%' || $1::text || '%' AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -883,6 +895,8 @@ func (q *Queries) SearchTracks(ctx context.Context, arg SearchTracksParams) ([]T &i.TagReadVersion, &i.MissingSince, &i.MbidSource, + &i.SongID, + &i.SongKey, ); err != nil { return nil, err } @@ -939,7 +953,7 @@ ON CONFLICT (file_path) DO UPDATE SET -- next scan can short-circuit them again (#2499). tag_read_version = EXCLUDED.tag_read_version, updated_at = now() -RETURNING id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source +RETURNING id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version, missing_since, mbid_source, song_id, song_key ` type UpsertTrackParams struct { @@ -999,6 +1013,8 @@ func (q *Queries) UpsertTrack(ctx context.Context, arg UpsertTrackParams) (Track &i.TagReadVersion, &i.MissingSince, &i.MbidSource, + &i.SongID, + &i.SongKey, ) return i, err } diff --git a/internal/db/migrations/0076_track_song_link.down.sql b/internal/db/migrations/0076_track_song_link.down.sql new file mode 100644 index 00000000..f81c06f2 --- /dev/null +++ b/internal/db/migrations/0076_track_song_link.down.sql @@ -0,0 +1,3 @@ +DROP INDEX IF EXISTS tracks_song_key_idx; +ALTER TABLE tracks DROP COLUMN IF EXISTS song_key; +ALTER TABLE tracks DROP COLUMN IF EXISTS song_id; diff --git a/internal/db/migrations/0076_track_song_link.up.sql b/internal/db/migrations/0076_track_song_link.up.sql new file mode 100644 index 00000000..b314512c --- /dev/null +++ b/internal/db/migrations/0076_track_song_link.up.sql @@ -0,0 +1,15 @@ +-- 0076_track_song_link.up.sql — the same song on several releases counts as +-- one song (Scribe milestone #498, #5438; operator decision D-b). +-- +-- A single and the album it came from each hold the song, and Lidarr keeps +-- both: each fulfils its own release. Minstrel keeps both files and links +-- them, so a like on one is a like on the song, and a mix or radio picks the +-- song once rather than once per release. +-- +-- song_id is set only on a linked track, to the song_key the link chose. +-- song_key is what everything reads: the track's own id until it is linked. +-- No foreign key: the track whose id names a song may itself be merged away +-- later, and the key still groups the copies left. +ALTER TABLE tracks ADD COLUMN song_id uuid; +ALTER TABLE tracks ADD COLUMN song_key uuid GENERATED ALWAYS AS (COALESCE(song_id, id)) STORED; +CREATE INDEX tracks_song_key_idx ON tracks (song_key); diff --git a/internal/db/queries/likes.sql b/internal/db/queries/likes.sql index 5a8b76f5..eedabc3b 100644 --- a/internal/db/queries/likes.sql +++ b/internal/db/queries/likes.sql @@ -1,20 +1,42 @@ --- name: LikeTrack :execrows +-- name: LikeTrack :many +-- A like is on the song (M498 #5438): every copy of it sharing the track's +-- song_key is liked. Returns the tracks newly liked, for the sync log; empty +-- when the song was already liked. INSERT INTO general_likes (user_id, track_id) -VALUES ($1, $2) -ON CONFLICT (user_id, track_id) DO NOTHING; +SELECT sqlc.arg(user_id)::uuid, t.id + FROM tracks t + WHERE t.song_key = (SELECT s.song_key FROM tracks s WHERE s.id = sqlc.arg(track_id)::uuid) +ON CONFLICT (user_id, track_id) DO NOTHING +RETURNING track_id; --- name: UnlikeTrack :exec -DELETE FROM general_likes WHERE user_id = $1 AND track_id = $2; +-- name: UnlikeTrack :many +-- Unlikes the song: every copy sharing the track's song_key. Returns the +-- tracks unliked, for the sync log. +DELETE FROM general_likes + WHERE user_id = sqlc.arg(user_id)::uuid + AND (track_id = sqlc.arg(track_id)::uuid + OR track_id IN (SELECT t.id FROM tracks t + WHERE t.song_key = (SELECT s.song_key FROM tracks s WHERE s.id = sqlc.arg(track_id)::uuid))) +RETURNING track_id; -- name: ListLikedTrackRows :many +-- One row per liked song: of its copies, the one present on disk, then the +-- one liked first. SELECT t.* FROM tracks t -JOIN general_likes l ON l.track_id = t.id -WHERE l.user_id = $1 -ORDER BY l.liked_at DESC +JOIN (SELECT DISTINCT ON (lt.song_key) l.track_id, l.liked_at + FROM general_likes l + JOIN tracks lt ON lt.id = l.track_id + WHERE l.user_id = $1 + ORDER BY lt.song_key, (lt.missing_since IS NOT NULL), l.liked_at, lt.id) l + ON l.track_id = t.id +ORDER BY l.liked_at DESC, t.id LIMIT $2 OFFSET $3; -- name: CountLikedTracks :one -SELECT count(*) FROM general_likes WHERE user_id = $1; +-- Liked songs, each counted once however many releases hold it. +SELECT count(DISTINCT t.song_key) FROM general_likes l + JOIN tracks t ON t.id = l.track_id + WHERE l.user_id = $1; -- name: ListLikedTrackIDs :many SELECT track_id FROM general_likes WHERE user_id = $1 ORDER BY liked_at DESC; diff --git a/internal/db/queries/songs.sql b/internal/db/queries/songs.sql new file mode 100644 index 00000000..d52ba873 --- /dev/null +++ b/internal/db/queries/songs.sql @@ -0,0 +1,55 @@ +-- Song links (M498 #5438): copies of one song on different releases share a +-- song_key (migration 0076). + +-- name: LinkTracksAsSong :one +-- Links the tracks into one song, joining any songs they already belong to. +-- The song keeps the lowest key among them, so linking the same set again +-- changes nothing. Returns that key and how many tracks moved to it. +WITH keys AS ( + SELECT DISTINCT song_key FROM tracks WHERE id = ANY(sqlc.arg(track_ids)::uuid[]) +), target AS ( + SELECT song_key FROM keys ORDER BY song_key LIMIT 1 +), moved AS ( + UPDATE tracks t + SET song_id = (SELECT song_key FROM target) + WHERE t.song_key IN (SELECT song_key FROM keys) + AND t.song_key <> (SELECT song_key FROM target) + RETURNING t.id +) +SELECT (SELECT song_key FROM target)::uuid AS song_key, + (SELECT count(*) FROM moved)::bigint AS moved; + +-- name: ShareSongLikes :many +-- A like on any copy of the song becomes a like on every copy, dated the +-- earliest. Returns the likes added, for the sync log. +INSERT INTO general_likes (user_id, track_id, liked_at) +SELECT l.user_id, t.id, min(l.liked_at) + FROM general_likes l + JOIN tracks lt ON lt.id = l.track_id + JOIN tracks t ON t.song_key = lt.song_key + WHERE lt.song_key = sqlc.arg(song_key)::uuid + GROUP BY l.user_id, t.id +ON CONFLICT (user_id, track_id) DO NOTHING +RETURNING user_id, track_id; + +-- name: UnlinkDuplicateGroupSongs :exec +-- "Not the same song": the group's copies stop sharing a song. Likes already +-- shared stay where they are. +UPDATE tracks SET song_id = NULL + WHERE song_id IS NOT NULL + AND id IN (SELECT track_id FROM duplicate_group_members WHERE group_id = sqlc.arg(group_id)); + +-- name: ListTrackSongKeys :many +SELECT id, song_key FROM tracks WHERE id = ANY(sqlc.arg(ids)::uuid[]); + +-- name: MergeInheritSongLink :exec +-- A merge keeps the removed copy's song link: when the copy being removed was +-- linked to copies on other releases and the survivor was not, the survivor +-- joins that song. +UPDATE tracks s + SET song_id = l.song_key + FROM tracks l + WHERE s.id = sqlc.arg(survivor_id)::uuid + AND l.id = sqlc.arg(loser_id)::uuid + AND s.song_id IS NULL + AND l.song_key <> l.id; diff --git a/internal/library/duplicate_merge.go b/internal/library/duplicate_merge.go index fc2680e4..aa5e411b 100644 --- a/internal/library/duplicate_merge.go +++ b/internal/library/duplicate_merge.go @@ -269,6 +269,9 @@ func foldTrackInto( if err := tq.MergeInheritTrackMbid(ctx, dbq.MergeInheritTrackMbidParams{SurvivorID: survivorID, LoserID: loserID}); err != nil { return f, fmt.Errorf("inherit recording mbid: %w", err) } + if err := tq.MergeInheritSongLink(ctx, dbq.MergeInheritSongLinkParams{SurvivorID: survivorID, LoserID: loserID}); err != nil { + return f, fmt.Errorf("inherit song link: %w", err) + } // Everything the loser carried now sits on the survivor, so the CASCADE // this delete sets off has nothing left to destroy. diff --git a/internal/library/duplicate_resolve.go b/internal/library/duplicate_resolve.go index 604be858..c16520ab 100644 --- a/internal/library/duplicate_resolve.go +++ b/internal/library/duplicate_resolve.go @@ -89,6 +89,8 @@ type DuplicateResolveResult struct { Merged int MergeFailed int ReleaseChanges []ReleaseChange + // SongsLinked counts cross-release groups newly linked as one song. + SongsLinked int } // LidarrPathKey is the part of a path Minstrel and Lidarr agree on: the last @@ -177,7 +179,18 @@ func ResolveDuplicates( return res, err } - if !act || !res.LidarrConsulted { + if !act { + return res, nil + } + + // Linking removes nothing and needs nothing from Lidarr. + linked, err := linkCrossRelease(ctx, pool, logger, groups) + res.SongsLinked = linked + if err != nil { + return res, err + } + + if !res.LidarrConsulted { return res, nil } @@ -222,6 +235,66 @@ func ResolveDuplicates( return res, nil } +// linkCrossRelease links each cross-release group's copies as one song +// (#5438): a like on one is a like on all, and the Liked list, shuffle and the +// mixes count the song once. Linking an already-linked group changes nothing, +// so this runs every pass; likes are shared only when a link is new, since a +// like made after it reaches every copy as it is made. +func linkCrossRelease(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, groups []*resolveGroup) (int, error) { + linked := 0 + for _, g := range groups { + if g.class != ClassCrossRelease { + continue + } + if ctx.Err() != nil { + return linked, ctx.Err() + } + ids := make([]pgtype.UUID, len(g.members)) + for i, m := range g.members { + ids[i] = m.TrackID + } + moved, err := linkSong(ctx, pool, ids) + if err != nil { + logger.Warn("duplicate resolve: song link failed", "group_id", syncpkg.FormatUUID(g.id), "err", err) + continue + } + if moved { + linked++ + } + } + return linked, nil +} + +// linkSong links the tracks as one song and, when that changed anything, +// shares their likes across the song and logs each added like for sync. +func linkSong(ctx context.Context, pool *pgxpool.Pool, ids []pgtype.UUID) (bool, error) { + tx, err := pool.Begin(ctx) + if err != nil { + return false, err + } + defer func() { _ = tx.Rollback(ctx) }() + tq := dbq.New(tx) + link, err := tq.LinkTracksAsSong(ctx, ids) + if err != nil { + return false, fmt.Errorf("link: %w", err) + } + if link.Moved == 0 { + return false, nil + } + added, err := tq.ShareSongLikes(ctx, link.SongKey) + if err != nil { + return false, fmt.Errorf("share likes: %w", err) + } + likeIDs := make([]string, len(added)) + for i, a := range added { + likeIDs[i] = syncpkg.EncodeLikeID(syncpkg.FormatUUID(a.UserID), syncpkg.FormatUUID(a.TrackID)) + } + if err := syncpkg.LogChanges(ctx, tx, syncpkg.EntityLikeTrack, likeIDs, syncpkg.OpUpsert); err != nil { + return false, fmt.Errorf("log shared likes: %w", err) + } + return true, tx.Commit(ctx) +} + func foldResolveGroups(rows []dbq.ListDuplicateGroupsForResolveRow) []*resolveGroup { var groups []*resolveGroup for _, r := range rows { @@ -250,7 +323,7 @@ func resolveNote(g *resolveGroup) string { return "Lidarr tracks every copy as its own track, so removing one would make Lidarr download it again." } case ClassCrossRelease: - return "Each copy belongs to its own release, and Lidarr keeps each one. Nothing is removed." + return "Each copy belongs to its own release, and Lidarr keeps each one. Nothing is removed; they count as one song." case ClassMismatch: return "Identical audio filed under different titles: one file carries another song's tags." } @@ -674,5 +747,5 @@ func (w *DuplicateResolveWorker) tickOnce(ctx context.Context) { } w.logger.Info("duplicate resolve complete", "groups", res.Groups, "lidarr", res.LidarrConsulted, "merged", res.Merged, - "merge_failed", res.MergeFailed, "release_changes", len(res.ReleaseChanges)) + "merge_failed", res.MergeFailed, "release_changes", len(res.ReleaseChanges), "songs_linked", res.SongsLinked) } diff --git a/internal/library/song_link_test.go b/internal/library/song_link_test.go new file mode 100644 index 00000000..97bf3aab --- /dev/null +++ b/internal/library/song_link_test.go @@ -0,0 +1,208 @@ +package library + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/dbtest" +) + +// songLinkFixture is the single and the album it came from: one song on two +// releases, in one pending acoustic group, and a user who liked the single. +type songLinkFixture struct { + single, albumCut dbq.Track + groupID pgtype.UUID + user dbq.User +} + +func newSongLinkFixture(t *testing.T, pool *pgxpool.Pool) songLinkFixture { + t.Helper() + ctx := context.Background() + q := dbq.New(pool) + exec := func(sql string, args ...any) { + t.Helper() + if _, err := pool.Exec(ctx, sql, args...); err != nil { + t.Fatalf("exec %q: %v", sql, err) + } + } + artist, err := q.UpsertArtist(ctx, dbq.UpsertArtistParams{Name: "Link Artist", SortName: "Link Artist"}) + if err != nil { + t.Fatal(err) + } + track := func(album, path string) dbq.Track { + t.Helper() + a, err := q.UpsertAlbum(ctx, dbq.UpsertAlbumParams{Title: album, SortTitle: album, ArtistID: artist.ID}) + if err != nil { + t.Fatal(err) + } + tr, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: "Feel Good Inc.", AlbumID: a.ID, ArtistID: artist.ID, + DurationMs: 1000, FilePath: path, FileSize: 100, FileFormat: "mp3", + }) + if err != nil { + t.Fatal(err) + } + return tr + } + f := songLinkFixture{ + single: track("Feel Good Inc. (single)", "/music/Link Artist/Feel Good Inc/01 Feel Good Inc.mp3"), + albumCut: track("Demon Days", "/music/Link Artist/Demon Days/06 Feel Good Inc.mp3"), + } + f.user, err = q.CreateUser(ctx, dbq.CreateUserParams{ + Username: dbtest.TestUserPrefix + "song-link", PasswordHash: "x", ApiTokenHash: "song-link-token", + }) + if err != nil { + t.Fatal(err) + } + if err := pool.QueryRow(ctx, + `INSERT INTO duplicate_groups (member_key, tier) VALUES ('song-link', 'acoustic') RETURNING id`, + ).Scan(&f.groupID); err != nil { + t.Fatal(err) + } + exec(`INSERT INTO duplicate_group_members (group_id, track_id) VALUES ($1, $2), ($1, $3)`, f.groupID, f.single.ID, f.albumCut.ID) + exec(`INSERT INTO general_likes (user_id, track_id) VALUES ($1, $2)`, f.user.ID, f.single.ID) + return f +} + +// A cross-release group becomes one song: its copies share a song key, the +// like on the single reaches the album cut (and is logged for sync), the Liked +// list shows the song once, a like or unlike on either copy reaches both, and +// "not the same song" undoes the link. +func TestCrossReleaseCopiesCountAsOneSong_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + f := newSongLinkFixture(t, pool) + + res, err := ResolveDuplicates(ctx, pool, nil, "", nil, true) + if err != nil { + t.Fatal(err) + } + if res.Classes[ClassCrossRelease] != 1 || res.SongsLinked != 1 { + t.Fatalf("classes %v, linked %d; want one cross-release group linked", res.Classes, res.SongsLinked) + } + keys := songKeys(t, q, f.single.ID, f.albumCut.ID) + if keys[0] != keys[1] { + t.Fatalf("song keys differ after linking: %v", keys) + } + + liked, err := q.ListLikedTrackIDs(ctx, f.user.ID) + if err != nil { + t.Fatal(err) + } + if len(liked) != 2 { + t.Errorf("liked track ids = %d, want both copies", len(liked)) + } + var logged int + if err := pool.QueryRow(ctx, + `SELECT count(*) FROM library_changes WHERE entity_type = 'like_track' AND op = 'upsert'`, + ).Scan(&logged); err != nil { + t.Fatal(err) + } + if logged != 1 { + t.Errorf("logged %d shared likes, want 1 (the album cut)", logged) + } + if n, err := q.CountLikedTracks(ctx, f.user.ID); err != nil || n != 1 { + t.Errorf("CountLikedTracks = %d (%v), want 1 song", n, err) + } + rows, err := q.ListLikedTrackRows(ctx, dbq.ListLikedTrackRowsParams{UserID: f.user.ID, Limit: 10}) + if err != nil { + t.Fatal(err) + } + if len(rows) != 1 { + t.Errorf("Liked list has %d rows, want the song once", len(rows)) + } + + // A second pass changes nothing. + if res, err := ResolveDuplicates(ctx, pool, nil, "", nil, true); err != nil || res.SongsLinked != 0 { + t.Errorf("second pass linked %d (%v), want 0", res.SongsLinked, err) + } + + unliked, err := q.UnlikeTrack(ctx, dbq.UnlikeTrackParams{UserID: f.user.ID, TrackID: f.albumCut.ID}) + if err != nil { + t.Fatal(err) + } + if len(unliked) != 2 { + t.Errorf("unlike removed %d likes, want both copies", len(unliked)) + } + added, err := q.LikeTrack(ctx, dbq.LikeTrackParams{UserID: f.user.ID, TrackID: f.single.ID}) + if err != nil { + t.Fatal(err) + } + if len(added) != 2 { + t.Errorf("like added %d, want both copies", len(added)) + } + if again, err := q.LikeTrack(ctx, dbq.LikeTrackParams{UserID: f.user.ID, TrackID: f.single.ID}); err != nil || len(again) != 0 { + t.Errorf("repeat like added %d (%v), want 0", len(again), err) + } + + if err := q.UnlinkDuplicateGroupSongs(ctx, f.groupID); err != nil { + t.Fatal(err) + } + keys = songKeys(t, q, f.single.ID, f.albumCut.ID) + if keys[0] == keys[1] { + t.Error("song keys still shared after unlinking") + } +} + +// Without the operator's permission the resolver links nothing. +func TestCrossReleaseLinkNeedsAutoResolve_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + f := newSongLinkFixture(t, pool) + if _, err := ResolveDuplicates(ctx, pool, nil, "", nil, false); err != nil { + t.Fatal(err) + } + keys := songKeys(t, q, f.single.ID, f.albumCut.ID) + if keys[0] == keys[1] { + t.Error("linked with auto-resolve off") + } +} + +// A merge keeps the removed copy's link to the song on another release. +func TestMergeInheritsSongLink_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + f := newSongLinkFixture(t, pool) + if _, err := q.LinkTracksAsSong(ctx, []pgtype.UUID{f.single.ID, f.albumCut.ID}); err != nil { + t.Fatal(err) + } + // A third, unlinked copy of the album cut stands in for a survivor. + other, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: "Feel Good Inc.", AlbumID: f.albumCut.AlbumID, ArtistID: f.albumCut.ArtistID, + DurationMs: 1000, FilePath: "/music/Link Artist/Demon Days/06 Feel Good Inc (1).mp3", FileSize: 90, FileFormat: "mp3", + }) + if err != nil { + t.Fatal(err) + } + if err := q.MergeInheritSongLink(ctx, dbq.MergeInheritSongLinkParams{SurvivorID: other.ID, LoserID: f.albumCut.ID}); err != nil { + t.Fatal(err) + } + keys := songKeys(t, q, other.ID, f.single.ID) + if keys[0] != keys[1] { + t.Error("survivor did not join the song") + } +} + +func songKeys(t *testing.T, q *dbq.Queries, ids ...pgtype.UUID) []pgtype.UUID { + t.Helper() + rows, err := q.ListTrackSongKeys(context.Background(), ids) + if err != nil { + t.Fatal(err) + } + byID := map[pgtype.UUID]pgtype.UUID{} + for _, r := range rows { + byID[r.ID] = r.SongKey + } + out := make([]pgtype.UUID, len(ids)) + for i, id := range ids { + out[i] = byID[id] + } + return out +} diff --git a/internal/playlists/system.go b/internal/playlists/system.go index 420c0811..f670977c 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -1070,6 +1070,10 @@ func insertSystemPlaylist(ctx context.Context, qtx *dbq.Queries, userID pgtype.U return pgtype.UUID{}, fmt.Errorf("insert playlist row: %w", err) } + tracks, err = oneCopyPerSong(ctx, qtx, tracks) + if err != nil { + return pgtype.UUID{}, err + } for _, t := range tracks { var pickKind *string if t.PickKind != "" { @@ -1096,6 +1100,34 @@ func insertSystemPlaylist(ctx context.Context, qtx *dbq.Queries, userID pgtype.U return p.ID, nil } +// oneCopyPerSong keeps the first of a song's copies on different releases +// (M498 #5438): a mix holds the song once, in its best-ranked place. +func oneCopyPerSong(ctx context.Context, qtx *dbq.Queries, tracks []rankedCandidate) ([]rankedCandidate, error) { + ids := make([]pgtype.UUID, len(tracks)) + for i, t := range tracks { + ids[i] = t.TrackID + } + rows, err := qtx.ListTrackSongKeys(ctx, ids) + if err != nil { + return nil, fmt.Errorf("song keys: %w", err) + } + songOf := make(map[pgtype.UUID]pgtype.UUID, len(rows)) + for _, r := range rows { + songOf[r.ID] = r.SongKey + } + seen := make(map[pgtype.UUID]bool, len(tracks)) + out := tracks[:0:0] + for _, t := range tracks { + song, ok := songOf[t.TrackID] + if ok && seen[song] { + continue + } + seen[song] = true + out = append(out, t) + } + return out, nil +} + // uuidStringPL renders a pgtype.UUID as the canonical 8-4-4-4-12 form. // (pgtype.UUID's String method exists on some pgx versions but not others; // also the playlists package needs this independent of test helpers.) diff --git a/internal/recommendation/shuffle.go b/internal/recommendation/shuffle.go index b7033d24..dd33b6d5 100644 --- a/internal/recommendation/shuffle.go +++ b/internal/recommendation/shuffle.go @@ -104,11 +104,24 @@ func Shuffle( deferred := make([]Candidate, 0, len(scored)-limit) artistCount := map[pgtype.UUID]int{} albumCount := map[pgtype.UUID]int{} + // One song on several releases is played once (M498 #5438): after its + // best-scoring copy, the others are dropped, not held back. + songs := map[pgtype.UUID]bool{} + repeat := func(c Candidate) bool { return c.Track.SongKey.Valid && songs[c.Track.SongKey] } + take := func(c Candidate) { + if c.Track.SongKey.Valid { + songs[c.Track.SongKey] = true + } + out = append(out, c) + } for _, s := range scored { if len(out) == limit { break } + if repeat(s.c) { + continue + } overArtist := caps.MaxPerArtist > 0 && artistCount[s.c.Track.ArtistID] >= caps.MaxPerArtist overAlbum := caps.MaxPerAlbum > 0 && albumCount[s.c.Track.AlbumID] >= caps.MaxPerAlbum if overArtist || overAlbum { @@ -118,7 +131,7 @@ func Shuffle( } artistCount[s.c.Track.ArtistID]++ albumCount[s.c.Track.AlbumID]++ - out = append(out, s.c) + take(s.c) } // Pass two: the caps could not fill the request, so relax them rather @@ -128,7 +141,10 @@ func Shuffle( if len(out) == limit { break } - out = append(out, c) + if repeat(c) { + continue + } + take(c) } return out } diff --git a/internal/recommendation/shuffle_test.go b/internal/recommendation/shuffle_test.go index 8b54107b..3406da13 100644 --- a/internal/recommendation/shuffle_test.go +++ b/internal/recommendation/shuffle_test.go @@ -84,3 +84,30 @@ func titles(cs []Candidate) []string { // pure tests; the Track.Title field is the human-readable handle. Track ID // is left as zero pgtype.UUID and never compared. var _ pgtype.UUID + +// One song on two releases is played once, in its better-scoring copy's place, +// and a capped copy held back for pass two does not slip back in either. +func TestShuffle_OneCopyPerSong(t *testing.T) { + song := func(c Candidate, key string) Candidate { + _ = c.Track.SongKey.Scan("00000000-0000-0000-0000-" + key) + return c + } + album := pgtype.UUID{Bytes: [16]byte{9}, Valid: true} + single := song(cand("000000000001", ScoringInputs{IsGeneralLiked: true}), "00000000000a") + albumCut := song(cand("000000000002", ScoringInputs{}), "00000000000a") + other := cand("000000000003", ScoringInputs{}) + single.Track.AlbumID, albumCut.Track.AlbumID = album, pgtype.UUID{Bytes: [16]byte{8}, Valid: true} + + out := Shuffle([]Candidate{albumCut, single, other}, defaultWeights(), time.Now(), fixedRNG(0.5), 10, DiversityCaps{}) + if got := titles(out); len(got) != 2 || got[0] != "000000000001" { + t.Errorf("got %v, want the liked single once and the other song", got) + } + + // The single is capped out of pass one; pass two takes one copy, not both. + capped := cand("000000000004", ScoringInputs{IsGeneralLiked: true}) + capped.Track.AlbumID = album + out = Shuffle([]Candidate{capped, single, albumCut}, defaultWeights(), time.Now(), fixedRNG(0.5), 3, DiversityCaps{MaxPerAlbum: 1}) + if len(out) != 2 { + t.Errorf("got %v, want two tracks: one copy of the song", titles(out)) + } +} diff --git a/internal/subsonic/star.go b/internal/subsonic/star.go index 7d717a3a..2a9fcc12 100644 --- a/internal/subsonic/star.go +++ b/internal/subsonic/star.go @@ -4,6 +4,7 @@ import ( "context" "errors" "net/http" + "slices" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" @@ -39,12 +40,13 @@ func (m *mediaHandlers) handleStar(w http.ResponseWriter, r *http.Request) { return } if trackID.Valid { - rows, err := q.LikeTrack(r.Context(), dbq.LikeTrackParams{UserID: user.ID, TrackID: trackID}) + // Stars the song: every copy of it (M498 #5438). + liked, err := q.LikeTrack(r.Context(), dbq.LikeTrackParams{UserID: user.ID, TrackID: trackID}) if err != nil { WriteFail(w, r, ErrGeneric, "Could not star track") return } - if rows == 1 { + if slices.Contains(liked, trackID) { _ = playevents.CaptureContextualLikeIfPlaying(r.Context(), q, user.ID, trackID, m.logger) } } @@ -75,7 +77,7 @@ func (m *mediaHandlers) handleUnstar(w http.ResponseWriter, r *http.Request) { params := r.URL.Query() q := dbq.New(m.pool) if t, ok := parseUUID(params.Get("id")); ok { - _ = q.UnlikeTrack(r.Context(), dbq.UnlikeTrackParams{UserID: user.ID, TrackID: t}) + _, _ = q.UnlikeTrack(r.Context(), dbq.UnlikeTrackParams{UserID: user.ID, TrackID: t}) _ = playevents.SoftDeleteContextualLikes(r.Context(), q, user.ID, t) } if a, ok := parseUUID(params.Get("albumId")); ok { diff --git a/web/src/routes/admin/duplicates/+page.svelte b/web/src/routes/admin/duplicates/+page.svelte index 834f9623..7a1dca40 100644 --- a/web/src/routes/admin/duplicates/+page.svelte +++ b/web/src/routes/admin/duplicates/+page.svelte @@ -30,7 +30,8 @@ // purpose. A third tab lists what the resolver did. // // Dismissing a group says "these are not duplicates", and the sweep will not - // propose that set again. Merging (#3911) keeps one copy, moves the others' + // propose that set again. On the cross-release tab it reads "Not the same + // song" and also undoes the link that made the copies one song (#5438). Merging (#3911) keeps one copy, moves the others' // likes, plays and playlist entries onto it, and deletes their files, so it // asks twice. The server refuses to remove a copy Lidarr uses. @@ -337,7 +338,8 @@

No songs repeated across releases.

A single and the album it's on, or two editions of one album, show here. Each copy - belongs to its own release, so none is removed. + belongs to its own release, so none is removed. They count as one song: a like on one + is a like on all, and a mix plays it once.

{:else}

Nothing needs review.

@@ -366,7 +368,7 @@ disabled={dismissing === group.id || merging === group.id} class="rounded-md border border-border px-3 py-1.5 text-sm text-text-secondary hover:bg-surface-hover hover:text-text-primary disabled:opacity-50" > - Not duplicates + {tab === 'cross_release' ? 'Not the same song' : 'Not duplicates'}