feat(cli): let artwork explain/refresh accept an id without its kind (#5988)

* feat(cmd): make artwork explain/refresh accept an id without its kind

The kind can now come from the id itself: a full artwork id (al-<id>)
carries it in the prefix, and a bare id is resolved across tables via
GetEntityByID. The explicit <kind> <id> leader still works.

* fix(cmd): keep refreshing resolvable ids when others fail to resolve

resolveArtworkTargets now collects a self-describing id it cannot resolve
as a failure instead of aborting, so refresh reports and skips the bad ones
and still queues the rest, matching refreshItems per-item behavior. explain
stays strict and rejects any unresolved input.

cmd/artwork.go

@@ -59,31 +59,27 @@ var artworkCmd = &cobra.Command{
 }
 
 var artworkExplainCmd = &cobra.Command{
-	Use:   "explain <kind> <id>",
+	Use:   "explain [<kind>] <id>",
 	Short: "Explain why an item's artwork resolved the way it did",
 	Long: "Explain why an item's artwork resolved the way it did.\n\n" +
+		"The item can be given as a bare id, a full artwork id (e.g. al-<id>), or a <kind> <id> pair.\n" +
 		"<kind> is one of: " + kindPrefixes(explainKinds) + ".\n" +
 		"A disc artwork id is the album id and the disc number, joined by a colon: <albumID>:2",
-	Args: cobra.ExactArgs(2),
+	Args: cobra.RangeArgs(1, 2),
 	Run: func(cmd *cobra.Command, args []string) {
-		kind, err := parseArtworkKind(args[0], explainKinds)
-		if err != nil {
-			log.Fatal(cmd.Context(), err)
-		}
-		runExplain(cmd.Context(), kind, args[1])
+		runExplain(cmd.Context(), args)
 	},
 }
 
 var artworkRefreshCmd = &cobra.Command{
-	Use:   "refresh <kind> <id>...",
+	Use:   "refresh [<kind>] <id>...",
 	Short: "Clear an item's artwork state and re-resolve it",
-	Args:  cobra.MinimumNArgs(2),
+	Long: "Clear an item's artwork state and re-resolve it.\n\n" +
+		"Each item can be given as a bare id, a full artwork id (e.g. al-<id>), or a shared\n" +
+		"<kind> <id>... leader. <kind> is one of: " + kindPrefixes(artwork.RefreshableKinds) + ".",
+	Args: cobra.MinimumNArgs(1),
 	Run: func(cmd *cobra.Command, args []string) {
-		kind, err := parseArtworkKind(args[0], artwork.RefreshableKinds)
-		if err != nil {
-			log.Fatal(cmd.Context(), err)
-		}
-		runRefresh(cmd.Context(), kind, args[1:])
+		runRefresh(cmd.Context(), args)
 	},
 }
 
@@ -499,19 +495,28 @@ func printReprocessPreview(out io.Writer, kinds []model.Kind, matched []int64, t
 	}
 }
 
