Merge pull request 'Lidarr: album adds that Lidarr accepts, artist monitoring that sticks' (#138) from dev into main

internal/lidarr/client.go

@@ -274,13 +274,20 @@ func (c *Client) LookupTrack(ctx context.Context, term string) ([]LookupResult,
 	return out, nil
 }
 
-// AddArtist posts to POST /api/v1/artist. MonitorAll=true sends monitor="all";
+// AddArtist posts to POST /api/v1/artist. MonitorAll=true monitors every
-// false sends "future". Returns nil on 2xx; typed error otherwise.
+// album ("all"); false monitors only releases from now on ("future"). New
+// releases are monitored either way. Returns nil on 2xx; typed error
+// otherwise.
 //
 // All four of artistName, foreignArtistId, qualityProfileId, and
 // metadataProfileId are required by Lidarr. Omitting any one produces a
 // 400 with field-level validation messages (e.g. "'Metadata Profile Id'
 // must be greater than '0'").
+//
+// The monitor choice belongs in addOptions (AddArtistOptions, a
+// MonitoringOptions). ArtistResource has no top-level `monitor`, so a
+// choice sent there is dropped, AddOptions.Monitor stays Unknown and
+// Lidarr skips applying it (#5239).
 func (c *Client) AddArtist(ctx context.Context, p AddArtistParams) error {
 	monitor := "future"
 	if p.MonitorAll {
@@ -293,8 +300,12 @@ func (c *Client) AddArtist(ctx context.Context, p AddArtistParams) error {
 		"metadataProfileId": p.MetadataProfileID,
 		"rootFolderPath":    p.RootFolderPath,
 		"monitored":         true,
-		"monitor":           monitor,
+		"monitorNewItems":   "all",
-		"addOptions":        map[string]any{"searchForMissingAlbums": true},
+		"addOptions": map[string]any{
+			"monitor":                monitor,
+			"monitored":              true,
+			"searchForMissingAlbums": true,
+		},
 	})
 	if err != nil {
 		return fmt.Errorf("%w: marshal: %v", ErrInvalidPayload, err)
@@ -324,19 +335,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
+// AddAlbum adds one album to Lidarr the way Lidarr's own UI does: look the
-// otherwise.
+// 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{
+	album, err := c.lookupAlbumResource(ctx, p.ForeignAlbumID)
-		"foreignAlbumId":    p.ForeignAlbumID,
+	if err != nil {
-		"foreignArtistId":   p.ForeignArtistID,
+		return err
-		"artistName":        p.ArtistName,
+	}
-		"qualityProfileId":  p.QualityProfileID,
+	album["monitored"] = true
-		"metadataProfileId": p.MetadataProfileID,
+	album["addOptions"] = map[string]any{"searchForNewAlbum": true}
-		"rootFolderPath":    p.RootFolderPath,
+
-		"monitored":         true,
+	artist, _ := album["artist"].(map[string]any)
-		"addOptions":        map[string]any{"searchForNewAlbum": true},
+	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)
 	}

internal/lidarr/client_test.go

@@ -288,13 +288,24 @@ func TestAddArtist_PostsCorrectBody(t *testing.T) {
 	if decoded["monitored"] != true {
 		t.Errorf("monitored = %v, want true", decoded["monitored"])
 	}
-	if decoded["monitor"] != "all" {
+	if decoded["monitorNewItems"] != "all" {
-		t.Errorf("monitor = %v, want all (MonitorAll=true)", decoded["monitor"])
+		t.Errorf("monitorNewItems = %v, want all", decoded["monitorNewItems"])
+	}
+	// #5239: ArtistResource has no top-level monitor; Lidarr reads the
+	// choice from addOptions and silently drops it anywhere else.
+	if _, set := decoded["monitor"]; set {
+		t.Errorf("monitor sent at top level (%v), where Lidarr ignores it", decoded["monitor"])
 	}
 	opts, ok := decoded["addOptions"].(map[string]any)
 	if !ok {
 		t.Fatalf("addOptions missing or wrong type: %T", decoded["addOptions"])
 	}
+	if opts["monitor"] != "all" {
+		t.Errorf("addOptions.monitor = %v, want all (MonitorAll=true)", opts["monitor"])
+	}
+	if opts["monitored"] != true {
+		t.Errorf("addOptions.monitored = %v, want true", opts["monitored"])
+	}
 	if opts["searchForMissingAlbums"] != true {
 		t.Errorf("addOptions.searchForMissingAlbums = %v, want true", opts["searchForMissingAlbums"])
 	}
@@ -309,8 +320,8 @@ func TestAddArtist_MonitorFuture(t *testing.T) {
 	defer srv.Close()
 
 	_ = c.AddArtist(context.Background(), AddArtistParams{MonitorAll: false})
-	if decoded["monitor"] != "future" {
+	if opts, _ := decoded["addOptions"].(map[string]any); opts["monitor"] != "future" {
-		t.Errorf("monitor = %v, want future (MonitorAll=false)", decoded["monitor"])
+		t.Errorf("addOptions.monitor = %v, want future (MonitorAll=false)", opts["monitor"])
 	}
 }
 
@@ -327,45 +338,143 @@ func TestAddArtist_ServerError(t *testing.T) {
 
 // --- AddAlbum ---
 
-func TestAddAlbum_PostsCorrectBody(t *testing.T) {
+const (
-	var decoded map[string]any
+	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 {
+		switch {
-			t.Errorf("method = %q, want POST", r.Method)
+		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{
+	if err := c.AddAlbum(context.Background(), addAlbumParams); err != nil {
-		ForeignAlbumID:   "a1b2c3d4-e5f6-7890-abcd-ef1234567890",
-		ForeignArtistID:  "069b64b6-7884-4f6a-94cc-e4c1d6c87a01",
-		QualityProfileID: 1,
-		RootFolderPath:   "/music",
-	})
-	if 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" {
+	artist, ok := body["artist"].(map[string]any)
-		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)
 	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 {
+	for k, want := range map[string]any{
-		t.Errorf("addOptions.searchForNewAlbum = %v, want true", opts["searchForNewAlbum"])
+		"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 +483,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)
 	}

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:<mbid>), 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)
+}

internal/lidarr/types.go

@@ -57,13 +57,13 @@ type AddArtistParams struct {
 	QualityProfileID  int
 	MetadataProfileID int
 	RootFolderPath    string
-	MonitorAll        bool // true => monitor="all"; false => "future"
+	MonitorAll        bool // true => addOptions.monitor="all"; false => "future"
 }
 
-// AddAlbumParams are the fields Lidarr requires on POST /api/v1/album.
+// AddAlbumParams identify the album to add and carry the settings Lidarr
-// MetadataProfileID is included for the case where Lidarr has to create
+// needs when it has to create the parent artist on the fly (an album by an
-// the parent artist on the fly (when adding an album by an artist not
+// artist not yet in Lidarr). The artist fields are a fallback only: the
-// yet in the library).
+// album lookup normally supplies the artist itself.
 type AddAlbumParams struct {
 	ForeignAlbumID    string
 	ForeignArtistID   string // Lidarr requires the artist's foreign id too