diff --git a/cmd/artwork.go b/cmd/artwork.go index 9a64cd8c3..0ab391425 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -746,81 +746,8 @@ func artworkKindAndID(ctx context.Context, ds model.DataStore, arg string) (mode return model.ArtworkID{Kind: kind, ID: arg}, nil } -// explainAgents accounts for every configured agent: one the CLI cannot construct (a plugin, or a -// built-in missing its credentials) never reaches the Chain, so the raw list alone overstates it. -func explainAgents(configured string, available []string) string { - if strings.TrimSpace(configured) == "" { - return "(none)" - } - var unavailable bool - names := slice.Map(strings.Split(configured, ","), func(name string) string { - name = strings.TrimSpace(name) - if slices.Contains(available, name) { - return name - } - unavailable = true - return name + "*" - }) - line := strings.Join(names, ", ") - if unavailable { - line += " (* not available to the CLI)" - } - return line -} - -// availableImageAgents names the agents that can actually supply an image for kind. -func availableImageAgents(ds model.DataStore, mgr *plugins.Manager, kind model.Kind) []string { - ag := agents.GetAgents(ds, mgr) - if kind == model.KindArtistArtwork { - return slice.Map(ag.ArtistImageAgents(), func(a agents.ArtistImageAgent) string { return a.Name }) - } - return slice.Map(ag.AlbumImageAgents(), func(a agents.AlbumImageAgent) string { return a.Name }) -} - -// explainResult states the verdict of the walk. A skipped or failed external tier, or a local -// candidate that would not open, leaves the outcome unknown: nothing observed that there is no artwork. -func explainResult(source string, steps []artwork.TraceStep) string { - if source != "" { - for _, s := range steps { - if s.Outcome == artwork.OutcomeHit { - break - } - // An external winner discards the earlier error, so the resolver settles it with no retry. - if s.Outcome == artwork.OutcomeError && strings.HasPrefix(s.Candidate, artwork.ExternalPrefix) && - !strings.HasPrefix(source, artwork.ExternalPrefix) { - return "resolved from " + source + - " (indeterminate: a higher-priority external lookup failed; this may resolve differently on a retry)" - } - } - return "resolved from " + source - } - for _, s := range steps { - switch { - case s.Outcome == artwork.OutcomeError && strings.HasPrefix(s.Candidate, artwork.ExternalPrefix): - return "indeterminate (an external lookup failed; the item may resolve on a later attempt)" - // A stage error or an unreadable candidate means a source was found but not processed; the - // worker retries rather than settling absent, so neither reads as a clean miss. - case s.Outcome == artwork.OutcomeError, s.Outcome == artwork.OutcomeUnreadable: - return "indeterminate (a candidate was found but could not be processed; the worker retries rather than settling absent)" - } - } - return "not resolved" -} - -// explainConfig names the setting that decides where a kind's artwork comes from, and its value. -func explainConfig(kind model.Kind) (name, value string) { - switch kind { - case model.KindArtistArtwork: - return "ArtistArtPriority", conf.Server.ArtistArtPriority - case model.KindAlbumArtwork: - return "CoverArtPriority", conf.Server.CoverArtPriority - case model.KindDiscArtwork: - return "DiscArtPriority", conf.Server.DiscArtPriority - case model.KindMediaFileArtwork: - return "EnableMediaFileCoverArt", strconv.FormatBool(conf.Server.EnableMediaFileCoverArt) - } - return "", "" -} +// cliUnavailableNote marks agents the CLI cannot construct; a running server loads them all. +const cliUnavailableNote = " (* not available to the CLI)" type explainReport struct { kind model.Kind @@ -912,7 +839,7 @@ func formatExplain(rep explainReport) string { } fmt.Fprintln(w, "\nConfig") - if setting, value := explainConfig(rep.kind); setting == "" { + if setting, value := artwork.ConfigFor(rep.kind); setting == "" { fmt.Fprintln(w, " (no artwork source configuration applies)") } else { fmt.Fprintf(w, " %s:\t%s\n", setting, value) @@ -948,7 +875,7 @@ func formatExplain(rep explainReport) string { case unrecorded: fmt.Fprintln(w, " not evaluated (nothing recorded; re-run with --live to walk the chain now)") default: - fmt.Fprintf(w, " %s\n", explainResult(rep.source, rep.steps)) + fmt.Fprintf(w, " %s\n", artwork.Result(rep.source, rep.steps)) } w.Flush() @@ -1002,7 +929,8 @@ func runExplain(ctx context.Context, args []string) { if kind == model.KindArtistArtwork || kind == model.KindAlbumArtwork { mgr := loadPluginAgents(ctx, explainLive) defer func() { _ = mgr.Stop() }() - rep.agents = explainAgents(conf.Server.Agents, availableImageAgents(ds, mgr, kind)) + rep.agents = artwork.FormatAgents(conf.Server.Agents, + artwork.ImageAgentNames(agents.GetAgents(ds, mgr), kind), cliUnavailableNote) } switch { case rep.walked: diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index 71b450914..dc33f9e13 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -122,106 +122,6 @@ var _ = Describe("resolveArtworkTargets", func() { }) }) -var _ = Describe("explainResult", func() { - It("reports the winning source", func() { - steps := []artwork.TraceStep{{Candidate: "folder", Outcome: "hit", Detail: "/music/a.jpg"}} - Expect(explainResult("folder", steps)).To(ContainSubstring("resolved from folder")) - }) - - It("reports not resolved when every candidate was tried and missed", func() { - steps := []artwork.TraceStep{ - {Candidate: "artist.*", Outcome: "miss"}, - {Candidate: "external:deezer", Outcome: "miss"}, - } - Expect(explainResult("", steps)).To(Equal("not resolved")) - }) - - It("reports indeterminate when a local candidate exists but could not be read", func() { - steps := []artwork.TraceStep{ - {Candidate: "cover.*", Outcome: "miss"}, - {Candidate: "embedded", Outcome: "unreadable"}, - } - Expect(explainResult("", steps)).To(ContainSubstring("indeterminate"), - "the worker retries an unreadable candidate instead of settling absent, so this is not a clean miss") - }) - - It("reports indeterminate when a processing stage errored after a candidate was found", func() { - steps := []artwork.TraceStep{ - {Candidate: "cover.*", Outcome: "hit", Detail: "/music/cover.jpg"}, - {Candidate: "store", Outcome: "error", Detail: "disk full"}, - } - Expect(explainResult("", steps)).To(ContainSubstring("indeterminate"), - "a stage error is a processing failure the worker retries, not a definitive miss") - }) - - It("does not qualify a hit that an earlier unreadable candidate preceded", func() { - // chainState.try stamps only the external error onto a hit and drops the local one, so the - // worker settles this as found; warning about it would be a false alarm. - steps := []artwork.TraceStep{ - {Candidate: "embedded", Outcome: "unreadable"}, - {Candidate: "cover.*", Outcome: "hit", Detail: "/music/cover.jpg"}, - } - Expect(explainResult("folder", steps)).To(Equal("resolved from folder")) - }) - - It("reports indeterminate when an external lookup failed transiently", func() { - steps := []artwork.TraceStep{ - {Candidate: "artist.*", Outcome: "miss"}, - {Candidate: "external:deezer", Outcome: "error", Detail: "context deadline exceeded"}, - } - Expect(explainResult("", steps)).To(ContainSubstring("indeterminate"), - "a failed network call is not evidence that the item has no artwork") - }) - - It("qualifies a win a failed higher-priority external lookup could have taken", func() { - steps := []artwork.TraceStep{ - {Candidate: "external:deezer", Outcome: "error", Detail: "context deadline exceeded"}, - {Candidate: "artist.*", Outcome: "hit", Detail: "/music/artist.jpg"}, - } - res := explainResult("artist.*", steps) - Expect(res).To(ContainSubstring("resolved from artist.*")) - Expect(res).To(ContainSubstring("indeterminate"), - "the resolver serves this hit but retries later, so the winner is provisional") - }) - - It("does not qualify an external win that followed a failed external lookup", func() { - steps := []artwork.TraceStep{ - {Candidate: "external:deezer", Outcome: "error", Detail: "context deadline exceeded"}, - {Candidate: "external:lastfm", Outcome: "hit", Detail: "http://img"}, - } - Expect(explainResult("external:lastfm", steps)).To(Equal("resolved from external:lastfm"), - "a later agent supplying the image discards the earlier error, so there is no retry to warn about") - }) - - It("does not qualify a win that outranked the failed external lookup", func() { - steps := []artwork.TraceStep{ - {Candidate: "artist.*", Outcome: "hit"}, - {Candidate: "external:deezer", Outcome: "error", Detail: "context deadline exceeded"}, - } - Expect(explainResult("artist.*", steps)).To(Equal("resolved from artist.*")) - }) -}) - -var _ = Describe("explainAgents", func() { - It("accounts for every configured agent, marking the ones the CLI could not use", func() { - out := explainAgents("artist-nfo-metadata,apple-music,deezer,lastfm", []string{"deezer"}) - for _, name := range []string{"artist-nfo-metadata", "apple-music", "deezer", "lastfm"} { - Expect(out).To(ContainSubstring(name), - "a configured agent missing from this line reads as if it had never been configured") - } - Expect(out).To(ContainSubstring("not available to the CLI")) - }) - - It("does not mark anything when every configured agent is available", func() { - out := explainAgents("deezer, lastfm", []string{"lastfm", "deezer"}) - Expect(out).To(Equal("deezer, lastfm")) - }) - - It("reports an empty configuration as none, not as an unavailable agent", func() { - Expect(explainAgents("", nil)).To(Equal("(none)")) - }) -}) - var _ = Describe("formatExplain", func() { var rep explainReport @@ -404,25 +304,6 @@ var _ = Describe("formatExplain", func() { }) }) -var _ = Describe("explainConfig", func() { - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - conf.Server.DiscArtPriority = "cover.jpg" - conf.Server.EnableMediaFileCoverArt = true - }) - - DescribeTable("names the setting that decides where a kind's artwork comes from", - func(kind model.Kind, setting, value string) { - gotSetting, gotValue := explainConfig(kind) - Expect(gotSetting).To(Equal(setting)) - Expect(gotValue).To(Equal(value)) - }, - Entry("disc", model.KindDiscArtwork, "DiscArtPriority", "cover.jpg"), - Entry("media file", model.KindMediaFileArtwork, "EnableMediaFileCoverArt", "true"), - Entry("playlist has none", model.KindPlaylistArtwork, "", ""), - ) -}) - var _ = Describe("artwork refresh command", func() { It("requires at least one argument", func() { Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{})).To(HaveOccurred()) diff --git a/core/artwork/explain.go b/core/artwork/explain.go new file mode 100644 index 000000000..4c0d465c9 --- /dev/null +++ b/core/artwork/explain.go @@ -0,0 +1,85 @@ +package artwork + +import ( + "slices" + "strconv" + "strings" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/core/agents" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" +) + +// Result states the verdict of the walk. A skipped or failed external tier, or a local candidate +// that would not open, leaves the outcome unknown: nothing observed that there is no artwork. +func Result(source string, steps []TraceStep) string { + if source != "" { + for _, s := range steps { + if s.Outcome == OutcomeHit { + break + } + // An external winner discards the earlier error, so the resolver settles it with no retry. + if s.Outcome == OutcomeError && strings.HasPrefix(s.Candidate, ExternalPrefix) && + !strings.HasPrefix(source, ExternalPrefix) { + return "resolved from " + source + + " (indeterminate: a higher-priority external lookup failed; this may resolve differently on a retry)" + } + } + return "resolved from " + source + } + for _, s := range steps { + switch { + case s.Outcome == OutcomeError && strings.HasPrefix(s.Candidate, ExternalPrefix): + return "indeterminate (an external lookup failed; the item may resolve on a later attempt)" + case s.Outcome == OutcomeError, s.Outcome == OutcomeUnreadable: + return "indeterminate (a candidate was found but could not be processed; the worker retries rather than settling absent)" + } + } + return "not resolved" +} + +// ConfigFor names the setting that decides where a kind's artwork comes from, and its value. +func ConfigFor(kind model.Kind) (setting, value string) { + switch kind { + case model.KindArtistArtwork: + return "ArtistArtPriority", conf.Server.ArtistArtPriority + case model.KindAlbumArtwork: + return "CoverArtPriority", conf.Server.CoverArtPriority + case model.KindDiscArtwork: + return "DiscArtPriority", conf.Server.DiscArtPriority + case model.KindMediaFileArtwork: + return "EnableMediaFileCoverArt", strconv.FormatBool(conf.Server.EnableMediaFileCoverArt) + } + return "", "" +} + +// ImageAgentNames names the agents that can actually supply an image for kind. +func ImageAgentNames(ag *agents.Agents, kind model.Kind) []string { + if kind == model.KindArtistArtwork { + return slice.Map(ag.ArtistImageAgents(), func(a agents.ArtistImageAgent) string { return a.Name }) + } + return slice.Map(ag.AlbumImageAgents(), func(a agents.AlbumImageAgent) string { return a.Name }) +} + +// FormatAgents accounts for every configured agent: one that cannot be constructed never reaches +// the chain, so the raw list alone overstates it. unavailableNote is appended only if some are. +func FormatAgents(configured string, available []string, unavailableNote string) string { + if strings.TrimSpace(configured) == "" { + return "(none)" + } + var unavailable bool + names := slice.Map(strings.Split(configured, ","), func(name string) string { + name = strings.TrimSpace(name) + if slices.Contains(available, name) { + return name + } + unavailable = true + return name + "*" + }) + line := strings.Join(names, ", ") + if unavailable { + line += unavailableNote + } + return line +} diff --git a/core/artwork/explain_test.go b/core/artwork/explain_test.go new file mode 100644 index 000000000..14df4bbbf --- /dev/null +++ b/core/artwork/explain_test.go @@ -0,0 +1,82 @@ +package artwork_test + +import ( + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/core/artwork" + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Result", func() { + It("reports the source when one was found", func() { + steps := []artwork.TraceStep{{Candidate: "cover.*", Outcome: artwork.OutcomeHit}} + Expect(artwork.Result("folder", steps)).To(Equal("resolved from folder")) + }) + + It("flags a local win that a failed external lookup could have outranked", func() { + steps := []artwork.TraceStep{ + {Candidate: artwork.ExternalPrefix + "deezer", Outcome: artwork.OutcomeError, Detail: "timeout"}, + {Candidate: "cover.*", Outcome: artwork.OutcomeHit}, + } + Expect(artwork.Result("folder", steps)).To(ContainSubstring("indeterminate")) + }) + + It("does not flag an external winner", func() { + steps := []artwork.TraceStep{ + {Candidate: artwork.ExternalPrefix + "lastfm", Outcome: artwork.OutcomeError}, + {Candidate: artwork.ExternalPrefix + "deezer", Outcome: artwork.OutcomeHit}, + } + Expect(artwork.Result(artwork.ExternalPrefix+"deezer", steps)). + To(Equal("resolved from external:deezer")) + }) + + It("is indeterminate when an external lookup failed and nothing resolved", func() { + steps := []artwork.TraceStep{{Candidate: artwork.ExternalPrefix + "deezer", Outcome: artwork.OutcomeError}} + Expect(artwork.Result("", steps)).To(ContainSubstring("an external lookup failed")) + }) + + It("is indeterminate when a candidate could not be processed", func() { + steps := []artwork.TraceStep{{Candidate: "cover.*", Outcome: artwork.OutcomeUnreadable}} + Expect(artwork.Result("", steps)).To(ContainSubstring("could not be processed")) + }) + + It("reports a clean miss", func() { + steps := []artwork.TraceStep{{Candidate: "cover.*", Outcome: artwork.OutcomeMiss}} + Expect(artwork.Result("", steps)).To(Equal("not resolved")) + }) +}) + +var _ = Describe("ConfigFor", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.ArtistArtPriority = "external" + }) + + It("names the setting for a kind that has one", func() { + setting, value := artwork.ConfigFor(model.KindArtistArtwork) + Expect(setting).To(Equal("ArtistArtPriority")) + Expect(value).To(Equal("external")) + }) + + It("returns nothing for a kind with no source configuration", func() { + setting, _ := artwork.ConfigFor(model.KindPlaylistArtwork) + Expect(setting).To(BeEmpty()) + }) +}) + +var _ = Describe("FormatAgents", func() { + It("reports none when nothing is configured", func() { + Expect(artwork.FormatAgents(" ", nil, "")).To(Equal("(none)")) + }) + + It("keeps the configured order and marks what is unavailable", func() { + got := artwork.FormatAgents("spotify, lastfm", []string{"lastfm"}, " (* missing)") + Expect(got).To(Equal("spotify*, lastfm (* missing)")) + }) + + It("omits the note when every configured agent is available", func() { + Expect(artwork.FormatAgents("lastfm", []string{"lastfm"}, " (* missing)")).To(Equal("lastfm")) + }) +})