mirror of
https://github.com/navidrome/navidrome.git
synced 2025-12-23 23:18:05 -05:00
fix(scanner): prevent foreign key constraint errors in album participant insertion (#4373)
* fix: prevent foreign key constraint error in album participants Prevent foreign key constraint errors when album participants contain artist IDs that don't exist in the artist table. The updateParticipants method now filters out non-existent artist IDs before attempting to insert album_artists relationships. - Add defensive filtering in updateParticipants() to query existing artist IDs - Only insert relationships for artist IDs that exist in the artist table - Add comprehensive regression test for both albums and media files - Fixes scanner errors when JSON participant data contains stale artist references Signed-off-by: Deluan <deluan@navidrome.org> * fix: optimize foreign key handling in album artists insertion Signed-off-by: Deluan <deluan@navidrome.org> * fix: improve participants foreign key tests Signed-off-by: Deluan <deluan@navidrome.org> * fix: clarify comments in album artists insertion query Signed-off-by: Deluan <deluan@navidrome.org> * test: add cleanup to album repository tests Added individual test cleanup to 4 album repository tests that create temporary artists and albums. This ensures proper test isolation by removing test data after each test completes, preventing test interference when running with shuffle mode. Each test now cleans up its own temporary data from the artist, library_artist, album, and album_artists tables using direct SQL deletion. Signed-off-by: Deluan <deluan@navidrome.org> * fix: refactor participant JSON handling for simpler and improved SQL processing Signed-off-by: Deluan <deluan@navidrome.org> * fix: update test command description in Makefile for clarity Signed-off-by: Deluan <deluan@navidrome.org> * fix: refactor album repository tests to use albumRepository type directly Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
Deluan Quintão
2
Makefile
2
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
|
||||
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user