diff --git a/CODESMELL.md b/CODESMELL.md index 63f3250..0904df9 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -167,19 +167,22 @@ --- -### CSM-009 — Thumbnail Generation: Missing Error Handling -- **File:** `lib/bds/media/thumbnails.ex:107-165` -- **What:** - - `Image.thumbnail!` (Z. 149,154) and `Image.write!` (Z. 159,164) are bang variants that crash on failure. - - `File.mkdir_p/1` (Z. 133) result is discarded — if directory creation fails, the write will crash with a confusing error. - - `Image.embed!` (Z. 150) and `Image.flatten!` (Z. 158) are also bang variants. -- **Note on scheduler blocking:** The `Image` library uses libvips NIFs which run image processing in a C thread pool, not on the BEAM scheduler. The original concern about scheduler blocking is **not substantiated** for this library. The real issues are the bang variants and unchecked `File.mkdir_p`. -- **Fix:** - - Replace `File.mkdir_p/1` with `case File.mkdir_p(dir) do :ok -> ...; {:error, reason} -> {:error, reason}`. - - Replace bang `Image.thumbnail!` with `Image.thumbnail` and handle `{:error, _}`. - - Replace bang `Image.write!` with `Image.write` and handle `{:error, _}`. - - Wrap the whole `write_all_thumbnails` in a try/return-error-tuple pattern. -- **Test:** Feed a corrupt image; assert `ensure_thumbnails` returns `{:error, _}` instead of crashing. +### ~~CSM-009 — Thumbnail Generation: Missing Error Handling~~ ✅ FIXED +- **Fixed:** 2026-05-09 +- **What was done:** + - Replaced all bang variants with non-bang error-tuple handling: + - `Image.autorotate!` → `Image.autorotate` with `{:ok, {image, rotation_info}}` destructuring. + - `Image.thumbnail!` → `Image.thumbnail` returning `{:ok, image}` / `{:error, reason}`. + - `Image.embed!` → `Image.embed` with `with` chain. + - `Image.flatten!` → `Image.flatten` with `with` chain. + - `Image.write!` → `Image.write` with `{:ok, _}` / `{:error, reason}` handling. + - `File.mkdir_p` result is now checked — errors halt thumbnail generation with `{:error, reason}`. + - `write_all_thumbnails` uses `Enum.reduce_while` to stop on first error and return `{:error, reason}`. + - `ensure_thumbnails` spec updated to `:ok | {:error, term()}`. + - `regenerate_thumbnails` propagates `{:error, reason}` from `ensure_thumbnails`. + - `regenerate_missing_thumbnails` replaced `try/rescue` with `case` on the new error tuples. + - Call sites in `BDS.Media` (`import_media`, `replace_media_binary`) use `log_thumbnail_error/2` — media operations succeed even if thumbnails fail, with a warning logged. + - Added 6 tests in `test/bds/csm009_thumbnail_error_handling_test.exs`: corrupt image returns `{:error, _}`, non-image returns `:ok`, missing source returns `{:error, _}`, regenerate corrupt returns `{:error, _}`, regenerate_missing counts failures, import succeeds despite thumbnail failure. --- @@ -431,6 +434,7 @@ - CSM-006: Fixed. Batch INSERT for reindexing, preloaded post records for rendering. - CSM-007: Fixed. Decomposed into refresh_layout, refresh_sidebar, refresh_content, reload_shell. - CSM-008: Fixed. Panel data pre-computed in event handlers, tab meta skips DB for complete entries. + - CSM-009: Fixed. All bang Image/File variants replaced with error-tuple handling, `ensure_thumbnails` returns `{:error, _}` instead of crashing. - [x] CSM-001 fix covers ALL 6 affected files, not just `import_definitions.ex`. - [x] CSM-003 fix covers ALL `Repo.delete!` call sites (posts, tags, scripts, media, projects, templates, translations). - [x] CSM-007 decomposition is the prerequisite for fixing CSM-008 (render-path queries). diff --git a/lib/bds/media.ex b/lib/bds/media.ex index 3b6b6ca..90585f3 100644 --- a/lib/bds/media.ex +++ b/lib/bds/media.ex @@ -24,6 +24,8 @@ defmodule BDS.Media do ensure_thumbnails: 2 ] + require Logger + import Ecto.Query alias BDS.Media.Media @@ -105,7 +107,7 @@ defmodule BDS.Media do end) do {:ok, media} -> :ok = write_sidecar(project, media) - :ok = ensure_thumbnails(project, media) + log_thumbnail_error(ensure_thumbnails(project, media), media.id) :ok = Search.sync_media(media) {:ok, media} @@ -321,7 +323,7 @@ defmodule BDS.Media do {:ok, updated_media} -> _ = File.rm(previous_destination_backup) :ok = write_sidecar(project, updated_media) - :ok = ensure_thumbnails(project, updated_media) + log_thumbnail_error(ensure_thumbnails(project, updated_media), updated_media.id) :ok = Search.sync_media(updated_media) {:ok, updated_media} @@ -342,4 +344,10 @@ defmodule BDS.Media do order_by: [asc: translation.language] ) end + + defp log_thumbnail_error(:ok, _media_id), do: :ok + + defp log_thumbnail_error({:error, reason}, media_id) do + Logger.warning("Thumbnail generation failed for media #{media_id}: #{inspect(reason)}") + end end diff --git a/lib/bds/media/thumbnails.ex b/lib/bds/media/thumbnails.ex index 924bb1a..56df118 100644 --- a/lib/bds/media/thumbnails.ex +++ b/lib/bds/media/thumbnails.ex @@ -38,8 +38,11 @@ defmodule BDS.Media.Thumbnails do media -> project = Projects.get_project!(media.project_id) - :ok = ensure_thumbnails(project, media) - {:ok, media} + + case ensure_thumbnails(project, media) do + :ok -> {:ok, media} + {:error, reason} -> {:error, reason} + end end end @@ -79,16 +82,15 @@ defmodule BDS.Media.Thumbnails do if missing_paths == [] do %{acc | processed: acc.processed + 1} else - try do - :ok = ensure_thumbnails(project, media) + case ensure_thumbnails(project, media) do + :ok -> + %{ + processed: acc.processed + 1, + generated: acc.generated + length(missing_paths), + failed: acc.failed + } - %{ - processed: acc.processed + 1, - generated: acc.generated + length(missing_paths), - failed: acc.failed - } - rescue - _error -> + {:error, _reason} -> %{acc | processed: acc.processed + 1, failed: acc.failed + 1} end end @@ -98,23 +100,19 @@ defmodule BDS.Media.Thumbnails do end) end - @spec ensure_thumbnails(BDS.Projects.Project.t(), Media.t()) :: :ok + @spec ensure_thumbnails(BDS.Projects.Project.t(), Media.t()) :: :ok | {:error, term()} def ensure_thumbnails(project, media) do if image_mime?(media.mime_type) do source_path = Path.join(Projects.project_data_dir(project), media.file_path) - case Image.open(source_path) do - {:ok, image} -> - image - |> Image.autorotate!() - |> write_all_thumbnails(project, media) - - {:error, _reason} -> - :ok + with {:ok, image} <- Image.open(source_path), + {:ok, {rotated, _rotation_info}} <- Image.autorotate(image), + :ok <- write_all_thumbnails(rotated, project, media) do + :ok end + else + :ok end - - :ok end @spec delete_thumbnail_files(String.t(), Media.t()) :: :ok @@ -128,16 +126,17 @@ defmodule BDS.Media.Thumbnails do defp write_all_thumbnails(image, project, media) do thumbnail_paths(media) - |> Enum.each(fn {size, relative_path} -> + |> Enum.reduce_while(:ok, fn {size, relative_path}, :ok -> destination = Path.join(Projects.project_data_dir(project), relative_path) - :ok = File.mkdir_p(Path.dirname(destination)) - image - |> render_thumbnail(size) - |> write_thumbnail(destination, size) + with :ok <- File.mkdir_p(Path.dirname(destination)), + {:ok, rendered} <- render_thumbnail(image, size), + :ok <- write_thumbnail(rendered, destination, size) do + {:cont, :ok} + else + {:error, reason} -> {:halt, {:error, reason}} + end end) - - :ok end defp render_thumbnail(image, :small), do: bounded_thumbnail(image, 150, 150) @@ -145,23 +144,41 @@ defmodule BDS.Media.Thumbnails do defp render_thumbnail(image, :large), do: bounded_thumbnail(image, 800, 800) defp render_thumbnail(image, :ai) do - image - |> Image.thumbnail!("448x448", fit: :contain, resize: :both, autorotate: false) - |> Image.embed!(448, 448, x: :center, y: :center, background_color: :black) + with {:ok, thumbnail} <- + Image.thumbnail(image, "448x448", + fit: :contain, + resize: :both, + autorotate: false + ), + {:ok, embedded} <- + Image.embed(thumbnail, 448, 448, + x: :center, + y: :center, + background_color: :black + ) do + {:ok, embedded} + end end defp bounded_thumbnail(image, width, height) do - Image.thumbnail!(image, "#{width}x#{height}", fit: :contain, resize: :down, autorotate: false) + Image.thumbnail(image, "#{width}x#{height}", + fit: :contain, + resize: :down, + autorotate: false + ) end defp write_thumbnail(image, destination, :ai) do - flattened = Image.flatten!(image, background_color: :black) - Image.write!(flattened, destination, quality: 85, strip_metadata: true) - :ok + with {:ok, flattened} <- Image.flatten(image, background_color: :black), + {:ok, _} <- Image.write(flattened, destination, quality: 85, strip_metadata: true) do + :ok + end end defp write_thumbnail(image, destination, _size) do - Image.write!(image, destination, quality: 80, strip_metadata: true) - :ok + case Image.write(image, destination, quality: 80, strip_metadata: true) do + {:ok, _} -> :ok + {:error, reason} -> {:error, reason} + end end end diff --git a/test/bds/csm009_thumbnail_error_handling_test.exs b/test/bds/csm009_thumbnail_error_handling_test.exs new file mode 100644 index 0000000..90be89e --- /dev/null +++ b/test/bds/csm009_thumbnail_error_handling_test.exs @@ -0,0 +1,128 @@ +defmodule BDS.CSM009ThumbnailErrorHandlingTest do + use ExUnit.Case, async: false + + alias BDS.Media.Thumbnails + alias BDS.Repo + + setup do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(BDS.Repo) + temp_dir = Path.join(System.tmp_dir!(), "bds-csm009-#{System.unique_integer([:positive])}") + File.mkdir_p!(temp_dir) + on_exit(fn -> File.rm_rf(temp_dir) end) + + {:ok, project} = BDS.Projects.create_project(%{name: "CSM009", data_path: temp_dir}) + %{project: project, temp_dir: temp_dir} + end + + test "ensure_thumbnails returns {:error, _} for a corrupt image instead of crashing", %{ + project: project, + temp_dir: temp_dir + } do + source_path = Path.join(temp_dir, "corrupt.jpg") + File.write!(source_path, "not a real image") + + {:ok, media} = + BDS.Media.import_media(%{project_id: project.id, source_path: source_path}) + + assert media.mime_type == "image/jpeg" + + Enum.each(Map.values(Thumbnails.thumbnail_paths(media)), fn path -> + File.rm(Path.join(temp_dir, path)) + end) + + result = Thumbnails.ensure_thumbnails(project, media) + assert {:error, _reason} = result + end + + test "ensure_thumbnails returns :ok for non-image media", %{project: project, temp_dir: temp_dir} do + source_path = Path.join(temp_dir, "readme.txt") + File.write!(source_path, "just text") + + {:ok, media} = + BDS.Media.import_media(%{project_id: project.id, source_path: source_path}) + + assert Thumbnails.ensure_thumbnails(project, media) == :ok + end + + test "ensure_thumbnails returns {:error, _} when source file is missing", %{ + project: project, + temp_dir: temp_dir + } do + source_path = Path.join(temp_dir, "sample.jpg") + File.write!(source_path, tiny_jpeg_binary()) + + {:ok, media} = + BDS.Media.import_media(%{project_id: project.id, source_path: source_path}) + + File.rm!(Path.join(temp_dir, media.file_path)) + + result = Thumbnails.ensure_thumbnails(project, media) + assert {:error, _reason} = result + end + + test "regenerate_thumbnails returns {:error, _} for corrupt source", %{ + project: project, + temp_dir: temp_dir + } do + source_path = Path.join(temp_dir, "corrupt2.jpg") + File.write!(source_path, "not valid jpeg data") + + {:ok, media} = + BDS.Media.import_media(%{project_id: project.id, source_path: source_path}) + + Enum.each(Map.values(Thumbnails.thumbnail_paths(media)), fn path -> + File.rm(Path.join(temp_dir, path)) + end) + + File.write!(Path.join(temp_dir, media.file_path), "not valid jpeg data") + + result = Thumbnails.regenerate_thumbnails(media.id) + assert {:error, _reason} = result + end + + test "regenerate_missing_thumbnails counts failures without crashing", %{ + project: project, + temp_dir: temp_dir + } do + source_path = Path.join(temp_dir, "good.jpg") + File.write!(source_path, tiny_jpeg_binary()) + {:ok, good_media} = BDS.Media.import_media(%{project_id: project.id, source_path: source_path}) + + corrupt_path = Path.join(temp_dir, "bad.jpg") + File.write!(corrupt_path, "corrupt data") + {:ok, bad_media} = BDS.Media.import_media(%{project_id: project.id, source_path: corrupt_path}) + + Enum.each(Map.values(Thumbnails.thumbnail_paths(good_media)), fn path -> + File.rm(Path.join(temp_dir, path)) + end) + + Enum.each(Map.values(Thumbnails.thumbnail_paths(bad_media)), fn path -> + File.rm(Path.join(temp_dir, path)) + end) + + File.write!(Path.join(temp_dir, bad_media.file_path), "corrupt data") + + result = Thumbnails.regenerate_missing_thumbnails(project.id) + assert result.failed >= 1 + assert result.processed >= 2 + end + + test "import_media succeeds even when thumbnail generation fails for corrupt image", %{ + project: project, + temp_dir: temp_dir + } do + source_path = Path.join(temp_dir, "bad_import.jpg") + File.write!(source_path, "not a real image at all") + + assert {:ok, media} = + BDS.Media.import_media(%{project_id: project.id, source_path: source_path}) + + assert media.mime_type == "image/jpeg" + assert Repo.get(BDS.Media.Media, media.id) != nil + end + + defp tiny_jpeg_binary do + Image.new!(3, 2, color: [255, 0, 0]) + |> Image.write!(:memory, suffix: ".jpg", quality: 85) + end +end