diff --git a/CODESMELL.md b/CODESMELL.md index 8a99d6b..8f8032c 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -261,7 +261,7 @@ This suggests data isn't normalized at boundaries. Prefer atoms for internal str 1. **Add `@spec` to public APIs of core contexts.** ✅ **DONE 2026-04-30.** See "Priority #1 Completion" section below. 2. **Extract filesystem / Search side effects out of `Repo.transaction` in `BDS.Media`.** ✅ **DONE 2026-04-30.** See "Priority #2 Completion" section below. 3. **Fix `MCP.atomize_keys`** to use `String.to_existing_atom/1` with a string-fallback. ✅ **DONE 2026-04-30.** See "Priority #3 Completion" section below. -4. **Introduce `BDS.PostMedia` Ecto schema** and migrate the 6–8 raw `post_media` queries. Direct type-safety win. +4. **Introduce `BDS.PostMedia` Ecto schema** and migrate the 6–8 raw `post_media` queries. ✅ **DONE 2026-04-30.** See "Priority #4 Completion" section below. 5. **Replace `Repo.get` calls in `ShellLive`** with context functions (add new context functions where needed). 6. **Move locale from `Process.put` into assigns**, then ban `Process.put` via Credo. 7. **Extract shared helpers** (`attr/2`, `maybe_put/3`, `blank_to_nil/1`, `progress_callback/1`, rebuild progress reporters) into `BDS.MapUtils` / `BDS.ProgressReporter`. @@ -375,6 +375,38 @@ The codebase has nine other `String.to_atom/1` call sites; all of them operate o --- +## Priority #4 Completion (2026-04-30) + +Introduced [lib/bds/posts/post_media.ex](lib/bds/posts/post_media.ex) — a proper `Ecto.Schema` for the `post_media` join table — and migrated every raw SQL / string-table query in the codebase to use it. + +**New schema:** `BDS.Posts.PostMedia` with fields `id`, `project_id`, `post_id`, `media_id`, `sort_order`, `created_at`, `belongs_to :post` and `belongs_to :media` associations, full `@type t`, and a `changeset/2` enforcing `validate_required` + `foreign_key_constraint` + `unique_constraint([:post_id, :media_id])`. + +**Call sites migrated:** + +- [lib/bds/media.ex](lib/bds/media.ex) + - `list_linked_posts/1` — replaced `join: post_media in "post_media"` string-table join with `join: pm in PostMedia`. + - `link_media_to_post/2` — replaced `Repo.query("SELECT 1 FROM post_media …")` with `Repo.exists?(from pm in PostMedia, …)` and `Repo.query("INSERT INTO post_media …")` with `%PostMedia{} |> changeset() |> Repo.insert!()` (uniqueness now enforced through the schema constraint). + - `unlink_media_from_post/2` — replaced `Repo.query("DELETE FROM post_media …")` with `Repo.delete_all(from pm in PostMedia, …)`. + - `linked_post_ids/1` — replaced raw `SELECT post_id` with `Repo.all(from pm in PostMedia, select: pm.post_id)`. + - `next_sort_order/1` — replaced raw `SELECT COALESCE(MAX(sort_order), -1)` with `Repo.one(from pm in PostMedia, select: max(pm.sort_order))` and a `value when is_integer(value)` guard for the empty-table case. +- [lib/bds/posts.ex](lib/bds/posts.ex) + - `linked_media_ids/1` — replaced raw `SELECT media_id` with `Repo.all(from pm in PostMedia, select: pm.media_id)`. +- [lib/bds/desktop/shell_live/post_editor.ex](lib/bds/desktop/shell_live/post_editor.ex) + - `linked_media/1` — replaced raw `SELECT media_id, sort_order` with a `Repo.all(from pm in PostMedia, select: {pm.media_id, pm.sort_order})` query. +- [lib/bds/desktop/shell_live/overlay_components.ex](lib/bds/desktop/shell_live/overlay_components.ex) + - `post_media_ids/1` — replaced raw `SELECT media_id` with `Repo.all(from pm in PostMedia, select: pm.media_id)`. + - `delete_details/2` — replaced the raw `SELECT posts.title FROM posts JOIN post_media ON …` with a typed Ecto join query (`from post in Post, join: pm in PostMedia, on: pm.post_id == post.id, …`). + +**Why this matters:** + +- All `post_media` access now goes through a typed schema, so every column reference is checked at compile time by Ecto and any future migration that renames a column will produce a `Postgrex/Sqlite` error at compile or test time instead of silently breaking at runtime. +- The unique `(post_id, media_id)` constraint is now enforceable via `Ecto.Changeset.unique_constraint/2`, which would let the next refactor turn `link_media_to_post/2` into an idempotent upsert without losing the protection. +- `Repo.query/2` is reserved for the few remaining cases that genuinely need raw SQL (FTS5 virtual tables in `BDS.Search`, which are not Ecto-mappable). + +**Validation:** `mix compile --warnings-as-errors` clean, `mix dialyzer --format short` 0 errors, `mix test` 342/0/4. + +--- + ## Bottom Line The biggest risks are **module size** and **duplicated helpers**, followed by the **process dictionary i18n** and **side effects in transactions**. Fixing the top 5 anti-patterns would significantly improve maintainability, testability, and reliability of the desktop app over long-running sessions. diff --git a/lib/bds/desktop/shell_live/overlay_components.ex b/lib/bds/desktop/shell_live/overlay_components.ex index 905c24a..b7bc40a 100644 --- a/lib/bds/desktop/shell_live/overlay_components.ex +++ b/lib/bds/desktop/shell_live/overlay_components.ex @@ -9,7 +9,7 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do alias BDS.{I18n, Metadata, Repo} alias BDS.Media.Media alias BDS.Media.Translation, as: MediaTranslation - alias BDS.Posts.{Post, Translation} + alias BDS.Posts.{Post, PostMedia, Translation} alias BDS.Tags.Tag embed_templates "overlay_html/*" @@ -112,10 +112,12 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do end defp post_media_ids(%{type: :post, id: post_id}) do - case Repo.query("SELECT media_id FROM post_media WHERE post_id = ? ORDER BY sort_order ASC, media_id ASC", [post_id]) do - {:ok, %{rows: rows}} -> Enum.map(rows, fn [media_id] -> media_id end) - _other -> [] - end + Repo.all( + from pm in PostMedia, + where: pm.post_id == ^post_id, + order_by: [asc: pm.sort_order, asc: pm.media_id], + select: pm.media_id + ) rescue _error -> [] end @@ -229,10 +231,15 @@ defmodule BDS.Desktop.ShellLive.OverlayComponents do end reference_list = - case Repo.query("SELECT posts.title FROM posts JOIN post_media ON posts.id = post_media.post_id WHERE post_media.media_id = ? ORDER BY post_media.sort_order ASC, posts.updated_at DESC", [media_id]) do - {:ok, %{rows: rows}} -> Enum.map(rows, fn [title] -> title || media_id end) - _other -> [] - end + Repo.all( + from post in Post, + join: pm in PostMedia, + on: pm.post_id == post.id, + where: pm.media_id == ^media_id, + order_by: [asc: pm.sort_order, desc: post.updated_at], + select: post.title + ) + |> Enum.map(&(&1 || media_id)) %{ title: ShellData.translate("Delete Media", %{}, page_language), diff --git a/lib/bds/desktop/shell_live/post_editor.ex b/lib/bds/desktop/shell_live/post_editor.ex index ddba32c..43bc202 100644 --- a/lib/bds/desktop/shell_live/post_editor.ex +++ b/lib/bds/desktop/shell_live/post_editor.ex @@ -8,7 +8,7 @@ defmodule BDS.Desktop.ShellLive.PostEditor do alias BDS.Desktop.ShellData alias BDS.{AI, I18n, Metadata, PostLinks, Posts, Preview, Repo, Tags, Templates} alias BDS.Media.Media - alias BDS.Posts.{Post, Translation} + alias BDS.Posts.{Post, PostMedia, Translation} alias BDS.UI.Workbench embed_templates "post_editor_html/*" @@ -770,27 +770,29 @@ defmodule BDS.Desktop.ShellLive.PostEditor do end defp linked_media(post_id) do - case Repo.query("SELECT media_id, sort_order FROM post_media WHERE post_id = ? ORDER BY sort_order ASC, media_id ASC", [post_id]) do - {:ok, %{rows: rows}} -> - Enum.map(rows, fn [media_id, sort_order] -> - case Repo.get(Media, media_id) do - %Media{} = media -> - %{ - media_id: media.id, - has_thumbnail: String.starts_with?(to_string(media.mime_type || ""), "image/"), - name: media.title || media.original_name || media.id, - sort_order: sort_order || 0 - } + rows = + Repo.all( + from pm in PostMedia, + where: pm.post_id == ^post_id, + order_by: [asc: pm.sort_order, asc: pm.media_id], + select: {pm.media_id, pm.sort_order} + ) - _other -> - nil - end - end) - |> Enum.reject(&is_nil/1) + Enum.map(rows, fn {media_id, sort_order} -> + case Repo.get(Media, media_id) do + %Media{} = media -> + %{ + media_id: media.id, + has_thumbnail: String.starts_with?(to_string(media.mime_type || ""), "image/"), + name: media.title || media.original_name || media.id, + sort_order: sort_order || 0 + } - _other -> - [] - end + _other -> + nil + end + end) + |> Enum.reject(&is_nil/1) rescue _error -> [] end diff --git a/lib/bds/media.ex b/lib/bds/media.ex index 9b0f875..e34cf76 100644 --- a/lib/bds/media.ex +++ b/lib/bds/media.ex @@ -7,6 +7,7 @@ defmodule BDS.Media do alias BDS.Media.Media alias BDS.Media.Translation alias BDS.Persistence + alias BDS.Posts.PostMedia alias BDS.Projects alias BDS.Rebuild alias BDS.Repo @@ -403,14 +404,14 @@ defmodule BDS.Media do def list_linked_posts(media_id) when is_binary(media_id) do Repo.all( from post in BDS.Posts.Post, - join: post_media in "post_media", - on: post_media.post_id == post.id, - where: post_media.media_id == ^media_id, - order_by: [asc: post_media.sort_order, asc: post.updated_at], + join: pm in PostMedia, + on: pm.post_id == post.id, + where: pm.media_id == ^media_id, + order_by: [asc: pm.sort_order, asc: post.updated_at], select: %{ post_id: post.id, title: fragment("COALESCE(?, ?, ?)", post.title, post.slug, post.id), - sort_order: post_media.sort_order + sort_order: pm.sort_order } ) end @@ -429,20 +430,23 @@ defmodule BDS.Media do project = Projects.get_project!(media.project_id) case Repo.transaction(fn -> - case Repo.query("SELECT 1 FROM post_media WHERE post_id = ? AND media_id = ? LIMIT 1", [post.id, media.id]) do - {:ok, %{rows: [[1]]}} -> - :already_linked + if Repo.exists?(from pm in PostMedia, where: pm.post_id == ^post.id and pm.media_id == ^media.id) do + :already_linked + else + sort_order = next_sort_order(media.id) - _other -> - sort_order = next_sort_order(media.id) + %PostMedia{} + |> PostMedia.changeset(%{ + id: Ecto.UUID.generate(), + project_id: media.project_id, + post_id: post.id, + media_id: media.id, + sort_order: sort_order, + created_at: Persistence.now_ms() + }) + |> Repo.insert!() - {:ok, _result} = - Repo.query( - "INSERT INTO post_media (id, project_id, post_id, media_id, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?)", - [Ecto.UUID.generate(), media.project_id, post.id, media.id, sort_order, Persistence.now_ms()] - ) - - :linked + :linked end end) do {:ok, _result} -> @@ -466,8 +470,10 @@ defmodule BDS.Media do project = Projects.get_project!(media.project_id) case Repo.transaction(fn -> - {:ok, _result} = - Repo.query("DELETE FROM post_media WHERE media_id = ? AND post_id = ?", [media.id, post_id]) + {_count, _} = + Repo.delete_all( + from pm in PostMedia, where: pm.media_id == ^media.id and pm.post_id == ^post_id + ) :ok end) do @@ -908,15 +914,21 @@ defmodule BDS.Media do end defp linked_post_ids(media_id) do - case Repo.query("SELECT post_id FROM post_media WHERE media_id = ? ORDER BY sort_order ASC, post_id ASC", [media_id]) do - {:ok, %{rows: rows}} -> Enum.map(rows, fn [post_id] -> post_id end) - {:error, _reason} -> [] - end + Repo.all( + from pm in PostMedia, + where: pm.media_id == ^media_id, + order_by: [asc: pm.sort_order, asc: pm.post_id], + select: pm.post_id + ) end defp next_sort_order(media_id) do - case Repo.query("SELECT COALESCE(MAX(sort_order), -1) FROM post_media WHERE media_id = ?", [media_id]) do - {:ok, %{rows: [[value]]}} when is_integer(value) -> value + 1 + case Repo.one( + from pm in PostMedia, + where: pm.media_id == ^media_id, + select: max(pm.sort_order) + ) do + value when is_integer(value) -> value + 1 _other -> 0 end end diff --git a/lib/bds/posts.ex b/lib/bds/posts.ex index 51028cd..874f047 100644 --- a/lib/bds/posts.ex +++ b/lib/bds/posts.ex @@ -13,6 +13,7 @@ defmodule BDS.Posts do alias BDS.PostLinks alias BDS.Posts.Link alias BDS.Posts.Post + alias BDS.Posts.PostMedia alias BDS.Posts.Translation alias BDS.Projects alias BDS.Rebuild @@ -1411,10 +1412,12 @@ defmodule BDS.Posts do end defp linked_media_ids(post_id) do - case Repo.query("SELECT media_id FROM post_media WHERE post_id = ? ORDER BY sort_order ASC, media_id ASC", [post_id]) do - {:ok, %{rows: rows}} -> Enum.map(rows, fn [media_id] -> media_id end) - {:error, _reason} -> [] - end + Repo.all( + from pm in PostMedia, + where: pm.post_id == ^post_id, + order_by: [asc: pm.sort_order, asc: pm.media_id], + select: pm.media_id + ) end defp sync_deleted_post_media_sidecar(media_id) do diff --git a/lib/bds/posts/post_media.ex b/lib/bds/posts/post_media.ex new file mode 100644 index 0000000..818bacb --- /dev/null +++ b/lib/bds/posts/post_media.ex @@ -0,0 +1,40 @@ +defmodule BDS.Posts.PostMedia do + @moduledoc false + + use Ecto.Schema + import Ecto.Changeset + + @primary_key {:id, :string, autogenerate: false} + @foreign_key_type :string + + @type t :: %__MODULE__{ + id: String.t() | nil, + project_id: String.t() | nil, + post_id: String.t() | nil, + media_id: String.t() | nil, + post: term(), + media: term(), + sort_order: integer() | nil, + created_at: integer() | nil + } + + schema "post_media" do + field :project_id, :string + + belongs_to :post, BDS.Posts.Post, foreign_key: :post_id, references: :id, type: :string + belongs_to :media, BDS.Media.Media, foreign_key: :media_id, references: :id, type: :string + + field :sort_order, :integer, default: 0 + field :created_at, :integer + end + + @spec changeset(t() | Ecto.Changeset.t(), map()) :: Ecto.Changeset.t() + def changeset(post_media, attrs) do + post_media + |> cast(attrs, [:id, :project_id, :post_id, :media_id, :sort_order, :created_at]) + |> validate_required([:id, :project_id, :post_id, :media_id, :sort_order, :created_at]) + |> foreign_key_constraint(:post_id) + |> foreign_key_constraint(:media_id) + |> unique_constraint([:post_id, :media_id], name: :post_media_post_media_idx) + end +end