From dc5dcbd474b76d566bf28981bdbfa2f32a14e2f0 Mon Sep 17 00:00:00 2001 From: Gabe Farrell Date: Sun, 15 Jun 2025 22:25:55 -0400 Subject: [PATCH] fix: associate artists with merged items --- internal/db/opts.go | 10 +- internal/db/psql/album.go | 10 ++ internal/db/psql/album_test.go | 11 +- internal/db/psql/merge.go | 45 +++++++- internal/db/psql/merge_test.go | 161 +++++++++++++++++++++++++++-- internal/repository/release.sql.go | 15 +++ 6 files changed, 232 insertions(+), 20 deletions(-) diff --git a/internal/db/opts.go b/internal/db/opts.go index 0ecd03f..018f23d 100644 --- a/internal/db/opts.go +++ b/internal/db/opts.go @@ -96,10 +96,12 @@ type UpdateArtistOpts struct { } type UpdateAlbumOpts struct { - ID int32 - MusicBrainzID uuid.UUID - Image uuid.UUID - ImageSrc string + ID int32 + MusicBrainzID uuid.UUID + Image uuid.UUID + ImageSrc string + VariousArtistsUpdate bool + VariousArtistsValue bool } type UpdateUserOpts struct { diff --git a/internal/db/psql/album.go b/internal/db/psql/album.go index 94c5782..111b4cc 100644 --- a/internal/db/psql/album.go +++ b/internal/db/psql/album.go @@ -200,6 +200,16 @@ func (d *Psql) UpdateAlbum(ctx context.Context, opts db.UpdateAlbumOpts) error { return err } } + if opts.VariousArtistsUpdate { + l.Debug().Msgf("Updating release with ID %d with image %s", opts.ID, opts.Image) + err := qtx.UpdateReleaseVariousArtists(ctx, repository.UpdateReleaseVariousArtistsParams{ + ID: opts.ID, + VariousArtists: opts.VariousArtistsValue, + }) + if err != nil { + return err + } + } return tx.Commit(ctx) } diff --git a/internal/db/psql/album_test.go b/internal/db/psql/album_test.go index d0848cc..49ebfbb 100644 --- a/internal/db/psql/album_test.go +++ b/internal/db/psql/album_test.go @@ -116,10 +116,12 @@ func TestUpdateAlbum(t *testing.T) { newMbzID := uuid.New() imgid := uuid.New() err = store.UpdateAlbum(ctx, db.UpdateAlbumOpts{ - ID: rg.ID, - MusicBrainzID: newMbzID, - Image: imgid, - ImageSrc: catalog.ImageSourceUserUpload, + ID: rg.ID, + MusicBrainzID: newMbzID, + Image: imgid, + ImageSrc: catalog.ImageSourceUserUpload, + VariousArtistsUpdate: true, + VariousArtistsValue: true, }) require.NoError(t, err) @@ -127,6 +129,7 @@ func TestUpdateAlbum(t *testing.T) { require.NoError(t, err) assert.Equal(t, newMbzID, *result.MbzID) assert.Equal(t, imgid, *result.Image) + assert.True(t, result.VariousArtists) truncateTestData(t) } diff --git a/internal/db/psql/merge.go b/internal/db/psql/merge.go index 0b4a24b..c7c46fe 100644 --- a/internal/db/psql/merge.go +++ b/internal/db/psql/merge.go @@ -19,12 +19,36 @@ func (d *Psql) MergeTracks(ctx context.Context, fromId, toId int32) error { } defer tx.Rollback(ctx) qtx := d.q.WithTx(tx) + from, err := qtx.GetTrack(ctx, fromId) + if err != nil { + return fmt.Errorf("MergeTracks: GetTrack: %w", err) + } + to, err := qtx.GetTrack(ctx, toId) + if err != nil { + return fmt.Errorf("MergeTracks: GetTrack: %w", err) + } err = qtx.UpdateTrackIdForListens(ctx, repository.UpdateTrackIdForListensParams{ TrackID: fromId, TrackID_2: toId, }) if err != nil { - return fmt.Errorf("MergeTracks: %w", err) + return fmt.Errorf("MergeTracks: UpdateTrackIdForListens: %w", err) + } + if from.ReleaseID != to.ReleaseID { + // tracks are from different releases, track artist should be associated with to.release + artists, err := qtx.GetTrackArtists(ctx, fromId) + if err != nil { + return fmt.Errorf("MergeTracks: GetTrackArtists: %w", err) + } + for _, artist := range artists { + err = qtx.AssociateArtistToRelease(ctx, repository.AssociateArtistToReleaseParams{ + ArtistID: artist.ID, + ReleaseID: to.ReleaseID, + }) + if err != nil { + return fmt.Errorf("MergeTracks: AssociateArtistToRelease: %w", err) + } + } } err = qtx.CleanOrphanedEntries(ctx) if err != nil { @@ -44,6 +68,12 @@ func (d *Psql) MergeAlbums(ctx context.Context, fromId, toId int32, replaceImage } defer tx.Rollback(ctx) qtx := d.q.WithTx(tx) + + fromArtists, err := qtx.GetReleaseArtists(ctx, fromId) + if err != nil { + return fmt.Errorf("MergeTracks: GetReleaseArtists: %w", err) + } + err = qtx.UpdateReleaseForAll(ctx, repository.UpdateReleaseForAllParams{ ReleaseID: fromId, ReleaseID_2: toId, @@ -65,10 +95,21 @@ func (d *Psql) MergeAlbums(ctx context.Context, fromId, toId int32, replaceImage return fmt.Errorf("MergeAlbums: %w", err) } } + + for _, artist := range fromArtists { + err = qtx.AssociateArtistToRelease(ctx, repository.AssociateArtistToReleaseParams{ + ArtistID: artist.ID, + ReleaseID: toId, + }) + if err != nil { + return fmt.Errorf("MergeAlbums: AssociateArtistToRelease: %w", err) + } + } + err = qtx.CleanOrphanedEntries(ctx) if err != nil { l.Err(err).Msg("Failed to clean orphaned entries") - return err + return fmt.Errorf("MergeAlbums: CleanOrphanedEntries: %w", err) } return tx.Commit(ctx) } diff --git a/internal/db/psql/merge_test.go b/internal/db/psql/merge_test.go index f71fccf..7977282 100644 --- a/internal/db/psql/merge_test.go +++ b/internal/db/psql/merge_test.go @@ -27,44 +27,52 @@ func setupTestDataForMerge(t *testing.T) { err = store.Exec(context.Background(), `INSERT INTO releases (musicbrainz_id, image, image_source) VALUES ('11111111-1111-1111-1111-111111111111', '20000000-0000-0000-0000-000000000000', 'source.com'), - ('22222222-2222-2222-2222-222222222222', NULL, NULL)`) + ('22222222-2222-2222-2222-222222222222', NULL, NULL), + (NULL, NULL, NULL)`) require.NoError(t, err) err = store.Exec(context.Background(), `INSERT INTO release_aliases (release_id, alias, source, is_primary) VALUES (1, 'Album One', 'Testing', true), - (2, 'Album Two', 'Testing', true)`) + (2, 'Album Two', 'Testing', true), + (3, 'Album Three', 'Testing', true)`) require.NoError(t, err) // Insert tracks err = store.Exec(context.Background(), `INSERT INTO tracks (musicbrainz_id, release_id) VALUES ('33333333-3333-3333-3333-333333333333', 1), - ('44444444-4444-4444-4444-444444444444', 2)`) + ('44444444-4444-4444-4444-444444444444', 2), + ('55555555-5555-5555-5555-555555555555', 1), + (NULL, 3)`) require.NoError(t, err) err = store.Exec(context.Background(), `INSERT INTO track_aliases (track_id, alias, source, is_primary) VALUES (1, 'Track One', 'Testing', true), - (2, 'Track Two', 'Testing', true)`) + (2, 'Track Two', 'Testing', true), + (3, 'Track Three', 'Testing', true), + (4, 'Track Four', 'Testing', true)`) require.NoError(t, err) // Associate artists with albums and tracks err = store.Exec(context.Background(), `INSERT INTO artist_releases (artist_id, release_id) - VALUES (1, 1), (2, 2)`) + VALUES (1, 1), (2, 2), (1, 3)`) require.NoError(t, err) err = store.Exec(context.Background(), `INSERT INTO artist_tracks (artist_id, track_id) - VALUES (1, 1), (2, 2)`) + VALUES (1, 1), (2, 2), (1, 3), (1, 4)`) require.NoError(t, err) // Insert listens err = store.Exec(context.Background(), `INSERT INTO listens (user_id, track_id, listened_at) VALUES (1, 1, NOW() - INTERVAL '1 day'), - (1, 2, NOW() - INTERVAL '2 days')`) + (1, 2, NOW() - INTERVAL '2 days'), + (1, 3, NOW() - INTERVAL '3 days'), + (1, 4, NOW() - INTERVAL '3 days')`) require.NoError(t, err) } @@ -82,6 +90,32 @@ func TestMergeTracks(t *testing.T) { require.NoError(t, err) assert.Equal(t, 2, count, "expected all listens to be merged into Track 2") + // Verify artist is associated with album + exists, err := store.RowExists(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM artist_releases + WHERE release_id = $1 AND artist_id = $2 + )`, 2, 1) + require.NoError(t, err) + assert.True(t, exists, "expected old artist to be associated with album") + + truncateTestData(t) +} + +func TestMergeTracks_SameRelease(t *testing.T) { + ctx := context.Background() + setupTestDataForMerge(t) + + // Merge Track 1 into Track 2 + err := store.MergeTracks(ctx, 1, 3) + require.NoError(t, err) + + // Verify listens are updated + var count int + count, err = store.Count(ctx, `SELECT COUNT(*) FROM listens WHERE track_id = 3`) + require.NoError(t, err) + assert.Equal(t, 2, count, "expected all listens to be merged into Track 3") + truncateTestData(t) } @@ -101,7 +135,32 @@ func TestMergeAlbums(t *testing.T) { // Verify tracks are updated count, err = store.Count(ctx, `SELECT COUNT(*) FROM tracks WHERE release_id = 2`) require.NoError(t, err) - assert.Equal(t, 2, count, "expected all tracks to be merged into Album 2") + assert.Equal(t, 3, count, "expected all tracks to be merged into Album 2") + + // Verify artist is associated with primary album + exists, err := store.RowExists(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM artist_releases + WHERE release_id = $1 AND artist_id = $2 + )`, 2, 1) + require.NoError(t, err) + assert.True(t, exists, "expected old album artist to be associated with new album") + + truncateTestData(t) +} + +func TestMergeAlbums_SameArtists(t *testing.T) { + ctx := context.Background() + setupTestDataForMerge(t) + + // Merge Album 1 into Album 3 + err := store.MergeAlbums(ctx, 1, 3, false) + require.NoError(t, err) + + // Verify tracks are updated + count, err := store.Count(ctx, `SELECT COUNT(*) FROM tracks WHERE release_id = 3`) + require.NoError(t, err) + assert.Equal(t, 3, count, "expected all tracks to be merged into Album 3") truncateTestData(t) } @@ -122,11 +181,93 @@ func TestMergeArtists(t *testing.T) { // Verify artist associations are updated count, err = store.Count(ctx, `SELECT COUNT(*) FROM artist_tracks WHERE artist_id = 2`) require.NoError(t, err) - assert.Equal(t, 2, count, "expected all tracks to be associated with Artist 2") + assert.Equal(t, 4, count, "expected all tracks to be associated with Artist 2") count, err = store.Count(ctx, `SELECT COUNT(*) FROM artist_releases WHERE artist_id = 2`) require.NoError(t, err) - assert.Equal(t, 2, count, "expected all releases to be associated with Artist 2") + assert.Equal(t, 3, count, "expected all releases to be associated with Artist 2") + + truncateTestData(t) +} +func TestMergeTracks_EnsureParentOfReleaseIsKept(t *testing.T) { truncateTestData(t) + + // Prepare test data with only album assigned to artist 1, + // and two tracks, both under album one, but under two different artists + + // Insert artists + err := store.Exec(context.Background(), + `INSERT INTO artists (musicbrainz_id, image, image_source) + VALUES ('00000000-0000-0000-0000-000000000001', '10000000-0000-0000-0000-000000000000', 'source.com'), + ('00000000-0000-0000-0000-000000000002', NULL, NULL)`) + require.NoError(t, err) + + err = store.Exec(context.Background(), + `INSERT INTO artist_aliases (artist_id, alias, source, is_primary) + VALUES (1, 'Artist One', 'Testing', true), + (2, 'Artist Two', 'Testing', true)`) + require.NoError(t, err) + + // Insert albums + err = store.Exec(context.Background(), + `INSERT INTO releases (musicbrainz_id, image, image_source) + VALUES ('11111111-1111-1111-1111-111111111111', '20000000-0000-0000-0000-000000000000', 'source.com')`) + require.NoError(t, err) + + err = store.Exec(context.Background(), + `INSERT INTO release_aliases (release_id, alias, source, is_primary) + VALUES (1, 'Album One', 'Testing', true)`) + require.NoError(t, err) + + // Insert tracks + err = store.Exec(context.Background(), + `INSERT INTO tracks (musicbrainz_id, release_id) + VALUES ('33333333-3333-3333-3333-333333333333', 1), + ('44444444-4444-4444-4444-444444444444', 1)`) + require.NoError(t, err) + + err = store.Exec(context.Background(), + `INSERT INTO track_aliases (track_id, alias, source, is_primary) + VALUES (1, 'Track One', 'Testing', true), + (2, 'Track Two', 'Testing', true)`) + require.NoError(t, err) + + // Associate artists with albums and tracks + err = store.Exec(context.Background(), + `INSERT INTO artist_releases (artist_id, release_id) + VALUES (1, 1)`) + require.NoError(t, err) + + err = store.Exec(context.Background(), + `INSERT INTO artist_tracks (artist_id, track_id) + VALUES (1, 1), (2, 2)`) + require.NoError(t, err) + + // Insert listens + err = store.Exec(context.Background(), + `INSERT INTO listens (user_id, track_id, listened_at) + VALUES (1, 1, NOW() - INTERVAL '1 day'), + (1, 2, NOW() - INTERVAL '2 days')`) + require.NoError(t, err) + + ctx := context.Background() + err = store.MergeTracks(ctx, 1, 2) + require.NoError(t, err) + + exists, err := store.RowExists(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM artists + WHERE id = $1 + )`, 1) + require.NoError(t, err) + assert.True(t, exists, "expected artist associated with release to still exist") + + exists, err = store.RowExists(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM releases + WHERE id = $1 + )`, 1) + require.NoError(t, err) + assert.True(t, exists, "expected release to still exist") } diff --git a/internal/repository/release.sql.go b/internal/repository/release.sql.go index 06a936e..6d5cc68 100644 --- a/internal/repository/release.sql.go +++ b/internal/repository/release.sql.go @@ -460,3 +460,18 @@ func (q *Queries) UpdateReleaseMbzID(ctx context.Context, arg UpdateReleaseMbzID _, err := q.db.Exec(ctx, updateReleaseMbzID, arg.ID, arg.MusicBrainzID) return err } + +const updateReleaseVariousArtists = `-- name: UpdateReleaseVariousArtists :exec +UPDATE releases SET various_artists = $2 +WHERE id = $1 +` + +type UpdateReleaseVariousArtistsParams struct { + ID int32 + VariousArtists bool +} + +func (q *Queries) UpdateReleaseVariousArtists(ctx context.Context, arg UpdateReleaseVariousArtistsParams) error { + _, err := q.db.Exec(ctx, updateReleaseVariousArtists, arg.ID, arg.VariousArtists) + return err +}