fix(quick-10): add migration 5 and fix entity cache for composite album key

- Migration 5 rebuilds release_groups with UNIQUE(name, album_artist_credit_id) - Drops and recreates track_metadata VIEW during table rebuild - Temporarily disables FK checks for safe table rebuild - Entity cache now keys by album name + artist credit ID - Update tests to use composite cache keys
2026-03-05 10:25:53 -05:00
parent 999ab967be
commit d43ba7bd0c
3 changed files with 208 additions and 5 deletions
@@ -285,6 +285,18 @@ func runMigrations(
 		}
 	}

+	// Migration 5: rebuild release_groups with composite unique
+	// constraint on (name, album_artist_credit_id) instead of
+	// name alone, so albums with the same name by different
+	// artists are stored as separate rows.
+	if version < 5 {
+		if err := migration5ReleaseGroupCompositeUnique(
+			ctx, db, logger,
+		); err != nil {
+			return err
+		}
+	}
+
 	return nil
 }

@@ -458,6 +470,184 @@ func migration4TrackMetadataView(
 	return nil
 }

+// migration5ReleaseGroupCompositeUnique rebuilds the release_groups
+// table with UNIQUE(name, album_artist_credit_id) instead of
+// UNIQUE(name).  SQLite cannot ALTER a UNIQUE constraint, so we
+// must rebuild the table.
+//
+// SAFETY: Hand-crafted SQL for schema migration.
+func migration5ReleaseGroupCompositeUnique(
+	ctx context.Context,
+	db *sql.DB,
+	logger *slog.Logger,
+) error {
+	logger.Info(
+		"applying migration 5: release_groups composite unique constraint",
+	)
+
+	// Temporarily disable FK checks for table rebuild.
+	if _, err := db.ExecContext(
+		ctx, "PRAGMA foreign_keys = OFF",
+	); err != nil {
+		return fmt.Errorf(
+			"migration 5: could not disable foreign keys: %w",
+			err,
+		)
+	}
+
+	// Drop the track_metadata VIEW that references release_groups
+	// so the table rebuild can proceed without SQLite complaining
+	// about a dangling VIEW reference.
+	if _, err := db.ExecContext(
+		ctx, "DROP VIEW IF EXISTS track_metadata",
+	); err != nil {
+		return fmt.Errorf(
+			"migration 5: could not drop track_metadata VIEW: %w",
+			err,
+		)
+	}
+
+	// Create new table with composite unique constraint.
+	if _, err := db.ExecContext(ctx, `
+		CREATE TABLE release_groups_new (
+			id                     INTEGER PRIMARY KEY,
+			name                   TEXT NOT NULL,
+			cover_art_id           INTEGER,
+			album_artist_credit_id INTEGER,
+			year                   INTEGER,
+			total_tracks           INTEGER,
+			total_discs            INTEGER,
+			FOREIGN KEY(cover_art_id) REFERENCES cover_art(id),
+			FOREIGN KEY(album_artist_credit_id) REFERENCES artist_credit(id),
+			UNIQUE(name, album_artist_credit_id)
+		)
+	`); err != nil {
+		return fmt.Errorf(
+			"migration 5: could not create release_groups_new: %w",
+			err,
+		)
+	}
+
+	// Copy all data.
+	if _, err := db.ExecContext(ctx, `
+		INSERT INTO release_groups_new
+		SELECT * FROM release_groups
+	`); err != nil {
+		return fmt.Errorf(
+			"migration 5: could not copy data: %w", err,
+		)
+	}
+
+	// Drop old table.
+	if _, err := db.ExecContext(
+		ctx, "DROP TABLE release_groups",
+	); err != nil {
+		return fmt.Errorf(
+			"migration 5: could not drop old table: %w", err,
+		)
+	}
+
+	// Rename new table.
+	if _, err := db.ExecContext(ctx, `
+		ALTER TABLE release_groups_new
+		RENAME TO release_groups
+	`); err != nil {
+		return fmt.Errorf(
+			"migration 5: could not rename table: %w", err,
+		)
+	}
+
+	// Recreate indexes.
+	if _, err := db.ExecContext(ctx, `
+		CREATE INDEX IF NOT EXISTS idx_release_groups_cover_art_id
+			ON release_groups(cover_art_id)
+	`); err != nil {
+		return fmt.Errorf(
+			"migration 5: could not create cover_art_id index: %w",
+			err,
+		)
+	}
+
+	if _, err := db.ExecContext(ctx, `
+		CREATE INDEX IF NOT EXISTS idx_release_groups_album_artist_credit_id
+			ON release_groups(album_artist_credit_id)
+	`); err != nil {
+		return fmt.Errorf(
+			"migration 5: could not create album_artist_credit_id index: %w",
+			err,
+		)
+	}
+
+	// Recreate the track_metadata VIEW that was dropped above.
+	// The definition must match the embedded schema file
+	// (sql/schemas/track_metadata_view.sql) exactly.
+	if _, err := db.ExecContext(ctx, `
+		CREATE VIEW IF NOT EXISTS track_metadata AS
+		SELECT
+			af.id,
+			af.file_path,
+			af.length_milliseconds,
+			COALESCE(r.name, '') AS title,
+			COALESCE(ac.text, '') AS artist_name,
+			r.track_number,
+			r.disc_number,
+			COALESCE(rg.name, '') AS album,
+			CAST(COALESCE(
+				(SELECT GROUP_CONCAT(g.name, '||')
+				 FROM recording_genres rg_sub
+				 JOIN genres g ON rg_sub.genre_id = g.id
+				 WHERE rg_sub.recording_id = r.id),
+				''
+			) AS TEXT) AS genre,
+			COALESCE(r.year, 0) AS year,
+			COALESCE(r.composer, '') AS composer,
+			COALESCE(ft.extension, '') AS file_type,
+			af.sample_rate,
+			af.bit_depth,
+			af.channels,
+			af.bitrate,
+			af.file_size
+		FROM audio_files af
+		LEFT JOIN recordings r ON af.recording_id = r.id
+		LEFT JOIN artist_credit ac ON r.artist_credit_id = ac.id
+		LEFT JOIN (
+			SELECT recording_id,
+				MIN(release_group_id) AS release_group_id
+			FROM release_group_recordings
+			GROUP BY recording_id
+		) rgr ON r.id = rgr.recording_id
+		LEFT JOIN release_groups rg ON rgr.release_group_id = rg.id
+		LEFT JOIN file_types ft ON af.file_type_id = ft.id
+	`); err != nil {
+		return fmt.Errorf(
+			"migration 5: could not recreate track_metadata VIEW: %w",
+			err,
+		)
+	}
+
+	// Re-enable FK checks.
+	if _, err := db.ExecContext(
+		ctx, "PRAGMA foreign_keys = ON",
+	); err != nil {
+		return fmt.Errorf(
+			"migration 5: could not re-enable foreign keys: %w",
+			err,
+		)
+	}
+
+	if _, err := db.ExecContext(
+		ctx, "PRAGMA user_version = 5",
+	); err != nil {
+		return fmt.Errorf(
+			"could not set user_version to 5: %w", err,
+		)
+	}
+
+	logger.Info("migration 5 complete")
+
+	return nil
+}
+
 // isDuplicateColumnErr returns true when the error is SQLite's
 // "duplicate column name" error from an ALTER TABLE ADD COLUMN
 // on a column that already exists.
