diff --git a/cmd/artwork.go b/cmd/artwork.go index f5dee3cbf..db7d5e1a5 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -10,7 +10,6 @@ import ( "slices" "strconv" "strings" - "text/tabwriter" "time" "github.com/navidrome/navidrome/conf" @@ -23,11 +22,6 @@ import ( "github.com/spf13/cobra" ) -var artworkKinds = []model.Kind{ - model.KindArtistArtwork, model.KindAlbumArtwork, - model.KindPlaylistArtwork, model.KindRadioArtwork, -} - var explainLive bool var ( @@ -128,13 +122,20 @@ type absentCount struct { } type statusReport struct { - queueTotal int64 - queue []model.ArtworkQueueStat - sources []sourceCount - absent []absentCount - inputs []artwork.FingerprintInput - stored string - current string + queue []model.ArtworkQueueStat + sources []sourceCount + absent []absentCount + inputs []artwork.FingerprintInput + stored string + current string +} + +func (r statusReport) queueTotal() int64 { + var n int64 + for _, s := range r.queue { + n += s.Count + } + return n } func (r statusReport) backfillQueued() int64 { @@ -151,15 +152,12 @@ func collectStatus(ctx context.Context, ds model.DataStore) (statusReport, error q := ds.ArtworkQueue(ctx) var rep statusReport var err error - if rep.queueTotal, err = q.Count(); err != nil { - return rep, fmt.Errorf("counting the artwork queue: %w", err) - } if rep.queue, err = q.CountByKindAndPriority(); err != nil { return rep, fmt.Errorf("breaking the artwork queue down by kind: %w", err) } cutoff := time.Now().Add(-artwork.StaleAbsentAge) - for _, k := range artworkKinds { + for _, k := range artwork.RecheckKinds { sources, err := q.SourcesInUse(k) if err != nil { return rep, fmt.Errorf("listing the sources in use by %s artwork: %w", k, err) @@ -188,7 +186,7 @@ func collectStatus(ctx context.Context, ds model.DataStore) (statusReport, error func formatStatus(rep statusReport) string { var sb strings.Builder - w := tabwriter.NewWriter(&sb, 0, 4, 2, ' ', 0) + w := newTabWriter(&sb) fmt.Fprintln(w, "Queue") if len(rep.queue) == 0 { @@ -198,7 +196,7 @@ func formatStatus(rep statusReport) string { for _, s := range rep.queue { fmt.Fprintf(w, " %s\t%s\t%d\n", kindName(s.ItemKind), priorityName(s.Priority), s.Count) } - fmt.Fprintf(w, " TOTAL\t\t%d\n", rep.queueTotal) + fmt.Fprintf(w, " TOTAL\t\t%d\n", rep.queueTotal()) } fmt.Fprintln(w, "\nSources") @@ -232,7 +230,6 @@ func formatStatus(rep statusReport) string { // backfillState leads with the queued backlog: by the time anyone runs this, backfill has usually // already stored the new fingerprint, and "up to date" would bury the flood it is still working through. func backfillState(rep statusReport) string { - // A stale stored fingerprint means backfill has not recorded the new one, so the flood repeats. pending := "fingerprint changed — every artist, album, playlist and radio will be re-enqueued on the next startup" if n := rep.backfillQueued(); n > 0 { if rep.stored != rep.current { @@ -268,7 +265,7 @@ func priorityName(p int) string { } func runReprocess(ctx context.Context) { - kinds, err := selectedKinds(reprocessKinds, reprocessSelectsAll(reprocessKinds, reprocessSources, reprocessAll)) + kinds, err := selectedKinds(reprocessKinds, reprocessSources, reprocessAll) if err != nil { log.Fatal(ctx, err) } @@ -282,15 +279,10 @@ func runReprocess(ctx context.Context) { } } -// reprocessSelectsAll reports whether every kind is targeted: a source filter on its own is already -// a complete selection, so it does not also need a kind. -func reprocessSelectsAll(kinds, sources []string, all bool) bool { - return all || (len(kinds) == 0 && len(sources) > 0) -} - -func selectedKinds(kinds []string, all bool) ([]model.Kind, error) { - if all { - return artworkKinds, nil +func selectedKinds(kinds, sources []string, all bool) ([]model.Kind, error) { + // A source filter on its own is already a complete selection, so it does not also need a kind. + if all || (len(kinds) == 0 && len(sources) > 0) { + return artwork.RecheckKinds, nil } if len(kinds) == 0 { return nil, fmt.Errorf("no selector given: pass --kind, --source or --all") @@ -301,12 +293,10 @@ func selectedKinds(kinds []string, all bool) ([]model.Kind, error) { if err != nil { return nil, err } - // A repeated kind would be counted twice, overstating the cost the operator confirms. - if !slices.Contains(out, kind) { - out = append(out, kind) - } + out = append(out, kind) } - return out, nil + // A repeated kind would be counted twice, overstating the cost the operator confirms. + return slice.Unique(out), nil } // absentSource is how the stored empty source — resolved, no image — is spelled on the CLI. @@ -323,11 +313,8 @@ func repositorySources(sources []string) []string { func displaySource(s string) string { return cmp.Or(s, absentSource) } -// confirmFunc reports whether the operator accepted queueing total items, of which external may -// reach an external agent. type confirmFunc func(out io.Writer, total, external int64) bool -// reprocessConfirm is the only bypass of the confirmation: --yes, for scripted use. func reprocessConfirm(yes bool, in io.Reader) confirmFunc { if yes { return func(io.Writer, int64, int64) bool { return true } @@ -335,11 +322,20 @@ func reprocessConfirm(yes bool, in io.Reader) confirmFunc { return promptConfirm(in) } +// externalEstimate is an upper bound: a higher-priority local candidate may win before the walk +// ever reaches an agent. +func externalEstimate(n int64) string { + if n == 0 { + return "none" + } + return fmt.Sprintf("up to %d", n) +} + func promptConfirm(in io.Reader) confirmFunc { return func(out io.Writer, total, external int64) bool { - cost := fmt.Sprintf(", requiring up to %d external lookups", external) - if external == 0 { - cost = "" + var cost string + if external > 0 { + cost = fmt.Sprintf(", requiring %s external lookups", externalEstimate(external)) } fmt.Fprintf(out, "\nThis will re-resolve %d items%s. Continue? [y/N] ", total, cost) var answer string @@ -358,16 +354,12 @@ func validateSources(q model.ArtworkQueueRepository, sources []string) error { return nil } var inUse []string - for _, k := range artworkKinds { + for _, k := range artwork.RecheckKinds { found, err := q.SourcesInUse(k) if err != nil { return fmt.Errorf("listing the sources in use by %s artwork: %w", k, err) } - for _, s := range found { - if !slices.Contains(inUse, s) { - inUse = append(inUse, s) - } - } + inUse = slice.Unique(append(inUse, found...)) } var unknown []string for _, s := range sources { @@ -402,15 +394,12 @@ func reprocessArtwork(ctx context.Context, ds model.DataStore, kinds []model.Kin } matched[i] = n total += n - if walksPriorityChain(k) { + if artwork.WalksPriorityChain(k) { external += n } } printReprocessPreview(out, kinds, matched, total, external, sources) - if total == 0 { - fmt.Fprintln(out, "\nNothing matches this selection.") - } switch { case dryRun: fmt.Fprintln(out, "\nDry run: nothing was queued.") @@ -442,10 +431,10 @@ func reprocessArtwork(ctx context.Context, ds model.DataStore, kinds []model.Kin return nil } -// printReprocessPreview also states the external estimate, which --dry-run must show precisely -// because it skips the prompt that would otherwise carry it. +// printReprocessPreview also states the external estimate, which --dry-run must show because it +// skips the prompt that would otherwise carry it. func printReprocessPreview(out io.Writer, kinds []model.Kind, matched []int64, total, external int64, sources []string) { - w := tabwriter.NewWriter(out, 0, 4, 2, ' ', 0) + w := newTabWriter(out) shown := slice.Map(sources, displaySource) fmt.Fprintf(w, "Sources:\t%s\n\n", cmp.Or(strings.Join(shown, ", "), "(any)")) fmt.Fprintln(w, "KIND\tMATCHED") @@ -455,11 +444,10 @@ func printReprocessPreview(out io.Writer, kinds []model.Kind, matched []int64, t fmt.Fprintf(w, "TOTAL\t%d\n", total) w.Flush() - estimate := fmt.Sprintf("up to %d", external) - if external == 0 { - estimate = "none" + fmt.Fprintf(out, "\nExternal lookups: %s\n", externalEstimate(external)) + if total == 0 { + fmt.Fprintln(out, "\nNothing matches this selection.") } - fmt.Fprintf(out, "\nExternal lookups: %s\n", estimate) } func runRefresh(ctx context.Context, kind model.Kind, ids []string) { @@ -493,19 +481,13 @@ func refreshItems(ctx context.Context, ds model.DataStore, kind model.Kind, ids func parseArtworkKind(s string) (model.Kind, error) { kind, ok := model.ParseKind(s) - if ok && slices.Contains(artworkKinds, kind) { + if ok && slices.Contains(artwork.RecheckKinds, kind) { return kind, nil } - valid := slice.Map(artworkKinds, func(k model.Kind) string { return k.Prefix() }) + valid := slice.Map(artwork.RecheckKinds, func(k model.Kind) string { return k.Prefix() }) return kind, fmt.Errorf("invalid kind %q, expected one of: %s", s, strings.Join(valid, ", ")) } -// walksPriorityChain reports whether the resolver picks this kind's artwork from a priority -// chain; playlists and radios resolve from a single source, so there is no walk to explain. -func walksPriorityChain(kind model.Kind) bool { - return kind == model.KindArtistArtwork || kind == model.KindAlbumArtwork -} - // explainResult states the verdict of the walk. An external tier that was skipped or that failed // leaves the outcome unknown: nothing observed that the item has no artwork. func explainResult(source string, steps []artwork.TraceStep) string { @@ -541,7 +523,6 @@ type explainReport struct { priorityName string priority string agents string - walksChain bool steps []artwork.TraceStep source string resolveErr error @@ -549,7 +530,8 @@ type explainReport struct { func formatExplain(rep explainReport) string { var sb strings.Builder - w := tabwriter.NewWriter(&sb, 0, 4, 2, ' ', 0) + w := newTabWriter(&sb) + walksChain := artwork.WalksPriorityChain(rep.kind) fmt.Fprintln(w, "Item") fmt.Fprintf(w, " Kind:\t%s (%s)\n", rep.kind, rep.kind.Prefix()) @@ -578,7 +560,7 @@ func formatExplain(rep explainReport) string { } fmt.Fprintln(w, "\nConfig") - if !rep.walksChain { + if !walksChain { fmt.Fprintln(w, " (no priority chain configuration applies)") } else { fmt.Fprintf(w, " %s:\t%s\n", rep.priorityName, rep.priority) @@ -586,7 +568,7 @@ func formatExplain(rep explainReport) string { } fmt.Fprintln(w, "\nChain") - if !rep.walksChain { + if !walksChain { fmt.Fprintf(w, " (%s artwork does not walk a priority chain)\n", rep.kind) } else { fmt.Fprintln(w, " CANDIDATE\tOUTCOME\tDETAIL") @@ -600,7 +582,7 @@ func formatExplain(rep explainReport) string { switch { case rep.resolveErr != nil: fmt.Fprintf(w, " resolution failed: %s\n", rep.resolveErr) - case !rep.walksChain: + case !walksChain: fmt.Fprintln(w, " not evaluated (no chain was walked; see Stored above)") default: fmt.Fprintf(w, " %s\n", explainResult(rep.source, rep.steps)) @@ -628,8 +610,7 @@ func runExplain(ctx context.Context, kind model.Kind, id string) { rep := explainReport{ kind: kind, id: id, name: name, priorityName: "CoverArtPriority", priority: conf.Server.CoverArtPriority, - agents: conf.Server.Agents, - walksChain: walksPriorityChain(kind), + agents: conf.Server.Agents, } if kind == model.KindArtistArtwork { rep.priorityName, rep.priority = "ArtistArtPriority", conf.Server.ArtistArtPriority @@ -644,7 +625,7 @@ func runExplain(ctx context.Context, kind model.Kind, id string) { log.Fatal(ctx, "Failed to read the artwork queue", "kind", kind, "id", id, err) } - if rep.walksChain { + if artwork.WalksPriorityChain(kind) { trace := &artwork.ChainTrace{} resolver := CreateArtworkResolver(trace, explainLive) resolve := resolver.ResolveAlbum diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index 5bce316e0..b00e42ab7 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -87,18 +87,6 @@ var _ = Describe("explainResult", func() { }) }) -var _ = Describe("walksPriorityChain", func() { - It("is true for the kinds the resolver walks", func() { - Expect(walksPriorityChain(model.KindArtistArtwork)).To(BeTrue()) - Expect(walksPriorityChain(model.KindAlbumArtwork)).To(BeTrue()) - }) - - It("is false for the kinds resolved directly", func() { - Expect(walksPriorityChain(model.KindPlaylistArtwork)).To(BeFalse()) - Expect(walksPriorityChain(model.KindRadioArtwork)).To(BeFalse()) - }) -}) - var _ = Describe("formatExplain", func() { var rep explainReport @@ -110,7 +98,6 @@ var _ = Describe("formatExplain", func() { priorityName: "ArtistArtPriority", priority: "external, artist.*", agents: "lastfm,spotify", - walksChain: true, steps: []artwork.TraceStep{ {Candidate: "upload", Outcome: "skipped", Detail: "no uploaded image"}, {Candidate: "external:deezer", Outcome: "would-try"}, @@ -119,11 +106,8 @@ var _ = Describe("formatExplain", func() { } }) - It("prints every block", func() { + It("reports the item, its config and the chain it walked", func() { out := formatExplain(rep) - for _, block := range []string{"Item", "Stored", "Queue", "Config", "Chain", "Result"} { - Expect(out).To(ContainSubstring(block)) - } Expect(out).To(ContainSubstring("Radiohead")) Expect(out).To(ContainSubstring("ar-1")) Expect(out).To(ContainSubstring("ArtistArtPriority")) @@ -171,7 +155,6 @@ var _ = Describe("formatExplain", func() { It("says a kind that does not walk a chain has no chain, without an empty table", func() { rep.kind = model.KindPlaylistArtwork - rep.walksChain = false rep.steps = nil rep.priorityName, rep.priority = "CoverArtPriority", "cover.*, embedded" @@ -196,55 +179,47 @@ var _ = Describe("artwork refresh command", func() { var _ = Describe("artwork reprocess selection", func() { It("errors when no selector is given", func() { - _, err := selectedKinds(nil, false) + _, err := selectedKinds(nil, nil, false) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("--all")) }) It("returns every kind for --all", func() { - ks, err := selectedKinds(nil, true) + ks, err := selectedKinds(nil, nil, true) Expect(err).ToNot(HaveOccurred()) - Expect(ks).To(ConsistOf(artworkKinds)) + Expect(ks).To(ConsistOf(artwork.RecheckKinds)) + }) + + It("returns every kind for a source filter given without a kind", func() { + ks, err := selectedKinds(nil, []string{"folder"}, false) + Expect(err).ToNot(HaveOccurred()) + Expect(ks).To(ConsistOf(artwork.RecheckKinds), "--source alone is already a complete selection") }) It("returns only the named kinds", func() { - ks, err := selectedKinds([]string{"ar"}, false) + ks, err := selectedKinds([]string{"ar"}, nil, false) + Expect(err).ToNot(HaveOccurred()) + Expect(ks).To(Equal([]model.Kind{model.KindArtistArtwork})) + }) + + It("keeps a named kind selection alongside a source filter", func() { + ks, err := selectedKinds([]string{"ar"}, []string{"folder"}, false) Expect(err).ToNot(HaveOccurred()) Expect(ks).To(Equal([]model.Kind{model.KindArtistArtwork})) }) It("rejects an unknown kind", func() { - _, err := selectedKinds([]string{"zz"}, false) + _, err := selectedKinds([]string{"zz"}, nil, false) Expect(err).To(HaveOccurred()) }) It("counts a repeated kind once", func() { - ks, err := selectedKinds([]string{"ar", "ar"}, false) + ks, err := selectedKinds([]string{"ar", "ar"}, nil, false) Expect(err).ToNot(HaveOccurred()) Expect(ks).To(Equal([]model.Kind{model.KindArtistArtwork})) }) }) -var _ = Describe("reprocessSelectsAll", func() { - It("keeps a named kind selection", func() { - Expect(reprocessSelectsAll([]string{"ar"}, nil, false)).To(BeFalse()) - Expect(reprocessSelectsAll([]string{"ar"}, []string{"folder"}, false)).To(BeFalse()) - }) - - It("targets every kind for a source filter given without a kind", func() { - Expect(reprocessSelectsAll(nil, []string{"folder"}, false)).To(BeTrue()) - }) - - It("targets every kind for --all", func() { - Expect(reprocessSelectsAll(nil, nil, true)).To(BeTrue()) - Expect(reprocessSelectsAll([]string{"ar"}, nil, true)).To(BeTrue()) - }) - - It("is false with no selector, leaving selectedKinds to reject it", func() { - Expect(reprocessSelectsAll(nil, nil, false)).To(BeFalse()) - }) -}) - var _ = Describe("explain/reprocess source round trip", func() { ctx := context.Background() @@ -506,7 +481,6 @@ var _ = Describe("collectStatus", func() { rep, err := collectStatus(ctx, ds) Expect(err).ToNot(HaveOccurred()) - Expect(rep.queueTotal).To(Equal(int64(1))) Expect(rep.queue).To(ConsistOf(model.ArtworkQueueStat{ItemKind: "ar", Priority: model.ArtworkPriorityBackfill, Count: 1})) Expect(rep.sources).To(ContainElements( @@ -528,18 +502,6 @@ var _ = Describe("collectStatus", func() { "the CLI must report the value backfill itself compares") }) - It("reports a never-recorded fingerprint as empty instead of failing", func() { - rep, err := collectStatus(ctx, ds) - Expect(err).ToNot(HaveOccurred()) - Expect(rep.stored).To(BeEmpty()) - }) - - It("collects the config inputs the fingerprint is computed from", func() { - rep, err := collectStatus(ctx, ds) - Expect(err).ToNot(HaveOccurred()) - Expect(rep.inputs).To(Equal(artwork.FingerprintInputs())) - }) - It("queues nothing", func() { _, err := collectStatus(ctx, ds) Expect(err).ToNot(HaveOccurred()) @@ -552,7 +514,6 @@ var _ = Describe("formatStatus", func() { BeforeEach(func() { rep = statusReport{ - queueTotal: 3, queue: []model.ArtworkQueueStat{ {ItemKind: "ar", Priority: model.ArtworkPriorityBackfill, Count: 2}, {ItemKind: "al", Priority: model.ArtworkPriorityScan, Count: 1}, @@ -579,14 +540,6 @@ var _ = Describe("formatStatus", func() { return body } - It("prints every block", func() { - out := formatStatus(rep) - for _, header := range []string{"Queue", "Sources", "Absent", "Backfill"} { - Expect(out).To(ContainSubstring(header)) - } - Expect(out).To(ContainSubstring("abc123")) - }) - It("names the kind and the priority of every queued row", func() { queue := block(formatStatus(rep), "Queue") Expect(queue).To(MatchRegexp(`artist\s+backfill\s+2`)) @@ -635,12 +588,14 @@ var _ = Describe("formatStatus", func() { }) It("echoes the config inputs a fingerprint change would have come from", func() { - Expect(block(formatStatus(rep), "Backfill")).To(MatchRegexp(`Agents:\s+deezer,lastfm`)) + out := block(formatStatus(rep), "Backfill") + Expect(out).To(MatchRegexp(`Agents:\s+deezer,lastfm`)) + Expect(out).To(ContainSubstring("abc123"), "the fingerprint values themselves must be printed") }) It("reports a changed fingerprint as a pending re-resolve of everything", func() { rep.stored = "older" - rep.queue, rep.queueTotal = nil, 0 + rep.queue = nil out := formatStatus(rep) Expect(out).To(ContainSubstring("fingerprint changed")) @@ -656,7 +611,7 @@ var _ = Describe("formatStatus", func() { }) It("says the queue is empty instead of printing a headless table", func() { - rep.queue, rep.queueTotal = nil, 0 + rep.queue = nil out := formatStatus(rep) Expect(out).To(ContainSubstring("empty")) diff --git a/cmd/plugin.go b/cmd/plugin.go index 6cce8ea5f..ded28e969 100644 --- a/cmd/plugin.go +++ b/cmd/plugin.go @@ -9,7 +9,6 @@ import ( "os" "strconv" "strings" - "text/tabwriter" "time" "github.com/navidrome/navidrome/conf" @@ -314,7 +313,7 @@ func formatPluginList(list model.Plugins, format string) (string, error) { return sb.String(), w.Error() case "table": var sb strings.Builder - w := tabwriter.NewWriter(&sb, 0, 4, 2, ' ', 0) + w := newTabWriter(&sb) fmt.Fprintln(w, "ID\tNAME\tVERSION\tENABLED\tLAST ERROR") for _, p := range list { name, version := manifestSummary(p) diff --git a/cmd/utils.go b/cmd/utils.go index 81d646cf1..74da51828 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + "io" + "text/tabwriter" "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/db" @@ -13,6 +15,11 @@ import ( "github.com/navidrome/navidrome/persistence" ) +// newTabWriter keeps every CLI table on the same column settings. +func newTabWriter(out io.Writer) *tabwriter.Writer { + return tabwriter.NewWriter(out, 0, 4, 2, ' ', 0) +} + func getAdminContext(ctx context.Context) (model.DataStore, context.Context) { sqlDB := db.Db() ds := persistence.New(sqlDB) diff --git a/tests/mock_artwork_queue_repo.go b/tests/mock_artwork_queue_repo.go index ba82f897e..c8e915daa 100644 --- a/tests/mock_artwork_queue_repo.go +++ b/tests/mock_artwork_queue_repo.go @@ -7,6 +7,7 @@ import ( "time" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" ) type MockArtworkQueueRepo struct { @@ -301,13 +302,8 @@ func (m *MockArtworkQueueRepo) SourcesInUse(kind model.Kind) ([]string, error) { if m.Err != nil { return nil, m.Err } - var res []string - for _, ia := range m.matchingSource(kind, nil) { - if !slices.Contains(res, ia.Source) { - res = append(res, ia.Source) - } - } - return res, nil + sources := slice.Map(m.matchingSource(kind, nil), func(ia model.ItemArtwork) string { return ia.Source }) + return slice.Unique(sources), nil } func (m *MockArtworkQueueRepo) EnqueueBySource(kind model.Kind, sources []string, priority int) (int64, error) {