fix: same-titled albums of two release groups are two albums, and a missing track with no disc is on disc 1
release / govulncheck (push) Successful in 16s
release / web (push) Successful in 1m23s
release / go (push) Successful in 1m30s
release / integration (push) Successful in 5m0s
release / android (push) Successful in 5m1s
release / Build signed APK (releases and dev) (push) Successful in 5m9s
release / Attach APK to the Release (tag releases only) (push) Skipped
release / Build + push container image (push) Successful in 29s
release / Verify release artifacts (tag releases only) (push) Skipped
release / govulncheck (push) Successful in 16s
release / web (push) Successful in 1m23s
release / go (push) Successful in 1m30s
release / integration (push) Successful in 5m0s
release / android (push) Successful in 5m1s
release / Build signed APK (releases and dev) (push) Successful in 5m9s
release / Attach APK to the Release (tag releases only) (push) Skipped
release / Build + push container image (push) Successful in 29s
release / Verify release artifacts (tag releases only) (push) Skipped
The scanner found an album by artist and title alone, so Weezer's six self-titled albums (1994–2026) were one 118-track row and their merges, wants and upgrades counted them as one. A same-named row whose release group is known and differs from the file's is now another album; tagReadVersion 7 re-reads the library so existing tracks move to their own rows. WantHoldings compared discs with IS NOT DISTINCT FROM, so a missing track 6 with no disc was never covered by a present disc 1 track 6. Air's Love 2 stayed wanted and was downloaded again for nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -72,17 +72,27 @@ func (q *Queries) DeleteAlbumIfEmpty(ctx context.Context, id pgtype.UUID) (Delet
|
|||||||
}
|
}
|
||||||
|
|
||||||
const getAlbumByArtistAndTitle = `-- name: GetAlbumByArtistAndTitle :one
|
const getAlbumByArtistAndTitle = `-- name: GetAlbumByArtistAndTitle :one
|
||||||
SELECT id, title, sort_title, artist_id, release_date, mbid, cover_art_path, created_at, updated_at, cover_art_source, cover_art_sources_version, release_group_mbid FROM albums WHERE artist_id = $1 AND title = $2 LIMIT 1
|
SELECT id, title, sort_title, artist_id, release_date, mbid, cover_art_path, created_at, updated_at, cover_art_source, cover_art_sources_version, release_group_mbid FROM albums
|
||||||
|
WHERE artist_id = $1 AND title = $2
|
||||||
|
AND ($3::text IS NULL OR release_group_mbid IS NULL
|
||||||
|
OR release_group_mbid = $3)
|
||||||
|
ORDER BY (release_group_mbid IS NOT DISTINCT FROM $3) DESC, id
|
||||||
|
LIMIT 1
|
||||||
`
|
`
|
||||||
|
|
||||||
type GetAlbumByArtistAndTitleParams struct {
|
type GetAlbumByArtistAndTitleParams struct {
|
||||||
ArtistID pgtype.UUID
|
ArtistID pgtype.UUID
|
||||||
Title string
|
Title string
|
||||||
|
ReleaseGroupMbid *string
|
||||||
}
|
}
|
||||||
|
|
||||||
// Scanner uses this for the no-mbid dedupe path: resolve-or-create.
|
// Scanner uses this for the no-mbid dedupe path: resolve-or-create. An album
|
||||||
|
// of the same name in another release group is a different album (Weezer's
|
||||||
|
// six self-titled albums shared one row until 2026-10-20), so a row whose
|
||||||
|
// group is known and differs is not returned. A row of the file's own group
|
||||||
|
// comes first, then one with no group yet.
|
||||||
func (q *Queries) GetAlbumByArtistAndTitle(ctx context.Context, arg GetAlbumByArtistAndTitleParams) (Album, error) {
|
func (q *Queries) GetAlbumByArtistAndTitle(ctx context.Context, arg GetAlbumByArtistAndTitleParams) (Album, error) {
|
||||||
row := q.db.QueryRow(ctx, getAlbumByArtistAndTitle, arg.ArtistID, arg.Title)
|
row := q.db.QueryRow(ctx, getAlbumByArtistAndTitle, arg.ArtistID, arg.Title, arg.ReleaseGroupMbid)
|
||||||
var i Album
|
var i Album
|
||||||
err := row.Scan(
|
err := row.Scan(
|
||||||
&i.ID,
|
&i.ID,
|
||||||
|
|||||||
@@ -1572,11 +1572,13 @@ SELECT a.id AS album_id,
|
|||||||
COALESCE(bool_or(t.missing_since IS NOT NULL AND t.missing_since < $1::timestamptz), false)::bool AS missing_before,
|
COALESCE(bool_or(t.missing_since IS NOT NULL AND t.missing_since < $1::timestamptz), false)::bool AS missing_before,
|
||||||
-- Missing tracks no track on disk stands in for, by position or
|
-- Missing tracks no track on disk stands in for, by position or
|
||||||
-- title. A missing row whose song is back at a new path (#5245) is
|
-- title. A missing row whose song is back at a new path (#5245) is
|
||||||
-- covered; a lost bonus track is not.
|
-- covered; a lost bonus track is not. A track with no disc number is
|
||||||
|
-- on disc 1: Air's Love 2 stayed wanted, and was downloaded again for
|
||||||
|
-- nothing, over a missing track 6 with no disc beside a present 1/6.
|
||||||
COUNT(*) FILTER (WHERE t.missing_since IS NOT NULL AND NOT EXISTS (
|
COUNT(*) FILTER (WHERE t.missing_since IS NOT NULL AND NOT EXISTS (
|
||||||
SELECT 1 FROM tracks p
|
SELECT 1 FROM tracks p
|
||||||
WHERE p.album_id = a.id AND p.missing_since IS NULL
|
WHERE p.album_id = a.id AND p.missing_since IS NULL
|
||||||
AND ((p.track_number = t.track_number AND p.disc_number IS NOT DISTINCT FROM t.disc_number)
|
AND ((p.track_number = t.track_number AND COALESCE(p.disc_number, 1) = COALESCE(t.disc_number, 1))
|
||||||
OR lower(p.title) = lower(t.title))))::int AS uncovered_missing
|
OR lower(p.title) = lower(t.title))))::int AS uncovered_missing
|
||||||
FROM albums a
|
FROM albums a
|
||||||
JOIN tracks t ON t.album_id = a.id
|
JOIN tracks t ON t.album_id = a.id
|
||||||
|
|||||||
@@ -36,8 +36,17 @@ WHERE albums.artist_id = $1
|
|||||||
ORDER BY release_date NULLS LAST, sort_title;
|
ORDER BY release_date NULLS LAST, sort_title;
|
||||||
|
|
||||||
-- name: GetAlbumByArtistAndTitle :one
|
-- name: GetAlbumByArtistAndTitle :one
|
||||||
-- Scanner uses this for the no-mbid dedupe path: resolve-or-create.
|
-- Scanner uses this for the no-mbid dedupe path: resolve-or-create. An album
|
||||||
SELECT * FROM albums WHERE artist_id = $1 AND title = $2 LIMIT 1;
|
-- of the same name in another release group is a different album (Weezer's
|
||||||
|
-- six self-titled albums shared one row until 2026-10-20), so a row whose
|
||||||
|
-- group is known and differs is not returned. A row of the file's own group
|
||||||
|
-- comes first, then one with no group yet.
|
||||||
|
SELECT * FROM albums
|
||||||
|
WHERE artist_id = sqlc.arg(artist_id) AND title = sqlc.arg(title)
|
||||||
|
AND (sqlc.narg(release_group_mbid)::text IS NULL OR release_group_mbid IS NULL
|
||||||
|
OR release_group_mbid = sqlc.narg(release_group_mbid))
|
||||||
|
ORDER BY (release_group_mbid IS NOT DISTINCT FROM sqlc.narg(release_group_mbid)) DESC, id
|
||||||
|
LIMIT 1;
|
||||||
|
|
||||||
-- name: ListAlbumsByArtist :many
|
-- name: ListAlbumsByArtist :many
|
||||||
SELECT * FROM albums WHERE artist_id = $1 ORDER BY release_date NULLS LAST, sort_title;
|
SELECT * FROM albums WHERE artist_id = $1 ORDER BY release_date NULLS LAST, sort_title;
|
||||||
|
|||||||
@@ -146,11 +146,13 @@ SELECT a.id AS album_id,
|
|||||||
COALESCE(bool_or(t.missing_since IS NOT NULL AND t.missing_since < sqlc.arg(since)::timestamptz), false)::bool AS missing_before,
|
COALESCE(bool_or(t.missing_since IS NOT NULL AND t.missing_since < sqlc.arg(since)::timestamptz), false)::bool AS missing_before,
|
||||||
-- Missing tracks no track on disk stands in for, by position or
|
-- Missing tracks no track on disk stands in for, by position or
|
||||||
-- title. A missing row whose song is back at a new path (#5245) is
|
-- title. A missing row whose song is back at a new path (#5245) is
|
||||||
-- covered; a lost bonus track is not.
|
-- covered; a lost bonus track is not. A track with no disc number is
|
||||||
|
-- on disc 1: Air's Love 2 stayed wanted, and was downloaded again for
|
||||||
|
-- nothing, over a missing track 6 with no disc beside a present 1/6.
|
||||||
COUNT(*) FILTER (WHERE t.missing_since IS NOT NULL AND NOT EXISTS (
|
COUNT(*) FILTER (WHERE t.missing_since IS NOT NULL AND NOT EXISTS (
|
||||||
SELECT 1 FROM tracks p
|
SELECT 1 FROM tracks p
|
||||||
WHERE p.album_id = a.id AND p.missing_since IS NULL
|
WHERE p.album_id = a.id AND p.missing_since IS NULL
|
||||||
AND ((p.track_number = t.track_number AND p.disc_number IS NOT DISTINCT FROM t.disc_number)
|
AND ((p.track_number = t.track_number AND COALESCE(p.disc_number, 1) = COALESCE(t.disc_number, 1))
|
||||||
OR lower(p.title) = lower(t.title))))::int AS uncovered_missing
|
OR lower(p.title) = lower(t.title))))::int AS uncovered_missing
|
||||||
FROM albums a
|
FROM albums a
|
||||||
JOIN tracks t ON t.album_id = a.id
|
JOIN tracks t ON t.album_id = a.id
|
||||||
|
|||||||
@@ -74,7 +74,11 @@ var audioExtensions = map[string]bool{
|
|||||||
// dhowden/tag reads it as 0, and Minstrel's own importer wrote it that
|
// dhowden/tag reads it as 0, and Minstrel's own importer wrote it that
|
||||||
// way, so all 245 FLAC tracks it imported came in with no track or disc
|
// way, so all 245 FLAC tracks it imported came in with no track or disc
|
||||||
// number and their albums lost their order.
|
// number and their albums lost their order.
|
||||||
const tagReadVersion int16 = 6
|
// 7: an album of the same name in another release group gets its own row
|
||||||
|
// (resolveAlbum). Weezer's six self-titled albums were one 118-track row,
|
||||||
|
// so their merges, wants and upgrades all counted as one album; a re-read
|
||||||
|
// files each existing track under its own album.
|
||||||
|
const tagReadVersion int16 = 7
|
||||||
|
|
||||||
type Stats struct {
|
type Stats struct {
|
||||||
Scanned int `json:"scanned"`
|
Scanned int `json:"scanned"`
|
||||||
@@ -562,7 +566,12 @@ func (s *Scanner) resolveArtist(ctx context.Context, q *dbq.Queries, name, mbid
|
|||||||
}
|
}
|
||||||
|
|
||||||
func (s *Scanner) resolveAlbum(ctx context.Context, q *dbq.Queries, artistID pgtype.UUID, title string, year int, mbid, releaseGroupMBID string) (dbq.Album, error) {
|
func (s *Scanner) resolveAlbum(ctx context.Context, q *dbq.Queries, artistID pgtype.UUID, title string, year int, mbid, releaseGroupMBID string) (dbq.Album, error) {
|
||||||
existing, err := q.GetAlbumByArtistAndTitle(ctx, dbq.GetAlbumByArtistAndTitleParams{ArtistID: artistID, Title: title})
|
lookup := dbq.GetAlbumByArtistAndTitleParams{ArtistID: artistID, Title: title}
|
||||||
|
if releaseGroupMBID != "" {
|
||||||
|
rg := releaseGroupMBID
|
||||||
|
lookup.ReleaseGroupMbid = &rg
|
||||||
|
}
|
||||||
|
existing, err := q.GetAlbumByArtistAndTitle(ctx, lookup)
|
||||||
if err == nil {
|
if err == nil {
|
||||||
// Heal the release group the same way. Not unique, so no conflict to
|
// Heal the release group the same way. Not unique, so no conflict to
|
||||||
// handle: every release of a group may carry it.
|
// handle: every release of a group may carry it.
|
||||||
|
|||||||
@@ -385,3 +385,94 @@ func TestScanner_ReleaseGroupMBID_Integration(t *testing.T) {
|
|||||||
t.Errorf("album A release group changed to %v", rg)
|
t.Errorf("album A release group changed to %v", rg)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Two albums of one name by one artist, in different release groups, are two
|
||||||
|
// albums: Weezer's six self-titled albums shared one row until 2026-10-20. A
|
||||||
|
// track of the same group joins its album, and a track filed under the other
|
||||||
|
// album's row by the old lookup moves to its own on the tagReadVersion re-read.
|
||||||
|
func TestScanner_SameTitledAlbumsOfTwoGroupsAreTwoAlbums_Integration(t *testing.T) {
|
||||||
|
if testing.Short() {
|
||||||
|
t.Skip("skipping scanner integration in -short mode")
|
||||||
|
}
|
||||||
|
dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL")
|
||||||
|
if dsn == "" {
|
||||||
|
t.Skip("MINSTREL_TEST_DATABASE_URL not set")
|
||||||
|
}
|
||||||
|
ctx := context.Background()
|
||||||
|
logger := slog.New(slog.NewTextHandler(io.Discard, nil))
|
||||||
|
if err := db.Migrate(dsn, logger); err != nil {
|
||||||
|
t.Fatalf("migrate: %v", err)
|
||||||
|
}
|
||||||
|
pool, err := pgxpool.New(ctx, dsn)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("pool: %v", err)
|
||||||
|
}
|
||||||
|
t.Cleanup(pool.Close)
|
||||||
|
if _, err := pool.Exec(ctx, "TRUNCATE tracks, albums, artists RESTART IDENTITY CASCADE"); err != nil {
|
||||||
|
t.Fatalf("truncate: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
const (
|
||||||
|
blue = "aaaaaaaa-1994-2222-3333-444444444444"
|
||||||
|
green = "bbbbbbbb-2001-2222-3333-444444444444"
|
||||||
|
)
|
||||||
|
root := t.TempDir()
|
||||||
|
tagged := func(rel, title, group string) string {
|
||||||
|
p := filepath.Join(root, rel)
|
||||||
|
writeTestMP3(t, p, map[string]string{
|
||||||
|
"TIT2": title, "TPE1": "Weezer", "TALB": "Weezer",
|
||||||
|
"TXXX": "MusicBrainz Release Group Id\x00" + group,
|
||||||
|
})
|
||||||
|
return p
|
||||||
|
}
|
||||||
|
tagged("Weezer (1994)/01.mp3", "My Name Is Jonas", blue)
|
||||||
|
tagged("Weezer (1994)/02.mp3", "No One Else", blue)
|
||||||
|
greenPath := tagged("Weezer (2001)/01.mp3", "Don't Let Go", green)
|
||||||
|
|
||||||
|
scanner := New(pool, logger, []string{root}, nil)
|
||||||
|
if _, err := scanner.Scan(ctx, nil); err != nil {
|
||||||
|
t.Fatalf("scan: %v", err)
|
||||||
|
}
|
||||||
|
albumsByGroup := func() map[string]int {
|
||||||
|
t.Helper()
|
||||||
|
rows, err := pool.Query(ctx, `SELECT a.release_group_mbid, count(t.id) FROM albums a
|
||||||
|
JOIN tracks t ON t.album_id = a.id WHERE a.title = 'Weezer' GROUP BY a.id, a.release_group_mbid`)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
defer rows.Close()
|
||||||
|
out := map[string]int{}
|
||||||
|
for rows.Next() {
|
||||||
|
var g *string
|
||||||
|
var n int
|
||||||
|
if err := rows.Scan(&g, &n); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if g == nil {
|
||||||
|
t.Fatal("an album with no release group")
|
||||||
|
}
|
||||||
|
out[*g] = n
|
||||||
|
}
|
||||||
|
return out
|
||||||
|
}
|
||||||
|
if got := albumsByGroup(); len(got) != 2 || got[blue] != 2 || got[green] != 1 {
|
||||||
|
t.Fatalf("albums by group = %v, want blue with 2 tracks and green with 1", got)
|
||||||
|
}
|
||||||
|
|
||||||
|
// The green track as the old lookup left it: on the blue album's row,
|
||||||
|
// under an older tag-read version, its own album row gone.
|
||||||
|
if _, err := pool.Exec(ctx, `
|
||||||
|
WITH b AS (SELECT id FROM albums WHERE release_group_mbid = $1)
|
||||||
|
UPDATE tracks SET album_id = (SELECT id FROM b), tag_read_version = 6 WHERE file_path = $2`, blue, greenPath); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if _, err := pool.Exec(ctx, `DELETE FROM albums WHERE release_group_mbid = $1`, green); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if _, err := scanner.Scan(ctx, nil); err != nil {
|
||||||
|
t.Fatalf("rescan: %v", err)
|
||||||
|
}
|
||||||
|
if got := albumsByGroup(); len(got) != 2 || got[blue] != 2 || got[green] != 1 {
|
||||||
|
t.Errorf("after the re-read, albums by group = %v, want the green track on its own album", got)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import (
|
|||||||
"testing"
|
"testing"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
|
"github.com/jackc/pgx/v5/pgtype"
|
||||||
"github.com/jackc/pgx/v5/pgxpool"
|
"github.com/jackc/pgx/v5/pgxpool"
|
||||||
|
|
||||||
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
|
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
|
||||||
@@ -135,3 +136,39 @@ func TestCheckUpgrades_ARipIsReplacedWhateverTheSetting_Integration(t *testing.T
|
|||||||
t.Errorf("a rip: upgrade %q", w.Upgrade)
|
t.Errorf("a rip: upgrade %q", w.Upgrade)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A missing track with no disc number is on disc 1, so a present 1/2 stands in
|
||||||
|
// for a missing "2". Air's Love 2 stayed wanted over one such row and was
|
||||||
|
// downloaded again for nothing (2026-10-20). A missing track no present track
|
||||||
|
// covers still counts.
|
||||||
|
func TestWantHoldings_AMissingTrackWithNoDiscIsOnDiscOne_Integration(t *testing.T) {
|
||||||
|
pool := newPool(t)
|
||||||
|
ctx := context.Background()
|
||||||
|
album := seedMP3s(t, pool, 320, false)
|
||||||
|
if _, err := pool.Exec(ctx, `UPDATE tracks SET disc_number = 1 WHERE album_id = $1`, album.ID); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
q := dbq.New(pool)
|
||||||
|
for _, n := range []int32{2, 3} {
|
||||||
|
num := n
|
||||||
|
tr, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{
|
||||||
|
Title: fmt.Sprintf("Stale %d", n), AlbumID: album.ID, ArtistID: album.ArtistID, TrackNumber: &num,
|
||||||
|
DurationMs: 200000, FilePath: fmt.Sprintf("/music/ballad/stale-%d.mp3", n), FileSize: 1, FileFormat: "mp3",
|
||||||
|
})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if _, err := pool.Exec(ctx, `UPDATE tracks SET missing_since = now() - interval '2 days' WHERE id = $1`, tr.ID); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
rows, err := q.WantHoldings(ctx, dbq.WantHoldingsParams{
|
||||||
|
Since: pgtype.Timestamptz{Time: time.Now(), Valid: true}, GroupMbid: theBallad, AlbumID: album.ID,
|
||||||
|
})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if len(rows) != 1 || rows[0].UncoveredMissing != 1 {
|
||||||
|
t.Fatalf("holdings = %+v, want one album with 1 uncovered missing track (track 3 only)", rows)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user