remote/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-10-10 19:37:08 +02:00
Code Issues Projects Releases Packages Wiki Activity

fix(cli): cover the reprocess selection rule and validate sources table-wide

Browse source 
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.
This commit is contained in:
Deluan 2026-08-14 13:55:10 -04:00
commit bd7b46c91c
2 changed files with 82 additions and 27 deletions
Download patch file Download diff file Expand all files Collapse all files

46
cmd/artwork.go
View file

@ -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
}

63
cmd/artwork_test.go
View file

@ -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() {