-func runRefresh(ctx context.Context, kind model.Kind, ids []string) {
+func runRefresh(ctx context.Context, args []string) {
 	defer db.Init(ctx)()
 	ds, ctx := getAdminContext(ctx)
 
-	if failed := refreshItems(ctx, ds, kind, ids, os.Stdout); failed > 0 {
-		log.Fatal(ctx, "Failed to refresh artwork", "kind", kind, "failed", failed, "total", len(ids))
+	targets, failures, err := resolveArtworkTargets(ctx, ds, args, artwork.RefreshableKinds)
+	if err != nil {
+		log.Fatal(ctx, err)
+	}
+	for _, f := range failures {
+		log.Error(ctx, "Skipping unresolved item", f)
+	}
+	failed := refreshItems(ctx, ds, targets, os.Stdout) + len(failures)
+	if failed > 0 {
+		log.Fatal(ctx, "Failed to refresh artwork", "failed", failed, "total", len(targets)+len(failures))
 	}
 }
 
-// refreshItems keeps going after a failure — the ids are independent — and returns how many failed.
-func refreshItems(ctx context.Context, ds model.DataStore, kind model.Kind, ids []string, out io.Writer) int {
+// refreshItems keeps going after a failure — the items are independent — and returns how many failed.
+func refreshItems(ctx context.Context, ds model.DataStore, targets []model.ArtworkID, out io.Writer) int {
 	var failed int
-	for _, id := range ids {
+	for _, t := range targets {
+		kind, id := t.Kind, t.ID
 		// artwork.Refresh would happily queue an id that does not exist, orphaning a queue row.
 		if _, err := artworkItemName(ctx, ds, kind, id); err != nil {
 			log.Error(ctx, "Item not found", "kind", kind, "id", id, err)
@@ -544,7 +549,56 @@ func parseArtworkKind(s string, valid []model.Kind) (model.Kind, error) {
 	if ok && slices.Contains(valid, kind) {
 		return kind, nil
 	}
-	return kind, fmt.Errorf("invalid kind %q, expected one of: %s", s, kindPrefixes(valid))
+	return kind, invalidKindErr(s, valid)
+}
+
+func invalidKindErr(s string, valid []model.Kind) error {
+	return fmt.Errorf("invalid kind %q, expected one of: %s", s, kindPrefixes(valid))
+}
+
+// resolveArtworkTargets resolves explain/refresh positional args into artwork ids, accepting a
+// shared "<kind> <id>..." leader or self-describing args (a bare id, or a full artwork id). A
+// self-describing arg that cannot be resolved is returned as a failure rather than aborting the
+// batch, so refresh can process the resolvable ids; a malformed <kind> leader is a usage error.
+func resolveArtworkTargets(ctx context.Context, ds model.DataStore, args []string, valid []model.Kind) ([]model.ArtworkID, []error, error) {
+	if kind, ok := model.ParseKind(args[0]); ok && len(args) > 1 {
+		if !slices.Contains(valid, kind) {
+			return nil, nil, invalidKindErr(args[0], valid)
+		}
+		return slice.Map(args[1:], func(id string) model.ArtworkID {
+			return model.ArtworkID{Kind: kind, ID: id}
+		}), nil, nil
+	}
+	var targets []model.ArtworkID
+	var failures []error
+	for _, arg := range args {
+		target, err := artworkKindAndID(ctx, ds, arg)
+		if err == nil && !slices.Contains(valid, target.Kind) {
+			err = invalidKindErr(target.Kind.Prefix(), valid)
+		}
+		if err != nil {
+			failures = append(failures, err)
+			continue
+		}
+		targets = append(targets, target)
+	}
+	return targets, failures, nil
+}
+
+// artworkKindAndID resolves one self-describing argument: a full artwork id (al-<id>) takes its kind
+// from the prefix, a bare id is looked up. Entity ids never start with "<kind>-", so no collision.
+func artworkKindAndID(ctx context.Context, ds model.DataStore, arg string) (model.ArtworkID, error) {
+	if artID, err := model.ParseArtworkID(arg); err == nil && artID.ID != "" {
+		return model.ArtworkID{Kind: artID.Kind, ID: artID.ID}, nil
+	}
+	kind, err := model.GetEntityKindByID(ctx, ds, arg)
+	if errors.Is(err, model.ErrNotFound) {
+		return model.ArtworkID{}, fmt.Errorf("could not determine kind for %q; pass an explicit <kind>", arg)
+	}
+	if err != nil {
+		return model.ArtworkID{}, err
+	}
+	return model.ArtworkID{Kind: kind, ID: arg}, nil
 }
 
 // explainAgents accounts for every configured agent: one the CLI cannot construct (a plugin, or a
@@ -721,10 +775,22 @@ func formatTime(t time.Time) string {
 	return t.Format(time.RFC3339)
 }
 
-func runExplain(ctx context.Context, kind model.Kind, id string) {
+func runExplain(ctx context.Context, args []string) {
 	defer db.Init(ctx)()
 	ds, ctx := getAdminContext(ctx)
 
+	targets, failures, err := resolveArtworkTargets(ctx, ds, args, explainKinds)
+	if err != nil {
+		log.Fatal(ctx, err)
+	}
+	if len(failures) > 0 {
+		log.Fatal(ctx, failures[0])
+	}
+	if len(targets) != 1 {
+		log.Fatal(ctx, "explain takes a single item; pass one id or a <kind> <id> pair")
+	}
+	kind, id := targets[0].Kind, targets[0].ID
+
 	name, err := artworkItemName(ctx, ds, kind, id)
 	if err != nil {
 		log.Fatal(ctx, "Item not found", "kind", kind, "id", id, err)

cmd/artwork_test.go

@@ -54,6 +54,74 @@ var _ = Describe("parseArtworkKind", func() {
 	})
 })
 
+var _ = Describe("resolveArtworkTargets", func() {
+	var ds *tests.MockDataStore
+	ctx := context.Background()
+
+	BeforeEach(func() {
+		artists := tests.CreateMockArtistRepo()
+		artists.SetData(model.Artists{{ID: "artist1"}})
+		ds = &tests.MockDataStore{MockedArtist: artists}
+	})
+
+	It("accepts the explicit <kind> <id> leader shared by every id", func() {
+		targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"al", "x", "y"}, explainKinds)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(failures).To(BeEmpty())
+		Expect(targets).To(Equal([]model.ArtworkID{
+			{Kind: model.KindAlbumArtwork, ID: "x"}, {Kind: model.KindAlbumArtwork, ID: "y"}}))
+	})
+
+	It("rejects an explicit kind the command does not accept as a usage error", func() {
+		_, _, err := resolveArtworkTargets(ctx, ds, []string{"dc", "x"}, artwork.RefreshableKinds)
+		Expect(err).To(MatchError(ContainSubstring("invalid kind")))
+	})
+
+	It("resolves a bare id by looking it up across tables", func() {
+		targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"artist1"}, explainKinds)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(failures).To(BeEmpty())
+		Expect(targets).To(Equal([]model.ArtworkID{{Kind: model.KindArtistArtwork, ID: "artist1"}}))
+	})
+
+	It("reads the kind from a full artwork id prefix without a database lookup", func() {
+		targets, _, err := resolveArtworkTargets(ctx, ds, []string{"al-realalbum"}, explainKinds)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(targets).To(Equal([]model.ArtworkID{{Kind: model.KindAlbumArtwork, ID: "realalbum"}}))
+	})
+
+	It("strips the hash suffix from a full artwork id", func() {
+		targets, _, err := resolveArtworkTargets(ctx, ds, []string{"al-realalbum_0123456789abcdef"}, explainKinds)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(targets).To(Equal([]model.ArtworkID{{Kind: model.KindAlbumArtwork, ID: "realalbum"}}))
+	})
+
+	It("collects a self-describing arg whose kind the command does not accept", func() {
+		targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"dc-realalbum:2"}, artwork.RefreshableKinds)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(targets).To(BeEmpty())
+		Expect(failures).To(HaveLen(1))
+		Expect(failures[0]).To(MatchError(ContainSubstring("invalid kind")))
+	})
+
+	It("collects an id that matches nothing and has no kind prefix", func() {
+		targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"nope"}, explainKinds)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(targets).To(BeEmpty())
+		Expect(failures).To(HaveLen(1))
+		Expect(failures[0]).To(MatchError(ContainSubstring("could not determine kind")))
+	})
+
+	It("resolves the valid ids and collects the unresolvable ones", func() {
+		targets, failures, err := resolveArtworkTargets(ctx, ds, []string{"artist1", "nope", "al-realalbum"}, explainKinds)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(targets).To(Equal([]model.ArtworkID{
+			{Kind: model.KindArtistArtwork, ID: "artist1"}, {Kind: model.KindAlbumArtwork, ID: "realalbum"}}))
+		Expect(failures).To(HaveLen(1))
+		Expect(failures[0]).To(MatchError(ContainSubstring("could not determine kind")))
+	})
+})
+
 var _ = Describe("explainResult", func() {
 	It("reports the winning source", func() {
 		steps := []artwork.TraceStep{{Candidate: "folder", Outcome: "hit", Detail: "/music/a.jpg"}}
@@ -337,10 +405,10 @@ var _ = Describe("discArtworkName", func() {
 })
 
 var _ = Describe("artwork refresh command", func() {
-	It("requires at least a kind and one id", func() {
-		Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{"ar"})).To(HaveOccurred())
+	It("requires at least one argument", func() {
+		Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{})).To(HaveOccurred())
+		Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{"id1"})).ToNot(HaveOccurred())
 		Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{"ar", "id1"})).ToNot(HaveOccurred())
-		Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{"ar", "id1", "id2"})).ToNot(HaveOccurred())
 	})
 })
 
