refactor: delete dead code, dedupe helpers, add tests for untested packages

Dead code Removed, each verified to have no non-test caller: promptSearchSelection (77 lines, superseded by promptSearchSelectionMenu), AddPlaylistByTrackIDs, download.NewWithVerifySSL, qobuz.Client.GetTrackMetadata, tidal.Client.trackSupportsAtmos (test-only, and the comment above GetDownloadable already documented its removal from the live path) and its now-orphaned test, yandex.canonicalTrackRequestID (a no-op returning its trimmed input; the call site uses strings.TrimSpace directly) and the unused defaultEstimatedKbps constant. Config keys that did nothing qobuz.download_booklets and misc.check_for_updates were declared, defaulted and documented in config.toml.example but read by no code. The whole [youtube] block went too: there is no YouTube provider. go-toml ignores unknown keys, verified, so existing user configs still load. Duplicated helpers tidal.stringify and yandex.firstNonEmpty were byte-identical copies of jsonutil.StringFromAny and jsonutil.FirstNonEmpty. displayArtistNames existed three times, byte-identical, in internal/app, provider/yandex and provider/beatport; it now lives in jsonutil. The CLI's normalizeCodec delegates to config.NormalizeCodec so the flag and the config file are checked by the same rules. Tests for packages that had none internal/jsonutil (0% -> 100%) and internal/netutil (0% -> 78%), the latter covering redactURL, which is a security control, and the zero-timeout contract that internal/download and the qobuz body copy depend on. internal/config rose to 82% with coverage of the new Validate(), the 0600 permissions, CloneConfigData not sharing slices, and a concurrent UpdateTokens test that exercises the mutex under -race. Test suite is ~85x faster Provider tests built clients with the default 60 requests/minute, so the rate limiter slept 1s per HTTP call: deezer took 31s, tidal 11s, qobuz 8s. Each provider test package now builds its config through a testConfigData helper that disables throttling. Whole suite: ~60s -> 0.7s, and 2.8s under -race. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-10-16 22:07:30 +02:00 · 2026-10-03 17:05:55 +02:00
1 parent 201aa5daa7
commit b3b6a2eb1d
@@ -38,7 +38,6 @@ type ConfigData struct {
 	Yandex       YandexConfig                 `toml:"yandex"`
 	Beatport     BeatportConfig               `toml:"beatport"`
 	Soundcloud   SoundcloudConfig             `toml:"soundcloud"`
-	Youtube      YoutubeConfig                `toml:"youtube"`
 	Database     DatabaseConfig               `toml:"database"`
 	Conversion   ConversionConfig             `toml:"conversion"`
 	QobuzFilters QobuzDiscographyFilterConfig `toml:"qobuz_filters"`
@@ -61,13 +60,12 @@ type DownloadsConfig struct {
 }

 type QobuzConfig struct {
-	Quality          int      `toml:"quality"`
-	DownloadBooklets bool     `toml:"download_booklets"`
-	UseAuthToken     bool     `toml:"use_auth_token"`
-	EmailOrUserID    string   `toml:"email_or_userid"`
-	PasswordOrToken  string   `toml:"password_or_token"`
-	AppID            string   `toml:"app_id"`
-	Secrets          []string `toml:"secrets"`
+	Quality         int      `toml:"quality"`
+	UseAuthToken    bool     `toml:"use_auth_token"`
+	EmailOrUserID   string   `toml:"email_or_userid"`
+	PasswordOrToken string   `toml:"password_or_token"`
+	AppID           string   `toml:"app_id"`
+	Secrets         []string `toml:"secrets"`
 }

 type TidalConfig struct {
@@ -112,12 +110,6 @@ type SoundcloudConfig struct {
 	AppVersion string `toml:"app_version"`
 }

-type YoutubeConfig struct {
-	Quality              int    `toml:"quality"`
-	DownloadVideos       bool   `toml:"download_videos"`
-	VideoDownloadsFolder string `toml:"video_downloads_folder"`
-}
-
 type DatabaseConfig struct {
 	DownloadsEnabled       bool   `toml:"downloads_enabled"`
 	DownloadsPath          string `toml:"downloads_path"`
@@ -177,8 +169,7 @@ type CLIConfig struct {
 }

 type MiscConfig struct {
-	Version         string `toml:"version"`
-	CheckForUpdates bool   `toml:"check_for_updates"`
+	Version string `toml:"version"`
 }

 func Load(path string) (*Config, error) {
@@ -366,9 +357,8 @@ func DefaultConfigData() ConfigData {
 			VerifySSL:            true,
 		},
 		Qobuz: QobuzConfig{
-			Quality:          3,
-			DownloadBooklets: true,
-			UseAuthToken:     false,
+			Quality:      3,
+			UseAuthToken: false,
 		},
 		Tidal: TidalConfig{
 			Quality:        3,
@@ -389,11 +379,6 @@ func DefaultConfigData() ConfigData {
 		Soundcloud: SoundcloudConfig{
 			Quality: 0,
 		},
-		Youtube: YoutubeConfig{
-			Quality:              0,
-			DownloadVideos:       false,
-			VideoDownloadsFolder: filepath.Join(downloadsFolder, "YouTubeVideos"),
-		},
 		Database: DatabaseConfig{
 			DownloadsEnabled:       true,
 			DownloadsPath:          filepath.Join(appDir, "downloads.db"),
@@ -437,8 +422,7 @@ func DefaultConfigData() ConfigData {
 			MaxSearchResults: 100,
 		},
 		Misc: MiscConfig{
-			Version:         CurrentConfigVersion,
-			CheckForUpdates: true,
+			Version: CurrentConfigVersion,
 		},
 	}

@@ -498,9 +482,6 @@ func applyRuntimeDefaults(data *ConfigData) {
 	if data.Database.FailedDownloadsPath == "" {
 		data.Database.FailedDownloadsPath = filepath.Join(appDir, "failed_downloads.db")
 	}
-	if data.Youtube.VideoDownloadsFolder == "" {
-		data.Youtube.VideoDownloadsFolder = filepath.Join(data.Downloads.Folder, "YouTubeVideos")
-	}
 }

 // CloneConfigData returns a deep copy of in. The slice fields must not be
@@ -2,8 +2,11 @@ package config

 import (
 	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
+	"sync"
 	"testing"

 	"github.com/pelletier/go-toml/v2"
@@ -139,3 +142,187 @@ func TestExampleConfigIsValid(t *testing.T) {
 		t.Errorf("config.toml.example version = %q, want %q", data.Misc.Version, CurrentConfigVersion)
 	}
 }
+
+func TestValidateRejectsBadValues(t *testing.T) {
+	tests := []struct {
+		name   string
+		mutate func(*ConfigData)
+		want   string
+	}{
+		{"qobuz quality 0", func(d *ConfigData) { d.Qobuz.Quality = 0 }, "qobuz.quality"},
+		{"qobuz quality 5", func(d *ConfigData) { d.Qobuz.Quality = 5 }, "qobuz.quality"},
+		{"beatport quality 0", func(d *ConfigData) { d.Beatport.Quality = 0 }, "beatport.quality"},
+		{"tidal quality 5", func(d *ConfigData) { d.Tidal.Quality = 5 }, "tidal.quality"},
+		{"deezer quality 3", func(d *ConfigData) { d.Deezer.Quality = 3 }, "deezer.quality"},
+		{"negative quality", func(d *ConfigData) { d.Tidal.Quality = -1 }, "tidal.quality"},
+		{"max_connections 0", func(d *ConfigData) { d.Downloads.MaxConnections = 0 }, "max_connections"},
+		{"max_connections negative", func(d *ConfigData) { d.Downloads.MaxConnections = -5 }, "max_connections"},
+		{"requests_per_minute 0", func(d *ConfigData) { d.Downloads.RequestsPerMinute = 0 }, "requests_per_minute"},
+		{"truncate_to 0", func(d *ConfigData) { d.Filepaths.TruncateTo = 0 }, "truncate_to"},
+		{"bogus codec", func(d *ConfigData) {
+			d.Conversion.Enabled = true
+			d.Conversion.Codec = "bogus"
+		}, "conversion.codec"},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			d := DefaultConfigData()
+			applyRuntimeDefaults(&d)
+			tc.mutate(&d)
+			err := Validate(&d)
+			if err == nil {
+				t.Fatalf("Validate() = nil, want an error mentioning %q", tc.want)
+			}
+			if !strings.Contains(err.Error(), tc.want) {
+				t.Fatalf("Validate() = %q, want it to mention %q", err, tc.want)
+			}
+		})
+	}
+}
+
+func TestValidateAcceptsValidValues(t *testing.T) {
+	d := DefaultConfigData()
+	applyRuntimeDefaults(&d)
+	// Negative requests_per_minute disables throttling, which is legitimate.
+	d.Downloads.RequestsPerMinute = -1
+	d.Soundcloud.Quality = 1
+	d.Deezer.Quality = 2
+	d.Qobuz.Quality = 4
+	if err := Validate(&d); err != nil {
+		t.Fatalf("Validate() = %v, want nil", err)
+	}
+}
+
+func TestValidateNormalizesCodec(t *testing.T) {
+	d := DefaultConfigData()
+	applyRuntimeDefaults(&d)
+	d.Conversion.Enabled = true
+	d.Conversion.Codec = "  ogg  "
+	if err := Validate(&d); err != nil {
+		t.Fatalf("Validate() = %v", err)
+	}
+	if d.Conversion.Codec != "VORBIS" {
+		t.Fatalf("codec = %q, want VORBIS", d.Conversion.Codec)
+	}
+}
+
+func TestNormalizeCodec(t *testing.T) {
+	for in, want := range map[string]string{
+		"flac": "FLAC", "FLAC": "FLAC", " mp3 ": "MP3",
+		"ogg": "VORBIS", "vorbis": "VORBIS", "alac": "ALAC", "aac": "AAC",
+	} {
+		got, err := NormalizeCodec(in)
+		if err != nil {
+			t.Errorf("NormalizeCodec(%q) err = %v", in, err)
+			continue
+		}
+		if got != want {
+			t.Errorf("NormalizeCodec(%q) = %q, want %q", in, got, want)
+		}
+	}
+	if _, err := NormalizeCodec("wav"); err == nil {
+		t.Error("NormalizeCodec(wav) should fail")
+	}
+}
+
+func TestCloneConfigDataDoesNotShareSlices(t *testing.T) {
+	a := DefaultConfigData()
+	a.Qobuz.Secrets = []string{"s1", "s2"}
+	a.Metadata.Exclude = []string{"e1"}
+
+	b := CloneConfigData(a)
+	b.Qobuz.Secrets[0] = "mutated"
+	b.Metadata.Exclude[0] = "mutated"
+
+	if a.Qobuz.Secrets[0] != "s1" {
+		t.Error("Qobuz.Secrets is shared between copies")
+	}
+	if a.Metadata.Exclude[0] != "e1" {
+		t.Error("Metadata.Exclude is shared between copies")
+	}
+}
+
+func TestSaveFileUsesOwnerOnlyPermissions(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "config.toml")
+	cfg := &Config{Path: path, File: DefaultConfigData()}
+	if err := cfg.SaveFile(); err != nil {
+		t.Fatalf("SaveFile() = %v", err)
+	}
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// The file holds plaintext passwords, ARL cookies and OAuth tokens.
+	if perm := info.Mode().Perm(); perm != 0o600 {
+		t.Fatalf("config permissions = %04o, want 0600", perm)
+	}
+}
+
+func TestSaveFileTightensExistingWorldReadableFile(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "config.toml")
+	if err := os.WriteFile(path, []byte("version = \"x\"\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	cfg := &Config{Path: path, File: DefaultConfigData()}
+	if err := cfg.SaveFile(); err != nil {
+		t.Fatal(err)
+	}
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if perm := info.Mode().Perm(); perm != 0o600 {
+		t.Fatalf("permissions = %04o, want 0600 after rewrite", perm)
+	}
+}
+
+func TestUpdateTokensIsSafeWithoutAPath(t *testing.T) {
+	cfg := &Config{File: DefaultConfigData(), Session: DefaultConfigData()}
+	err := cfg.UpdateTokens(func(file, session *ConfigData) {
+		file.Deezer.RefreshToken = "rt"
+		session.Deezer.RefreshToken = "rt"
+	})
+	if err != nil {
+		t.Fatalf("UpdateTokens() = %v", err)
+	}
+	// The in-memory update must still happen even with nothing to write to.
+	if cfg.File.Deezer.RefreshToken != "rt" || cfg.Session.Deezer.RefreshToken != "rt" {
+		t.Fatal("UpdateTokens did not apply the mutation")
+	}
+}
+
+func TestUpdateTokensPersistsAndIsConcurrencySafe(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "config.toml")
+	cfg := &Config{Path: path, File: DefaultConfigData(), Session: DefaultConfigData()}
+
+	var wg sync.WaitGroup
+	for i := 0; i < 16; i++ {
+		wg.Add(1)
+		go func(n int) {
+			defer wg.Done()
+			token := fmt.Sprintf("token-%d", n)
+			if err := cfg.UpdateTokens(func(file, session *ConfigData) {
+				file.Tidal.AccessToken = token
+				session.Tidal.AccessToken = token
+			}); err != nil {
+				t.Errorf("UpdateTokens: %v", err)
+			}
+		}(i)
+	}
+	wg.Wait()
+
+	raw, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var round ConfigData
+	if err := toml.Unmarshal(raw, &round); err != nil {
+		t.Fatalf("concurrent writes produced an unparseable config: %v", err)
+	}
+	if round.Tidal.AccessToken == "" {
+		t.Error("no token persisted")
+	}
+}