From d18e0ef7f275cca4f3cb4a2a4d28635af7d2cbff Mon Sep 17 00:00:00 2001 From: Chili Palmer Date: Wed, 27 May 2026 18:12:23 +0200 Subject: [PATCH] fix(rendering): replace inline try/rescue with with-chains and safe_liquex_render helpers (CSM-031) --- CODESMELL.md | 11 ++-- lib/bds/rendering/filters.ex | 32 +++++++----- lib/bds/rendering/template_selection.ex | 52 +++++++++---------- test/bds/csm031_try_rescue_test.exs | 68 +++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 43 deletions(-) create mode 100644 test/bds/csm031_try_rescue_test.exs diff --git a/CODESMELL.md b/CODESMELL.md index f0e090d..267ec22 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -457,9 +457,14 @@ --- -### CSM-031 — `try/rescue` Instead of `with` and Error Tuples -- **Files:** `lib/bds/rendering/filters.ex`, `lib/bds/rendering/template_selection.ex`, `lib/bds/desktop/shell_data.ex` -- **Fix:** Replace `try/rescue` around expected failures with non-bang functions and `with` chains. +### ~~CSM-031 — `try/rescue` Instead of `with` and Error Tuples~~ ✅ FIXED +- **Fixed:** 2026-05-27 +- **What was done:** + - **`lib/bds/rendering/filters.ex`** — Extracted `safe_liquex_render/3` private helper that isolates the unavoidable `Liquex.render!` rescue into a single function returning `{:ok, binary} | {:error, String.t()}`. Replaced inline `try/rescue` in `render_macro_source/4` with a `with` chain using the helper. + - **`lib/bds/rendering/template_selection.ex`** — Same pattern: extracted `safe_liquex_render/3` helper, replaced inline `try/rescue` in `render_template/3` with a `with` chain. + - **`lib/bds/rendering/template_selection.ex`** — `load_bundled_template_source/3`: Replaced raising `Liquex.FileSystem.read_template_file` with `FileSystem.try_read` (returns `{:ok, source} | {:error, :enoent}`), eliminating the function-level `rescue` block entirely. Uses a `with` chain for control flow. + - **`lib/bds/desktop/shell_data.ex`** — Already fixed by CSM-010; no `try/rescue` blocks remain. + - Added 7 tests in `test/bds/csm031_try_rescue_test.exs`: source-level assertions that no inline `try/rescue` around `Liquex.render!` exists in either file, both files define `safe_liquex_render` helpers, `load_bundled_template_source` has no rescue block and uses `FileSystem.try_read`, and `shell_data.ex` has no try/rescue. --- diff --git a/lib/bds/rendering/filters.ex b/lib/bds/rendering/filters.ex index 9813274..acaad86 100644 --- a/lib/bds/rendering/filters.ex +++ b/lib/bds/rendering/filters.ex @@ -148,27 +148,31 @@ defmodule BDS.Rendering.Filters do end defp render_macro_source(template_path, template_source, assigns, context) do - case Liquex.parse(template_source) do - {:ok, template_ast} -> - isolated_context = Liquex.Context.new_isolated_subscope(context, assigns) - - try do - {result, _context} = Liquex.render!(template_ast, isolated_context) - IO.iodata_to_binary(result) - rescue - e in Liquex.Error -> - require Logger - Logger.warning("Macro template render failed (#{template_path}): #{e.message}") - "" - end - + with {:ok, template_ast} <- Liquex.parse(template_source), + {:ok, rendered} <- safe_liquex_render(template_ast, context, assigns) do + rendered + else {:error, reason, line} -> require Logger Logger.warning("Macro template parse failed (#{template_path}): #{reason} at line #{line}") "" + + {:error, message} -> + require Logger + Logger.warning("Macro template render failed (#{template_path}): #{message}") + "" end end + defp safe_liquex_render(template_ast, context, assigns) do + isolated_context = Liquex.Context.new_isolated_subscope(context, assigns) + + {result, _context} = Liquex.render!(template_ast, isolated_context) + {:ok, IO.iodata_to_binary(result)} + rescue + e in Liquex.Error -> {:error, e.message} + end + defp render_markdown_html(markdown) do case Earmark.as_html(markdown) do {:ok, html, _messages} -> html diff --git a/lib/bds/rendering/template_selection.ex b/lib/bds/rendering/template_selection.ex index 4e190e2..575d2f7 100644 --- a/lib/bds/rendering/template_selection.ex +++ b/lib/bds/rendering/template_selection.ex @@ -93,45 +93,45 @@ defmodule BDS.Rendering.TemplateSelection do @spec render_template(String.t(), String.t(), map()) :: {:ok, String.t()} | {:error, String.t()} def render_template(project_id, source, assigns) do - with {:ok, template_ast} <- Liquex.parse(source) do - project = Projects.get_project!(project_id) - - context = - Liquex.Context.new(assigns, - static_environment: assigns, - filter_module: Filters, - file_system: FileSystem.new(StarterTemplates.template_roots(project)) - ) - - try do - {result, _context} = Liquex.render!(template_ast, context) - {:ok, IO.iodata_to_binary(result)} - rescue - e in Liquex.Error -> {:error, e.message} - end + with {:ok, template_ast} <- Liquex.parse(source), + {:ok, _rendered} = ok <- safe_liquex_render(template_ast, project_id, assigns) do + ok else - {:error, reason, line} -> {:error, "#{reason} at line #{line}"} + {:error, reason, line} when is_integer(line) -> {:error, "#{reason} at line #{line}"} + {:error, _message} = error -> error end end + defp safe_liquex_render(template_ast, project_id, assigns) do + project = Projects.get_project!(project_id) + + context = + Liquex.Context.new(assigns, + static_environment: assigns, + filter_module: Filters, + file_system: FileSystem.new(StarterTemplates.template_roots(project)) + ) + + {result, _context} = Liquex.render!(template_ast, context) + {:ok, IO.iodata_to_binary(result)} + rescue + e in Liquex.Error -> {:error, e.message} + end + defp load_bundled_template_source(project, kind, slug) do desired_slug = bundled_template_slug(kind, slug) - if is_binary(desired_slug) do - file_system = project |> StarterTemplates.template_roots() |> FileSystem.new() - source = Liquex.FileSystem.read_template_file(file_system, desired_slug) - + with true <- is_binary(desired_slug), + file_system = project |> StarterTemplates.template_roots() |> FileSystem.new(), + {:ok, source} <- FileSystem.try_read(file_system, desired_slug) do case Frontmatter.parse_document(source) do {:ok, %{body: body}} -> {:ok, body} {:error, :invalid_frontmatter} -> {:ok, source} end else - {:error, :template_not_found} + false -> {:error, :template_not_found} + {:error, :enoent} -> {:error, :template_not_found} end - rescue - error in [Liquex.Error] -> - _ = error - {:error, :template_not_found} end defp maybe_load_bundled_template_source(project, kind, slug, template, reason, error) diff --git a/test/bds/csm031_try_rescue_test.exs b/test/bds/csm031_try_rescue_test.exs new file mode 100644 index 0000000..e84a946 --- /dev/null +++ b/test/bds/csm031_try_rescue_test.exs @@ -0,0 +1,68 @@ +defmodule BDS.CSM031TryRescueTest do + use ExUnit.Case, async: true + + describe "source-level: no inline try/rescue around Liquex.render!" do + test "filters.ex has no try/rescue block in render_macro_source" do + source = File.read!("lib/bds/rendering/filters.ex") + refute source =~ ~r/try do\s+.*Liquex\.render!/s, + "render_macro_source should use safe_liquex_render helper, not inline try/rescue" + end + + test "filters.ex isolates Liquex.render! rescue in safe_liquex_render" do + source = File.read!("lib/bds/rendering/filters.ex") + assert source =~ "defp safe_liquex_render" + end + + test "template_selection.ex has no try/rescue block in render_template" do + source = File.read!("lib/bds/rendering/template_selection.ex") + lines = String.split(source, "\n") + + in_render_template = + lines + |> Enum.drop_while(&(not String.contains?(&1, "def render_template("))) + |> Enum.take_while(&(not String.match?(&1, ~r/^\s+def[p]?\s/))) + + body = Enum.join(in_render_template, "\n") + refute body =~ "try do", "render_template should not contain inline try/rescue" + end + + test "template_selection.ex isolates Liquex.render! rescue in safe_liquex_render" do + source = File.read!("lib/bds/rendering/template_selection.ex") + assert source =~ "defp safe_liquex_render" + end + end + + describe "source-level: no function-level rescue in load_bundled_template_source" do + test "template_selection.ex load_bundled_template_source has no rescue block" do + source = File.read!("lib/bds/rendering/template_selection.ex") + lines = String.split(source, "\n") + + in_load_bundled = + lines + |> Enum.drop_while(&(not String.contains?(&1, "defp load_bundled_template_source("))) + |> Enum.take_while(fn line -> + not (String.match?(line, ~r/^\s+def[p]?\s/) and + not String.contains?(line, "load_bundled_template_source")) + end) + + body = Enum.join(in_load_bundled, "\n") + refute body =~ ~r/^\s+rescue\b/m, "load_bundled_template_source should use with, not rescue" + end + + test "template_selection.ex uses FileSystem.try_read instead of read_template_file" do + source = File.read!("lib/bds/rendering/template_selection.ex") + refute source =~ "read_template_file", + "should use FileSystem.try_read, not the raising read_template_file" + + assert source =~ "FileSystem.try_read" + end + end + + describe "source-level: shell_data.ex has no try/rescue" do + test "shell_data.ex contains no try/rescue blocks" do + source = File.read!("lib/bds/desktop/shell_data.ex") + refute source =~ ~r/\btry\s+do\b/, "shell_data.ex should not contain try/rescue blocks" + refute source =~ ~r/\brescue\b/, "shell_data.ex should not contain rescue clauses" + end + end +end