diff --git a/Makefile b/Makefile index 90b4012f7..034015740 100644 --- a/Makefile +++ b/Makefile @@ -37,7 +37,7 @@ watch: ##@Development Start Go tests in watch mode (re-run when code changes) .PHONY: watch PKG ?= ./... -test: ##@Development Run Go tests +test: ##@Development Run Go tests. Use PKG variable to specify packages to test, e.g. make test PKG=./server go test -tags netgo $(PKG) .PHONY: test diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index 529458c26..4be89bcb8 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -1,13 +1,12 @@ package persistence import ( - "context" "fmt" "time" + "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" "github.com/navidrome/navidrome/model/request" @@ -16,16 +15,16 @@ import ( ) var _ = Describe("AlbumRepository", func() { - var repo model.AlbumRepository + var albumRepo *albumRepository BeforeEach(func() { - ctx := request.WithUser(log.NewContext(context.TODO()), model.User{ID: "userid", UserName: "johndoe"}) - repo = NewAlbumRepository(ctx, GetDBXBuilder()) + ctx := request.WithUser(GinkgoT().Context(), model.User{ID: "userid", UserName: "johndoe"}) + albumRepo = NewAlbumRepository(ctx, GetDBXBuilder()).(*albumRepository) }) Describe("Get", func() { var Get = func(id string) (*model.Album, error) { - album, err := repo.Get(id) + album, err := albumRepo.Get(id) if album != nil { album.ImportedAt = time.Time{} } @@ -42,7 +41,7 @@ var _ = Describe("AlbumRepository", func() { Describe("GetAll", func() { var GetAll = func(opts ...model.QueryOptions) (model.Albums, error) { - albums, err := repo.GetAll(opts...) + albums, err := albumRepo.GetAll(opts...) for i := range albums { albums[i].ImportedAt = time.Time{} } @@ -83,12 +82,12 @@ var _ = Describe("AlbumRepository", func() { conf.Server.AlbumPlayCountMode = consts.AlbumPlayCountModeAbsolute newID := id.NewRandom() - Expect(repo.Put(&model.Album{LibraryID: 1, ID: newID, Name: "name", SongCount: songCount})).To(Succeed()) + Expect(albumRepo.Put(&model.Album{LibraryID: 1, ID: newID, Name: "name", SongCount: songCount})).To(Succeed()) for i := 0; i < playCount; i++ { - Expect(repo.IncPlayCount(newID, time.Now())).To(Succeed()) + Expect(albumRepo.IncPlayCount(newID, time.Now())).To(Succeed()) } - album, err := repo.Get(newID) + album, err := albumRepo.Get(newID) Expect(err).ToNot(HaveOccurred()) Expect(album.PlayCount).To(Equal(int64(expected))) }, @@ -106,12 +105,12 @@ var _ = Describe("AlbumRepository", func() { conf.Server.AlbumPlayCountMode = consts.AlbumPlayCountModeNormalized newID := id.NewRandom() - Expect(repo.Put(&model.Album{LibraryID: 1, ID: newID, Name: "name", SongCount: songCount})).To(Succeed()) + Expect(albumRepo.Put(&model.Album{LibraryID: 1, ID: newID, Name: "name", SongCount: songCount})).To(Succeed()) for i := 0; i < playCount; i++ { - Expect(repo.IncPlayCount(newID, time.Now())).To(Succeed()) + Expect(albumRepo.IncPlayCount(newID, time.Now())).To(Succeed()) } - album, err := repo.Get(newID) + album, err := albumRepo.Get(newID) Expect(err).ToNot(HaveOccurred()) Expect(album.PlayCount).To(Equal(int64(expected))) }, @@ -283,6 +282,235 @@ var _ = Describe("AlbumRepository", func() { Expect(err).To(HaveOccurred()) }) }) + + Describe("Participant Foreign Key Handling", func() { + // albumArtistRecord represents a record in the album_artists table + type albumArtistRecord struct { + ArtistID string `db:"artist_id"` + Role string `db:"role"` + SubRole string `db:"sub_role"` + } + + var artistRepo *artistRepository + + BeforeEach(func() { + ctx := request.WithUser(GinkgoT().Context(), adminUser) + artistRepo = NewArtistRepository(ctx, GetDBXBuilder()).(*artistRepository) + }) + + // Helper to verify album_artists records + verifyAlbumArtists := func(albumID string, expected []albumArtistRecord) { + GinkgoHelper() + var actual []albumArtistRecord + sq := squirrel.Select("artist_id", "role", "sub_role"). + From("album_artists"). + Where(squirrel.Eq{"album_id": albumID}). + OrderBy("role", "artist_id", "sub_role") + + err := albumRepo.queryAll(sq, &actual) + Expect(err).ToNot(HaveOccurred()) + Expect(actual).To(Equal(expected)) + } + + It("verifies that participant records are actually inserted into database", func() { + // Create a real artist in the database first + artist := &model.Artist{ + ID: "real-artist-1", + Name: "Real Artist", + OrderArtistName: "real artist", + SortArtistName: "Artist, Real", + } + err := createArtistWithLibrary(artistRepo, artist, 1) + Expect(err).ToNot(HaveOccurred()) + + // Create an album with participants that reference the real artist + album := &model.Album{ + LibraryID: 1, + ID: "test-album-db-insert", + Name: "Test Album DB Insert", + AlbumArtistID: "real-artist-1", + AlbumArtist: "Real Artist", + Participants: model.Participants{ + model.RoleArtist: { + {Artist: model.Artist{ID: "real-artist-1", Name: "Real Artist"}}, + }, + model.RoleComposer: { + {Artist: model.Artist{ID: "real-artist-1", Name: "Real Artist"}, SubRole: "primary"}, + }, + }, + } + + // Insert the album + err = albumRepo.Put(album) + Expect(err).ToNot(HaveOccurred()) + + // Verify that participant records were actually inserted into album_artists table + expected := []albumArtistRecord{ + {ArtistID: "real-artist-1", Role: "artist", SubRole: ""}, + {ArtistID: "real-artist-1", Role: "composer", SubRole: "primary"}, + } + verifyAlbumArtists(album.ID, expected) + + // Clean up the test artist and album created for this test + _, _ = artistRepo.executeSQL(squirrel.Delete("artist").Where(squirrel.Eq{"id": artist.ID})) + _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID})) + }) + + It("filters out invalid artist IDs leaving only valid participants in database", func() { + // Create two real artists in the database + artist1 := &model.Artist{ + ID: "real-artist-mix-1", + Name: "Real Artist 1", + OrderArtistName: "real artist 1", + } + artist2 := &model.Artist{ + ID: "real-artist-mix-2", + Name: "Real Artist 2", + OrderArtistName: "real artist 2", + } + err := createArtistWithLibrary(artistRepo, artist1, 1) + Expect(err).ToNot(HaveOccurred()) + err = createArtistWithLibrary(artistRepo, artist2, 1) + Expect(err).ToNot(HaveOccurred()) + + // Create an album with mix of valid and invalid artist IDs + album := &model.Album{ + LibraryID: 1, + ID: "test-album-mixed-validity", + Name: "Test Album Mixed Validity", + AlbumArtistID: "real-artist-mix-1", + AlbumArtist: "Real Artist 1", + Participants: model.Participants{ + model.RoleArtist: { + {Artist: model.Artist{ID: "real-artist-mix-1", Name: "Real Artist 1"}}, + {Artist: model.Artist{ID: "non-existent-mix-1", Name: "Non Existent 1"}}, + {Artist: model.Artist{ID: "real-artist-mix-2", Name: "Real Artist 2"}}, + }, + model.RoleComposer: { + {Artist: model.Artist{ID: "non-existent-mix-2", Name: "Non Existent 2"}}, + {Artist: model.Artist{ID: "real-artist-mix-1", Name: "Real Artist 1"}}, + }, + }, + } + + // This should not fail - only valid artists should be inserted + err = albumRepo.Put(album) + Expect(err).ToNot(HaveOccurred()) + + // Verify that only valid artist IDs were inserted into album_artists table + // Non-existent artists should be filtered out by the INNER JOIN + expected := []albumArtistRecord{ + {ArtistID: "real-artist-mix-1", Role: "artist", SubRole: ""}, + {ArtistID: "real-artist-mix-2", Role: "artist", SubRole: ""}, + {ArtistID: "real-artist-mix-1", Role: "composer", SubRole: ""}, + } + verifyAlbumArtists(album.ID, expected) + + // Clean up the test artists and album created for this test + artistIDs := []string{artist1.ID, artist2.ID} + _, _ = artistRepo.executeSQL(squirrel.Delete("artist").Where(squirrel.Eq{"id": artistIDs})) + _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID})) + }) + + It("handles complex nested JSON with multiple roles and sub-roles", func() { + // Create 4 artists for this test + artists := []*model.Artist{ + {ID: "complex-artist-1", Name: "Lead Vocalist", OrderArtistName: "lead vocalist"}, + {ID: "complex-artist-2", Name: "Guitarist", OrderArtistName: "guitarist"}, + {ID: "complex-artist-3", Name: "Producer", OrderArtistName: "producer"}, + {ID: "complex-artist-4", Name: "Engineer", OrderArtistName: "engineer"}, + } + + for _, artist := range artists { + err := createArtistWithLibrary(artistRepo, artist, 1) + Expect(err).ToNot(HaveOccurred()) + } + + // Create album with complex participant structure + album := &model.Album{ + LibraryID: 1, + ID: "test-album-complex-json", + Name: "Test Album Complex JSON", + AlbumArtistID: "complex-artist-1", + AlbumArtist: "Lead Vocalist", + Participants: model.Participants{ + model.RoleArtist: { + {Artist: model.Artist{ID: "complex-artist-1", Name: "Lead Vocalist"}}, + {Artist: model.Artist{ID: "complex-artist-2", Name: "Guitarist"}, SubRole: "lead guitar"}, + {Artist: model.Artist{ID: "complex-artist-2", Name: "Guitarist"}, SubRole: "rhythm guitar"}, + }, + model.RoleProducer: { + {Artist: model.Artist{ID: "complex-artist-3", Name: "Producer"}, SubRole: "executive"}, + }, + model.RoleEngineer: { + {Artist: model.Artist{ID: "complex-artist-4", Name: "Engineer"}, SubRole: "mixing"}, + {Artist: model.Artist{ID: "complex-artist-4", Name: "Engineer"}, SubRole: "mastering"}, + }, + }, + } + + err := albumRepo.Put(album) + Expect(err).ToNot(HaveOccurred()) + + // Verify complex JSON structure was correctly parsed and inserted + expected := []albumArtistRecord{ + {ArtistID: "complex-artist-1", Role: "artist", SubRole: ""}, + {ArtistID: "complex-artist-2", Role: "artist", SubRole: "lead guitar"}, + {ArtistID: "complex-artist-2", Role: "artist", SubRole: "rhythm guitar"}, + {ArtistID: "complex-artist-4", Role: "engineer", SubRole: "mastering"}, + {ArtistID: "complex-artist-4", Role: "engineer", SubRole: "mixing"}, + {ArtistID: "complex-artist-3", Role: "producer", SubRole: "executive"}, + } + verifyAlbumArtists(album.ID, expected) + + // Clean up the test artists and album created for this test + artistIDs := make([]string, len(artists)) + for i, artist := range artists { + artistIDs[i] = artist.ID + } + _, _ = artistRepo.executeSQL(squirrel.Delete("artist").Where(squirrel.Eq{"id": artistIDs})) + _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID})) + }) + + It("handles albums with non-existent artist IDs without constraint errors", func() { + // Regression test for foreign key constraint error when album participants + // contain artist IDs that don't exist in the artist table + + // Create an album with participants that reference non-existent artist IDs + album := &model.Album{ + LibraryID: 1, + ID: "test-album-fk-constraints", + Name: "Test Album with Invalid Artist References", + AlbumArtistID: "non-existent-artist-1", + AlbumArtist: "Non Existent Album Artist", + Participants: model.Participants{ + model.RoleArtist: { + {Artist: model.Artist{ID: "non-existent-artist-1", Name: "Non Existent Artist 1"}}, + {Artist: model.Artist{ID: "non-existent-artist-2", Name: "Non Existent Artist 2"}}, + }, + model.RoleComposer: { + {Artist: model.Artist{ID: "non-existent-composer-1", Name: "Non Existent Composer 1"}}, + {Artist: model.Artist{ID: "non-existent-composer-2", Name: "Non Existent Composer 2"}}, + }, + model.RoleAlbumArtist: { + {Artist: model.Artist{ID: "non-existent-album-artist-1", Name: "Non Existent Album Artist 1"}}, + }, + }, + } + + // This should not fail with foreign key constraint error + // The updateParticipants method should handle non-existent artist IDs gracefully + err := albumRepo.Put(album) + Expect(err).ToNot(HaveOccurred()) + + // Verify that no participant records were inserted since all artist IDs were invalid + // The INNER JOIN with the artist table should filter out all non-existent artists + verifyAlbumArtists(album.ID, []albumArtistRecord{}) + + // Clean up the test album created for this test + _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID})) + }) + }) }) func _p(id, name string, sortName ...string) model.Participant { diff --git a/persistence/sql_participations.go b/persistence/sql_participations.go index 4345184f1..d88eca45e 100644 --- a/persistence/sql_participations.go +++ b/persistence/sql_participations.go @@ -15,6 +15,13 @@ type participant struct { SubRole string `json:"subRole,omitempty"` } +// flatParticipant represents a flattened participant structure for SQL processing +type flatParticipant struct { + ArtistID string `json:"artist_id"` + Role string `json:"role"` + SubRole string `json:"sub_role,omitempty"` +} + func marshalParticipants(participants model.Participants) string { dbParticipants := make(map[model.Role][]participant) for role, artists := range participants { @@ -53,15 +60,40 @@ func (r sqlRepository) updateParticipants(itemID string, participants model.Part if len(participants) == 0 { return nil } - sqi := Insert(r.tableName+"_artists"). - Columns(r.tableName+"_id", "artist_id", "role", "sub_role"). - Suffix(fmt.Sprintf("on conflict (artist_id, %s_id, role, sub_role) do nothing", r.tableName)) + + var flatParticipants []flatParticipant for role, artists := range participants { for _, artist := range artists { - sqi = sqi.Values(itemID, artist.ID, role.String(), artist.SubRole) + flatParticipants = append(flatParticipants, flatParticipant{ + ArtistID: artist.ID, + Role: role.String(), + SubRole: artist.SubRole, + }) } } - _, err = r.executeSQL(sqi) + + participantsJSON, err := json.Marshal(flatParticipants) + if err != nil { + return fmt.Errorf("marshaling participants: %w", err) + } + + // Build the INSERT query using json_each and INNER JOIN to artist table + // to automatically filter out non-existent artist IDs + query := fmt.Sprintf(` + INSERT INTO %[1]s_artists (%[1]s_id, artist_id, role, sub_role) + SELECT ?, + json_extract(value, '$.artist_id') as artist_id, + json_extract(value, '$.role') as role, + COALESCE(json_extract(value, '$.sub_role'), '') as sub_role + -- Parse the flat JSON array: [{"artist_id": "id", "role": "role", "sub_role": "subRole"}] + FROM json_each(?) -- Iterate through each array element + -- CRITICAL: Only insert records for artists that actually exist in the database + JOIN artist ON artist.id = json_extract(value, '$.artist_id') -- Filter out non-existent artist IDs via INNER JOIN + -- Handle duplicate insertions gracefully (e.g., if called multiple times) + ON CONFLICT (artist_id, %[1]s_id, role, sub_role) DO NOTHING -- Ignore duplicates + `, r.tableName) + + _, err = r.executeSQL(Expr(query, itemID, string(participantsJSON))) return err }