From c2d393aa03ea2e221f3e9a00d0cfee35be75bc5c Mon Sep 17 00:00:00 2001 From: safierinx-a Date: Tue, 24 Mar 2026 23:58:45 +0530 Subject: [PATCH] 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) --- engine/engine.go | 80 ++++++++++--------- engine/import_test.go | 55 +++++++++++++ internal/importer/maloja.go | 47 +++++++---- internal/importer/spotify.go | 4 +- test_assets/maloja_api_format_test.json | 41 ++++++++++ .../maloja_import_null_album_test.json | 52 ++++++++++++ 6 files changed, 224 insertions(+), 55 deletions(-) create mode 100644 test_assets/maloja_api_format_test.json create mode 100644 test_assets/maloja_import_null_album_test.json diff --git a/engine/engine.go b/engine/engine.go index 979667e..f515869 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -263,47 +263,51 @@ func RunImporter(l *zerolog.Logger, store db.DB, mbzc mbz.MusicBrainzCaller) { } else { return } - defer func() { - if r := recover(); r != nil { - l.Error().Interface("recover", r).Msg("Importer: Panic when importing files") - } - }() for _, file := range files { if file.IsDir() { continue } - if strings.Contains(file.Name(), "Streaming_History_Audio") { - 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()) - } - } else if strings.Contains(file.Name(), "maloja") { - l.Info().Msgf("Importer: Import file %s detecting as being Maloja export", file.Name()) - err := importer.ImportMalojaFile(logger.NewContext(l), store, file.Name()) - if err != nil { - l.Err(err).Msgf("Importer: Failed to import file: %s", file.Name()) - } - } else if strings.Contains(file.Name(), "recenttracks") { - l.Info().Msgf("Importer: Import file %s detecting as being ghan.nl LastFM export", file.Name()) - err := importer.ImportLastFMFile(logger.NewContext(l), store, mbzc, file.Name()) - if err != nil { - l.Err(err).Msgf("Importer: Failed to import file: %s", file.Name()) - } - } else if strings.Contains(file.Name(), "listenbrainz") { - l.Info().Msgf("Importer: Import file %s detecting as being ListenBrainz export", file.Name()) - err := importer.ImportListenBrainzExport(logger.NewContext(l), store, mbzc, file.Name()) - if err != nil { - l.Err(err).Msgf("Importer: Failed to import file: %s", file.Name()) - } - } else if strings.Contains(file.Name(), "koito") { - l.Info().Msgf("Importer: Import file %s detecting as being Koito export", file.Name()) - err := importer.ImportKoitoFile(logger.NewContext(l), store, file.Name()) - if err != nil { - l.Err(err).Msgf("Importer: Failed to import file: %s", file.Name()) - } - } else { - l.Warn().Msgf("Importer: File %s not recognized as a valid import file; make sure it is valid and named correctly", file.Name()) - } + importFile(l, store, mbzc, file.Name()) + } +} + +func importFile(l *zerolog.Logger, store db.DB, mbzc mbz.MusicBrainzCaller, filename string) { + defer func() { + if r := recover(); r != nil { + l.Error().Interface("recover", r).Msgf("Importer: Panic when importing file: %s", filename) + } + }() + if strings.Contains(filename, "Streaming_History_Audio") { + l.Info().Msgf("Importer: Import file %s detecting as being Spotify export", filename) + err := importer.ImportSpotifyFile(logger.NewContext(l), store, mbzc, filename) + if err != nil { + l.Err(err).Msgf("Importer: Failed to import file: %s", filename) + } + } else if strings.Contains(filename, "maloja") { + l.Info().Msgf("Importer: Import file %s detecting as being Maloja export", filename) + err := importer.ImportMalojaFile(logger.NewContext(l), store, mbzc, filename) + if err != nil { + l.Err(err).Msgf("Importer: Failed to import file: %s", filename) + } + } else if strings.Contains(filename, "recenttracks") { + l.Info().Msgf("Importer: Import file %s detecting as being ghan.nl LastFM export", filename) + err := importer.ImportLastFMFile(logger.NewContext(l), store, mbzc, filename) + if err != nil { + l.Err(err).Msgf("Importer: Failed to import file: %s", filename) + } + } else if strings.Contains(filename, "listenbrainz") { + l.Info().Msgf("Importer: Import file %s detecting as being ListenBrainz export", filename) + err := importer.ImportListenBrainzExport(logger.NewContext(l), store, mbzc, filename) + 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) } } diff --git a/engine/import_test.go b/engine/import_test.go index fa69e73..0055490 100644 --- a/engine/import_test.go +++ b/engine/import_test.go @@ -44,6 +44,61 @@ func TestImportMaloja(t *testing.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) { src := path.Join("..", "test_assets", "Streaming_History_Audio_spotify_import_test.json") diff --git a/internal/importer/maloja.go b/internal/importer/maloja.go index 8d7c041..3dca0d2 100644 --- a/internal/importer/maloja.go +++ b/internal/importer/maloja.go @@ -17,22 +17,25 @@ import ( "github.com/gabehf/koito/internal/utils" ) -type MalojaExport struct { +type MalojaAlbum struct { + Title string `json:"albumtitle"` +} + +type MalojaFile struct { Scrobbles []MalojaExportItem `json:"scrobbles"` + List []MalojaExportItem `json:"list"` } type MalojaExportItem struct { Time int64 `json:"time"` Track MalojaTrack `json:"track"` } type MalojaTrack struct { - Artists []string `json:"artists"` - Title string `json:"title"` - Album struct { - Title string `json:"albumtitle"` - } `json:"album"` + Artists []string `json:"artists"` + Title string `json:"title"` + Album *MalojaAlbum `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.Info().Msgf("Beginning maloja import on file: %s", 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) } } - export := new(MalojaExport) + export := new(MalojaFile) err = json.NewDecoder(file).Decode(&export) if err != nil { 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) - // 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'] item.Track.Artists = utils.MoveFirstMatchToFront(item.Track.Artists, " \u2022 ") 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") continue } + releaseTitle := "" + if item.Track.Album != nil { + releaseTitle = item.Track.Album.Title + } opts := catalog.SubmitListenOpts{ - MbzCaller: &mbz.MusicBrainzClient{}, + MbzCaller: mbzc, Artist: item.Track.Artists[0], ArtistNames: artists, TrackTitle: item.Track.Title, - ReleaseTitle: item.Track.Album.Title, + ReleaseTitle: releaseTitle, Time: ts.Local(), Client: "maloja", UserID: 1, @@ -84,10 +97,14 @@ func ImportMalojaFile(ctx context.Context, store db.DB, filename string) error { } err = catalog.SubmitListen(ctx, store, opts) if err != nil { - l.Err(err).Msg("Failed to import maloja playback item") - return fmt.Errorf("ImportMalojaFile: %w", err) + l.Err(err).Msgf("Failed to import maloja item %d/%d", i+1, total) + continue + } + count++ + if count%500 == 0 { + l.Info().Msgf("Maloja import progress: %d/%d", count, total) } throttleFunc() } - return finishImport(ctx, filename, len(export.Scrobbles)) + return finishImport(ctx, filename, count) } diff --git a/internal/importer/spotify.go b/internal/importer/spotify.go index 5594fc2..4ac296d 100644 --- a/internal/importer/spotify.go +++ b/internal/importer/spotify.go @@ -24,7 +24,7 @@ type SpotifyExportItem struct { 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.Info().Msgf("Beginning spotify import on file: %s", 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 } opts := catalog.SubmitListenOpts{ - MbzCaller: &mbz.MusicBrainzClient{}, + MbzCaller: mbzc, Artist: item.ArtistName, TrackTitle: item.TrackName, ReleaseTitle: item.AlbumName, diff --git a/test_assets/maloja_api_format_test.json b/test_assets/maloja_api_format_test.json new file mode 100644 index 0000000..a898b47 --- /dev/null +++ b/test_assets/maloja_api_format_test.json @@ -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" + } + ] +} diff --git a/test_assets/maloja_import_null_album_test.json b/test_assets/maloja_import_null_album_test.json new file mode 100644 index 0000000..c4840a7 --- /dev/null +++ b/test_assets/maloja_import_null_album_test.json @@ -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" + } + ] +}