From 4154dbe2502cb314ee63d63109459e697fa262f1 Mon Sep 17 00:00:00 2001 From: David Newhall II Date: Sun, 13 Sep 2026 16:16:54 -0700 Subject: [PATCH] Do not watch the working directory when a folder overlay has no path, and name Starr skip errors by instance key. Co-authored-by: Cursor --- pkg/folders/folder_test.go | 50 +++++++++++++++++++++++++++++++ pkg/folders/path.go | 5 ++++ pkg/folders/validate.go | 8 +++++ pkg/unpackerr/cnfgfile.go | 14 ++++----- pkg/unpackerr/cnfgfile_test.go | 40 +++++++++++++++++++++++-- pkg/unpackerr/configput.go | 4 ++- pkg/unpackerr/configput_test.go | 52 +++++++++++++++++++++++++++++++++ pkg/unpackerr/folder.go | 10 ++++++- pkg/unpackerr/starrpoll.go | 18 +++++++----- pkg/unpackerr/start_test.go | 41 ++++++++++++++++++++++++++ 10 files changed, 223 insertions(+), 19 deletions(-) diff --git a/pkg/folders/folder_test.go b/pkg/folders/folder_test.go index 7d3da211..8b7f6bfe 100644 --- a/pkg/folders/folder_test.go +++ b/pkg/folders/folder_test.go @@ -1,8 +1,11 @@ package folders import ( + "errors" + "fmt" "os" "path/filepath" + "strings" "testing" "time" ) @@ -13,6 +16,53 @@ func (noopLogger) Printf(string, ...any) {} func (noopLogger) Errorf(string, ...any) {} func (noopLogger) Debugf(string, ...any) {} +type captureLogger struct { + errs []string +} + +func (c *captureLogger) Printf(string, ...any) {} +func (c *captureLogger) Debugf(string, ...any) {} +func (c *captureLogger) Errorf(msg string, v ...any) { + c.errs = append(c.errs, fmt.Sprintf(msg, v...)) +} + +func TestValidateListRequiresPath(t *testing.T) { + t.Parallel() + + err := ValidateList([]*FolderConfig{{DeleteOrig: true}}, func(string) (uint64, bool, error) { + return 0, false, nil + }) + if !errors.Is(err, ErrNoPath) { + t.Fatalf("empty path: %v", err) + } +} + +func TestCheckSkipsEmptyPath(t *testing.T) { + t.Parallel() + + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + + log := &captureLogger{} + + good, names := Check([]*FolderConfig{{Path: "", DeleteOrig: true}}, log) + if len(good) != 0 || len(names) != 0 { + t.Fatalf("watched empty path: %v %v", names, good) + } + + for _, name := range names { + if name == cwd { + t.Fatal("empty path must not watch the working directory") + } + } + + if len(log.errs) == 0 || !strings.Contains(log.errs[0], "empty path") { + t.Fatalf("log %v", log.errs) + } +} + func TestNormalizeExcludePaths(t *testing.T) { t.Parallel() diff --git a/pkg/folders/path.go b/pkg/folders/path.go index d3f2d758..5eff2367 100644 --- a/pkg/folders/path.go +++ b/pkg/folders/path.go @@ -26,6 +26,11 @@ func Check(list []*FolderConfig, log Logs) ([]*FolderConfig, []string) { ) for _, folder := range list { + if strings.TrimSpace(folder.Path) == "" { + log.Errorf("Folder (empty path): cannot watch") + continue + } + folder.Path, err = filepath.Abs(expandHomedir(folder.Path)) if err != nil { log.Errorf("Folder '%s' (bad path): %v", folder.Path, err) diff --git a/pkg/folders/validate.go b/pkg/folders/validate.go index fa6eeadc..942bdd90 100644 --- a/pkg/folders/validate.go +++ b/pkg/folders/validate.go @@ -3,6 +3,7 @@ package folders import ( "errors" "fmt" + "strings" "golift.io/cnfg" ) @@ -10,6 +11,9 @@ import ( // ErrNilConfig is returned when a folder list contains a nil entry. var ErrNilConfig = errors.New("nil config entry") +// ErrNoPath is returned when a folder instance has no watch path. +var ErrNoPath = errors.New("path is required") + // ValidateList applies folder defaults and parses max_bytes. func ValidateList(list []*FolderConfig, parseMax func(string) (uint64, bool, error)) error { for idx := range list { @@ -17,6 +21,10 @@ func ValidateList(list []*FolderConfig, parseMax func(string) (uint64, bool, err return ErrNilConfig } + if strings.TrimSpace(list[idx].Path) == "" { + return ErrNoPath + } + if list[idx].DeleteAfter == nil { list[idx].DeleteAfter = &cnfg.Duration{Duration: DefaultDeleteAfter} } diff --git a/pkg/unpackerr/cnfgfile.go b/pkg/unpackerr/cnfgfile.go index 3f3c805a..62d9e143 100644 --- a/pkg/unpackerr/cnfgfile.go +++ b/pkg/unpackerr/cnfgfile.go @@ -451,7 +451,7 @@ func expandHomedir(filePath string) string { return expanded } -func (u *Unpackerr) validateApp(conf *StarrConfig, app starr.App) error { +func (u *Unpackerr) validateApp(conf *StarrConfig, app starr.App, slug string) error { conf.Name = strings.TrimSpace(conf.Name) if err := checkStarrName(conf.Name); err != nil { @@ -460,7 +460,7 @@ func (u *Unpackerr) validateApp(conf *StarrConfig, app starr.App) error { label := conf.Label(app) - if err := u.requireStarrAccess(conf, label); err != nil { + if err := u.requireStarrAccess(conf, label, slug); err != nil { return err } @@ -502,14 +502,14 @@ func (u *Unpackerr) validateApp(conf *StarrConfig, app starr.App) error { return nil } -func (u *Unpackerr) requireStarrAccess(conf *StarrConfig, label string) error { +func (u *Unpackerr) requireStarrAccess(conf *StarrConfig, label, slug string) error { if conf.URL == "" { - u.Errorf("Missing %s URL in one of your configurations, skipped and ignored.", label) + u.Errorf("Missing %s URL in instance %q, skipped and ignored.", label, slug) return ErrInvalidURL // this error is not printed. } if conf.APIKey == "" { - u.Errorf("Missing %s API Key in one of your configurations, skipped and ignored.", label) + u.Errorf("Missing %s API Key in instance %q, skipped and ignored.", label, slug) return ErrInvalidKey // this error is not printed at startup; PUT returns it. } @@ -518,8 +518,8 @@ func (u *Unpackerr) requireStarrAccess(conf *StarrConfig, label string) error { } if len(conf.APIKey) < apiKeyMinLength { - u.Errorf("%s (%s) API Key is too short (%d < %d), skipped and ignored.", - label, conf.URL, len(conf.APIKey), apiKeyMinLength) + u.Errorf("%s instance %q (%s) API Key is too short (%d < %d), skipped and ignored.", + label, slug, conf.URL, len(conf.APIKey), apiKeyMinLength) return fmt.Errorf("%s (%s) %w, your key length: %d", label, conf.URL, ErrInvalidKey, len(conf.APIKey)) diff --git a/pkg/unpackerr/cnfgfile_test.go b/pkg/unpackerr/cnfgfile_test.go index c304e247..52be608c 100644 --- a/pkg/unpackerr/cnfgfile_test.go +++ b/pkg/unpackerr/cnfgfile_test.go @@ -1,6 +1,7 @@ package unpackerr import ( + "bytes" "encoding/json" "errors" "os" @@ -765,7 +766,7 @@ func TestValidateAppUsesInstanceLabel(t *testing.T) { conf.URL = "ftp://127.0.0.1:8989" conf.APIKey = key - err := unpack.validateApp(conf, starr.Sonarr) + err := unpack.validateApp(conf, starr.Sonarr, "uhd") if err == nil || !strings.Contains(err.Error(), "Sportarr") { t.Fatalf("invalid URL: %v", err) } @@ -774,7 +775,7 @@ func TestValidateAppUsesInstanceLabel(t *testing.T) { conf.URL = "http://127.0.0.1:8989" conf.APIKey = "short" - err = unpack.validateApp(conf, starr.Sonarr) + err = unpack.validateApp(conf, starr.Sonarr, "uhd") if err == nil || !strings.Contains(err.Error(), "Sportarr") { t.Fatalf("short key: %v", err) } @@ -783,12 +784,41 @@ func TestValidateAppUsesInstanceLabel(t *testing.T) { conf.URL = "http://127.0.0.1:8989" conf.APIKey = key - err = unpack.validateApp(conf, starr.Sonarr) + err = unpack.validateApp(conf, starr.Sonarr, "uhd") if err == nil || !strings.Contains(err.Error(), "Sportarr") { t.Fatalf("max bytes: %v", err) } } +func TestValidateAppSkipLogsInstanceSlug(t *testing.T) { + t.Parallel() + + var logs bytes.Buffer + + unpack := New() + unpack.Error.SetOutput(&logs) + + err := unpack.validateApp(&StarrConfig{}, starr.Readarr, "0") + if !errors.Is(err, ErrInvalidURL) { + t.Fatalf("empty URL: %v", err) + } + + if !strings.Contains(logs.String(), `instance "0"`) { + t.Fatalf("skip log missing slug: %q", logs.String()) + } + + logs.Reset() + + err = unpack.validateApp(&StarrConfig{URL: "http://127.0.0.1:8787"}, starr.Readarr, "books") + if !errors.Is(err, ErrInvalidKey) { + t.Fatalf("empty key: %v", err) + } + + if !strings.Contains(logs.String(), `instance "books"`) { + t.Fatalf("skip log missing slug: %q", logs.String()) + } +} + func TestValidateStarrListRejectsBadName(t *testing.T) { t.Parallel() @@ -804,6 +834,10 @@ func TestValidateStarrListRejectsBadName(t *testing.T) { t.Fatalf("got %v want ErrInvalidName", err) } + if !strings.Contains(err.Error(), `instance "0"`) { + t.Fatalf("fatal error missing slug: %v", err) + } + if len(unpack.Sonarr) != 1 { t.Fatalf("bad name must not skip the instance, got %d", len(unpack.Sonarr)) } diff --git a/pkg/unpackerr/configput.go b/pkg/unpackerr/configput.go index 5550285e..8e2e2c33 100644 --- a/pkg/unpackerr/configput.go +++ b/pkg/unpackerr/configput.go @@ -722,7 +722,7 @@ func putStarrList[T any, P starrApp[T]]( } server := asStarr[T, P](item) - if err := unpackerr.validateApp(server.conf(), app); err != nil { + if err := unpackerr.validateApp(server.conf(), app, key); err != nil { // Env overlay may recreate a slug the PUT omitted. Incomplete // leftovers (URL in env, key in the deleted file row) must not // 400 the save; startup already skips those. A PUT-body row @@ -809,6 +809,8 @@ func (u *Unpackerr) putFolders(raw json.RawMessage) (bool, error) { return false, err } + dropInvalidEnvOverlay(next.Folder, preview.Folders, validateFolderList) + if err := validateFolderList(preview.Folders); err != nil { return false, err } diff --git a/pkg/unpackerr/configput_test.go b/pkg/unpackerr/configput_test.go index 46f3ae8a..08574b90 100644 --- a/pkg/unpackerr/configput_test.go +++ b/pkg/unpackerr/configput_test.go @@ -499,6 +499,11 @@ func TestConfigPutFoldersValidationDoesNotApply(t *testing.T) { if len(unpack.fileConfig.Folders) != 0 { t.Fatalf("rejected folder staged: %+v", unpack.fileConfig.Folders) } + + emptyPath := `{"interval":"1s","buffer":1000,"folder":{"foo2":{"delete_original":true}}}` + if rec := doAuth(t, unpack, http.MethodPut, "/api/config/folders", emptyPath, key); rec.Code != http.StatusBadRequest { + t.Fatalf("empty folder path %d %s", rec.Code, rec.Body.String()) + } } func TestConfigPutWebhooksValidationDoesNotApply(t *testing.T) { @@ -1984,6 +1989,53 @@ func TestConfigPutFoldersKeepsSiblingExcludePaths(t *testing.T) { } } +// Env DELETE_ORIGINAL without a path must not 400 a save of a different slug. +func TestConfigPutFoldersOtherSlugWhenEnvHasNoPath(t *testing.T) { + t.Setenv("UN_FOLDER_foo2_DELETE_ORIGINAL", "true") + + unpack := testAuthUnpackerr(t) + unpack.ConfigFile = filepath.Join(t.TempDir(), "unpackerr.conf") + unpack.snapshotFileConfig() + + res, err := cnfg.ParseENV(unpack.Config, unpack.EnvPrefix) + if err != nil { + t.Fatal(err) + } + + unpack.envUsed = envSuffixes(res.Used, unpack.EnvPrefix) + + watch := t.TempDir() + + body, err := json.Marshal(map[string]any{ + "interval": "1s", + "buffer": 1000, + "folder": map[string]any{ + "tv": map[string]any{"path": watch}, + }, + }) + if err != nil { + t.Fatal(err) + } + + put := doAuth(t, unpack, http.MethodPut, "/api/config/folders", string(body), putKey(unpack)) + if put.Code != http.StatusOK { + t.Fatalf("put %d %s", put.Code, put.Body.String()) + } + + if unpack.fileConfig.Folders["foo2"] != nil { + t.Fatalf("env slug written to file: %+v", unpack.fileConfig.Folders) + } + + if unpack.Folders["foo2"] != nil { + t.Fatalf("incomplete env leftover on live: %+v", unpack.Folders["foo2"]) + } + + got := unpack.Folders["tv"] + if got == nil || got.Path != watch { + t.Fatalf("live tv %+v", got) + } +} + func envSonarrURLUnpackerr(t *testing.T, secret string) *Unpackerr { t.Helper() diff --git a/pkg/unpackerr/folder.go b/pkg/unpackerr/folder.go index 1f14105b..435a3349 100644 --- a/pkg/unpackerr/folder.go +++ b/pkg/unpackerr/folder.go @@ -20,10 +20,18 @@ func (u *Unpackerr) validateFolders() error { } func validateFolderList(list InstanceMap[FolderConfig]) error { - for key := range list { + for key, folder := range list { if err := validateInstanceSlug(key); err != nil { return err } + + if folder == nil { + return errNilConfigEntry + } + + if strings.TrimSpace(folder.Path) == "" { + return fmt.Errorf("folder %q: %w", key, folders.ErrNoPath) + } } if err := folders.ValidateList(instanceValues(list), parseOptionalMaxBytes); err != nil { diff --git a/pkg/unpackerr/starrpoll.go b/pkg/unpackerr/starrpoll.go index 78e648d1..7251a313 100644 --- a/pkg/unpackerr/starrpoll.go +++ b/pkg/unpackerr/starrpoll.go @@ -31,13 +31,13 @@ func validateStarrList[T any, P starrApp[T]](unpack *Unpackerr, list InstanceMap } server := asStarr[T, P](item) - if err := unpack.validateApp(server.conf(), app); err != nil { + if err := unpack.validateApp(server.conf(), app, key); err != nil { if skipInvalidApp(err) { delete(list, key) continue } - return err + return fmt.Errorf("%s instance %q: %w", app, key, err) } server.connect() @@ -49,7 +49,11 @@ func validateStarrList[T any, P starrApp[T]](unpack *Unpackerr, list InstanceMap func warnDuplicateStarrNames[T any, P starrApp[T]]( unpack *Unpackerr, seen map[string]string, app starr.App, list InstanceMap[T], ) { - for _, item := range instanceValues(list) { + for slug, item := range list { + if item == nil { + continue + } + server := asStarr[T, P](item) cfg := server.conf() name := strings.TrimSpace(cfg.Name) @@ -58,17 +62,17 @@ func warnDuplicateStarrNames[T any, P starrApp[T]]( continue } - key := strings.ToLower(name) - loc := fmt.Sprintf("%s (%s)", app, cfg.URL) + dupKey := strings.ToLower(name) + loc := fmt.Sprintf("%s instance %q (%s)", app, slug, cfg.URL) - if prev, ok := seen[key]; ok { + if prev, ok := seen[dupKey]; ok { unpack.Errorf("Config Warning: duplicate Starr instance name %q on %s and %s; hook exclude cannot tell them apart", name, prev, loc) continue } - seen[key] = loc + seen[dupKey] = loc } } diff --git a/pkg/unpackerr/start_test.go b/pkg/unpackerr/start_test.go index c6441ba2..6d0d9ebd 100644 --- a/pkg/unpackerr/start_test.go +++ b/pkg/unpackerr/start_test.go @@ -1,11 +1,14 @@ package unpackerr import ( + "errors" "os" "path/filepath" "testing" "time" + "github.com/Unpackerr/unpackerr/pkg/folders" + "golift.io/cnfg" "golift.io/starr" "golift.io/xtractr" ) @@ -261,6 +264,44 @@ func TestValidateFoldersExtrasDefaults(t *testing.T) { } } +func TestValidateFoldersRequiresPath(t *testing.T) { + t.Parallel() + + unpack := New() + unpack.Folders = InstanceMap[FolderConfig]{ + "foo2": {DeleteOrig: true}, + } + + err := unpack.validateFolders() + if !errors.Is(err, folders.ErrNoPath) { + t.Fatalf("empty path: %v", err) + } + + if err.Error() != `folder "foo2": path is required` { + t.Fatalf("error %q", err) + } +} + +func TestValidateFoldersEnvWithoutPath(t *testing.T) { + t.Setenv("UN_FOLDER_foo2_DELETE_ORIGINAL", "true") + + unpack := New() + + if _, err := cnfg.ParseENV(unpack.Config, unpack.EnvPrefix); err != nil { + t.Fatal(err) + } + + got := unpack.Folders["foo2"] + if got == nil || !got.DeleteOrig || got.Path != "" { + t.Fatalf("env overlay %+v", got) + } + + err := unpack.validateFolders() + if !errors.Is(err, folders.ErrNoPath) { + t.Fatalf("env folder without path: %v", err) + } +} + func TestMaxRetriesZeroUsesDefault(t *testing.T) { t.Parallel()