From bd7066e0c309549aef6bca331e6134bfef32e7b2 Mon Sep 17 00:00:00 2001 From: Michael Mayer Date: Wed, 6 May 2026 07:50:27 +0000 Subject: [PATCH] Commands: Count file download failures & pluralize logs --- internal/commands/download.go | 8 +++- internal/commands/download_e2e_test.go | 57 ++++++++++++++++++++++++- internal/commands/download_impl.go | 8 +++- internal/commands/download_impl_test.go | 39 +++++++++++++++++ internal/commands/messages.go | 17 ++++++++ internal/commands/messages_test.go | 46 ++++++++++++++++++++ internal/commands/video_remux.go | 9 +++- internal/commands/video_transcode.go | 9 +++- internal/commands/video_trim.go | 9 +++- internal/photoprism/faces_optimize.go | 11 ++++- 10 files changed, 201 insertions(+), 12 deletions(-) create mode 100644 internal/commands/messages.go create mode 100644 internal/commands/messages_test.go diff --git a/internal/commands/download.go b/internal/commands/download.go index 00f31e456..96d62839d 100644 --- a/internal/commands/download.go +++ b/internal/commands/download.go @@ -340,6 +340,10 @@ func downloadAction(ctx *cli.Context) error { if dlErr != nil { log.Errorf("download failed: %v", dlErr) // even on error, any completed files returned will be imported + if len(files) == 0 { + failures++ + continue + } } // Ensure container/metadata per remux policy for file method @@ -370,13 +374,13 @@ func downloadAction(ctx *cli.Context) error { elapsed := time.Since(start) if failures > 0 { - log.Warnf("completed with %d error(s) in %s", failures, elapsed) + log.Warnf("completed with %s in %s", formatCount(failures, "error", "errors"), elapsed) } else { log.Infof("completed in %s", elapsed) } if failures > 0 { - return fmt.Errorf("some downloads failed: %d", failures) + return fmt.Errorf("%s", formatFailedCount(failures, "download", "downloads")) } return nil diff --git a/internal/commands/download_e2e_test.go b/internal/commands/download_e2e_test.go index 2bb6f9ef6..2d46a7d4b 100644 --- a/internal/commands/download_e2e_test.go +++ b/internal/commands/download_e2e_test.go @@ -3,6 +3,7 @@ package commands import ( + "bytes" "os" "path/filepath" "runtime" @@ -58,7 +59,7 @@ func createFakeYtDlp(t *testing.T) string { b.WriteString("for a in \"$@\"; do if [[ \"$a\" == \"--version\" ]]; then echo '2025.09.23'; exit 0; fi; done\n") b.WriteString("OUT_TPL=\"\"\n") b.WriteString("i=0; while [[ $i -lt $# ]]; do i=$((i+1)); arg=\"${!i}\"; if [[ \"$arg\" == \"--dump-single-json\" ]]; then echo '{\"id\":\"abc\",\"title\":\"Test\",\"url\":\"http://example.com\",\"_type\":\"video\"}'; exit 0; fi; if [[ \"$arg\" == \"--output\" ]]; then i=$((i+1)); OUT_TPL=\"${!i}\"; fi; done\n") - b.WriteString("if [[ $* == *'--print '* ]]; then OUT=\"$OUT_TPL\"; OUT=${OUT//%(id)s/abc}; OUT=${OUT//%(ext)s/mp4}; mkdir -p \"$(dirname \"$OUT\")\"; CONTENT=\"${YTDLP_DUMMY_CONTENT:-dummy}\"; echo \"$CONTENT\" > \"$OUT\"; echo \"$OUT\"; exit 0; fi\n") + b.WriteString("if [[ $* == *'--print '* ]]; then if [[ \"${YTDLP_FAIL_FILE_DOWNLOAD:-}\" == \"1\" ]]; then echo 'simulated yt-dlp failure' 1>&2; exit 1; fi; OUT=\"$OUT_TPL\"; OUT=${OUT//%(id)s/abc}; OUT=${OUT//%(ext)s/mp4}; mkdir -p \"$(dirname \"$OUT\")\"; CONTENT=\"${YTDLP_DUMMY_CONTENT:-dummy}\"; echo \"$CONTENT\" > \"$OUT\"; echo \"$OUT\"; exit 0; fi\n") if err := os.WriteFile(path, []byte(b.String()), 0o755); err != nil { t.Fatalf("failed to write fake yt-dlp: %v", err) } @@ -217,9 +218,63 @@ func TestDownloadImpl_FileMethod_Always_RemuxFails(t *testing.T) { if err == nil { t.Fatalf("expected failure when remux is required but ffmpeg is unavailable") } + if err.Error() != "1 download failed" { + t.Fatalf("unexpected error: %v", err) + } // Cleanup destination folder if anything was created c := get.Config() outDir := filepath.Join(c.OriginalsPath(), dest) _ = os.RemoveAll(outDir) } + +func TestDownloadImpl_FileMethod_ErrorWithoutFilesCountsFailure(t *testing.T) { + t.Setenv("YTDLP_FORCE_SHELL", "1") + t.Setenv("YTDLP_FAIL_FILE_DOWNLOAD", "1") + dl.ResetVersionWarningForTest() + + fake := createFakeYtDlp(t) + orig := dl.YtDlpBin + defer func() { dl.YtDlpBin = orig }() + + dest := "dl-e2e-file-error" + if c := get.Config(); c != nil { + c.Options().FFmpegBin = "/bin/false" + s := c.Settings() + s.Index.Convert = false + } + conf := get.Config() + if conf == nil { + t.Fatalf("missing test config") + } + + conf.RegisterDb() + dl.YtDlpBin = fake + + var logOutput bytes.Buffer + log.SetOutput(&logOutput) + t.Cleanup(func() { + log.SetOutput(os.Stderr) + }) + + var err error + err = runDownload(conf, DownloadOpts{ + Dest: dest, + Method: "file", + FileRemux: "skip", + }, []string{"https://example.com/video"}) + + if err == nil { + t.Fatalf("expected file method failure when yt-dlp returns no files") + } + if err.Error() != "1 download failed" { + t.Fatalf("unexpected error: %v", err) + } + if !strings.Contains(logOutput.String(), "completed with 1 error in") { + t.Fatalf("expected singularized summary log, got: %q", logOutput.String()) + } + + c := get.Config() + outDir := filepath.Join(c.OriginalsPath(), dest) + _ = os.RemoveAll(outDir) +} diff --git a/internal/commands/download_impl.go b/internal/commands/download_impl.go index e2cf95169..853656829 100644 --- a/internal/commands/download_impl.go +++ b/internal/commands/download_impl.go @@ -236,6 +236,10 @@ func runDownload(conf *config.Config, opts DownloadOpts, inputURLs []string) err }) if err != nil { log.Errorf("download failed: %v", err) + if len(files) == 0 { + failures++ + continue + } } if fileRemux != "skip" { for _, fp := range files { @@ -261,8 +265,8 @@ func runDownload(conf *config.Config, opts DownloadOpts, inputURLs []string) err elapsed := time.Since(start) if failures > 0 { - log.Warnf("completed with %d error(s) in %s", failures, elapsed) - return fmt.Errorf("some downloads failed: %d", failures) + log.Warnf("completed with %s in %s", formatCount(failures, "error", "errors"), elapsed) + return fmt.Errorf("%s", formatFailedCount(failures, "download", "downloads")) } log.Infof("completed in %s", elapsed) return nil diff --git a/internal/commands/download_impl_test.go b/internal/commands/download_impl_test.go index 727058654..3c931487f 100644 --- a/internal/commands/download_impl_test.go +++ b/internal/commands/download_impl_test.go @@ -1,9 +1,13 @@ package commands import ( + "bytes" + "os" + "strings" "testing" "github.com/photoprism/photoprism/internal/photoprism/dl" + "github.com/photoprism/photoprism/internal/photoprism/get" ) func TestMissingFormatsHint(t *testing.T) { @@ -22,6 +26,7 @@ func TestMissingFormatsHint(t *testing.T) { func TestResolveDownloadMethodEnv(t *testing.T) { t.Setenv(downloadMethodEnv, "FILE") + method, fromEnv, err := resolveDownloadMethod("") if err != nil { t.Fatalf("unexpected error: %v", err) @@ -36,6 +41,7 @@ func TestResolveDownloadMethodEnv(t *testing.T) { func TestResolveDownloadMethodInvalidEnv(t *testing.T) { t.Setenv(downloadMethodEnv, "weird") + if _, _, err := resolveDownloadMethod(""); err == nil { t.Fatalf("expected error for invalid env method") } @@ -43,6 +49,7 @@ func TestResolveDownloadMethodInvalidEnv(t *testing.T) { func TestResolveDownloadMethodFlagTakesPriority(t *testing.T) { t.Setenv(downloadMethodEnv, "file") + method, fromEnv, err := resolveDownloadMethod("pipe") if err != nil { t.Fatalf("unexpected error: %v", err) @@ -54,3 +61,35 @@ func TestResolveDownloadMethodFlagTakesPriority(t *testing.T) { t.Fatalf("did not expect env to be used when flag provided") } } + +func TestRunDownload_InvalidURLsReportPluralFailures(t *testing.T) { + conf := get.Config() + if conf == nil { + t.Fatalf("missing test config") + } + + conf.RegisterDb() + + var logOutput bytes.Buffer + log.SetOutput(&logOutput) + t.Cleanup(func() { + log.SetOutput(os.Stderr) + }) + + err := runDownload(conf, DownloadOpts{ + Dest: "dl-invalid", + }, []string{ + "not a url", + "ftp://example.com/video.mp4", + }) + + if err == nil { + t.Fatalf("expected invalid URLs to fail") + } + if err.Error() != "2 downloads failed" { + t.Fatalf("unexpected error: %v", err) + } + if !strings.Contains(logOutput.String(), "completed with 2 errors in") { + t.Fatalf("expected pluralized summary log, got: %q", logOutput.String()) + } +} diff --git a/internal/commands/messages.go b/internal/commands/messages.go new file mode 100644 index 000000000..d8ec82881 --- /dev/null +++ b/internal/commands/messages.go @@ -0,0 +1,17 @@ +package commands + +import ( + "fmt" + + "github.com/dustin/go-humanize/english" +) + +// formatCount returns a human-readable count phrase with the correct noun form. +func formatCount(count int, singular, plural string) string { + return english.Plural(count, singular, plural) +} + +// formatFailedCount returns a human-readable failure phrase with the correct noun form. +func formatFailedCount(count int, singular, plural string) string { + return fmt.Sprintf("%s failed", formatCount(count, singular, plural)) +} diff --git a/internal/commands/messages_test.go b/internal/commands/messages_test.go new file mode 100644 index 000000000..746714af6 --- /dev/null +++ b/internal/commands/messages_test.go @@ -0,0 +1,46 @@ +package commands + +import "testing" + +func TestFormatCount(t *testing.T) { + tests := []struct { + name string + count int + singular string + plural string + want string + }{ + {name: "Zero", count: 0, singular: "error", plural: "errors", want: "0 errors"}, + {name: "One", count: 1, singular: "download", plural: "downloads", want: "1 download"}, + {name: "Many", count: 2, singular: "file", plural: "files", want: "2 files"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := formatCount(tt.count, tt.singular, tt.plural); got != tt.want { + t.Fatalf("formatCount() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestFormatFailedCount(t *testing.T) { + tests := []struct { + name string + count int + singular string + plural string + want string + }{ + {name: "One", count: 1, singular: "download", plural: "downloads", want: "1 download failed"}, + {name: "Many", count: 2, singular: "file", plural: "files", want: "2 files failed"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := formatFailedCount(tt.count, tt.singular, tt.plural); got != tt.want { + t.Fatalf("formatFailedCount() = %q, want %q", got, tt.want) + } + }) + } +} diff --git a/internal/commands/video_remux.go b/internal/commands/video_remux.go index e4873c670..ac66b3df0 100644 --- a/internal/commands/video_remux.go +++ b/internal/commands/video_remux.go @@ -95,10 +95,15 @@ func videoRemuxAction(ctx *cli.Context) error { processed++ } - log.Infof("remux: processed %d, skipped %d, failed %d", processed, skipped, failed) + log.Infof( + "remux: processed %s, skipped %s, failed %s", + formatCount(processed, "file", "files"), + formatCount(skipped, "file", "files"), + formatCount(failed, "file", "files"), + ) if failed > 0 { - return fmt.Errorf("remux: %d files failed", failed) + return fmt.Errorf("remux: %s", formatFailedCount(failed, "file", "files")) } return nil diff --git a/internal/commands/video_transcode.go b/internal/commands/video_transcode.go index 328b2828a..73011a483 100644 --- a/internal/commands/video_transcode.go +++ b/internal/commands/video_transcode.go @@ -103,10 +103,15 @@ func videoTranscodeAction(ctx *cli.Context) error { processed++ } - log.Infof("transcode: processed %d, skipped %d, failed %d", processed, skipped, failed) + log.Infof( + "transcode: processed %s, skipped %s, failed %s", + formatCount(processed, "file", "files"), + formatCount(skipped, "file", "files"), + formatCount(failed, "file", "files"), + ) if failed > 0 { - return fmt.Errorf("transcode: %d files failed", failed) + return fmt.Errorf("transcode: %s", formatFailedCount(failed, "file", "files")) } return nil diff --git a/internal/commands/video_trim.go b/internal/commands/video_trim.go index 191668eda..6982a2cf8 100644 --- a/internal/commands/video_trim.go +++ b/internal/commands/video_trim.go @@ -104,10 +104,15 @@ func videoTrimAction(ctx *cli.Context) error { processed++ } - log.Infof("trim: processed %d, skipped %d, failed %d", processed, skipped, failed) + log.Infof( + "trim: processed %s, skipped %s, failed %s", + formatCount(processed, "file", "files"), + formatCount(skipped, "file", "files"), + formatCount(failed, "file", "files"), + ) if failed > 0 { - return fmt.Errorf("trim: %d files failed", failed) + return fmt.Errorf("trim: %s", formatFailedCount(failed, "file", "files")) } return nil diff --git a/internal/photoprism/faces_optimize.go b/internal/photoprism/faces_optimize.go index 36a0da679..57066a34e 100644 --- a/internal/photoprism/faces_optimize.go +++ b/internal/photoprism/faces_optimize.go @@ -5,6 +5,8 @@ import ( "fmt" "strings" + "github.com/dustin/go-humanize/english" + "github.com/photoprism/photoprism/internal/ai/face" "github.com/photoprism/photoprism/internal/entity" "github.com/photoprism/photoprism/internal/entity/query" @@ -63,7 +65,14 @@ func (w *Faces) OptimizeFor(subjUID string) (result FacesOptimizeResult, err err "subject %s, iteration %d, cluster %d, count %d, ids %s", }, subject, i, j, len(merge), clusterIDs) - log.Debugf("faces: retained manual clusters after merge: kept %d candidate cluster(s) [%s] for subject %s (merge) itr %d cluster %d", len(merge), clusterIDs, subject, i, j) + log.Debugf( + "faces: retained manual clusters after merge: kept %s [%s] for subject %s (merge) itr %d cluster %d", + english.Plural(len(merge), "candidate cluster", "candidate clusters"), + clusterIDs, + subject, + i, + j, + ) } else { log.Errorf("%s (merge) itr %d cluster %d count %d", mergeErr, i, j, len(merge)) }