@@ -1261,8 +1261,18 @@ func (l *Library) resolveReleaseGroup(
 		return sql.NullInt64{}
 	}

+	// Build composite cache key: "albumName\x00artistCreditID"
+	// (or "albumName\x00-1" if no artist).  This prevents albums
+	// with the same name by different artists from colliding.
+	artistID := int64(-1)
+	if albumArtistCreditID.Valid {
+		artistID = albumArtistCreditID.Int64
+	}
+
+	cacheKey := fmt.Sprintf("%s\x00%d", tags.Album, artistID)
+
 	// Check cache first.
-	if cached, ok := cache.releaseGroups[tags.Album]; ok {
+	if cached, ok := cache.releaseGroups[cacheKey]; ok {
 		// If the cached release group lacks cover art and we now
 		// have it, update it.
 		if coverArtID.Valid && !cached.CoverArtID.Valid {
@@ -1280,7 +1290,7 @@ func (l *Library) resolveReleaseGroup(
 				)
 			} else {
 				cached.CoverArtID = coverArtID
-				cache.releaseGroups[tags.Album] = cached
+				cache.releaseGroups[cacheKey] = cached
 			}
 		}

@@ -1321,7 +1331,7 @@ func (l *Library) resolveReleaseGroup(
 		}
 	}

-	cache.releaseGroups[tags.Album] = rg
+	cache.releaseGroups[cacheKey] = rg

 	return sql.NullInt64{Int64: rg.ID, Valid: true}
 }
@@ -533,7 +533,9 @@ func TestResolveReleaseGroup(t *testing.T) {
 	}

 	// Cover art should be updated on the cached release group.
-	cachedRG := cache.releaseGroups["A Night at the Opera"]
+	// Cache key is composite: "albumName\x00artistCreditID".
+	cacheKey := fmt.Sprintf("%s\x00%d", "A Night at the Opera", ac.ID)
+	cachedRG := cache.releaseGroups[cacheKey]
 	if !cachedRG.CoverArtID.Valid {
 		t.Error("expected CoverArtID to be set after update")
 	}
@@ -559,7 +561,8 @@ func TestResolveReleaseGroup_CacheHit(t *testing.T) {
 	q := lib.db.Queries

 	// Pre-populate cache with a known release group.
-	cache.releaseGroups["Cached Album"] = sqlcgen.ReleaseGroup{
+	// Cache key is composite: "albumName\x00artistCreditID" (use -1 for no artist).
+	cache.releaseGroups[fmt.Sprintf("%s\x00%d", "Cached Album", int64(-1))] = sqlcgen.ReleaseGroup{
 		ID:   42,
 		Name: "Cached Album",
 	}