From 670b30c95483cd848c51e52f89b374f75ae07338 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 7 Oct 2026 10:55:41 -0400 Subject: [PATCH] fix(lidarr): add an album as the looked-up resource with its artist nested (#5234) Lidarr's POST /api/v1/album validates `artist` as a nested resource (AlbumController: RuleFor(s => s.Artist).NotNull()), so the flat payload we sent was refused with "'Artist' must not be empty" every time. The album add has never worked against a real Lidarr; approved album and track requests sat in the reconciler retrying every 5 minutes. AddAlbum now does what Lidarr's own add-album UI does (getNewAlbum / getNewArtist): look the album up by MBID (album/lookup?term=lidarr:), then POST that resource back with monitored + searchForNewAlbum. When Lidarr doesn't have the artist yet, the nested artist gets the request's quality/metadata profile and root folder, monitors this album only (monitor "none" + albumsToMonitor, which AlbumMonitoredService prefers) and no future releases. An artist Lidarr already has is left as it is. An MBID Lidarr's metadata doesn't know is ErrNotFound with no POST. Co-Authored-By: Claude Opus 5.5 --- internal/lidarr/client.go | 57 +++++++++--- internal/lidarr/client_test.go | 158 ++++++++++++++++++++++++++------- internal/lidarr/lookup_mbid.go | 27 ++++++ internal/lidarr/types.go | 8 +- 4 files changed, 204 insertions(+), 46 deletions(-) diff --git a/internal/lidarr/client.go b/internal/lidarr/client.go index dcbbe0c0..3c6b7d4b 100644 --- a/internal/lidarr/client.go +++ b/internal/lidarr/client.go @@ -324,19 +324,52 @@ func isAlreadyExists(err error) bool { strings.Contains(m, "already in your library") } -// AddAlbum posts to POST /api/v1/album. Returns nil on 2xx; typed error -// otherwise. +// AddAlbum adds one album to Lidarr the way Lidarr's own UI does: look the +// album up by MBID, then POST that album resource back with `monitored`, +// `addOptions` and — when Lidarr doesn't have the artist yet — the +// artist's profiles, root folder and monitoring filled in. +// +// POST /api/v1/album validates `artist` as a nested resource (#5234: a +// flat payload is refused with "'Artist' must not be empty"), and the +// lookup is what supplies it: the existing Lidarr artist when there is one, +// otherwise a new one carrying its metadata. A new artist monitors only +// this album (monitor "none" with albumsToMonitor, which Lidarr applies in +// preference to the monitor type) and no future releases, so approving one +// album never subscribes the library to the artist's whole catalogue. +// +// Returns nil on 2xx, ErrAlreadyExists when Lidarr already has the album, +// ErrNotFound when Lidarr's metadata has no album under that MBID. func (c *Client) AddAlbum(ctx context.Context, p AddAlbumParams) error { - body, err := json.Marshal(map[string]any{ - "foreignAlbumId": p.ForeignAlbumID, - "foreignArtistId": p.ForeignArtistID, - "artistName": p.ArtistName, - "qualityProfileId": p.QualityProfileID, - "metadataProfileId": p.MetadataProfileID, - "rootFolderPath": p.RootFolderPath, - "monitored": true, - "addOptions": map[string]any{"searchForNewAlbum": true}, - }) + album, err := c.lookupAlbumResource(ctx, p.ForeignAlbumID) + if err != nil { + return err + } + album["monitored"] = true + album["addOptions"] = map[string]any{"searchForNewAlbum": true} + + artist, _ := album["artist"].(map[string]any) + if artist == nil { + artist = map[string]any{ + "foreignArtistId": p.ForeignArtistID, + "artistName": p.ArtistName, + } + album["artist"] = artist + } + if id, _ := artist["id"].(float64); id == 0 { + artist["qualityProfileId"] = p.QualityProfileID + artist["metadataProfileId"] = p.MetadataProfileID + artist["rootFolderPath"] = p.RootFolderPath + artist["monitored"] = true + artist["monitorNewItems"] = "none" + artist["addOptions"] = map[string]any{ + "monitor": "none", + "albumsToMonitor": []string{p.ForeignAlbumID}, + "monitored": true, + "searchForMissingAlbums": false, + } + } + + body, err := json.Marshal(album) if err != nil { return fmt.Errorf("%w: marshal: %v", ErrInvalidPayload, err) } diff --git a/internal/lidarr/client_test.go b/internal/lidarr/client_test.go index 6ece5c5f..c797af00 100644 --- a/internal/lidarr/client_test.go +++ b/internal/lidarr/client_test.go @@ -327,45 +327,143 @@ func TestAddArtist_ServerError(t *testing.T) { // --- AddAlbum --- -func TestAddAlbum_PostsCorrectBody(t *testing.T) { - var decoded map[string]any +const ( + addAlbumMBID = "a1b2c3d4-e5f6-7890-abcd-ef1234567890" + addArtistMBID = "069b64b6-7884-4f6a-94cc-e4c1d6c87a01" +) + +// albumLookupStub answers Lidarr's album lookup with lookupBody and records +// the body of the album POST that follows it. +func albumLookupStub(t *testing.T, lookupBody string, postStatus int, postBody string) (*Client, *httptest.Server, *map[string]any, *int) { + t.Helper() + var posted map[string]any + posts := 0 c, srv := newTestClient(func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPost { - t.Errorf("method = %q, want POST", r.Method) + switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/album/lookup": + if got := r.URL.Query().Get("term"); got != "lidarr:"+addAlbumMBID { + t.Errorf("lookup term = %q, want lidarr:%s", got, addAlbumMBID) + } + _, _ = w.Write([]byte(lookupBody)) + case r.Method == http.MethodPost && r.URL.Path == "/api/v1/album": + posts++ + _ = json.NewDecoder(r.Body).Decode(&posted) + w.WriteHeader(postStatus) + _, _ = w.Write([]byte(postBody)) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) } - if r.URL.Path != "/api/v1/album" { - t.Errorf("path = %q, want /api/v1/album", r.URL.Path) - } - _ = json.NewDecoder(r.Body).Decode(&decoded) - w.WriteHeader(http.StatusCreated) }) + return c, srv, &posted, &posts +} + +var addAlbumParams = AddAlbumParams{ + ForeignAlbumID: addAlbumMBID, + ForeignArtistID: addArtistMBID, + ArtistName: "Boards of Canada", + QualityProfileID: 1, + MetadataProfileID: 3, + RootFolderPath: "/music", +} + +// #5234: Lidarr refuses a flat album add with "'Artist' must not be empty". +// The add has to be the looked-up album resource with its artist nested. +func TestAddAlbum_NewArtist_PostsTheLookedUpAlbumWithTheArtistNested(t *testing.T) { + lookup := `[ + {"foreignAlbumId": "someone-else", "title": "Other"}, + {"foreignAlbumId": "` + addAlbumMBID + `", "title": "Music Has the Right to Children", + "releases": [{"foreignReleaseId": "r1", "monitored": true}], + "artist": {"foreignArtistId": "` + addArtistMBID + `", "artistName": "Boards of Canada"}} + ]` + c, srv, posted, _ := albumLookupStub(t, lookup, http.StatusCreated, `{"id":9}`) defer srv.Close() - err := c.AddAlbum(context.Background(), AddAlbumParams{ - ForeignAlbumID: "a1b2c3d4-e5f6-7890-abcd-ef1234567890", - ForeignArtistID: "069b64b6-7884-4f6a-94cc-e4c1d6c87a01", - QualityProfileID: 1, - RootFolderPath: "/music", - }) - if err != nil { + if err := c.AddAlbum(context.Background(), addAlbumParams); err != nil { t.Fatalf("AddAlbum: %v", err) } + body := *posted + if body["foreignAlbumId"] != addAlbumMBID || body["title"] != "Music Has the Right to Children" { + t.Errorf("posted album = %v / %v, want the looked-up album", body["foreignAlbumId"], body["title"]) + } + if rel, _ := body["releases"].([]any); len(rel) != 1 { + t.Errorf("releases = %v, want the lookup's releases passed through", body["releases"]) + } + if body["monitored"] != true { + t.Errorf("monitored = %v, want true", body["monitored"]) + } + if opts, _ := body["addOptions"].(map[string]any); opts["searchForNewAlbum"] != true { + t.Errorf("addOptions = %v, want searchForNewAlbum true", body["addOptions"]) + } - if decoded["foreignAlbumId"] != "a1b2c3d4-e5f6-7890-abcd-ef1234567890" { - t.Errorf("foreignAlbumId = %v", decoded["foreignAlbumId"]) - } - if decoded["foreignArtistId"] != "069b64b6-7884-4f6a-94cc-e4c1d6c87a01" { - t.Errorf("foreignArtistId = %v", decoded["foreignArtistId"]) - } - if decoded["monitored"] != true { - t.Errorf("monitored = %v, want true", decoded["monitored"]) - } - opts, ok := decoded["addOptions"].(map[string]any) + artist, ok := body["artist"].(map[string]any) if !ok { - t.Fatalf("addOptions missing or wrong type: %T", decoded["addOptions"]) + t.Fatalf("artist = %T, want a nested object", body["artist"]) } - if opts["searchForNewAlbum"] != true { - t.Errorf("addOptions.searchForNewAlbum = %v, want true", opts["searchForNewAlbum"]) + for k, want := range map[string]any{ + "foreignArtistId": addArtistMBID, + "qualityProfileId": float64(1), + "metadataProfileId": float64(3), + "rootFolderPath": "/music", + "monitored": true, + "monitorNewItems": "none", + } { + if artist[k] != want { + t.Errorf("artist.%s = %v, want %v", k, artist[k], want) + } + } + // A new artist monitors this album only: albumsToMonitor wins over the + // monitor type in Lidarr's AlbumMonitoredService. + aOpts, _ := artist["addOptions"].(map[string]any) + if aOpts["monitor"] != "none" || aOpts["searchForMissingAlbums"] != false { + t.Errorf("artist.addOptions = %v, want monitor none, no catalogue search", aOpts) + } + if only, _ := aOpts["albumsToMonitor"].([]any); len(only) != 1 || only[0] != addAlbumMBID { + t.Errorf("artist.addOptions.albumsToMonitor = %v, want [%s]", aOpts["albumsToMonitor"], addAlbumMBID) + } +} + +// An artist Lidarr already has keeps its own profiles and monitoring. +func TestAddAlbum_ExistingArtist_IsLeftAsLidarrHasIt(t *testing.T) { + lookup := `[{"foreignAlbumId": "` + addAlbumMBID + `", + "artist": {"id": 42, "foreignArtistId": "` + addArtistMBID + `", "qualityProfileId": 5, + "metadataProfileId": 2, "rootFolderPath": "/lib", "monitorNewItems": "all"}}]` + c, srv, posted, _ := albumLookupStub(t, lookup, http.StatusCreated, `{"id":9}`) + defer srv.Close() + + if err := c.AddAlbum(context.Background(), addAlbumParams); err != nil { + t.Fatalf("AddAlbum: %v", err) + } + artist, _ := (*posted)["artist"].(map[string]any) + if artist["qualityProfileId"] != float64(5) || artist["rootFolderPath"] != "/lib" || artist["monitorNewItems"] != "all" { + t.Errorf("existing artist was rewritten: %v", artist) + } + if _, set := artist["addOptions"]; set { + t.Errorf("existing artist got addOptions %v", artist["addOptions"]) + } +} + +func TestAddAlbum_NotInLidarrsMetadata_IsNotFoundAndPostsNothing(t *testing.T) { + c, srv, _, posts := albumLookupStub(t, `[{"foreignAlbumId": "someone-else"}]`, http.StatusCreated, "") + defer srv.Close() + + err := c.AddAlbum(context.Background(), addAlbumParams) + if !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } + if *posts != 0 { + t.Errorf("posted %d times, want 0", *posts) + } +} + +func TestAddAlbum_AlreadyAdded_IsErrAlreadyExists(t *testing.T) { + lookup := `[{"foreignAlbumId": "` + addAlbumMBID + `", "artist": {"id": 42}}]` + c, srv, _, _ := albumLookupStub(t, lookup, http.StatusBadRequest, + `[{"propertyName":"ForeignAlbumId","errorMessage":"This album has already been added."}]`) + defer srv.Close() + + if err := c.AddAlbum(context.Background(), addAlbumParams); !errors.Is(err, ErrAlreadyExists) { + t.Fatalf("err = %v, want ErrAlreadyExists", err) } } @@ -374,7 +472,7 @@ func TestAddAlbum_AuthFailed(t *testing.T) { w.WriteHeader(http.StatusUnauthorized) }) defer srv.Close() - err := c.AddAlbum(context.Background(), AddAlbumParams{}) + err := c.AddAlbum(context.Background(), addAlbumParams) if !errors.Is(err, ErrAuthFailed) { t.Fatalf("err = %v, want ErrAuthFailed", err) } diff --git a/internal/lidarr/lookup_mbid.go b/internal/lidarr/lookup_mbid.go index b6e17781..f419597c 100644 --- a/internal/lidarr/lookup_mbid.go +++ b/internal/lidarr/lookup_mbid.go @@ -52,3 +52,30 @@ func (c *Client) LookupAlbumByMBID(ctx context.Context, mbid string) (LidarrAlbu } return rows[0], nil } + +// lookupAlbumResource fetches the album resource Lidarr's metadata holds +// under mbid (GET /api/v1/album/lookup?term=lidarr:), kept as raw +// JSON so AddAlbum can POST it back with every field Lidarr sent — the +// round trip Lidarr's own add-album UI makes. Returns ErrNotFound when no +// result carries that MBID. +func (c *Client) lookupAlbumResource(ctx context.Context, mbid string) (map[string]any, error) { + if mbid == "" { + return nil, fmt.Errorf("lidarr: empty album mbid") + } + resp, err := c.get(ctx, "/api/v1/album/lookup", url.Values{"term": {"lidarr:" + mbid}}) + if err != nil { + return nil, err + } + defer func() { _ = resp.Body.Close() }() + + var rows []map[string]any + if err := json.NewDecoder(resp.Body).Decode(&rows); err != nil { + return nil, fmt.Errorf("%w: decode album lookup: %v", ErrInvalidPayload, err) + } + for _, row := range rows { + if id, _ := row["foreignAlbumId"].(string); id == mbid { + return row, nil + } + } + return nil, fmt.Errorf("%w: album %s in Lidarr's metadata", ErrNotFound, mbid) +} diff --git a/internal/lidarr/types.go b/internal/lidarr/types.go index a3775cc5..7a5858f7 100644 --- a/internal/lidarr/types.go +++ b/internal/lidarr/types.go @@ -60,10 +60,10 @@ type AddArtistParams struct { MonitorAll bool // true => monitor="all"; false => "future" } -// AddAlbumParams are the fields Lidarr requires on POST /api/v1/album. -// MetadataProfileID is included for the case where Lidarr has to create -// the parent artist on the fly (when adding an album by an artist not -// yet in the library). +// AddAlbumParams identify the album to add and carry the settings Lidarr +// needs when it has to create the parent artist on the fly (an album by an +// artist not yet in Lidarr). The artist fields are a fallback only: the +// album lookup normally supplies the artist itself. type AddAlbumParams struct { ForeignAlbumID string ForeignArtistID string // Lidarr requires the artist's foreign id too