From f1445120fcab7519b7bc6a6613f28d808666dcf9 Mon Sep 17 00:00:00 2001 From: Chili Palmer Date: Sat, 9 May 2026 14:31:44 +0200 Subject: [PATCH] fix: fix CSM-010 --- CODESMELL.md | 19 +++-- lib/bds/desktop/shell_data.ex | 82 ++++++++---------- lib/bds/desktop/shell_live.ex | 47 ++++++++--- lib/bds/repo.ex | 18 ++++ test/bds/csm010_rescue_control_flow_test.exs | 88 ++++++++++++++++++++ 5 files changed, 185 insertions(+), 69 deletions(-) create mode 100644 test/bds/csm010_rescue_control_flow_test.exs diff --git a/CODESMELL.md b/CODESMELL.md index 0904df9..ab2f496 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -186,13 +186,15 @@ --- -### CSM-010 — `rescue` for Control Flow in Data Layer -- **File:** `lib/bds/desktop/shell_data.ex:64-74, 81-103, 152-182` -- **What:** `rescue Exqlite.Error`, `rescue DBConnection.OwnershipError` to return empty defaults in `project_snapshot/0`, `dashboard/1`, `sidebar_view/3`, and `git_badge_count/2`. -- **Why it's bad:** Exceptions are for exceptional conditions. The rescue here handles the boot-phase case where the DB table doesn't exist yet — this is an expected state during startup. -- **Nuance:** The code does check `Exception.message(error)` for "no such table" and re-raises everything else, which limits the damage. But it's still control-flow-via-exception. -- **Fix:** Add an explicit `DB.ready?/0` check (or `Ecto.Adapters.SQL.query!/2` with a lightweight probe) before calling the data functions. Return `{:ok, result}` / `{:error, :not_ready}` tuples from the lowest-level callers and handle them upstream. Only `rescue` for truly unexpected DB errors. -- **Test:** Call `project_snapshot/0` with no DB connection; assert it returns `{:error, :not_ready}` instead of a default map. +### ~~CSM-010 — `rescue` for Control Flow in Data Layer~~ ✅ FIXED +- **Fixed:** 2026-05-09 +- **What was done:** + - Added `BDS.Repo.ready?/0` — a lightweight probe that queries `sqlite_master` (parameterized) to check if core tables exist, without raising exceptions. + - Replaced all 4 `rescue` blocks in `ShellData` (`project_snapshot/0`, `dashboard/1`, `sidebar_view/3`, `git_badge_count/2`) with upfront `Repo.ready?()` checks. + - All four functions now return `{:ok, result}` / `{:error, :not_ready}` tuples instead of silently returning defaults via rescue. + - Updated callers in `ShellLive.refresh_content/2` and `ShellLive.refresh_sidebar/2` to pattern-match the new tuples and fall back to empty defaults only on `{:error, :not_ready}`. + - Made `default_project_snapshot/0` public for use by callers handling the not-ready case. + - Added 10 tests in `test/bds/csm010_rescue_control_flow_test.exs`: `Repo.ready?` returns true when DB is available, each of the 4 functions returns `{:ok, _}` when DB is ready and `{:error, :not_ready}` when the Repo is stopped. --- @@ -430,11 +432,12 @@ - CSM-001: Fixed. All `String.to_atom` on dynamic data replaced with `MapUtils.safe_atomize_key/keys` or `String.to_existing_atom`. - CSM-002: Fixed. Search now pushes all filtering and pagination into SQL via Ecto queries and CTEs. - CSM-004: Fixed. `attach_runner` moved to `handle_continue`, `terminate/2` added for cleanup, `restart: :temporary` set, JobStore `detach_runner` bug fixed. -- [ ] All high-severity items (CSM-006 to CSM-010) have been addressed. +- [x] All high-severity items (CSM-006 to CSM-010) have been addressed. - 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. + - CSM-010: Fixed. Replaced rescue blocks with `Repo.ready?/0` probe and `{:ok, _}`/`{:error, :not_ready}` tuples. - [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/desktop/shell_data.ex b/lib/bds/desktop/shell_data.ex index 61c0c1b..04dd636 100644 --- a/lib/bds/desktop/shell_data.ex +++ b/lib/bds/desktop/shell_data.ex @@ -6,6 +6,7 @@ defmodule BDS.Desktop.ShellData do alias BDS.Git alias BDS.I18n alias BDS.Projects + alias BDS.Repo alias BDS.UI.Dashboard alias BDS.UI.Sidebar alias BDS.UI.Workbench @@ -62,15 +63,11 @@ defmodule BDS.Desktop.ShellData do end def project_snapshot do - Projects.shell_snapshot() - rescue - error in [Exqlite.Error, DBConnection.OwnershipError] -> - if match?(%Exqlite.Error{}, error) and - not String.contains?(Exception.message(error), "no such table: projects") do - reraise error, __STACKTRACE__ - end - - default_project_snapshot() + if Repo.ready?() do + {:ok, Projects.shell_snapshot()} + else + {:error, :not_ready} + end end def current_project(projects_snapshot) do @@ -79,27 +76,19 @@ defmodule BDS.Desktop.ShellData do end def dashboard(project_id) do - Dashboard.snapshot(project_id) - rescue - error in [Exqlite.Error, DBConnection.OwnershipError] -> - if match?(%Exqlite.Error{}, error) and - not String.contains?(Exception.message(error), "no such table") do - reraise error, __STACKTRACE__ - end - - Dashboard.empty_snapshot() + if Repo.ready?() do + {:ok, Dashboard.snapshot(project_id)} + else + {:error, :not_ready} + end end def sidebar_view(project_id, view_id, params \\ %{}) do - Sidebar.view(project_id, view_id, params) - rescue - error in [Exqlite.Error, DBConnection.OwnershipError] -> - if match?(%Exqlite.Error{}, error) and - not String.contains?(Exception.message(error), "no such table") do - reraise error, __STACKTRACE__ - end - - Sidebar.view(nil, view_id, params) + if Repo.ready?() do + {:ok, Sidebar.view(project_id, view_id, params)} + else + {:error, :not_ready} + end end def assistant_cards do @@ -146,14 +135,16 @@ defmodule BDS.Desktop.ShellData do def git_badge_count(project_id, opts \\ []) - def git_badge_count(nil, _opts), do: 0 - def git_badge_count("default", _opts), do: 0 + def git_badge_count(nil, _opts), do: {:ok, 0} + def git_badge_count("default", _opts), do: {:ok, 0} def git_badge_count(project_id, opts) when is_binary(project_id) do - provider = Keyword.get(opts, :provider, git_remote_state_provider()) - custom_provider? = provider != (&BDS.Git.remote_state/2) + if not Repo.ready?() do + {:error, :not_ready} + else + provider = Keyword.get(opts, :provider, git_remote_state_provider()) + custom_provider? = provider != (&BDS.Git.remote_state/2) - try do has_git = custom_provider? || case BDS.Projects.get_project(project_id) do @@ -161,23 +152,18 @@ defmodule BDS.Desktop.ShellData do project -> File.dir?(Path.join(BDS.Projects.project_data_dir(project), ".git")) end - if has_git do - case provider.(project_id, []) do - {:ok, %{behind: behind}} when is_integer(behind) and behind > 0 -> behind - {:ok, %{behind: behind}} when is_binary(behind) -> parse_positive_count(behind) - _other -> 0 - end - else - 0 - end - rescue - error in [DBConnection.OwnershipError, Exqlite.Error] -> - if match?(%Exqlite.Error{}, error) and - not String.contains?(Exception.message(error), "no such table") do - reraise error, __STACKTRACE__ + count = + if has_git do + case provider.(project_id, []) do + {:ok, %{behind: behind}} when is_integer(behind) and behind > 0 -> behind + {:ok, %{behind: behind}} when is_binary(behind) -> parse_positive_count(behind) + _other -> 0 + end + else + 0 end - 0 + {:ok, count} end end @@ -303,7 +289,7 @@ defmodule BDS.Desktop.ShellData do defp maybe_add_panel_tab(tabs, _route, _tab), do: tabs - defp default_project_snapshot do + def default_project_snapshot do %{ active_project_id: "default", projects: [ diff --git a/lib/bds/desktop/shell_live.ex b/lib/bds/desktop/shell_live.ex index f8d8702..8be73f6 100644 --- a/lib/bds/desktop/shell_live.ex +++ b/lib/bds/desktop/shell_live.ex @@ -633,11 +633,14 @@ defmodule BDS.Desktop.ShellLive do active_view_id = Atom.to_string(workbench.active_view) sidebar_data = - ShellData.sidebar_view( - project_id, - active_view_id, - ShellSidebarState.current_filters(socket, active_view_id) - ) + case ShellData.sidebar_view( + project_id, + active_view_id, + ShellSidebarState.current_filters(socket, active_view_id) + ) do + {:ok, data} -> data + {:error, :not_ready} -> BDS.UI.Sidebar.view(nil, active_view_id, %{}) + end sidebar_data = ShellSidebarState.merge_ui_state(socket, active_view_id, sidebar_data) @@ -647,17 +650,35 @@ defmodule BDS.Desktop.ShellLive do end defp refresh_content(socket, workbench) do - projects = ShellData.project_snapshot() - dashboard = ShellData.dashboard(projects.active_project_id) - git_badge_count = ShellData.git_badge_count(projects.active_project_id) + projects = + case ShellData.project_snapshot() do + {:ok, data} -> data + {:error, :not_ready} -> ShellData.default_project_snapshot() + end + + dashboard = + case ShellData.dashboard(projects.active_project_id) do + {:ok, data} -> data + {:error, :not_ready} -> BDS.UI.Dashboard.empty_snapshot() + end + + git_badge_count = + case ShellData.git_badge_count(projects.active_project_id) do + {:ok, count} -> count + {:error, :not_ready} -> 0 + end + active_view_id = Atom.to_string(workbench.active_view) sidebar_data = - ShellData.sidebar_view( - projects.active_project_id, - active_view_id, - ShellSidebarState.current_filters(socket, active_view_id) - ) + case ShellData.sidebar_view( + projects.active_project_id, + active_view_id, + ShellSidebarState.current_filters(socket, active_view_id) + ) do + {:ok, data} -> data + {:error, :not_ready} -> BDS.UI.Sidebar.view(nil, active_view_id, %{}) + end sidebar_data = ShellSidebarState.merge_ui_state(socket, active_view_id, sidebar_data) diff --git a/lib/bds/repo.ex b/lib/bds/repo.ex index 5037e99..7b92855 100644 --- a/lib/bds/repo.ex +++ b/lib/bds/repo.ex @@ -2,4 +2,22 @@ defmodule BDS.Repo do use Ecto.Repo, otp_app: :bds, adapter: Ecto.Adapters.SQLite3 + + @doc """ + Returns true if the database is connected and core tables exist. + Used to guard data access during startup before migrations have run. + """ + def ready? do + case Ecto.Adapters.SQL.query( + __MODULE__, + "SELECT 1 FROM sqlite_master WHERE type = ?1 AND name = ?2 LIMIT 1", + ["table", "projects"] + ) do + {:ok, %{num_rows: 1}} -> true + {:ok, _} -> false + {:error, _} -> false + end + rescue + _ -> false + end end diff --git a/test/bds/csm010_rescue_control_flow_test.exs b/test/bds/csm010_rescue_control_flow_test.exs new file mode 100644 index 0000000..260c871 --- /dev/null +++ b/test/bds/csm010_rescue_control_flow_test.exs @@ -0,0 +1,88 @@ +defmodule BDS.CSM010RescueControlFlowTest do + use ExUnit.Case, async: false + + alias BDS.Desktop.ShellData + alias BDS.Repo + + setup do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(BDS.Repo) + :ok + end + + describe "Repo.ready?/0" do + test "returns true when the database is available" do + assert Repo.ready?() + end + end + + describe "project_snapshot/0" do + test "returns {:ok, snapshot} when DB is ready" do + assert {:ok, snapshot} = ShellData.project_snapshot() + assert is_map(snapshot) + assert Map.has_key?(snapshot, :active_project_id) + assert Map.has_key?(snapshot, :projects) + end + + test "returns {:error, :not_ready} when DB is unavailable" do + Ecto.Adapters.SQL.Sandbox.checkin(BDS.Repo) + + assert {:error, :not_ready} = with_stopped_repo(&ShellData.project_snapshot/0) + end + end + + describe "dashboard/1" do + test "returns {:ok, snapshot} when DB is ready" do + assert {:ok, dashboard} = ShellData.dashboard("default") + assert is_map(dashboard) + assert Map.has_key?(dashboard, :post_stats) + end + + test "returns {:error, :not_ready} when DB is unavailable" do + Ecto.Adapters.SQL.Sandbox.checkin(BDS.Repo) + + assert {:error, :not_ready} = with_stopped_repo(fn -> ShellData.dashboard("default") end) + end + end + + describe "sidebar_view/3" do + test "returns {:ok, view} when DB is ready" do + assert {:ok, view} = ShellData.sidebar_view("default", "posts") + assert is_map(view) + end + + test "returns {:error, :not_ready} when DB is unavailable" do + Ecto.Adapters.SQL.Sandbox.checkin(BDS.Repo) + + assert {:error, :not_ready} = + with_stopped_repo(fn -> ShellData.sidebar_view("default", "posts") end) + end + end + + describe "git_badge_count/2" do + test "returns {:ok, count} when DB is ready" do + assert {:ok, count} = ShellData.git_badge_count("default") + assert is_integer(count) + end + + test "returns {:ok, 0} for nil project_id" do + assert {:ok, 0} = ShellData.git_badge_count(nil) + end + + test "returns {:error, :not_ready} when DB is unavailable" do + Ecto.Adapters.SQL.Sandbox.checkin(BDS.Repo) + + assert {:error, :not_ready} = + with_stopped_repo(fn -> ShellData.git_badge_count("some-project") end) + end + end + + defp with_stopped_repo(fun) do + Supervisor.terminate_child(BDS.Supervisor, BDS.Repo) + + try do + fun.() + after + Supervisor.restart_child(BDS.Supervisor, BDS.Repo) + end + end +end