From 26fb466afc689b26c70ead0b0fb4a7c72f743872 Mon Sep 17 00:00:00 2001 From: Joren Date: Sat, 3 Oct 2026 17:26:24 +0200 Subject: [PATCH] refactor: finish the jsonutil dedupe; back up config before auto-upgrade Duplicated helpers cmd/rip had its own copies of six helpers that already existed in internal/jsonutil: asString (StringFromAny), searchInt (IntFromAny), nestedSearchInt and nestedSearchString (NestedAny/NestedString), firstNonEmpty (FirstNonEmpty) and firstPositiveInt. firstPositiveInt was duplicated a third and fourth time in provider/yandex and provider/beatport, so it moves into jsonutil alongside the rest. jsonutil is back to 100% coverage. searchBool is deliberately left alone, with a comment saying why: it accepts only a real bool, and switching it to jsonutil.BoolFromAny would start coercing strings and numbers, changing how search results get labelled. config auto-upgrade is no longer destructive UpgradeOutdated re-serializes the whole config from the parsed struct, so any key this version does not know about is dropped, and it runs automatically from run() without prompting. It now copies the original to .bak- (0600, since it holds the same credentials) and validates the result before writing. It returns the backup path so the CLI can tell the user where it went. The comment states plainly that this is a version stamp rather than a migration framework. cmd/rip tests The command bodies are testable now that they are functions returning exit codes rather than arms of a switch that called os.Exit. Covers the usage errors for every command, the quality floor on `rip id`, config path output, the --version flag, and that applyQualityOverride reaches all six sources and clamps only the two with a floor. cmd/rip coverage 17% -> 26%. Co-Authored-By: Claude Opus 5 --- cmd/rip/args.go | 15 -- cmd/rip/cmd_smoke.go | 23 +-- cmd/rip/cmd_test.go | 248 +++++++++++++++++++++++++++ cmd/rip/helpers.go | 9 +- cmd/rip/lastfm.go | 18 +- cmd/rip/main.go | 4 +- cmd/rip/search.go | 225 ++++++++++-------------- internal/config/config.go | 32 +++- internal/config/config_test.go | 24 ++- internal/jsonutil/jsonutil.go | 13 ++ internal/jsonutil/jsonutil_test.go | 19 ++ internal/provider/beatport/client.go | 13 +- internal/provider/yandex/client.go | 15 +- 13 files changed, 447 insertions(+), 211 deletions(-) create mode 100644 cmd/rip/cmd_test.go diff --git a/cmd/rip/args.go b/cmd/rip/args.go index d303623..c26ad0e 100644 --- a/cmd/rip/args.go +++ b/cmd/rip/args.go @@ -244,18 +244,3 @@ func parseQuality(raw string, min int, max int) (int, error) { } return q, nil } - -func asString(v any) string { - switch t := v.(type) { - case string: - return t - case int: - return strconv.Itoa(t) - case int64: - return strconv.FormatInt(t, 10) - case float64: - return strconv.FormatFloat(t, 'f', -1, 64) - default: - return "" - } -} diff --git a/cmd/rip/cmd_smoke.go b/cmd/rip/cmd_smoke.go index bb3bd1d..019b19a 100644 --- a/cmd/rip/cmd_smoke.go +++ b/cmd/rip/cmd_smoke.go @@ -8,6 +8,7 @@ import ( "streamrip-go/internal/app" "streamrip-go/internal/config" + "streamrip-go/internal/jsonutil" "streamrip-go/internal/provider" ) @@ -24,7 +25,7 @@ func cmdSoundcloudSmoke(ctx context.Context, cfg *config.Config, _ globalOptions fmt.Fprintf(os.Stderr, "soundcloud smoke error: %v\n", err) return 1 } - fmt.Printf("soundcloud oembed ok: title=%q author=%q provider=%q\n", asString(meta["title"]), asString(meta["author_name"]), asString(meta["provider_name"])) + fmt.Printf("soundcloud oembed ok: title=%q author=%q provider=%q\n", jsonutil.StringFromAny(meta["title"]), jsonutil.StringFromAny(meta["author_name"]), jsonutil.StringFromAny(meta["provider_name"])) return 0 } @@ -157,11 +158,11 @@ func cmdQobuzSearchSmoke(ctx context.Context, cfg *config.Config, _ globalOption if !ok { continue } - id := asString(item["id"]) - title := asString(item["title"]) - version := asString(item["version"]) - bitDepth := asString(item["maximum_bit_depth"]) - sr := asString(item["maximum_sampling_rate"]) + id := jsonutil.StringFromAny(item["id"]) + title := jsonutil.StringFromAny(item["title"]) + version := jsonutil.StringFromAny(item["version"]) + bitDepth := jsonutil.StringFromAny(item["maximum_bit_depth"]) + sr := jsonutil.StringFromAny(item["maximum_sampling_rate"]) if version != "" { title = title + " (" + version + ")" } @@ -211,7 +212,7 @@ func cmdTidalSearchSmoke(ctx context.Context, cfg *config.Config, _ globalOption if !ok { item = wrapper } - fmt.Printf("album_id=%s | %s\n", asString(item["id"]), asString(item["title"])) + fmt.Printf("album_id=%s | %s\n", jsonutil.StringFromAny(item["id"]), jsonutil.StringFromAny(item["title"])) } } @@ -244,9 +245,9 @@ func cmdTidalMetadataSmoke(ctx context.Context, cfg *config.Config, _ globalOpti fmt.Fprintf(os.Stderr, "metadata error: %v\n", err) return 1 } - title := asString(meta["title"]) + title := jsonutil.StringFromAny(meta["title"]) if title == "" { - title = asString(meta["name"]) + title = jsonutil.StringFromAny(meta["name"]) } trackCount := 0 if tracksMap, ok := meta["tracks"].(map[string]any); ok { @@ -298,9 +299,9 @@ func cmdTidalVideoSmoke(ctx context.Context, cfg *config.Config, _ globalOptions fmt.Fprintf(os.Stderr, "video downloadable error: %v\n", err) return 1 } - title := asString(meta["title"]) + title := jsonutil.StringFromAny(meta["title"]) if title == "" { - title = asString(meta["name"]) + title = jsonutil.StringFromAny(meta["name"]) } fmt.Printf("tidal video ok: id=%s title=%q ext=%s\n", videoID, title, d.Extension) fmt.Printf("stream_url=%s\n", d.URL) diff --git a/cmd/rip/cmd_test.go b/cmd/rip/cmd_test.go new file mode 100644 index 0000000..3d28538 --- /dev/null +++ b/cmd/rip/cmd_test.go @@ -0,0 +1,248 @@ +package main + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "streamrip-go/internal/config" +) + +// testConfig returns a config rooted in a temp dir, so command functions can +// run without touching the user's real config or download folder. +func testConfig(t *testing.T) *config.Config { + t.Helper() + tmp := t.TempDir() + d := config.DefaultConfigData() + d.Downloads.Folder = filepath.Join(tmp, "downloads") + d.Database.DownloadsEnabled = false + d.Database.FailedDownloadsEnabled = false + d.CLI.ProgressBars = false + return &config.Config{Path: filepath.Join(tmp, "config.toml"), File: d, Session: d} +} + +// The command functions became testable when main() was split into run() int +// plus one function per command; previously they were arms of a 1196-line +// switch that called os.Exit directly. + +func TestCommandsRejectMissingArguments(t *testing.T) { + tests := []struct { + name string + run func(ctx context.Context, cfg *config.Config, gopts globalOptions, args []string) int + args []string + }{ + {"url with no args", cmdURL, nil}, + {"file with no args", cmdFile, nil}, + {"config with no args", cmdConfig, nil}, + {"database with no args", cmdDatabase, nil}, + {"database without browse", cmdDatabase, []string{"downloads"}}, + {"database browse without table", cmdDatabase, []string{"browse"}}, + {"id with too few args", cmdID, []string{"qobuz", "album"}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := tc.run(context.Background(), testConfig(t), globalOptions{}, tc.args) + if got != 2 { + t.Fatalf("exit code = %d, want 2 (usage error)", got) + } + }) + } +} + +func TestCmdDatabaseRejectsUnknownTable(t *testing.T) { + if got := cmdDatabase(context.Background(), testConfig(t), globalOptions{}, []string{"browse", "bogus"}); got != 2 { + t.Fatalf("exit code = %d, want 2", got) + } +} + +func TestCmdConfigRejectsUnknownSubcommand(t *testing.T) { + if got := cmdConfig(context.Background(), testConfig(t), globalOptions{}, []string{"bogus"}); got != 2 { + t.Fatalf("exit code = %d, want 2", got) + } +} + +func TestCmdConfigPathPrintsPath(t *testing.T) { + cfg := testConfig(t) + out := captureStdout(t, func() { + if got := cmdConfig(context.Background(), cfg, globalOptions{}, []string{"path"}); got != 0 { + t.Errorf("exit code = %d, want 0", got) + } + }) + if !strings.Contains(out, cfg.Path) { + t.Fatalf("output %q does not contain the config path %q", out, cfg.Path) + } +} + +func TestCmdConfigPathRejectsExtraArgument(t *testing.T) { + if got := cmdConfig(context.Background(), testConfig(t), globalOptions{}, []string{"path", "extra"}); got != 2 { + t.Fatalf("exit code = %d, want 2", got) + } +} + +func TestCmdFileReportsMissingFile(t *testing.T) { + missing := filepath.Join(t.TempDir(), "nope.txt") + if got := cmdFile(context.Background(), testConfig(t), globalOptions{}, []string{missing}); got != 1 { + t.Fatalf("exit code = %d, want 1 (read error)", got) + } +} + +func TestCmdFileRejectsUnknownOption(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "urls.txt") + if err := os.WriteFile(path, []byte("https://play.qobuz.com/album/abc\n"), 0o600); err != nil { + t.Fatal(err) + } + if got := cmdFile(context.Background(), testConfig(t), globalOptions{}, []string{path, "--bogus"}); got != 2 { + t.Fatalf("exit code = %d, want 2", got) + } +} + +func TestCmdURLRejectsInvalidURL(t *testing.T) { + // Nothing resolvable is queued, so this reports "nothing to rip" and exits 0. + out := captureStdout(t, func() { + if got := cmdURL(context.Background(), testConfig(t), globalOptions{}, []string{"not-a-url"}); got != 0 { + t.Errorf("exit code = %d, want 0", got) + } + }) + if !strings.Contains(out, "nothing to rip") { + t.Fatalf("output %q should report that nothing was queued", out) + } +} + +func TestCmdIDRejectsQualityBelowSourceFloor(t *testing.T) { + // Qobuz has no quality 0. + if got := cmdID(context.Background(), testConfig(t), globalOptions{}, []string{"qobuz", "album", "abc", "0"}); got != 2 { + t.Fatalf("exit code = %d, want 2", got) + } + if got := cmdID(context.Background(), testConfig(t), globalOptions{}, []string{"beatport", "album", "abc", "0"}); got != 2 { + t.Fatalf("beatport exit code = %d, want 2", got) + } +} + +func TestCmdIDRejectsUnknownOption(t *testing.T) { + if got := cmdID(context.Background(), testConfig(t), globalOptions{}, []string{"qobuz", "album", "abc", "--bogus"}); got != 2 { + t.Fatalf("exit code = %d, want 2", got) + } +} + +func TestSmokeCommandsRejectMissingArguments(t *testing.T) { + tests := []struct { + name string + run func(ctx context.Context, cfg *config.Config, gopts globalOptions, args []string) int + args []string + }{ + {"soundcloud", cmdSoundcloudSmoke, nil}, + {"qobuz", cmdQobuzSmoke, nil}, + {"qobuz convert", cmdQobuzConvertRipSmoke, []string{"id"}}, + {"qobuz search", cmdQobuzSearchSmoke, nil}, + {"tidal search", cmdTidalSearchSmoke, nil}, + {"tidal metadata", cmdTidalMetadataSmoke, []string{"track"}}, + {"tidal video", cmdTidalVideoSmoke, nil}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := tc.run(context.Background(), testConfig(t), globalOptions{}, tc.args); got != 2 { + t.Fatalf("exit code = %d, want 2", got) + } + }) + } +} + +// captureStdout swaps os.Stdout for a pipe while fn runs. The commands print +// user-facing output with fmt.Println rather than taking a writer. +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + orig := os.Stdout + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + os.Stdout = w + + done := make(chan string, 1) + go func() { + var sb strings.Builder + buf := make([]byte, 4096) + for { + n, readErr := r.Read(buf) + if n > 0 { + sb.Write(buf[:n]) + } + if readErr != nil { + break + } + } + done <- sb.String() + }() + + fn() + + _ = w.Close() + os.Stdout = orig + out := <-done + _ = r.Close() + return out +} + +func TestVersionOutput(t *testing.T) { + var sb strings.Builder + printVersion(&sb) + out := sb.String() + if !strings.HasPrefix(out, "rip ") { + t.Errorf("version output should start with \"rip \": %q", out) + } + for _, want := range []string{"go:", "target:"} { + if !strings.Contains(out, want) { + t.Errorf("version output missing %q: %q", want, out) + } + } +} + +func TestParseGlobalArgsVersionFlag(t *testing.T) { + for _, flag := range []string{"--version", "-V"} { + opts, err := parseGlobalArgs([]string{flag}) + if err != nil { + t.Fatalf("parseGlobalArgs(%q) error = %v", flag, err) + } + if !opts.version { + t.Errorf("parseGlobalArgs(%q) did not set version", flag) + } + } +} + +func TestApplyQualityOverrideCoversEverySource(t *testing.T) { + cfg := testConfig(t) + applyQualityOverride(cfg, 4) + + got := map[string]int{ + "qobuz": cfg.Session.Qobuz.Quality, + "tidal": cfg.Session.Tidal.Quality, + "deezer": cfg.Session.Deezer.Quality, + "soundcloud": cfg.Session.Soundcloud.Quality, + "yandex": cfg.Session.Yandex.Quality, + "beatport": cfg.Session.Beatport.Quality, + } + for source, q := range got { + if q != 4 { + t.Errorf("%s quality = %d, want 4; -q must reach every source", source, q) + } + } +} + +func TestApplyQualityOverrideClampsSourceFloor(t *testing.T) { + cfg := testConfig(t) + applyQualityOverride(cfg, 0) + + if cfg.Session.Qobuz.Quality != 1 { + t.Errorf("qobuz quality = %d, want it clamped to 1", cfg.Session.Qobuz.Quality) + } + if cfg.Session.Beatport.Quality != 1 { + t.Errorf("beatport quality = %d, want it clamped to 1", cfg.Session.Beatport.Quality) + } + if cfg.Session.Tidal.Quality != 0 { + t.Errorf("tidal quality = %d, want 0 preserved", cfg.Session.Tidal.Quality) + } +} diff --git a/cmd/rip/helpers.go b/cmd/rip/helpers.go index c430bb6..441fbbc 100644 --- a/cmd/rip/helpers.go +++ b/cmd/rip/helpers.go @@ -13,6 +13,7 @@ import ( "strings" "streamrip-go/internal/app" + "streamrip-go/internal/jsonutil" "streamrip-go/internal/urlparse" ) @@ -94,12 +95,12 @@ func parseFileInput(content []byte) ([]fileIDItem, []string, int, bool, error) { if !ok { return nil, nil, 0, true, fmt.Errorf("json item %d must be an object", i+1) } - source := strings.ToLower(strings.TrimSpace(asString(entry["source"]))) - mediaType := strings.ToLower(strings.TrimSpace(asString(entry["media_type"]))) + source := strings.ToLower(strings.TrimSpace(jsonutil.StringFromAny(entry["source"]))) + mediaType := strings.ToLower(strings.TrimSpace(jsonutil.StringFromAny(entry["media_type"]))) if mediaType == "" { - mediaType = strings.ToLower(strings.TrimSpace(asString(entry["mediaType"]))) + mediaType = strings.ToLower(strings.TrimSpace(jsonutil.StringFromAny(entry["mediaType"]))) } - id := strings.TrimSpace(asString(entry["id"])) + id := strings.TrimSpace(jsonutil.StringFromAny(entry["id"])) if source == "" || mediaType == "" || id == "" { return nil, nil, 0, true, fmt.Errorf("json item %d missing source/media_type/id", i+1) } diff --git a/cmd/rip/lastfm.go b/cmd/rip/lastfm.go index 2d96fb3..76135ef 100644 --- a/cmd/rip/lastfm.go +++ b/cmd/rip/lastfm.go @@ -13,6 +13,7 @@ import ( "time" "streamrip-go/internal/app" + "streamrip-go/internal/jsonutil" "streamrip-go/internal/netutil" "streamrip-go/internal/provider" ) @@ -241,8 +242,8 @@ func extractLastFMTitleArtistPairs(page string) []lastFMTrack { if len(dataPairs) > 0 { out := make([]lastFMTrack, 0, len(dataPairs)) for _, m := range dataPairs { - title := html.UnescapeString(strings.TrimSpace(firstNonEmpty(m[1], m[2]))) - artist := html.UnescapeString(strings.TrimSpace(firstNonEmpty(m[3], m[4]))) + title := html.UnescapeString(strings.TrimSpace(jsonutil.FirstNonEmpty(m[1], m[2]))) + artist := html.UnescapeString(strings.TrimSpace(jsonutil.FirstNonEmpty(m[3], m[4]))) if title == "" || artist == "" { continue } @@ -256,8 +257,8 @@ func extractLastFMTitleArtistPairs(page string) []lastFMTrack { titles := lastFMTitleTagsRe.FindAllStringSubmatch(page, -1) out := make([]lastFMTrack, 0, len(titles)/2) for i := 0; i+1 < len(titles); i += 2 { - titleRaw := strings.TrimSpace(firstNonEmpty(titles[i][1], titles[i][2])) - artistRaw := strings.TrimSpace(firstNonEmpty(titles[i+1][1], titles[i+1][2])) + titleRaw := strings.TrimSpace(jsonutil.FirstNonEmpty(titles[i][1], titles[i][2])) + artistRaw := strings.TrimSpace(jsonutil.FirstNonEmpty(titles[i+1][1], titles[i+1][2])) if strings.EqualFold(titleRaw, "Play on YouTube") || strings.EqualFold(artistRaw, "Play on YouTube") { continue } @@ -271,15 +272,6 @@ func extractLastFMTitleArtistPairs(page string) []lastFMTrack { return out } -func firstNonEmpty(items ...string) string { - for _, item := range items { - if strings.TrimSpace(item) != "" { - return strings.TrimSpace(item) - } - } - return "" -} - func extractLastFMTracksFromMirrorMarkdown(md string) (string, []lastFMTrack) { lines := strings.Split(strings.ReplaceAll(md, "\r\n", "\n"), "\n") title := "" diff --git a/cmd/rip/main.go b/cmd/rip/main.go index be2600a..7c67997 100644 --- a/cmd/rip/main.go +++ b/cmd/rip/main.go @@ -65,13 +65,13 @@ func run() int { cfg, err := config.Load(gopts.configPath) if err != nil { if errors.Is(err, config.ErrOutdatedConfig) { - resolvedPath, upErr := config.UpgradeOutdated(gopts.configPath) + resolvedPath, backupPath, upErr := config.UpgradeOutdated(gopts.configPath) if upErr != nil { fmt.Fprintf(os.Stderr, "config error: %v\n", err) fmt.Fprintf(os.Stderr, "config auto-upgrade failed: %v\n", upErr) return 1 } - fmt.Fprintf(os.Stderr, "config upgraded at %s\n", resolvedPath) + fmt.Fprintf(os.Stderr, "config upgraded at %s (previous copy kept at %s)\n", resolvedPath, backupPath) cfg, err = config.Load(gopts.configPath) } } diff --git a/cmd/rip/search.go b/cmd/rip/search.go index 93dc49f..a2c22fd 100644 --- a/cmd/rip/search.go +++ b/cmd/rip/search.go @@ -330,38 +330,38 @@ func normalizeSearchResults(source, mediaType string, pages []map[string]any) [] if !ok { continue } - id := asString(itm["id"]) - title := asString(itm["title"]) + id := jsonutil.StringFromAny(itm["id"]) + title := jsonutil.StringFromAny(itm["title"]) if title == "" { - title = asString(itm["name"]) + title = jsonutil.StringFromAny(itm["name"]) } - if version := asString(itm["version"]); version != "" { + if version := jsonutil.StringFromAny(itm["version"]); version != "" { title += " (" + version + ")" } - artist := nestedSearchString(itm, "artist", "name") + artist := jsonutil.NestedString(itm, "artist", "name") if artist == "" { - artist = nestedSearchString(itm, "performer", "name") + artist = jsonutil.NestedString(itm, "performer", "name") } - album := nestedSearchString(itm, "album", "title") - trackCount := searchInt(itm["tracks_count"]) + album := jsonutil.NestedString(itm, "album", "title") + trackCount := jsonutil.IntFromAny(itm["tracks_count"]) if trackCount == 0 { - trackCount = searchInt(itm["track_count"]) + trackCount = jsonutil.IntFromAny(itm["track_count"]) } explicit := searchBool(itm["parental_warning"]) - date := firstNonEmpty( - asString(itm["release_date_original"]), - asString(itm["release_date"]), - asString(itm["releaseDate"]), - asString(itm["streamStartDate"]), - nestedSearchString(itm, "album", "release_date_original"), - nestedSearchString(itm, "album", "release_date"), + date := jsonutil.FirstNonEmpty( + jsonutil.StringFromAny(itm["release_date_original"]), + jsonutil.StringFromAny(itm["release_date"]), + jsonutil.StringFromAny(itm["releaseDate"]), + jsonutil.StringFromAny(itm["streamStartDate"]), + jsonutil.NestedString(itm, "album", "release_date_original"), + jsonutil.NestedString(itm, "album", "release_date"), ) releases := 0 if mediaType == "artist" { - releases = firstPositiveInt( - searchInt(itm["albums_count"]), - searchInt(itm["albumsCount"]), - nestedSearchInt(itm, "albums", "total"), + releases = jsonutil.FirstPositiveInt( + jsonutil.IntFromAny(itm["albums_count"]), + jsonutil.IntFromAny(itm["albumsCount"]), + jsonutil.IntFromAny(jsonutil.NestedAny(itm, "albums", "total")), ) } appendUnique(searchResult{ID: id, Title: title, Artist: artist, Album: album, Date: date, Releases: releases, TrackCount: trackCount, Explicit: explicit}) @@ -379,39 +379,39 @@ func normalizeSearchResults(source, mediaType string, pages []map[string]any) [] if wrapped, ok := itm["item"].(map[string]any); ok { itm = wrapped } - id := asString(itm["id"]) - title := asString(itm["title"]) + id := jsonutil.StringFromAny(itm["id"]) + title := jsonutil.StringFromAny(itm["title"]) if title == "" { - title = asString(itm["name"]) + title = jsonutil.StringFromAny(itm["name"]) } - artist := nestedSearchString(itm, "artist", "name") + artist := jsonutil.NestedString(itm, "artist", "name") if artist == "" { if artists, ok := itm["artists"].([]any); ok && len(artists) > 0 { if a0, ok := artists[0].(map[string]any); ok { - artist = asString(a0["name"]) + artist = jsonutil.StringFromAny(a0["name"]) } } } - album := nestedSearchString(itm, "album", "title") - trackCount := searchInt(itm["numberOfTracks"]) + album := jsonutil.NestedString(itm, "album", "title") + trackCount := jsonutil.IntFromAny(itm["numberOfTracks"]) if trackCount == 0 { - trackCount = searchInt(itm["tracks_count"]) + trackCount = jsonutil.IntFromAny(itm["tracks_count"]) } explicit := searchBool(itm["explicit"]) - date := firstNonEmpty( - asString(itm["releaseDate"]), - asString(itm["streamStartDate"]), - asString(itm["release_date"]), - nestedSearchString(itm, "album", "releaseDate"), - nestedSearchString(itm, "album", "release_date"), + date := jsonutil.FirstNonEmpty( + jsonutil.StringFromAny(itm["releaseDate"]), + jsonutil.StringFromAny(itm["streamStartDate"]), + jsonutil.StringFromAny(itm["release_date"]), + jsonutil.NestedString(itm, "album", "releaseDate"), + jsonutil.NestedString(itm, "album", "release_date"), ) releases := 0 if mediaType == "artist" { - releases = firstPositiveInt( - searchInt(itm["numberOfAlbums"]), - searchInt(itm["albums_count"]), - searchInt(itm["albumsCount"]), - nestedSearchInt(itm, "albums", "total"), + releases = jsonutil.FirstPositiveInt( + jsonutil.IntFromAny(itm["numberOfAlbums"]), + jsonutil.IntFromAny(itm["albums_count"]), + jsonutil.IntFromAny(itm["albumsCount"]), + jsonutil.IntFromAny(jsonutil.NestedAny(itm, "albums", "total")), ) } appendUnique(searchResult{ID: id, Title: title, Artist: artist, Album: album, Date: date, Releases: releases, TrackCount: trackCount, Explicit: explicit}) @@ -431,25 +431,25 @@ func normalizeSearchResults(source, mediaType string, pages []map[string]any) [] if !ok { continue } - id := asString(itm["id"]) - title := asString(itm["title"]) + id := jsonutil.StringFromAny(itm["id"]) + title := jsonutil.StringFromAny(itm["title"]) if title == "" { - title = asString(itm["name"]) + title = jsonutil.StringFromAny(itm["name"]) } - artist := nestedSearchString(itm, "artist", "name") - album := nestedSearchString(itm, "album", "title") - trackCount := searchInt(itm["nb_tracks"]) + artist := jsonutil.NestedString(itm, "artist", "name") + album := jsonutil.NestedString(itm, "album", "title") + trackCount := jsonutil.IntFromAny(itm["nb_tracks"]) explicit := searchBool(itm["explicit_lyrics"]) - date := firstNonEmpty( - asString(itm["release_date"]), - nestedSearchString(itm, "album", "release_date"), + date := jsonutil.FirstNonEmpty( + jsonutil.StringFromAny(itm["release_date"]), + jsonutil.NestedString(itm, "album", "release_date"), ) releases := 0 if mediaType == "artist" { - releases = firstPositiveInt( - searchInt(itm["nb_album"]), - searchInt(itm["albums_count"]), - searchInt(itm["numberOfAlbums"]), + releases = jsonutil.FirstPositiveInt( + jsonutil.IntFromAny(itm["nb_album"]), + jsonutil.IntFromAny(itm["albums_count"]), + jsonutil.IntFromAny(itm["numberOfAlbums"]), ) } appendUnique(searchResult{ID: id, Title: title, Artist: artist, Album: album, Date: date, Releases: releases, TrackCount: trackCount, Explicit: explicit}) @@ -464,13 +464,13 @@ func normalizeSearchResults(source, mediaType string, pages []map[string]any) [] if !ok { continue } - id := asString(itm["id"]) - title := asString(itm["title"]) - artist := nestedSearchString(itm, "artist", "name") - trackCount := searchInt(itm["tracks_count"]) - date := firstNonEmpty( - asString(itm["release_date"]), - asString(itm["display_date"]), + id := jsonutil.StringFromAny(itm["id"]) + title := jsonutil.StringFromAny(itm["title"]) + artist := jsonutil.NestedString(itm, "artist", "name") + trackCount := jsonutil.IntFromAny(itm["tracks_count"]) + date := jsonutil.FirstNonEmpty( + jsonutil.StringFromAny(itm["release_date"]), + jsonutil.StringFromAny(itm["display_date"]), ) appendUnique(searchResult{ID: id, Title: title, Artist: artist, Date: date, TrackCount: trackCount}) } @@ -484,34 +484,34 @@ func normalizeSearchResults(source, mediaType string, pages []map[string]any) [] if !ok { continue } - id := asString(itm["id"]) - title := asString(itm["title"]) + id := jsonutil.StringFromAny(itm["id"]) + title := jsonutil.StringFromAny(itm["title"]) if title == "" { - title = asString(itm["name"]) + title = jsonutil.StringFromAny(itm["name"]) } - artist := nestedSearchString(itm, "artist", "name") + artist := jsonutil.NestedString(itm, "artist", "name") if artist == "" { - artist = nestedSearchString(itm, "performer", "name") + artist = jsonutil.NestedString(itm, "performer", "name") } - album := nestedSearchString(itm, "album", "title") - trackCount := firstPositiveInt( - searchInt(itm["trackCount"]), - searchInt(itm["track_count"]), - searchInt(itm["tracks_count"]), + album := jsonutil.NestedString(itm, "album", "title") + trackCount := jsonutil.FirstPositiveInt( + jsonutil.IntFromAny(itm["trackCount"]), + jsonutil.IntFromAny(itm["track_count"]), + jsonutil.IntFromAny(itm["tracks_count"]), ) explicit := searchBool(itm["explicit"]) - date := firstNonEmpty( - asString(itm["release_date"]), - asString(itm["releaseDate"]), - nestedSearchString(itm, "album", "release_date"), - nestedSearchString(itm, "album", "releaseDate"), + date := jsonutil.FirstNonEmpty( + jsonutil.StringFromAny(itm["release_date"]), + jsonutil.StringFromAny(itm["releaseDate"]), + jsonutil.NestedString(itm, "album", "release_date"), + jsonutil.NestedString(itm, "album", "releaseDate"), ) releases := 0 if mediaType == "artist" { - releases = firstPositiveInt( - searchInt(itm["albums_count"]), - searchInt(itm["numberOfAlbums"]), - nestedSearchInt(itm, "albums", "total"), + releases = jsonutil.FirstPositiveInt( + jsonutil.IntFromAny(itm["albums_count"]), + jsonutil.IntFromAny(itm["numberOfAlbums"]), + jsonutil.IntFromAny(jsonutil.NestedAny(itm, "albums", "total")), ) } appendUnique(searchResult{ID: id, Title: title, Artist: artist, Album: album, Date: date, Releases: releases, TrackCount: trackCount, Explicit: explicit}) @@ -526,18 +526,18 @@ func normalizeSearchResults(source, mediaType string, pages []map[string]any) [] if !ok { continue } - id := asString(itm["id"]) - title := asString(itm["title"]) + id := jsonutil.StringFromAny(itm["id"]) + title := jsonutil.StringFromAny(itm["title"]) if title == "" { - title = asString(itm["name"]) + title = jsonutil.StringFromAny(itm["name"]) } - if version := asString(itm["version"]); version != "" { + if version := jsonutil.StringFromAny(itm["version"]); version != "" { title += " (" + version + ")" } - artist := nestedSearchString(itm, "artist", "name") - album := nestedSearchString(itm, "album", "title") - trackCount := firstPositiveInt(searchInt(itm["tracks_count"]), searchInt(itm["track_count"])) - date := firstNonEmpty(asString(itm["release_date_original"]), asString(itm["release_date"]), nestedSearchString(itm, "album", "release_date_original")) + artist := jsonutil.NestedString(itm, "artist", "name") + album := jsonutil.NestedString(itm, "album", "title") + trackCount := jsonutil.FirstPositiveInt(jsonutil.IntFromAny(itm["tracks_count"]), jsonutil.IntFromAny(itm["track_count"])) + date := jsonutil.FirstNonEmpty(jsonutil.StringFromAny(itm["release_date_original"]), jsonutil.StringFromAny(itm["release_date"]), jsonutil.NestedString(itm, "album", "release_date_original")) appendUnique(searchResult{ID: id, Title: title, Artist: artist, Album: album, Date: date, TrackCount: trackCount}) } } @@ -608,55 +608,10 @@ func formatSearchOptionLabel(index int, mediaType string, r searchResult) string } } -func nestedSearchString(v map[string]any, keys ...string) string { - cur := any(v) - for _, key := range keys { - m, ok := cur.(map[string]any) - if !ok { - return "" - } - cur = m[key] - } - return asString(cur) -} - -func nestedSearchInt(v map[string]any, keys ...string) int { - cur := any(v) - for _, key := range keys { - m, ok := cur.(map[string]any) - if !ok { - return 0 - } - cur = m[key] - } - return searchInt(cur) -} - -func firstPositiveInt(values ...int) int { - for _, v := range values { - if v > 0 { - return v - } - } - return 0 -} - -func searchInt(v any) int { - switch t := v.(type) { - case int: - return t - case int64: - return int(t) - case float64: - return int(t) - case string: - i, _ := strconv.Atoi(t) - return i - default: - return 0 - } -} - +// searchBool is deliberately stricter than jsonutil.BoolFromAny: it accepts +// only a real bool. The explicit/parental-warning fields it reads are proper +// booleans in every provider's search payload, and widening this to coerce +// strings and numbers would change how results are labelled. func searchBool(v any) bool { b, ok := v.(bool) return ok && b diff --git a/internal/config/config.go b/internal/config/config.go index 603b5b0..1655cba 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strings" "sync" + "time" "github.com/pelletier/go-toml/v2" ) @@ -370,25 +371,42 @@ func NormalizeCodec(raw string) (string, error) { } } -func UpgradeOutdated(path string) (string, error) { +// UpgradeOutdated rewrites an outdated config at the current version. It runs +// automatically, without prompting, so it copies the original aside first: the +// rewrite re-serializes the whole file from the parsed struct, which drops any +// key this version does not know about. It returns the config path and the +// backup path. +// +// This is a version stamp, not a migration framework. There are no per-version +// transforms, so a future key rename still needs explicit handling here. +func UpgradeOutdated(path string) (resolved string, backup string, err error) { resolvedPath, err := resolvePath(path) if err != nil { - return "", err + return "", "", err } raw, err := os.ReadFile(resolvedPath) if err != nil { - return "", err + return "", "", err } + + backupPath := fmt.Sprintf("%s.bak-%d", resolvedPath, time.Now().Unix()) + if err = os.WriteFile(backupPath, raw, 0o600); err != nil { + return "", "", fmt.Errorf("could not back up %s before upgrading: %w", resolvedPath, err) + } + data := DefaultConfigData() if err = toml.Unmarshal(raw, &data); err != nil { - return "", err + return "", "", err } applyRuntimeDefaults(&data) data.Misc.Version = CurrentConfigVersion - if err = saveConfigData(resolvedPath, data); err != nil { - return "", err + if err = Validate(&data); err != nil { + return "", "", err } - return resolvedPath, nil + if err = saveConfigData(resolvedPath, data); err != nil { + return "", "", err + } + return resolvedPath, backupPath, nil } func (c *Config) SaveFile() error { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index b75d3e9..b21b442 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -70,7 +70,7 @@ func TestUpgradeOutdatedConfig(t *testing.T) { t.Fatalf("saveConfigData() error = %v", err) } - resolved, err := UpgradeOutdated(path) + resolved, backup, err := UpgradeOutdated(path) if err != nil { t.Fatalf("UpgradeOutdated() error = %v", err) } @@ -78,6 +78,28 @@ func TestUpgradeOutdatedConfig(t *testing.T) { t.Fatalf("resolved path = %q, want %q", resolved, path) } + // The upgrade re-serializes the whole file and runs without prompting, so + // it must leave the original behind. + if backup == "" { + t.Fatal("UpgradeOutdated() returned no backup path") + } + backupRaw, err := os.ReadFile(backup) + if err != nil { + t.Fatalf("backup not written: %v", err) + } + var backedUp ConfigData + if err := toml.Unmarshal(backupRaw, &backedUp); err != nil { + t.Fatalf("backup is not valid toml: %v", err) + } + if backedUp.Misc.Version != "1.0.0" { + t.Errorf("backup version = %q, want the pre-upgrade 1.0.0", backedUp.Misc.Version) + } + if info, err := os.Stat(backup); err != nil { + t.Fatal(err) + } else if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("backup permissions = %04o, want 0600 (it holds the same credentials)", perm) + } + cfg, err := Load(path) if err != nil { t.Fatalf("Load() after upgrade error = %v", err) diff --git a/internal/jsonutil/jsonutil.go b/internal/jsonutil/jsonutil.go index ddc7497..2cf28de 100644 --- a/internal/jsonutil/jsonutil.go +++ b/internal/jsonutil/jsonutil.go @@ -145,3 +145,16 @@ func DisplayArtistNames(names []string) string { return strings.Join(names[:len(names)-1], ", ") + " & " + names[len(names)-1] } } + +// FirstPositiveInt returns the first value greater than zero, or 0 if there is +// none. Useful for picking among optional metadata fields where 0 means +// "absent". Previously duplicated in provider/yandex, provider/beatport and +// cmd/rip. +func FirstPositiveInt(values ...int) int { + for _, v := range values { + if v > 0 { + return v + } + } + return 0 +} diff --git a/internal/jsonutil/jsonutil_test.go b/internal/jsonutil/jsonutil_test.go index f1cf88e..02f2369 100644 --- a/internal/jsonutil/jsonutil_test.go +++ b/internal/jsonutil/jsonutil_test.go @@ -182,3 +182,22 @@ func TestDisplayArtistNames(t *testing.T) { } } } + +func TestFirstPositiveInt(t *testing.T) { + tests := []struct { + in []int + want int + }{ + {nil, 0}, + {[]int{0, 0}, 0}, + {[]int{0, 5, 7}, 5}, + {[]int{-3, 0, 2}, 2}, + {[]int{9}, 9}, + {[]int{-1}, 0}, + } + for _, tc := range tests { + if got := FirstPositiveInt(tc.in...); got != tc.want { + t.Errorf("FirstPositiveInt(%v) = %d, want %d", tc.in, got, tc.want) + } + } +} diff --git a/internal/provider/beatport/client.go b/internal/provider/beatport/client.go index 1f16c2c..75045cd 100644 --- a/internal/provider/beatport/client.go +++ b/internal/provider/beatport/client.go @@ -532,7 +532,7 @@ func (c *Client) normalizeRelease(raw map[string]any, tracks []any) map[string]a "title": jsonutil.StringFromAny(raw["name"]), "artist": map[string]any{"name": artist}, "release_date_original": jsonutil.StringFromAny(raw["new_release_date"]), - "tracks_count": firstPositiveInt(jsonutil.IntFromAny(raw["track_count"]), len(items)), + "tracks_count": jsonutil.FirstPositiveInt(jsonutil.IntFromAny(raw["track_count"]), len(items)), "maximum_bit_depth": 16, "maximum_sampling_rate": "44.1", "image": imageMap(raw["image"]), @@ -648,7 +648,7 @@ func playlistMetadata(raw map[string]any, tracks []any) map[string]any { "id": jsonutil.StringFromAny(raw["id"]), "name": name, "title": name, - "tracks_count": firstPositiveInt(jsonutil.IntFromAny(raw["track_count"]), len(tracks)), + "tracks_count": jsonutil.FirstPositiveInt(jsonutil.IntFromAny(raw["track_count"]), len(tracks)), "image": imageMap(raw["image"]), "tracks": map[string]any{"items": tracks}, } @@ -829,15 +829,6 @@ func sliceAny(v any) []any { return s } -func firstPositiveInt(vals ...int) int { - for _, v := range vals { - if v > 0 { - return v - } - } - return 0 -} - func cloneValues(in url.Values) url.Values { out := url.Values{} for k, vals := range in { diff --git a/internal/provider/yandex/client.go b/internal/provider/yandex/client.go index 0f1b816..a20b20e 100644 --- a/internal/provider/yandex/client.go +++ b/internal/provider/yandex/client.go @@ -581,7 +581,7 @@ func normalizeTrack(raw map[string]any, fallbackID string, artistSeparator strin } trackNumber, discNumber := trackNumbers(albumRaw) trackTotal := jsonutil.IntFromAny(albumRaw["trackCount"]) - discTotal := firstPositiveInt( + discTotal := jsonutil.FirstPositiveInt( jsonutil.IntFromAny(raw["volumesCount"]), jsonutil.IntFromAny(albumRaw["volumesCount"]), ) @@ -726,7 +726,7 @@ func normalizeArtistSearchItem(raw map[string]any) map[string]any { "id": jsonutil.StringFromAny(raw["id"]), "title": jsonutil.StringFromAny(raw["name"]), "name": jsonutil.StringFromAny(raw["name"]), - "albums_count": firstPositiveInt( + "albums_count": jsonutil.FirstPositiveInt( jsonutil.IntFromAny(jsonutil.NestedAny(raw, "counts", "directAlbums")), jsonutil.IntFromAny(jsonutil.NestedAny(raw, "counts", "tracks")), ), @@ -833,7 +833,7 @@ func trackNumbers(albumRaw map[string]any) (int, int) { if trackNumber > 0 { trackNumber++ } - return trackNumber, firstPositiveInt(jsonutil.IntFromAny(position["volume"]), 1) + return trackNumber, jsonutil.FirstPositiveInt(jsonutil.IntFromAny(position["volume"]), 1) } func canonicalTrackRefFromRaw(raw map[string]any, fallback string) string { @@ -1019,15 +1019,6 @@ func waitRetry(ctx context.Context, delay time.Duration) error { } } -func firstPositiveInt(vals ...int) int { - for _, v := range vals { - if v > 0 { - return v - } - } - return 0 -} - // signKey and legacyMP3Salt read the vendor request-signing keys from config, // falling back to the built-in defaults. func (c *Client) signKey() string {