@@ -873,7 +941,8 @@ var _ = Describe("refreshItems", func() {
 		Expect(art.PutItemArtwork(&model.ItemArtwork{ItemKind: model.KindAlbumArtwork.Prefix(),
 			ItemID: "al-1", ImageType: model.ImageTypePrimary, Hash: "abc123"})).To(Succeed())
 
-		Expect(refreshItems(ctx, ds, model.KindAlbumArtwork, []string{"al-1", "al-3"}, &out)).To(BeZero())
+		Expect(refreshItems(ctx, ds, []model.ArtworkID{
+			{Kind: model.KindAlbumArtwork, ID: "al-1"}, {Kind: model.KindAlbumArtwork, ID: "al-3"}}, &out)).To(BeZero())
 
 		_, err := art.GetItemArtwork(model.KindAlbumArtwork, "al-1", model.ImageTypePrimary)
 		Expect(err).To(MatchError(model.ErrNotFound))
@@ -884,7 +953,7 @@ var _ = Describe("refreshItems", func() {
 	})
 
 	It("skips an id that does not exist instead of queuing it", func() {
-		Expect(refreshItems(ctx, ds, model.KindAlbumArtwork, []string{"al-2"}, &out)).To(Equal(1))
+		Expect(refreshItems(ctx, ds, []model.ArtworkID{{Kind: model.KindAlbumArtwork, ID: "al-2"}}, &out)).To(Equal(1))
 
 		_, err := queue.Get(model.KindAlbumArtwork, "al-2", model.ImageTypePrimary)
 		Expect(err).To(MatchError(model.ErrNotFound), "a typo must not leave an orphan queue row")
@@ -892,8 +961,8 @@ var _ = Describe("refreshItems", func() {
 	})
 
 	It("continues past a failing id and counts the failures", func() {
-		Expect(refreshItems(ctx, ds, model.KindAlbumArtwork,
-			[]string{"al-1", "al-2", "al-3"}, &out)).To(Equal(1))
+		Expect(refreshItems(ctx, ds, []model.ArtworkID{{Kind: model.KindAlbumArtwork, ID: "al-1"},
+			{Kind: model.KindAlbumArtwork, ID: "al-2"}, {Kind: model.KindAlbumArtwork, ID: "al-3"}}, &out)).To(Equal(1))
 
 		Expect(out.String()).To(Equal("al/al-1: queued\nal/al-3: queued\n"),
 			"the ids after a failure are still refreshed")

model/get_entity.go

@@ -7,21 +7,36 @@ import (
 
 // TODO: Should the type be encoded in the ID?
 func GetEntityByID(ctx context.Context, ds DataStore, id string) (any, error) {
-	getters := []func() (any, error){
-		func() (any, error) { return ds.Artist(ctx).Get(id) },
-		func() (any, error) { return ds.Album(ctx).Get(id) },
-		func() (any, error) { return ds.Playlist(ctx).Get(id) },
-		func() (any, error) { return ds.MediaFile(ctx).Get(id) },
-		func() (any, error) { return ds.Radio(ctx).Get(id) },
+	entity, _, err := getEntity(ctx, ds, id)
+	return entity, err
+}
+
+// GetEntityKindByID resolves a bare entity id to its artwork Kind, searching the same tables as
+// GetEntityByID. It reports ErrNotFound when no entity owns the id.
+func GetEntityKindByID(ctx context.Context, ds DataStore, id string) (Kind, error) {
+	_, kind, err := getEntity(ctx, ds, id)
+	return kind, err
+}
+
+func getEntity(ctx context.Context, ds DataStore, id string) (any, Kind, error) {
+	getters := []struct {
+		kind Kind
+		get  func() (any, error)
+	}{
+		{KindArtistArtwork, func() (any, error) { return ds.Artist(ctx).Get(id) }},
+		{KindAlbumArtwork, func() (any, error) { return ds.Album(ctx).Get(id) }},
+		{KindPlaylistArtwork, func() (any, error) { return ds.Playlist(ctx).Get(id) }},
+		{KindMediaFileArtwork, func() (any, error) { return ds.MediaFile(ctx).Get(id) }},
+		{KindRadioArtwork, func() (any, error) { return ds.Radio(ctx).Get(id) }},
 	}
-	for _, get := range getters {
-		entity, err := get()
+	for _, g := range getters {
+		entity, err := g.get()
 		if err == nil {
-			return entity, nil
+			return entity, g.kind, nil
 		}
 		if !errors.Is(err, ErrNotFound) {
-			return nil, err
+			return nil, Kind{}, err
 		}
 	}
-	return nil, ErrNotFound
+	return nil, Kind{}, ErrNotFound
 }

model/get_entity_test.go

@@ -38,3 +38,25 @@ var _ = Describe("GetEntityByID", func() {
 		Expect(err).ToNot(MatchError(model.ErrNotFound))
 	})
 })
+
+var _ = Describe("GetEntityKindByID", func() {
+	var ds *tests.MockDataStore
+	var ctx context.Context
+
+	BeforeEach(func() {
+		ds = &tests.MockDataStore{}
+		ctx = GinkgoT().Context()
+	})
+
+	It("returns the artwork kind for the matching id", func() {
+		ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1"}})
+		kind, err := model.GetEntityKindByID(ctx, ds, "a1")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(kind).To(Equal(model.KindAlbumArtwork))
+	})
+
+	It("returns ErrNotFound when no entity matches", func() {
+		_, err := model.GetEntityKindByID(ctx, ds, "missing")
+		Expect(err).To(MatchError(model.ErrNotFound))
+	})
+})