bvandeusen/minstrel
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 38 Wiki Activity

fix(lidarr): address review findings on HTTP client

Browse Source 
- Add ArtistMBID/AlbumMBID to LookupResult so handlers can build the
  full lidarr_requests row from album/track lookups (parent MBID is
  needed for the AddAlbum API call and for the schema's NOT NULL
  lidarr_artist_mbid)
- Trim trailing slash on BaseURL before joining paths to avoid
  double-slash URLs when operators enter "http://lidarr.lan/"
- Check json.Marshal errors in AddArtist/AddAlbum
- Extract lidarrImage as a named unexported type instead of repeating
  the anonymous struct three times
- Add NewClient(baseURL, apiKey) constructor with 30s default timeout
- Add ErrLookupFailed test coverage for the 4xx-non-auth branch on
  both get and post helpers
- Rename TestListRootFolders_BadJSON -> _NullBody (it tests null,
  not malformed); add a real malformed-JSON test
- Defensive separator guard on LookupAlbum.Secondary so an empty
  ArtistName doesn't produce a leading " · "

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Bryan Van Deusen
2026-04-29 16:26:23 -04:00
parent a43fa09a04
commit ebff8f69a0
5 changed files with 161 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/lidarr/client.go
+64 -42
View File
@@ -9,6 +9,8 @@ import (
"net/http"
"net/url"
"strconv"
"strings"
"time"
)
// Client wraps Lidarr's v1 HTTP API. BaseURL is the Lidarr instance
@@ -19,6 +21,17 @@ type Client struct {
HTTP *http.Client
}
// NewClient builds a Client with sensible HTTP defaults (30s timeout,
// matching the pattern in internal/scrobble/listenbrainz). Call sites
// that need a custom client can construct the Client struct directly.
func NewClient(baseURL, apiKey string) *Client {
return &Client{
BaseURL: baseURL,
APIKey: apiKey,
HTTP: &http.Client{Timeout: 30 * time.Second},
}
}
// get executes an authenticated GET request and maps HTTP status codes to
// typed errors. The caller is responsible for closing the returned body.
func (c *Client) get(ctx context.Context, path string, q url.Values) (*http.Response, error) {
@@ -26,7 +39,7 @@ func (c *Client) get(ctx context.Context, path string, q url.Values) (*http.Resp
if err != nil {
return nil, fmt.Errorf("%w: %v", ErrUnreachable, err)
}
u.Path = u.Path + path
u.Path = strings.TrimRight(u.Path, "/") + path
if q != nil {
u.RawQuery = q.Encode()
}
@@ -62,7 +75,7 @@ func (c *Client) post(ctx context.Context, path string, body []byte) (*http.Resp
if err != nil {
return nil, fmt.Errorf("%w: %v", ErrUnreachable, err)
}
u.Path = u.Path + path
u.Path = strings.TrimRight(u.Path, "/") + path
req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), bytes.NewReader(body))
if err != nil {
return nil, err
@@ -102,15 +115,11 @@ func (c *Client) LookupArtist(ctx context.Context, term string) ([]LookupResult,
return nil, fmt.Errorf("%w: %v", ErrInvalidPayload, err)
}
var raw []struct {
ForeignArtistID string `json:"foreignArtistId"`
ArtistName string `json:"artistName"`
Genres []string `json:"genres"`
AlbumCount int `json:"albumCount"`
Images []struct {
CoverType string `json:"coverType"`
RemoteURL string `json:"remoteUrl"`
URL string `json:"url"`
} `json:"images"`
ForeignArtistID string `json:"foreignArtistId"`
ArtistName string `json:"artistName"`
Genres []string `json:"genres"`
AlbumCount int `json:"albumCount"`
Images []lidarrImage `json:"images"`
}
if err := json.Unmarshal(body, &raw); err != nil {
return nil, fmt.Errorf("%w: %v", ErrInvalidPayload, err)
@@ -150,16 +159,13 @@ func (c *Client) LookupAlbum(ctx context.Context, term string) ([]LookupResult,
return nil, fmt.Errorf("%w: %v", ErrInvalidPayload, err)
}
var raw []struct {
ForeignAlbumID string `json:"foreignAlbumId"`
Title string `json:"title"`
ArtistName string `json:"artistName"`
ReleaseDate string `json:"releaseDate"`
TrackCount int `json:"trackCount"`
Images []struct {
CoverType string `json:"coverType"`
RemoteURL string `json:"remoteUrl"`
URL string `json:"url"`
} `json:"images"`
ForeignAlbumID string `json:"foreignAlbumId"`
ForeignArtistID string `json:"foreignArtistId"`
Title string `json:"title"`
ArtistName string `json:"artistName"`
ReleaseDate string `json:"releaseDate"`
TrackCount int `json:"trackCount"`
Images []lidarrImage `json:"images"`
}
if err := json.Unmarshal(body, &raw); err != nil {
return nil, fmt.Errorf("%w: %v", ErrInvalidPayload, err)
@@ -170,18 +176,28 @@ func (c *Client) LookupAlbum(ctx context.Context, term string) ([]LookupResult,
if len(r.ReleaseDate) >= 4 {
year = r.ReleaseDate[:4]
}
secondary := r.ArtistName
secondary := ""
if r.ArtistName != "" {
secondary = r.ArtistName
}
if year != "" {
secondary += " · " + year
if secondary != "" {
secondary += " · "
}
secondary += year
}
if r.TrackCount > 0 {
secondary += " · " + strconv.Itoa(r.TrackCount) + " tracks"
if secondary != "" {
secondary += " · "
}
secondary += strconv.Itoa(r.TrackCount) + " tracks"
}
out = append(out, LookupResult{
MBID: r.ForeignAlbumID,
Name: r.Title,
Secondary: secondary,
ImageURL: pickPosterImage(r.Images),
MBID: r.ForeignAlbumID,
ArtistMBID: r.ForeignArtistID,
Name: r.Title,
Secondary: secondary,
ImageURL: pickPosterImage(r.Images),
})
}
return out, nil
@@ -200,10 +216,12 @@ func (c *Client) LookupTrack(ctx context.Context, term string) ([]LookupResult,
return nil, fmt.Errorf("%w: %v", ErrInvalidPayload, err)
}
var raw []struct {
ForeignTrackID string `json:"foreignTrackId"`
Title string `json:"title"`
AlbumTitle string `json:"albumTitle"`
ArtistName string `json:"artistName"`
ForeignTrackID string `json:"foreignTrackId"`
ForeignAlbumID string `json:"foreignAlbumId"`
ForeignArtistID string `json:"foreignArtistId"`
Title string `json:"title"`
AlbumTitle string `json:"albumTitle"`
ArtistName string `json:"artistName"`
}
if err := json.Unmarshal(body, &raw); err != nil {
return nil, fmt.Errorf("%w: %v", ErrInvalidPayload, err)
@@ -218,9 +236,11 @@ func (c *Client) LookupTrack(ctx context.Context, term string) ([]LookupResult,
secondary += r.ArtistName
}
out = append(out, LookupResult{
MBID: r.ForeignTrackID,
Name: r.Title,
Secondary: secondary,
MBID: r.ForeignTrackID,
ArtistMBID: r.ForeignArtistID,
AlbumMBID: r.ForeignAlbumID,
Name: r.Title,
Secondary: secondary,
})
}
return out, nil
@@ -233,7 +253,7 @@ func (c *Client) AddArtist(ctx context.Context, p AddArtistParams) error {
if p.MonitorAll {
monitor = "all"
}
body, _ := json.Marshal(map[string]any{
body, err := json.Marshal(map[string]any{
"foreignArtistId": p.ForeignArtistID,
"qualityProfileId": p.QualityProfileID,
"rootFolderPath": p.RootFolderPath,
@@ -241,6 +261,9 @@ func (c *Client) AddArtist(ctx context.Context, p AddArtistParams) error {
"monitor": monitor,
"addOptions": map[string]any{"searchForMissingAlbums": true},
})
if err != nil {
return fmt.Errorf("%w: marshal: %v", ErrInvalidPayload, err)
}
resp, err := c.post(ctx, "/api/v1/artist", body)
if err != nil {
return err
@@ -252,7 +275,7 @@ func (c *Client) AddArtist(ctx context.Context, p AddArtistParams) error {
// AddAlbum posts to POST /api/v1/album. Returns nil on 2xx; typed error
// otherwise.
func (c *Client) AddAlbum(ctx context.Context, p AddAlbumParams) error {
body, _ := json.Marshal(map[string]any{
body, err := json.Marshal(map[string]any{
"foreignAlbumId": p.ForeignAlbumID,
"foreignArtistId": p.ForeignArtistID,
"qualityProfileId": p.QualityProfileID,
@@ -260,6 +283,9 @@ func (c *Client) AddAlbum(ctx context.Context, p AddAlbumParams) error {
"monitored": true,
"addOptions": map[string]any{"searchForNewAlbum": true},
})
if err != nil {
return fmt.Errorf("%w: marshal: %v", ErrInvalidPayload, err)
}
resp, err := c.post(ctx, "/api/v1/album", body)
if err != nil {
return err
@@ -344,11 +370,7 @@ func (c *Client) Ping(ctx context.Context) (PingResult, error) {
// pickPosterImage returns the remote URL (preferred) or local URL for the
// first image with coverType=="poster". Returns "" when none match.
func pickPosterImage(imgs []struct {
CoverType string `json:"coverType"`
RemoteURL string `json:"remoteUrl"`
URL string `json:"url"`
}) string {
func pickPosterImage(imgs []lidarrImage) string {
for _, img := range imgs {
if img.CoverType == "poster" {
if img.RemoteURL != "" {
internal/lidarr/client_test.go
+73 -16
View File
@@ -154,6 +154,12 @@ func TestLookupAlbum_HappyPath(t *testing.T) {
if got[0].MBID != "a1b2c3d4-e5f6-7890-abcd-ef1234567890" {
t.Errorf("MBID = %q", got[0].MBID)
}
if got[0].ArtistMBID != "069b64b6-7884-4f6a-94cc-e4c1d6c87a01" {
t.Errorf("ArtistMBID = %q, want 069b64b6-7884-4f6a-94cc-e4c1d6c87a01", got[0].ArtistMBID)
}
if got[0].AlbumMBID != "" {
t.Errorf("AlbumMBID = %q, want empty on album result", got[0].AlbumMBID)
}
if got[0].Name != "Music Has the Right to Children" {
t.Errorf("Name = %q", got[0].Name)
}
@@ -213,6 +219,12 @@ func TestLookupTrack_HappyPath(t *testing.T) {
if got[0].Secondary != want {
t.Errorf("Secondary = %q, want %q", got[0].Secondary, want)
}
if got[0].ArtistMBID != "069b64b6-7884-4f6a-94cc-e4c1d6c87a01" {
t.Errorf("ArtistMBID = %q, want 069b64b6-7884-4f6a-94cc-e4c1d6c87a01", got[0].ArtistMBID)
}
if got[0].AlbumMBID != "a1b2c3d4-e5f6-7890-abcd-ef1234567890" {
t.Errorf("AlbumMBID = %q, want a1b2c3d4-e5f6-7890-abcd-ef1234567890", got[0].AlbumMBID)
}
if got[0].ImageURL != "" {
t.Errorf("tracks carry no images; ImageURL = %q", got[0].ImageURL)
}
@@ -406,6 +418,51 @@ func TestListQualityProfiles_BadJSON(t *testing.T) {
}
}
// --- LookupArtist (additional) ---
func TestLookupArtist_LookupFailed(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusNotFound)
}))
defer srv.Close()
c := &Client{BaseURL: srv.URL, APIKey: "x", HTTP: srv.Client()}
_, err := c.LookupArtist(context.Background(), "boards")
if !errors.Is(err, ErrLookupFailed) {
t.Fatalf("err = %v, want ErrLookupFailed", err)
}
}
func TestGet_BaseURLWithTrailingSlash(t *testing.T) {
body, _ := os.ReadFile("testdata/lookup_artist.json")
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Verify path is /api/v1/artist/lookup, NOT //api/v1/artist/lookup
if r.URL.Path != "/api/v1/artist/lookup" {
t.Errorf("got path %q; trailing-slash BaseURL produced double-slash", r.URL.Path)
}
_, _ = w.Write(body)
}))
defer srv.Close()
c := &Client{BaseURL: srv.URL + "/", APIKey: "k", HTTP: srv.Client()}
_, err := c.LookupArtist(context.Background(), "boards")
if err != nil {
t.Fatalf("LookupArtist: %v", err)
}
}
// --- AddArtist (additional) ---
func TestAddArtist_PostFailed(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusUnprocessableEntity)
}))
defer srv.Close()
c := &Client{BaseURL: srv.URL, APIKey: "x", HTTP: srv.Client()}
err := c.AddArtist(context.Background(), AddArtistParams{ForeignArtistID: "x", QualityProfileID: 1, RootFolderPath: "/m"})
if !errors.Is(err, ErrLookupFailed) {
t.Fatalf("err = %v, want ErrLookupFailed", err)
}
}
// --- ListRootFolders ---
func TestListRootFolders_HappyPath(t *testing.T) {
@@ -439,7 +496,7 @@ func TestListRootFolders_HappyPath(t *testing.T) {
}
}
func TestListRootFolders_BadJSON(t *testing.T) {
func TestListRootFolders_NullBody(t *testing.T) {
c, srv := newTestClient(func(w http.ResponseWriter, _ *http.Request) {
_, _ = w.Write([]byte(`null`))
})
@@ -454,6 +511,18 @@ func TestListRootFolders_BadJSON(t *testing.T) {
}
}
func TestListRootFolders_BadJSON(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
_, _ = w.Write([]byte("{not valid json"))
}))
defer srv.Close()
c := &Client{BaseURL: srv.URL, APIKey: "x", HTTP: srv.Client()}
_, err := c.ListRootFolders(context.Background())
if !errors.Is(err, ErrInvalidPayload) {
t.Fatalf("err = %v, want ErrInvalidPayload", err)
}
}
// --- Ping ---
func TestPing_ReturnsVersion(t *testing.T) {
@@ -507,11 +576,7 @@ func TestPing_BadJSON(t *testing.T) {
// --- pickPosterImage ---
func TestPickPosterImage_PreferRemoteURL(t *testing.T) {
imgs := []struct {
CoverType string `json:"coverType"`
RemoteURL string `json:"remoteUrl"`
URL string `json:"url"`
}{
imgs := []lidarrImage{
{CoverType: "poster", RemoteURL: "https://remote.example/img.jpg", URL: "https://local.example/img.jpg"},
}
got := pickPosterImage(imgs)
@@ -521,11 +586,7 @@ func TestPickPosterImage_PreferRemoteURL(t *testing.T) {
}
func TestPickPosterImage_FallsBackToLocalURL(t *testing.T) {
imgs := []struct {
CoverType string `json:"coverType"`
RemoteURL string `json:"remoteUrl"`
URL string `json:"url"`
}{
imgs := []lidarrImage{
{CoverType: "poster", RemoteURL: "", URL: "https://local.example/img.jpg"},
}
got := pickPosterImage(imgs)
@@ -535,11 +596,7 @@ func TestPickPosterImage_FallsBackToLocalURL(t *testing.T) {
}
func TestPickPosterImage_SkipsNonPoster(t *testing.T) {
imgs := []struct {
CoverType string `json:"coverType"`
RemoteURL string `json:"remoteUrl"`
URL string `json:"url"`
}{
imgs := []lidarrImage{
{CoverType: "banner", RemoteURL: "https://banner.example/img.jpg"},
{CoverType: "cover", RemoteURL: "https://cover.example/img.jpg"},
}
internal/lidarr/testdata/lookup_album.json
Vendored
+2
View File
@@ -1,6 +1,7 @@
[
{
"foreignAlbumId": "a1b2c3d4-e5f6-7890-abcd-ef1234567890",
"foreignArtistId": "069b64b6-7884-4f6a-94cc-e4c1d6c87a01",
"title": "Music Has the Right to Children",
"artistName": "Boards of Canada",
"releaseDate": "1998-04-27",
@@ -12,6 +13,7 @@
},
{
"foreignAlbumId": "b2c3d4e5-f6a7-8901-bcde-f12345678901",
"foreignArtistId": "069b64b6-7884-4f6a-94cc-e4c1d6c87a01",
"title": "Geogaddi",
"artistName": "Boards of Canada",
"releaseDate": "2002-02-11",
internal/lidarr/testdata/lookup_track.json
Vendored
+4
View File
@@ -1,12 +1,16 @@
[
{
"foreignTrackId": "t1a2b3c4-d5e6-7890-abcd-ef1234567890",
"foreignAlbumId": "a1b2c3d4-e5f6-7890-abcd-ef1234567890",
"foreignArtistId": "069b64b6-7884-4f6a-94cc-e4c1d6c87a01",
"title": "Roygbiv",
"albumTitle": "Music Has the Right to Children",
"artistName": "Boards of Canada"
},
{
"foreignTrackId": "t2b3c4d5-e6f7-8901-bcde-f12345678901",
"foreignAlbumId": "a1b2c3d4-e5f6-7890-abcd-ef1234567890",
"foreignArtistId": "069b64b6-7884-4f6a-94cc-e4c1d6c87a01",
"title": "Aquarius",
"albumTitle": "Music Has the Right to Children",
"artistName": "Boards of Canada"
internal/lidarr/types.go
+18 -6
View File
@@ -4,13 +4,25 @@
package lidarr
// LookupResult is the normalized shape returned by Lookup{Artist,Album,Track}.
// It is what we store on the request row (via the user's request) and
// what /api/lidarr/search returns to the SPA.
// MBID is the kind's primary identifier (foreignArtistId / foreignAlbumId /
// foreignTrackId from Lidarr). ArtistMBID and AlbumMBID carry parent
// identifiers when the kind has them — request creation and admin
// approval need them to construct the lidarr_requests row and the
// AddArtist/AddAlbum API calls.
type LookupResult struct {
MBID string // foreignArtistId / foreignAlbumId / foreignTrackId
Name string // artist name; album/track returns Title here too
Secondary string // genre + album count for artist; year for album; album for track
ImageURL string // cover-art URL Lidarr surfaced (may be empty)
MBID string // foreignArtistId / foreignAlbumId / foreignTrackId
ArtistMBID string // parent artist's foreignArtistId — set on album/track results, empty on artist results
AlbumMBID string // parent album's foreignAlbumId — set on track results only
Name string // artist name; album/track returns Title here too
Secondary string // genre + album count for artist; year for album; album for track
ImageURL string // cover-art URL Lidarr surfaced (may be empty)
}
// lidarrImage is the wire shape for image entries in Lidarr's API responses.
type lidarrImage struct {
CoverType string `json:"coverType"`
RemoteURL string `json:"remoteUrl"`
URL string `json:"url"`
}
// QualityProfile is the dropdown choice in /admin/integrations.