From bd7b46c91c46a1be52475526dec0aeb550939b88 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 14 Aug 2026 13:55:10 -0400 Subject: [PATCH] fix(cli): cover the reprocess selection rule and validate sources table-wide The reconciliation that makes --source alone target every kind was only exercised through runReprocess, which no test calls: mutating it to `all := reprocessAll` left the suite green. It is now reprocessSelectsAll, covered for all three selectors. Scoping source validation to the selected kinds made the same well-formed filter valid or invalid depending on which other kinds were selected, and its error read the same for a typo as for a source that simply does not apply to the chosen kind. Validation is now table-wide: a typo still aborts, while a valid-but-inapplicable source falls through to "Nothing matches". Also: the prompt now counts only the kinds that reach an external agent as external cost, and --dry-run on an empty selection reports a dry run. --- cmd/artwork.go | 46 +++++++++++++++++++-------------- cmd/artwork_test.go | 63 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 82 insertions(+), 27 deletions(-) diff --git a/cmd/artwork.go b/cmd/artwork.go index f69f02ddc..8ffe05135 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -94,9 +94,7 @@ var artworkReprocessCmd = &cobra.Command{ } func runReprocess(ctx context.Context) { - // A source filter alone is a complete selection, so it targets every kind. - all := reprocessAll || (len(reprocessKinds) == 0 && len(reprocessSources) > 0) - kinds, err := selectedKinds(reprocessKinds, all) + kinds, err := selectedKinds(reprocessKinds, reprocessSelectsAll(reprocessKinds, reprocessSources, reprocessAll)) if err != nil { log.Fatal(ctx, err) } @@ -106,7 +104,7 @@ func runReprocess(ctx context.Context) { confirm := promptConfirm(os.Stdin) if reprocessYes { - confirm = func(io.Writer, int64) bool { return true } + confirm = func(io.Writer, int64, int64) bool { return true } } if err := reprocessArtwork(ctx, ds, kinds, repositorySources(reprocessSources), reprocessDryRun, confirm, os.Stdout); err != nil { @@ -114,6 +112,12 @@ 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 @@ -149,13 +153,14 @@ func repositorySources(sources []string) []string { func displaySource(s string) string { return cmp.Or(s, absentSource) } -// confirmFunc reports whether the operator accepted queueing total items. -type confirmFunc func(out io.Writer, total int64) bool +// 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 func promptConfirm(in io.Reader) confirmFunc { - return func(out io.Writer, total int64) bool { + return func(out io.Writer, total, external int64) bool { fmt.Fprintf(out, "\nThis will re-resolve %d items, requiring up to %d external lookups. Continue? [y/N] ", - total, total) + total, external) var answer string if _, err := fmt.Fscanln(in, &answer); err != nil { return false @@ -165,14 +170,14 @@ func promptConfirm(in io.Reader) confirmFunc { } } -// validateSources rejects a source no selected item resolves from: silently matching nothing reads -// as "nothing to do" when it actually means the filter was wrong. -func validateSources(q model.ArtworkQueueRepository, kinds []model.Kind, sources []string) error { +// validateSources rejects a typo'd source: matching nothing silently reads as "nothing to do" when +// it means the filter was wrong. Checked table-wide, so a filter is never a typo for one --kind only. +func validateSources(q model.ArtworkQueueRepository, sources []string) error { if len(sources) == 0 { return nil } var inUse []string - for _, k := range kinds { + for _, k := range artworkKinds { found, err := q.SourcesInUse(k) if err != nil { return fmt.Errorf("listing the sources in use by %s artwork: %w", k, err) @@ -194,7 +199,7 @@ func validateSources(q model.ArtworkQueueRepository, kinds []model.Kind, sources } valid := slice.Map(inUse, displaySource) slices.Sort(valid) - return fmt.Errorf("no selected artwork resolves from %s; sources in use: %s", + return fmt.Errorf("no artwork resolves from %s; sources in use: %s", strings.Join(unknown, ", "), cmp.Or(strings.Join(valid, ", "), "(none)")) } @@ -203,12 +208,12 @@ func validateSources(q model.ArtworkQueueRepository, kinds []model.Kind, sources func reprocessArtwork(ctx context.Context, ds model.DataStore, kinds []model.Kind, sources []string, dryRun bool, confirm confirmFunc, out io.Writer) error { q := ds.ArtworkQueue(ctx) - if err := validateSources(q, kinds, sources); err != nil { + if err := validateSources(q, sources); err != nil { return err } matched := make([]int64, len(kinds)) - var total int64 + var total, external int64 for i, k := range kinds { n, err := q.CountBySource(k, sources) if err != nil { @@ -216,17 +221,20 @@ func reprocessArtwork(ctx context.Context, ds model.DataStore, kinds []model.Kin } matched[i] = n total += n + if walksPriorityChain(k) { + external += n + } } printReprocessPreview(out, kinds, matched, total, sources) switch { - case total == 0: - fmt.Fprintln(out, "\nNothing matches this selection; nothing was queued.") - return nil case dryRun: fmt.Fprintln(out, "\nDry run: nothing was queued.") return nil - case !confirm(out, total): + case total == 0: + fmt.Fprintln(out, "\nNothing matches this selection; nothing was queued.") + return nil + case !confirm(out, total, external): fmt.Fprintln(out, "Aborted: nothing was queued.") return nil } diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index 805c0071a..19e4f7e2d 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -224,6 +224,26 @@ var _ = Describe("artwork reprocess selection", func() { }) }) +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("repositorySources", func() { It("maps the user-facing absent name onto the stored empty source", func() { Expect(repositorySources([]string{"absent", "folder"})).To(Equal([]string{"", "folder"})) @@ -240,15 +260,15 @@ var _ = Describe("promptConfirm", func() { BeforeEach(func() { out.Reset() }) It("states the external cost and accepts an explicit yes", func() { - Expect(promptConfirm(strings.NewReader("y\n"))(&out, 42)).To(BeTrue()) + Expect(promptConfirm(strings.NewReader("y\n"))(&out, 42, 7)).To(BeTrue()) Expect(out.String()).To(ContainSubstring("re-resolve 42 items")) - Expect(out.String()).To(ContainSubstring("42 external lookups")) + Expect(out.String()).To(ContainSubstring("7 external lookups")) }) It("defaults to no on anything else", func() { - Expect(promptConfirm(strings.NewReader("\n"))(&out, 1)).To(BeFalse()) - Expect(promptConfirm(strings.NewReader("nope\n"))(&out, 1)).To(BeFalse()) - Expect(promptConfirm(strings.NewReader(""))(&out, 1)).To(BeFalse()) + Expect(promptConfirm(strings.NewReader("\n"))(&out, 1, 1)).To(BeFalse()) + Expect(promptConfirm(strings.NewReader("nope\n"))(&out, 1, 1)).To(BeFalse()) + Expect(promptConfirm(strings.NewReader(""))(&out, 1, 1)).To(BeFalse()) }) }) @@ -259,8 +279,8 @@ var _ = Describe("reprocessArtwork", func() { var out strings.Builder ctx := context.Background() kinds := []model.Kind{model.KindArtistArtwork, model.KindAlbumArtwork} - accept := func(io.Writer, int64) bool { return true } - decline := func(io.Writer, int64) bool { return false } + accept := func(io.Writer, int64, int64) bool { return true } + decline := func(io.Writer, int64, int64) bool { return false } put := func(kind model.Kind, id, source string) { Expect(art.PutItemArtwork(&model.ItemArtwork{ItemKind: kind.Prefix(), ItemID: id, @@ -335,7 +355,7 @@ var _ = Describe("reprocessArtwork", func() { It("stops at a selection that matches nothing instead of prompting", func() { Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, false, - func(io.Writer, int64) bool { + func(io.Writer, int64, int64) bool { Fail("must not prompt when there is nothing to queue") return true }, &out)).To(Succeed()) @@ -344,6 +364,24 @@ var _ = Describe("reprocessArtwork", func() { Expect(queue.Count()).To(BeZero()) }) + It("reports an empty selection as a dry run when one was asked for", func() { + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, true, accept, &out)).To(Succeed()) + + Expect(out.String()).To(ContainSubstring("Dry run")) + }) + + It("counts only the kinds that call an external agent as external cost", func() { + put(model.KindPlaylistArtwork, "pl-1", "playlist") + var total, external int64 + capture := func(_ io.Writer, t, e int64) bool { total, external = t, e; return false } + + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindAlbumArtwork, model.KindPlaylistArtwork}, + nil, false, capture, &out)).To(Succeed()) + + Expect(total).To(Equal(int64(3))) + Expect(external).To(Equal(int64(2)), "playlist artwork never reaches an external agent") + }) + It("rejects an unknown source and names the ones in use", func() { err := reprocessArtwork(ctx, ds, kinds, []string{"externa:deezer"}, true, accept, &out) @@ -354,6 +392,15 @@ var _ = Describe("reprocessArtwork", func() { Expect(err.Error()).To(ContainSubstring("absent"), "the empty source prints under its user-facing name") Expect(queue.Count()).To(BeZero()) }) + + It("accepts a source another kind uses, letting the empty selection report itself", func() { + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindArtistArtwork}, []string{"folder"}, + false, decline, &out)).To(Succeed()) + + Expect(out.String()).To(ContainSubstring("Nothing matches"), + "a well-formed filter must not be reported as a typo because of the kinds selected") + Expect(queue.Count()).To(BeZero()) + }) }) var _ = Describe("refreshItems", func() {