From b052d593762a92be894ad28c9ba77cc369fa3143 Mon Sep 17 00:00:00 2001 From: Chili Palmer Date: Mon, 11 May 2026 20:25:06 +0200 Subject: [PATCH] fix(fs): handle File.mkdir_p errors and remove bang variants in sidecars and release packaging (CSM-030) --- CODESMELL.md | 13 +++++--- lib/bds/media.ex | 16 ++++++--- lib/bds/media/linking.ex | 12 +++++-- lib/bds/media/sidecars.ex | 18 +++++----- lib/bds/release_packaging.ex | 16 ++++----- test/bds/csm030_unchecked_mkdir_test.exs | 42 ++++++++++++++++++++++++ 6 files changed, 88 insertions(+), 29 deletions(-) create mode 100644 test/bds/csm030_unchecked_mkdir_test.exs diff --git a/CODESMELL.md b/CODESMELL.md index c3bb086..f0e090d 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -445,10 +445,15 @@ --- -### CSM-030 — Unchecked `File.mkdir_p` / `File.mkdir_p!` -- **Files:** `lib/bds/media/thumbnails.ex:133`, `lib/bds/media/sidecars.ex:24,56`, `lib/bds/release_packaging.ex:80,85` -- **What:** Result of `File.mkdir_p/1` is discarded. `File.mkdir_p!/1` in `release_packaging` can crash on permission errors. -- **Fix:** Pattern-match `File.mkdir_p/1` or use `with`; replace bang variants with non-bang and handle errors. +### ~~CSM-030 — Unchecked `File.mkdir_p` / `File.mkdir_p!`~~ ✅ FIXED +- **Fixed:** 2026-05-11 +- **What was done:** + - **`lib/bds/media/thumbnails.ex`** — Already fixed in CSM-009; `File.mkdir_p` is inside a `with` chain in `write_all_thumbnails`. + - **`lib/bds/media/sidecars.ex`** — Removed redundant `File.mkdir_p` calls from `write_sidecar/2` and `write_translation_sidecar/3` (the underlying `Persistence.atomic_write` already handles `mkdir_p`). Updated specs to return `:ok | {:error, File.posix()}`. Updated callers (`sync_media_sidecar`, `sync_media_translation_sidecar`) to propagate errors. + - **`lib/bds/media.ex`** — Replaced all `:ok = write_sidecar(...)` and `:ok = write_translation_sidecar(...)` match assertions with `log_sidecar_error/2` (mirrors existing `log_thumbnail_error/2` pattern). Sidecar write failures are logged as warnings but don't fail the DB operation. + - **`lib/bds/media/linking.ex`** — Same `log_sidecar_error/2` pattern for post link/unlink sidecar writes. + - **`lib/bds/release_packaging.ex`** — Replaced `File.mkdir_p!` with `File.mkdir_p` in `reset_output/1` (return value propagated through `with` chain in `package/1`). Replaced `File.mkdir_p!` with `with :ok <- File.mkdir_p(...)` in `copy_release/2`. Replaced `File.write!` with `File.write` in `write_manifest/1`. + - Added 6 tests in `test/bds/csm030_unchecked_mkdir_test.exs`: source-level assertions for no unchecked `File.mkdir_p`, no bang variants, no `:ok =` match assertions on sidecar writes. --- diff --git a/lib/bds/media.ex b/lib/bds/media.ex index 90585f3..8a15d76 100644 --- a/lib/bds/media.ex +++ b/lib/bds/media.ex @@ -106,7 +106,7 @@ defmodule BDS.Media do |> Repo.insert!() end) do {:ok, media} -> - :ok = write_sidecar(project, media) + log_sidecar_error(write_sidecar(project, media), media.id) log_thumbnail_error(ensure_thumbnails(project, media), media.id) :ok = Search.sync_media(media) {:ok, media} @@ -148,7 +148,7 @@ defmodule BDS.Media do |> Repo.update!() end) do {:ok, updated_media} -> - :ok = write_sidecar(project, updated_media) + log_sidecar_error(write_sidecar(project, updated_media), updated_media.id) :ok = Search.sync_media(updated_media) {:ok, updated_media} @@ -240,7 +240,7 @@ defmodule BDS.Media do |> Repo.insert_or_update!() end) do {:ok, updated_translation} -> - :ok = write_translation_sidecar(project, media, updated_translation) + log_sidecar_error(write_translation_sidecar(project, media, updated_translation), media.id) :ok = Search.sync_media(media.id) {:ok, updated_translation} @@ -275,7 +275,7 @@ defmodule BDS.Media do ) :ok = Search.sync_media(media) - :ok = write_sidecar(project, media) + log_sidecar_error(write_sidecar(project, media), media.id) {:ok, true} {:error, changeset} -> @@ -322,7 +322,7 @@ defmodule BDS.Media do end) do {:ok, updated_media} -> _ = File.rm(previous_destination_backup) - :ok = write_sidecar(project, updated_media) + log_sidecar_error(write_sidecar(project, updated_media), updated_media.id) log_thumbnail_error(ensure_thumbnails(project, updated_media), updated_media.id) :ok = Search.sync_media(updated_media) {:ok, updated_media} @@ -350,4 +350,10 @@ defmodule BDS.Media do defp log_thumbnail_error({:error, reason}, media_id) do Logger.warning("Thumbnail generation failed for media #{media_id}: #{inspect(reason)}") end + + defp log_sidecar_error(:ok, _media_id), do: :ok + + defp log_sidecar_error({:error, reason}, media_id) do + Logger.warning("Sidecar write failed for media #{media_id}: #{inspect(reason)}") + end end diff --git a/lib/bds/media/linking.ex b/lib/bds/media/linking.ex index 470a9c8..8269640 100644 --- a/lib/bds/media/linking.ex +++ b/lib/bds/media/linking.ex @@ -1,6 +1,8 @@ defmodule BDS.Media.Linking do @moduledoc false + require Logger + import Ecto.Query alias BDS.Media.Media @@ -64,7 +66,7 @@ defmodule BDS.Media.Linking do end end) do {:ok, _result} -> - :ok = Sidecars.write_sidecar(project, media) + log_sidecar_error(Sidecars.write_sidecar(project, media), media.id) {:ok, :linked} {:error, reason} -> @@ -93,7 +95,7 @@ defmodule BDS.Media.Linking do :ok end) do {:ok, :ok} -> - :ok = Sidecars.write_sidecar(project, media) + log_sidecar_error(Sidecars.write_sidecar(project, media), media.id) {:ok, :unlinked} {:error, reason} -> @@ -112,6 +114,12 @@ defmodule BDS.Media.Linking do ) end + defp log_sidecar_error(:ok, _media_id), do: :ok + + defp log_sidecar_error({:error, reason}, media_id) do + Logger.warning("Sidecar write failed for media #{media_id}: #{inspect(reason)}") + end + defp next_sort_order(media_id) do case Repo.one( from pm in PostMedia, diff --git a/lib/bds/media/sidecars.ex b/lib/bds/media/sidecars.ex index 27942aa..f6e749b 100644 --- a/lib/bds/media/sidecars.ex +++ b/lib/bds/media/sidecars.ex @@ -18,10 +18,9 @@ defmodule BDS.Media.Sidecars do alias BDS.Search alias BDS.Sidecar - @spec write_sidecar(BDS.Projects.Project.t(), Media.t()) :: :ok + @spec write_sidecar(BDS.Projects.Project.t(), Media.t()) :: :ok | {:error, File.posix()} def write_sidecar(project, media) do path = Path.join(Projects.project_data_dir(project), media.sidecar_path) - :ok = File.mkdir_p(Path.dirname(path)) atomic_write( path, @@ -45,7 +44,8 @@ defmodule BDS.Media.Sidecars do ) end - @spec write_translation_sidecar(BDS.Projects.Project.t(), Media.t(), Translation.t()) :: :ok + @spec write_translation_sidecar(BDS.Projects.Project.t(), Media.t(), Translation.t()) :: + :ok | {:error, File.posix()} def write_translation_sidecar(project, media, translation) do path = Path.join( @@ -53,8 +53,6 @@ defmodule BDS.Media.Sidecars do translation_sidecar_path(media, translation.language) ) - :ok = File.mkdir_p(Path.dirname(path)) - atomic_write( path, Sidecar.serialize_document([ @@ -189,8 +187,7 @@ defmodule BDS.Media.Sidecars do media -> project = Projects.get_project!(media.project_id) - :ok = write_sidecar(project, media) - :ok + write_sidecar(project, media) end end @@ -224,8 +221,11 @@ defmodule BDS.Media.Sidecars do %Translation{} = translation -> media = Repo.get!(Media, translation.translation_for) project = Projects.get_project!(media.project_id) - :ok = write_translation_sidecar(project, media, translation) - {:ok, translation} + + case write_translation_sidecar(project, media, translation) do + :ok -> {:ok, translation} + {:error, reason} -> {:error, reason} + end end end diff --git a/lib/bds/release_packaging.ex b/lib/bds/release_packaging.ex index 2975333..b222085 100644 --- a/lib/bds/release_packaging.ex +++ b/lib/bds/release_packaging.ex @@ -77,16 +77,15 @@ defmodule BDS.ReleasePackaging do defp reset_output(metadata) do File.rm_rf!(metadata.payload_root) File.rm_rf!(metadata.archive_path) - File.mkdir_p!(metadata.output_dir) - :ok + File.mkdir_p(metadata.output_dir) end defp copy_release(source, destination) do - File.mkdir_p!(Path.dirname(destination)) - - case File.cp_r(source, destination) do - {:ok, _files} -> :ok - {:error, reason, _file} -> {:error, reason} + with :ok <- File.mkdir_p(Path.dirname(destination)) do + case File.cp_r(source, destination) do + {:ok, _files} -> :ok + {:error, reason, _file} -> {:error, reason} + end end end @@ -102,8 +101,7 @@ defmodule BDS.ReleasePackaging do } manifest_path = Path.join(metadata.payload_root, "manifest.json") - File.write!(manifest_path, Jason.encode!(manifest, pretty: true)) - :ok + File.write(manifest_path, Jason.encode!(manifest, pretty: true)) end defp create_archive(%Metadata{platform: :windows} = metadata) do diff --git a/test/bds/csm030_unchecked_mkdir_test.exs b/test/bds/csm030_unchecked_mkdir_test.exs new file mode 100644 index 0000000..9f4fcf3 --- /dev/null +++ b/test/bds/csm030_unchecked_mkdir_test.exs @@ -0,0 +1,42 @@ +defmodule BDS.CSM030UncheckedMkdirTest do + use ExUnit.Case, async: true + + describe "source-level: no unchecked File.mkdir_p" do + test "sidecars.ex has no bare File.mkdir_p calls" do + source = File.read!("lib/bds/media/sidecars.ex") + refute source =~ ~r/:ok\s*=\s*File\.mkdir_p/ + refute source =~ ~r/File\.mkdir_p!/ + end + + test "release_packaging.ex has no File.mkdir_p! calls" do + source = File.read!("lib/bds/release_packaging.ex") + refute source =~ ~r/File\.mkdir_p!/ + end + + test "thumbnails.ex mkdir_p is inside a with chain" do + source = File.read!("lib/bds/media/thumbnails.ex") + refute source =~ ~r/:ok\s*=\s*File\.mkdir_p/ + refute source =~ ~r/File\.mkdir_p!/ + end + end + + describe "source-level: sidecar write errors are handled" do + test "media.ex does not assert :ok on write_sidecar" do + source = File.read!("lib/bds/media.ex") + refute source =~ ~r/:ok\s*=\s*write_sidecar/ + refute source =~ ~r/:ok\s*=\s*write_translation_sidecar/ + end + + test "linking.ex does not assert :ok on write_sidecar" do + source = File.read!("lib/bds/media/linking.ex") + refute source =~ ~r/:ok\s*=\s*Sidecars\.write_sidecar/ + end + end + + describe "source-level: release_packaging.ex write_manifest" do + test "uses File.write not File.write!" do + source = File.read!("lib/bds/release_packaging.ex") + refute source =~ ~r/File\.write!/ + end + end +end