mirror of
https://github.com/gabehf/Koito.git
synced 2026-04-22 12:01:52 -07:00
Fix Maloja importer resilience: nil MBZ client, null album, error handling
Root cause of the panic at ~1,693 items: importers created
`&mbz.MusicBrainzClient{}` (nil requestQueue) instead of using the
engine's properly initialized client. When any code path called an MBZ
method, it panicked on the nil channel.
Changes:
- Pass engine's MBZ client to Maloja and Spotify importers
- Change MalojaTrack.Album to pointer type to handle null album JSON
- Continue on error instead of aborting the entire import
- Accept both Maloja export formats ("scrobbles" and "list" keys)
- Extract per-file import into importFile() with its own defer/recover
- Add progress logging every 500 items
Test fixtures:
- maloja_import_null_album_test.json (null album, valid album, empty artists)
- maloja_api_format_test.json (API "list" format)
New tests: TestImportMaloja_NullAlbum, TestImportMaloja_ApiFormat
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
0ec7b458cc
commit
c2d393aa03
6 changed files with 224 additions and 55 deletions
|
|
@ -263,47 +263,51 @@ func RunImporter(l *zerolog.Logger, store db.DB, mbzc mbz.MusicBrainzCaller) {
|
||||||
} else {
|
} else {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
defer func() {
|
|
||||||
if r := recover(); r != nil {
|
|
||||||
l.Error().Interface("recover", r).Msg("Importer: Panic when importing files")
|
|
||||||
}
|
|
||||||
}()
|
|
||||||
for _, file := range files {
|
for _, file := range files {
|
||||||
if file.IsDir() {
|
if file.IsDir() {
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
if strings.Contains(file.Name(), "Streaming_History_Audio") {
|
importFile(l, store, mbzc, file.Name())
|
||||||
l.Info().Msgf("Importer: Import file %s detecting as being Spotify export", file.Name())
|
}
|
||||||
err := importer.ImportSpotifyFile(logger.NewContext(l), store, file.Name())
|
}
|
||||||
if err != nil {
|
|
||||||
l.Err(err).Msgf("Importer: Failed to import file: %s", file.Name())
|
func importFile(l *zerolog.Logger, store db.DB, mbzc mbz.MusicBrainzCaller, filename string) {
|
||||||
}
|
defer func() {
|
||||||
} else if strings.Contains(file.Name(), "maloja") {
|
if r := recover(); r != nil {
|
||||||
l.Info().Msgf("Importer: Import file %s detecting as being Maloja export", file.Name())
|
l.Error().Interface("recover", r).Msgf("Importer: Panic when importing file: %s", filename)
|
||||||
err := importer.ImportMalojaFile(logger.NewContext(l), store, file.Name())
|
}
|
||||||
if err != nil {
|
}()
|
||||||
l.Err(err).Msgf("Importer: Failed to import file: %s", file.Name())
|
if strings.Contains(filename, "Streaming_History_Audio") {
|
||||||
}
|
l.Info().Msgf("Importer: Import file %s detecting as being Spotify export", filename)
|
||||||
} else if strings.Contains(file.Name(), "recenttracks") {
|
err := importer.ImportSpotifyFile(logger.NewContext(l), store, mbzc, filename)
|
||||||
l.Info().Msgf("Importer: Import file %s detecting as being ghan.nl LastFM export", file.Name())
|
if err != nil {
|
||||||
err := importer.ImportLastFMFile(logger.NewContext(l), store, mbzc, file.Name())
|
l.Err(err).Msgf("Importer: Failed to import file: %s", filename)
|
||||||
if err != nil {
|
}
|
||||||
l.Err(err).Msgf("Importer: Failed to import file: %s", file.Name())
|
} else if strings.Contains(filename, "maloja") {
|
||||||
}
|
l.Info().Msgf("Importer: Import file %s detecting as being Maloja export", filename)
|
||||||
} else if strings.Contains(file.Name(), "listenbrainz") {
|
err := importer.ImportMalojaFile(logger.NewContext(l), store, mbzc, filename)
|
||||||
l.Info().Msgf("Importer: Import file %s detecting as being ListenBrainz export", file.Name())
|
if err != nil {
|
||||||
err := importer.ImportListenBrainzExport(logger.NewContext(l), store, mbzc, file.Name())
|
l.Err(err).Msgf("Importer: Failed to import file: %s", filename)
|
||||||
if err != nil {
|
}
|
||||||
l.Err(err).Msgf("Importer: Failed to import file: %s", file.Name())
|
} else if strings.Contains(filename, "recenttracks") {
|
||||||
}
|
l.Info().Msgf("Importer: Import file %s detecting as being ghan.nl LastFM export", filename)
|
||||||
} else if strings.Contains(file.Name(), "koito") {
|
err := importer.ImportLastFMFile(logger.NewContext(l), store, mbzc, filename)
|
||||||
l.Info().Msgf("Importer: Import file %s detecting as being Koito export", file.Name())
|
if err != nil {
|
||||||
err := importer.ImportKoitoFile(logger.NewContext(l), store, file.Name())
|
l.Err(err).Msgf("Importer: Failed to import file: %s", filename)
|
||||||
if err != nil {
|
}
|
||||||
l.Err(err).Msgf("Importer: Failed to import file: %s", file.Name())
|
} else if strings.Contains(filename, "listenbrainz") {
|
||||||
}
|
l.Info().Msgf("Importer: Import file %s detecting as being ListenBrainz export", filename)
|
||||||
} else {
|
err := importer.ImportListenBrainzExport(logger.NewContext(l), store, mbzc, filename)
|
||||||
l.Warn().Msgf("Importer: File %s not recognized as a valid import file; make sure it is valid and named correctly", file.Name())
|
if err != nil {
|
||||||
}
|
l.Err(err).Msgf("Importer: Failed to import file: %s", filename)
|
||||||
|
}
|
||||||
|
} else if strings.Contains(filename, "koito") {
|
||||||
|
l.Info().Msgf("Importer: Import file %s detecting as being Koito export", filename)
|
||||||
|
err := importer.ImportKoitoFile(logger.NewContext(l), store, filename)
|
||||||
|
if err != nil {
|
||||||
|
l.Err(err).Msgf("Importer: Failed to import file: %s", filename)
|
||||||
|
}
|
||||||
|
} else {
|
||||||
|
l.Warn().Msgf("Importer: File %s not recognized as a valid import file; make sure it is valid and named correctly", filename)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -44,6 +44,61 @@ func TestImportMaloja(t *testing.T) {
|
||||||
truncateTestData(t)
|
truncateTestData(t)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestImportMaloja_NullAlbum(t *testing.T) {
|
||||||
|
|
||||||
|
src := path.Join("..", "test_assets", "maloja_import_null_album_test.json")
|
||||||
|
destDir := filepath.Join(cfg.ConfigDir(), "import")
|
||||||
|
dest := filepath.Join(destDir, "maloja_import_null_album_test.json")
|
||||||
|
|
||||||
|
input, err := os.ReadFile(src)
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
require.NoError(t, os.WriteFile(dest, input, os.ModePerm))
|
||||||
|
|
||||||
|
engine.RunImporter(logger.Get(), store, &mbz.MbzErrorCaller{})
|
||||||
|
|
||||||
|
// The null-album track should be imported with album falling back to track name
|
||||||
|
a, err := store.GetArtist(context.Background(), db.GetArtistOpts{Name: "Null Album Artist"})
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.Equal(t, "Null Album Artist", a.Name)
|
||||||
|
assert.EqualValues(t, 1, a.ListenCount)
|
||||||
|
|
||||||
|
// The valid-album track should also be imported
|
||||||
|
a, err = store.GetArtist(context.Background(), db.GetArtistOpts{Name: "Valid Album Artist"})
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.Equal(t, "Valid Album Artist", a.Name)
|
||||||
|
assert.EqualValues(t, 1, a.ListenCount)
|
||||||
|
|
||||||
|
// The empty artists item should be skipped
|
||||||
|
count, err := store.CountArtists(context.Background(), db.Timeframe{Period: db.PeriodAllTime})
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.EqualValues(t, 2, count)
|
||||||
|
|
||||||
|
truncateTestData(t)
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestImportMaloja_ApiFormat(t *testing.T) {
|
||||||
|
|
||||||
|
src := path.Join("..", "test_assets", "maloja_api_format_test.json")
|
||||||
|
destDir := filepath.Join(cfg.ConfigDir(), "import")
|
||||||
|
dest := filepath.Join(destDir, "maloja_api_format_test.json")
|
||||||
|
|
||||||
|
input, err := os.ReadFile(src)
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
require.NoError(t, os.WriteFile(dest, input, os.ModePerm))
|
||||||
|
|
||||||
|
engine.RunImporter(logger.Get(), store, &mbz.MbzErrorCaller{})
|
||||||
|
|
||||||
|
// API format uses "list" key instead of "scrobbles"
|
||||||
|
a, err := store.GetArtist(context.Background(), db.GetArtistOpts{Name: "API Format Artist"})
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.Equal(t, "API Format Artist", a.Name)
|
||||||
|
assert.EqualValues(t, 2, a.ListenCount)
|
||||||
|
|
||||||
|
truncateTestData(t)
|
||||||
|
}
|
||||||
|
|
||||||
func TestImportSpotify(t *testing.T) {
|
func TestImportSpotify(t *testing.T) {
|
||||||
|
|
||||||
src := path.Join("..", "test_assets", "Streaming_History_Audio_spotify_import_test.json")
|
src := path.Join("..", "test_assets", "Streaming_History_Audio_spotify_import_test.json")
|
||||||
|
|
|
||||||
|
|
@ -17,8 +17,13 @@ import (
|
||||||
"github.com/gabehf/koito/internal/utils"
|
"github.com/gabehf/koito/internal/utils"
|
||||||
)
|
)
|
||||||
|
|
||||||
type MalojaExport struct {
|
type MalojaAlbum struct {
|
||||||
|
Title string `json:"albumtitle"`
|
||||||
|
}
|
||||||
|
|
||||||
|
type MalojaFile struct {
|
||||||
Scrobbles []MalojaExportItem `json:"scrobbles"`
|
Scrobbles []MalojaExportItem `json:"scrobbles"`
|
||||||
|
List []MalojaExportItem `json:"list"`
|
||||||
}
|
}
|
||||||
type MalojaExportItem struct {
|
type MalojaExportItem struct {
|
||||||
Time int64 `json:"time"`
|
Time int64 `json:"time"`
|
||||||
|
|
@ -27,12 +32,10 @@ type MalojaExportItem struct {
|
||||||
type MalojaTrack struct {
|
type MalojaTrack struct {
|
||||||
Artists []string `json:"artists"`
|
Artists []string `json:"artists"`
|
||||||
Title string `json:"title"`
|
Title string `json:"title"`
|
||||||
Album struct {
|
Album *MalojaAlbum `json:"album"`
|
||||||
Title string `json:"albumtitle"`
|
|
||||||
} `json:"album"`
|
|
||||||
}
|
}
|
||||||
|
|
||||||
func ImportMalojaFile(ctx context.Context, store db.DB, filename string) error {
|
func ImportMalojaFile(ctx context.Context, store db.DB, mbzc mbz.MusicBrainzCaller, filename string) error {
|
||||||
l := logger.FromContext(ctx)
|
l := logger.FromContext(ctx)
|
||||||
l.Info().Msgf("Beginning maloja import on file: %s", filename)
|
l.Info().Msgf("Beginning maloja import on file: %s", filename)
|
||||||
file, err := os.Open(path.Join(cfg.ConfigDir(), "import", filename))
|
file, err := os.Open(path.Join(cfg.ConfigDir(), "import", filename))
|
||||||
|
|
@ -47,14 +50,20 @@ func ImportMalojaFile(ctx context.Context, store db.DB, filename string) error {
|
||||||
time.Sleep(time.Duration(ms) * time.Millisecond)
|
time.Sleep(time.Duration(ms) * time.Millisecond)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
export := new(MalojaExport)
|
export := new(MalojaFile)
|
||||||
err = json.NewDecoder(file).Decode(&export)
|
err = json.NewDecoder(file).Decode(&export)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return fmt.Errorf("ImportMalojaFile: %w", err)
|
return fmt.Errorf("ImportMalojaFile: %w", err)
|
||||||
}
|
}
|
||||||
for _, item := range export.Scrobbles {
|
items := export.Scrobbles
|
||||||
|
if len(items) == 0 {
|
||||||
|
items = export.List
|
||||||
|
}
|
||||||
|
count := 0
|
||||||
|
total := len(items)
|
||||||
|
for i, item := range items {
|
||||||
martists := make([]string, 0)
|
martists := make([]string, 0)
|
||||||
// Maloja has a tendency to have the the artist order ['feature', 'main \u2022 feature'], so
|
// Maloja has a tendency to have the the artist order ['feature', 'main ● feature'], so
|
||||||
// here we try to turn that artist array into ['main', 'feature']
|
// here we try to turn that artist array into ['main', 'feature']
|
||||||
item.Track.Artists = utils.MoveFirstMatchToFront(item.Track.Artists, " \u2022 ")
|
item.Track.Artists = utils.MoveFirstMatchToFront(item.Track.Artists, " \u2022 ")
|
||||||
for _, an := range item.Track.Artists {
|
for _, an := range item.Track.Artists {
|
||||||
|
|
@ -71,12 +80,16 @@ func ImportMalojaFile(ctx context.Context, store db.DB, filename string) error {
|
||||||
l.Debug().Msgf("Skipping import due to import time rules")
|
l.Debug().Msgf("Skipping import due to import time rules")
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
releaseTitle := ""
|
||||||
|
if item.Track.Album != nil {
|
||||||
|
releaseTitle = item.Track.Album.Title
|
||||||
|
}
|
||||||
opts := catalog.SubmitListenOpts{
|
opts := catalog.SubmitListenOpts{
|
||||||
MbzCaller: &mbz.MusicBrainzClient{},
|
MbzCaller: mbzc,
|
||||||
Artist: item.Track.Artists[0],
|
Artist: item.Track.Artists[0],
|
||||||
ArtistNames: artists,
|
ArtistNames: artists,
|
||||||
TrackTitle: item.Track.Title,
|
TrackTitle: item.Track.Title,
|
||||||
ReleaseTitle: item.Track.Album.Title,
|
ReleaseTitle: releaseTitle,
|
||||||
Time: ts.Local(),
|
Time: ts.Local(),
|
||||||
Client: "maloja",
|
Client: "maloja",
|
||||||
UserID: 1,
|
UserID: 1,
|
||||||
|
|
@ -84,10 +97,14 @@ func ImportMalojaFile(ctx context.Context, store db.DB, filename string) error {
|
||||||
}
|
}
|
||||||
err = catalog.SubmitListen(ctx, store, opts)
|
err = catalog.SubmitListen(ctx, store, opts)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
l.Err(err).Msg("Failed to import maloja playback item")
|
l.Err(err).Msgf("Failed to import maloja item %d/%d", i+1, total)
|
||||||
return fmt.Errorf("ImportMalojaFile: %w", err)
|
continue
|
||||||
|
}
|
||||||
|
count++
|
||||||
|
if count%500 == 0 {
|
||||||
|
l.Info().Msgf("Maloja import progress: %d/%d", count, total)
|
||||||
}
|
}
|
||||||
throttleFunc()
|
throttleFunc()
|
||||||
}
|
}
|
||||||
return finishImport(ctx, filename, len(export.Scrobbles))
|
return finishImport(ctx, filename, count)
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -24,7 +24,7 @@ type SpotifyExportItem struct {
|
||||||
MsPlayed int32 `json:"ms_played"`
|
MsPlayed int32 `json:"ms_played"`
|
||||||
}
|
}
|
||||||
|
|
||||||
func ImportSpotifyFile(ctx context.Context, store db.DB, filename string) error {
|
func ImportSpotifyFile(ctx context.Context, store db.DB, mbzc mbz.MusicBrainzCaller, filename string) error {
|
||||||
l := logger.FromContext(ctx)
|
l := logger.FromContext(ctx)
|
||||||
l.Info().Msgf("Beginning spotify import on file: %s", filename)
|
l.Info().Msgf("Beginning spotify import on file: %s", filename)
|
||||||
file, err := os.Open(path.Join(cfg.ConfigDir(), "import", filename))
|
file, err := os.Open(path.Join(cfg.ConfigDir(), "import", filename))
|
||||||
|
|
@ -59,7 +59,7 @@ func ImportSpotifyFile(ctx context.Context, store db.DB, filename string) error
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
opts := catalog.SubmitListenOpts{
|
opts := catalog.SubmitListenOpts{
|
||||||
MbzCaller: &mbz.MusicBrainzClient{},
|
MbzCaller: mbzc,
|
||||||
Artist: item.ArtistName,
|
Artist: item.ArtistName,
|
||||||
TrackTitle: item.TrackName,
|
TrackTitle: item.TrackName,
|
||||||
ReleaseTitle: item.AlbumName,
|
ReleaseTitle: item.AlbumName,
|
||||||
|
|
|
||||||
41
test_assets/maloja_api_format_test.json
Normal file
41
test_assets/maloja_api_format_test.json
Normal file
|
|
@ -0,0 +1,41 @@
|
||||||
|
{
|
||||||
|
"status": "ok",
|
||||||
|
"list": [
|
||||||
|
{
|
||||||
|
"time": 1746434410,
|
||||||
|
"track": {
|
||||||
|
"artists": [
|
||||||
|
"API Format Artist"
|
||||||
|
],
|
||||||
|
"title": "API Format Track",
|
||||||
|
"album": {
|
||||||
|
"artists": [
|
||||||
|
"API Format Artist"
|
||||||
|
],
|
||||||
|
"albumtitle": "API Format Album"
|
||||||
|
},
|
||||||
|
"length": null
|
||||||
|
},
|
||||||
|
"duration": null,
|
||||||
|
"origin": "client:default"
|
||||||
|
},
|
||||||
|
{
|
||||||
|
"time": 1746434682,
|
||||||
|
"track": {
|
||||||
|
"artists": [
|
||||||
|
"API Format Artist"
|
||||||
|
],
|
||||||
|
"title": "API Format Track 2",
|
||||||
|
"album": {
|
||||||
|
"artists": [
|
||||||
|
"API Format Artist"
|
||||||
|
],
|
||||||
|
"albumtitle": "API Format Album"
|
||||||
|
},
|
||||||
|
"length": null
|
||||||
|
},
|
||||||
|
"duration": null,
|
||||||
|
"origin": "client:default"
|
||||||
|
}
|
||||||
|
]
|
||||||
|
}
|
||||||
52
test_assets/maloja_import_null_album_test.json
Normal file
52
test_assets/maloja_import_null_album_test.json
Normal file
|
|
@ -0,0 +1,52 @@
|
||||||
|
{
|
||||||
|
"maloja": {
|
||||||
|
"export_time": 1748738994
|
||||||
|
},
|
||||||
|
"scrobbles": [
|
||||||
|
{
|
||||||
|
"time": 1746434410,
|
||||||
|
"track": {
|
||||||
|
"artists": [
|
||||||
|
"Null Album Artist"
|
||||||
|
],
|
||||||
|
"title": "Track With Null Album",
|
||||||
|
"album": null,
|
||||||
|
"length": null
|
||||||
|
},
|
||||||
|
"duration": null,
|
||||||
|
"origin": "client:default"
|
||||||
|
},
|
||||||
|
{
|
||||||
|
"time": 1746434682,
|
||||||
|
"track": {
|
||||||
|
"artists": [
|
||||||
|
"Valid Album Artist"
|
||||||
|
],
|
||||||
|
"title": "Track With Valid Album",
|
||||||
|
"album": {
|
||||||
|
"artists": [
|
||||||
|
"Valid Album Artist"
|
||||||
|
],
|
||||||
|
"albumtitle": "My Great Album"
|
||||||
|
},
|
||||||
|
"length": null
|
||||||
|
},
|
||||||
|
"duration": null,
|
||||||
|
"origin": "client:default"
|
||||||
|
},
|
||||||
|
{
|
||||||
|
"time": 1746434900,
|
||||||
|
"track": {
|
||||||
|
"artists": [],
|
||||||
|
"title": "Track With Empty Artists",
|
||||||
|
"album": {
|
||||||
|
"artists": [],
|
||||||
|
"albumtitle": "Some Album"
|
||||||
|
},
|
||||||
|
"length": null
|
||||||
|
},
|
||||||
|
"duration": null,
|
||||||
|
"origin": "client:default"
|
||||||
|
}
|
||||||
|
]
|
||||||
|
}
|
||||||
Loading…
Add table
Add a link
Reference in a new issue