From a695fb4de341db02a61038c4aed896fd43f6e70c Mon Sep 17 00:00:00 2001 From: Mathis Maquenne <124215603+mathismqn@users.noreply.github.com> Date: Wed, 5 Aug 2026 16:06:10 +0200 Subject: [PATCH] fix: resolve bugs found in pre-release review --- cmd/login.go | 63 ++++++++++++++++++++++++++++------- internal/config/migrate.go | 48 +++++++++++++++++++++++--- internal/deezer/client.go | 1 + internal/deezer/gateway.go | 4 +++ internal/deezer/login.go | 8 ++++- internal/deezer/session.go | 5 ++- internal/deezer/track.go | 26 +++++++++++++-- internal/download/bpm.go | 2 +- internal/download/download.go | 1 + internal/download/genre.go | 10 ++++-- internal/download/stream.go | 38 +++++++++++++++++++-- internal/download/track.go | 33 ++++++++++++++---- internal/store/store.go | 8 ++++- internal/tag/flac.go | 11 +++--- internal/tag/id3v2.go | 46 ++++++++++++++++--------- internal/update/apply.go | 9 +++-- internal/update/update.go | 5 +-- 17 files changed, 258 insertions(+), 60 deletions(-) diff --git a/cmd/login.go b/cmd/login.go index 3c153fd..a120dfa 100644 --- a/cmd/login.go +++ b/cmd/login.go @@ -2,6 +2,8 @@ package cmd import ( "bufio" + "context" + "errors" "fmt" "os" "strings" @@ -21,24 +23,61 @@ func newLoginCmd() *cobra.Command { return err } - email, password, err := promptCredentials() - if err != nil { - return err + err := runLogin(cmd.Context()) + if errors.Is(err, context.Canceled) { + return nil } - _, username, err := deezer.Login(cmd.Context(), email, password) - if err != nil { - return err - } - - fmt.Printf("Successfully logged in as %s.\n", username) - - return nil + return err }, } } -func promptCredentials() (string, string, error) { +func runLogin(ctx context.Context) error { + email, password, err := promptCredentials(ctx) + if err != nil { + return err + } + + _, username, err := deezer.Login(ctx, email, password) + if err != nil { + return err + } + + fmt.Printf("Successfully logged in as %s.\n", username) + + return nil +} + +func promptCredentials(ctx context.Context) (string, string, error) { + oldState, stateErr := term.GetState(int(os.Stdin.Fd())) + + type credentials struct { + email string + password string + err error + } + resultChan := make(chan credentials, 1) + go func() { + var c credentials + c.email, c.password, c.err = readCredentials() + resultChan <- c + }() + + select { + case c := <-resultChan: + return c.email, c.password, c.err + case <-ctx.Done(): + if stateErr == nil { + term.Restore(int(os.Stdin.Fd()), oldState) + } + fmt.Println() + + return "", "", ctx.Err() + } +} + +func readCredentials() (string, string, error) { fmt.Print("Email: ") line, err := bufio.NewReader(os.Stdin).ReadString('\n') if err != nil { diff --git a/internal/config/migrate.go b/internal/config/migrate.go index 9bcfbb8..5b48256 100644 --- a/internal/config/migrate.go +++ b/internal/config/migrate.go @@ -1,6 +1,7 @@ package config import ( + "io" "os" "path/filepath" ) @@ -11,15 +12,54 @@ func MigrateLegacy(outputDir string) { return } - oldDB := filepath.Join(homeDir, ".godeez", "tracks.db") + oldDir := filepath.Join(homeDir, ".godeez") + oldDB := filepath.Join(oldDir, "tracks.db") newDB := filepath.Join(outputDir, ".tracks.db") if _, err := os.Stat(oldDB); err == nil { if _, err := os.Stat(newDB); os.IsNotExist(err) { - os.Rename(oldDB, newDB) + if err := os.Rename(oldDB, newDB); err != nil { + if err := copyFile(oldDB, newDB); err != nil { + return + } + os.Remove(oldDB) + } } } - oldDir := filepath.Join(homeDir, ".godeez") - os.Remove(filepath.Join(oldDir, "config.toml")) os.Remove(oldDir) } + +func copyFile(src, dst string) error { + in, err := os.Open(src) + if err != nil { + return err + } + defer in.Close() + + out, err := os.CreateTemp(filepath.Dir(dst), ".tracks.db-*.tmp") + if err != nil { + return err + } + tmp := out.Name() + + if _, err := io.Copy(out, in); err != nil { + out.Close() + os.Remove(tmp) + return err + } + if err := out.Sync(); err != nil { + out.Close() + os.Remove(tmp) + return err + } + if err := out.Close(); err != nil { + os.Remove(tmp) + return err + } + if err := os.Rename(tmp, dst); err != nil { + os.Remove(tmp) + return err + } + + return nil +} diff --git a/internal/deezer/client.go b/internal/deezer/client.go index 80364fa..a5b8a95 100644 --- a/internal/deezer/client.go +++ b/internal/deezer/client.go @@ -213,6 +213,7 @@ func (c *Client) MediaStream(ctx context.Context, media *Media) (io.ReadCloser, } if resp.StatusCode != http.StatusOK { + resp.Body.Close() return nil, fmt.Errorf("unexpected status code: %d", resp.StatusCode) } diff --git a/internal/deezer/gateway.go b/internal/deezer/gateway.go index 73f5ddd..62451c7 100644 --- a/internal/deezer/gateway.go +++ b/internal/deezer/gateway.go @@ -120,6 +120,10 @@ func (m *mobileClient) authenticate(ctx context.Context) (string, string, string return "", "", "", err } + if len(decrypted) < 96 { + return "", "", "", fmt.Errorf("unexpected response from gateway") + } + token := string(decrypted[0:64]) tokenKey := string(decrypted[64:80]) userKey := string(decrypted[80:96]) diff --git a/internal/deezer/login.go b/internal/deezer/login.go index 4b7fecd..7237789 100644 --- a/internal/deezer/login.go +++ b/internal/deezer/login.go @@ -2,6 +2,7 @@ package deezer import ( "context" + "errors" "fmt" ) @@ -12,9 +13,14 @@ func resolveARL(ctx context.Context, validate func(ctx context.Context, arl stri } if creds != nil && creds.ARL != "" { - if verr := validate(ctx, creds.ARL); verr == nil { + verr := validate(ctx, creds.ARL) + if verr == nil { return creds.ARL, nil } + + if !errors.Is(verr, ErrInvalidARL) { + return "", fmt.Errorf("stored session could not be validated: %w", verr) + } } if creds != nil && creds.Email != "" && creds.Password != "" { diff --git a/internal/deezer/session.go b/internal/deezer/session.go index 8d13067..ba535bf 100644 --- a/internal/deezer/session.go +++ b/internal/deezer/session.go @@ -3,6 +3,7 @@ package deezer import ( "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -10,6 +11,8 @@ import ( "time" ) +var ErrInvalidARL = errors.New("invalid or expired ARL cookie") + type Session struct { APIToken string LicenseToken string @@ -71,7 +74,7 @@ func authenticate(ctx context.Context, arlCookie string) (*Session, error) { } if res.Results.User.ID == 0 { - return nil, fmt.Errorf("invalid or expired ARL cookie") + return nil, ErrInvalidARL } opts := res.Results.User.Options diff --git a/internal/deezer/track.go b/internal/deezer/track.go index 7707b9e..a28d7ce 100644 --- a/internal/deezer/track.go +++ b/internal/deezer/track.go @@ -62,7 +62,27 @@ func (t *Track) Filename(kind Kind, mediaFormat string) string { } } - fileName := fmt.Sprintf("%s%s - %s.%s", prefix, t.Artist, t.FullTitle(), ext) - fileName, _ = filenamify.Filenamify(fileName, filenamify.Options{MaxLength: 255}) - return fileName + base := fmt.Sprintf("%s%s - %s", prefix, t.Artist, t.FullTitle()) + base, _ = filenamify.Filenamify(base, filenamify.Options{MaxLength: 255}) + base = truncateBytes(base, 255-len(ext)-1-len("-id3v2")) + + return base + "." + ext +} + +func truncateBytes(s string, max int) string { + if max <= 0 { + return "" + } + if len(s) <= max { + return s + } + + last := 0 + for i := range s { + if i > max { + return s[:last] + } + last = i + } + return s[:last] } diff --git a/internal/download/bpm.go b/internal/download/bpm.go index bda64b3..cb0cfc1 100644 --- a/internal/download/bpm.go +++ b/internal/download/bpm.go @@ -93,7 +93,7 @@ func findTrackURL(ctx context.Context, httpClient *http.Client, artist, title, d const toleranceSec = 2 foundDuration := minutes*60 + seconds - if foundDuration <= wantDuration-toleranceSec || foundDuration >= wantDuration+toleranceSec { + if foundDuration < wantDuration-toleranceSec || foundDuration > wantDuration+toleranceSec { return true } diff --git a/internal/download/download.go b/internal/download/download.go index a97339c..93172a8 100644 --- a/internal/download/download.go +++ b/internal/download/download.go @@ -81,6 +81,7 @@ func (d *Downloader) prepareResource(ctx context.Context, id string, opts Option if err := fsutil.EnsureDir(outputDir); err != nil { return nil, "", fmt.Errorf("failed to create output directory: %w", err) } + sweepPartFiles(outputDir) return resource, outputDir, nil } diff --git a/internal/download/genre.go b/internal/download/genre.go index f98c4a8..a3ab6bc 100644 --- a/internal/download/genre.go +++ b/internal/download/genre.go @@ -4,7 +4,10 @@ import ( "context" "fmt" "net/http" + "net/url" "strings" + "unicode" + "unicode/utf8" "github.com/PuerkitoBio/goquery" ) @@ -31,7 +34,9 @@ func toLower(ss []string) []string { } func fetchGenre(ctx context.Context, httpClient *http.Client, artist, title string) (string, error) { - reqURL := fmt.Sprintf("https://www.last.fm/music/%s/%s/+tags", artist, title) + // Escape the path segments: names containing '/', '?', or '#' would + // otherwise change the URL structure and fetch the wrong page. + reqURL := fmt.Sprintf("https://www.last.fm/music/%s/%s/+tags", url.PathEscape(artist), url.PathEscape(title)) doc, err := fetchGenrePage(ctx, httpClient, reqURL) if err != nil { @@ -116,7 +121,8 @@ func formatTags(tags []string) string { } words := strings.Fields(tag) for i, w := range words { - words[i] = strings.ToUpper(w[:1]) + strings.ToLower(w[1:]) + r, size := utf8.DecodeRuneInString(w) + words[i] = string(unicode.ToUpper(r)) + strings.ToLower(w[size:]) } formatted = append(formatted, strings.Join(words, " ")) } diff --git a/internal/download/stream.go b/internal/download/stream.go index bdb0ad4..8170ffb 100644 --- a/internal/download/stream.go +++ b/internal/download/stream.go @@ -5,20 +5,41 @@ import ( "errors" "io" "os" + "path/filepath" "github.com/mathismqn/godeez/internal/deezer" ) -const chunkSize = 2048 +const ( + chunkSize = 2048 + partPattern = ".godeez-*.part" +) + +func sweepPartFiles(dir string) { + matches, err := filepath.Glob(filepath.Join(dir, partPattern)) + if err != nil { + return + } + for _, match := range matches { + os.Remove(match) + } +} func (d *Downloader) streamToFile(ctx context.Context, stream io.ReadCloser, outputPath string, key []byte) error { defer stream.Close() - file, err := os.Create(outputPath) + file, err := os.CreateTemp(filepath.Dir(outputPath), partPattern) if err != nil { return err } - defer file.Close() + tmpPath := file.Name() + done := false + defer func() { + if !done { + file.Close() + os.Remove(tmpPath) + } + }() buffer := make([]byte, chunkSize) for chunk := 0; ; chunk++ { @@ -60,5 +81,16 @@ func (d *Downloader) streamToFile(ctx context.Context, stream io.ReadCloser, out } } + if err := file.Sync(); err != nil { + return err + } + if err := file.Close(); err != nil { + return err + } + if err := os.Rename(tmpPath, outputPath); err != nil { + return err + } + done = true + return nil } diff --git a/internal/download/track.go b/internal/download/track.go index 1783c57..1a97f02 100644 --- a/internal/download/track.go +++ b/internal/download/track.go @@ -34,20 +34,19 @@ func (d *Downloader) downloadTrack(ctx context.Context, resource deezer.Resource metadataChan <- fetchMetadata(d.deezerClient.Session.HTTPClient, ctx, track, opts) }() - stream, err := d.deezerClient.MediaStream(ctx, media) + dlCtx, cancel := context.WithTimeout(ctx, opts.Timeout) + defer cancel() + + stream, err := d.deezerClient.MediaStream(dlCtx, media) if err != nil { return downloadResult{err: fmt.Errorf("failed to get media stream: %w", err)} } - dlCtx, cancel := context.WithTimeout(ctx, opts.Timeout) - defer cancel() - fileName := track.Filename(d.kind, mediaFormat) - outputPath := filepath.Join(outputDir, fileName) + outputPath := d.uniqueOutputPath(track.ID, filepath.Join(outputDir, fileName)) key := deezer.BlowfishKey(track.ID) if err := d.streamToFile(dlCtx, stream, outputPath, key); err != nil { - fsutil.Remove(outputPath) return downloadResult{err: fmt.Errorf("failed to stream to file: %w", err)} } @@ -63,12 +62,34 @@ func (d *Downloader) downloadTrack(ctx context.Context, resource deezer.Resource } metadata := <-metadataChan + + if err := ctx.Err(); err != nil { + fsutil.Remove(outputPath) + return downloadResult{err: err} + } + warnings = append(warnings, metadata.warnings...) warnings = append(warnings, d.finalizeDownload(resource, track, outputPath, mediaFormat, metadata.genre, cover, metadata.bpmKey)...) return downloadResult{warnings: warnings} } +func (d *Downloader) uniqueOutputPath(trackID, path string) string { + owned := "" + if info, err := d.store.DownloadInfo(trackID); err == nil { + owned = info.Path + } + + ext := filepath.Ext(path) + stem := strings.TrimSuffix(path, ext) + candidate := path + for i := 2; candidate != owned && fsutil.Exists(candidate); i++ { + candidate = fmt.Sprintf("%s (%d)%s", stem, i, ext) + } + + return candidate +} + func (d *Downloader) finalizeDownload(resource deezer.Resource, track *deezer.Track, outputPath, mediaFormat, genre string, cover []byte, bpmKey bpmKey) []string { var warnings []string diff --git a/internal/store/store.go b/internal/store/store.go index 8080bec..71ce365 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -1,10 +1,13 @@ package store import ( + "errors" "fmt" "path/filepath" + "time" bolt "go.etcd.io/bbolt" + bolterrors "go.etcd.io/bbolt/errors" ) type Store struct { @@ -12,8 +15,11 @@ type Store struct { } func Open(dir string) (*Store, error) { - db, err := bolt.Open(filepath.Join(dir, ".tracks.db"), 0600, nil) + db, err := bolt.Open(filepath.Join(dir, ".tracks.db"), 0600, &bolt.Options{Timeout: 5 * time.Second}) if err != nil { + if errors.Is(err, bolterrors.ErrTimeout) { + return nil, fmt.Errorf("database is already in use by another process") + } return nil, fmt.Errorf("failed to open database: %w", err) } diff --git a/internal/tag/flac.go b/internal/tag/flac.go index acb64f1..a141348 100644 --- a/internal/tag/flac.go +++ b/internal/tag/flac.go @@ -51,15 +51,16 @@ func (t *flacTagger) write(m Metadata) error { t.file.Meta = append(t.file.Meta, &cmtsMeta) } - picture, err := flacpicture.NewFromImageData(flacpicture.PictureTypeFrontCover, "Front cover", m.Cover, "image/jpeg") - if err != nil { - return err + if len(m.Cover) > 0 { + if picture, err := flacpicture.NewFromImageData(flacpicture.PictureTypeFrontCover, "Front cover", m.Cover, "image/jpeg"); err == nil { + pictureMeta := picture.Marshal() + t.file.Meta = append(t.file.Meta, &pictureMeta) + } } - pictureMeta := picture.Marshal() - t.file.Meta = append(t.file.Meta, &pictureMeta) tmpPath := t.path + ".tmp" if err := t.file.Save(tmpPath); err != nil { + os.Remove(tmpPath) return err } return os.Rename(tmpPath, t.path) diff --git a/internal/tag/id3v2.go b/internal/tag/id3v2.go index 625b391..5ff22b9 100644 --- a/internal/tag/id3v2.go +++ b/internal/tag/id3v2.go @@ -3,6 +3,7 @@ package tag import ( "fmt" "strconv" + "strings" "github.com/bogem/id3v2/v2" ) @@ -14,20 +15,19 @@ type id3v2Tagger struct { func (t *id3v2Tagger) write(m Metadata) error { defer t.tag.Close() - duration, err := strconv.Atoi(m.Duration) - if err != nil { - return err - } - length := fmt.Sprintf("%d", duration*1000) - if m.Album != nil { + year := m.Album.ReleaseDate + if parts := strings.Split(year, "-"); len(parts) == 3 { + year = parts[0] + } + t.addTag("TRCK", m.TrackNumber) t.addTag("TPE2", m.Album.Artist) t.addTag("TALB", m.Album.Title) t.addTag("TPUB", m.Album.Label) t.addTag("TDOR", m.Album.OriginalReleaseDate) - t.addTag("TYER", m.Album.ReleaseDate) - t.addTag("COMM", m.Album.ProducerLine) + t.addTag("TYER", year) + t.addComment(m.Album.ProducerLine) t.addTag("TCOP", m.Album.Copyright) } @@ -36,19 +36,23 @@ func (t *id3v2Tagger) write(m Metadata) error { t.addTag("TCOM", m.Composers) t.addTag("TEXT", m.Lyricists) t.addTag("TCON", m.Genre) - t.addTag("TLEN", length) + if duration, err := strconv.Atoi(m.Duration); err == nil { + t.addTag("TLEN", fmt.Sprintf("%d", duration*1000)) + } t.addTag("TBPM", m.BPM) t.addTag("TKEY", m.Key) t.addTXXX("GAIN", m.Gain) t.addTXXX("ISRC", m.ISRC) - t.tag.AddAttachedPicture(id3v2.PictureFrame{ - Encoding: t.tag.DefaultEncoding(), - MimeType: "image/jpeg", - PictureType: id3v2.PTFrontCover, - Description: "Cover", - Picture: m.Cover, - }) + if len(m.Cover) > 0 { + t.tag.AddAttachedPicture(id3v2.PictureFrame{ + Encoding: t.tag.DefaultEncoding(), + MimeType: "image/jpeg", + PictureType: id3v2.PTFrontCover, + Description: "Cover", + Picture: m.Cover, + }) + } return t.tag.Save() } @@ -59,6 +63,16 @@ func (t *id3v2Tagger) addTag(name, value string) { } } +func (t *id3v2Tagger) addComment(value string) { + if value != "" { + t.tag.AddCommentFrame(id3v2.CommentFrame{ + Encoding: t.tag.DefaultEncoding(), + Language: "eng", + Text: value, + }) + } +} + func (t *id3v2Tagger) addTXXX(description, value string) { if value != "" { t.tag.AddUserDefinedTextFrame(id3v2.UserDefinedTextFrame{ diff --git a/internal/update/apply.go b/internal/update/apply.go index efce159..ba29158 100644 --- a/internal/update/apply.go +++ b/internal/update/apply.go @@ -42,7 +42,7 @@ func resolveTarget() (string, error) { } for _, prefix := range managedPrefixes { - if strings.HasPrefix(target, prefix) { + if target == prefix || strings.HasPrefix(target, prefix+"/") { return "", fmt.Errorf("%s was installed by a package manager. Update it with that instead", target) } } @@ -113,7 +113,7 @@ func (u *Updater) Apply(ctx context.Context, release *Release) error { u.step("Replacing %s", target) - return replaceBinary(target, tmp) + return u.replaceBinary(target, tmp) } func (u *Updater) fetchChecksum(ctx context.Context, release *Release, assetName string) (string, error) { @@ -153,6 +153,9 @@ func parseChecksums(r io.Reader, name string) (string, error) { } func (u *Updater) download(ctx context.Context, dir string, asset Asset) (string, string, error) { + ctx, cancel := context.WithTimeout(ctx, downloadTimeout) + defer cancel() + body, err := u.get(ctx, asset.URL, nil) if err != nil { return "", "", err @@ -181,7 +184,7 @@ func (u *Updater) download(ctx context.Context, dir string, asset Asset) (string return tmp, hex.EncodeToString(hash.Sum(nil)), nil } -func replaceBinary(target, tmp string) error { +func (u *Updater) replaceBinary(target, tmp string) error { if runtime.GOOS != "windows" { return os.Rename(tmp, target) } diff --git a/internal/update/update.go b/internal/update/update.go index d198e24..e69ecd3 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -11,8 +11,9 @@ import ( ) const ( - apiTimeout = 30 * time.Second - tmpPattern = ".godeez-update-*" + apiTimeout = 30 * time.Second + downloadTimeout = 5 * time.Minute + tmpPattern = ".godeez-update-*" ) type Updater struct {