diff --git a/lib/bds/rendering/list_archive.ex b/lib/bds/rendering/list_archive.ex index 2697e84..710cc90 100644 --- a/lib/bds/rendering/list_archive.ex +++ b/lib/bds/rendering/list_archive.ex @@ -24,9 +24,20 @@ defmodule BDS.Rendering.ListArchive do canonical_media_paths = LinksAndLanguages.canonical_media_path_by_source_path(project_id) + input_posts = MapUtils.attr(assigns, :posts, []) + + post_ids = + input_posts + |> Enum.map(&MapUtils.attr(&1, :id)) + |> Enum.reject(&is_nil/1) + |> Enum.uniq() + + post_records = PostRendering.load_post_records_batch(post_ids) + posts = normalize_list_posts( - MapUtils.attr(assigns, :posts, []), + input_posts, + post_records, canonical_post_paths, canonical_media_paths, language, @@ -172,13 +183,15 @@ defmodule BDS.Rendering.ListArchive do defp normalize_list_posts( posts, + post_records, canonical_post_paths, canonical_media_paths, language, template_context ) do Enum.map(posts, fn post -> - post_record = PostRendering.load_post_record(post) + post_id = MapUtils.attr(post, :id) + post_record = Map.get(post_records, post_id) raw_content = Map.get( @@ -332,4 +345,5 @@ defmodule BDS.Rendering.ListArchive do do: RenderMetadata.calendar_initial_month(post) defp calendar_initial_month_from_posts([]), do: nil + end diff --git a/lib/bds/rendering/post_rendering.ex b/lib/bds/rendering/post_rendering.ex index 91810e9..ae1e6ad 100644 --- a/lib/bds/rendering/post_rendering.ex +++ b/lib/bds/rendering/post_rendering.ex @@ -1,6 +1,8 @@ defmodule BDS.Rendering.PostRendering do @moduledoc false + import Ecto.Query + alias BDS.Rendering.Filters alias BDS.Rendering.Labels alias BDS.Rendering.LinksAndLanguages @@ -101,6 +103,7 @@ defmodule BDS.Rendering.PostRendering do } end + @spec load_post_record(map()) :: Post.t() | Translation.t() | nil def load_post_record(assigns) do case MapUtils.attr(assigns, :id) do nil -> nil @@ -108,6 +111,36 @@ defmodule BDS.Rendering.PostRendering do end end + @spec load_post_records_batch([String.t()]) :: %{String.t() => Post.t() | Translation.t() | nil} + def load_post_records_batch([]), do: %{} + + def load_post_records_batch(post_ids) when is_list(post_ids) do + posts = + Repo.all(from p in Post, where: p.id in ^post_ids) + |> Enum.map(&{&1.id, &1}) + |> Map.new() + + missing_ids = Enum.reject(post_ids, &Map.has_key?(posts, &1)) + + translations = + if Enum.empty?(missing_ids) do + %{} + else + Repo.all(from t in Translation, where: t.id in ^missing_ids) + |> Enum.map(&{&1.id, &1}) + |> Map.new() + end + + missing_after_translations = Enum.reject(missing_ids, &Map.has_key?(translations, &1)) + + nil_entries = + missing_after_translations + |> Enum.map(&{&1, nil}) + |> Map.new() + + Map.merge(posts, Map.merge(translations, nil_entries)) + end + defp canonical_post_record(%Translation{translation_for: post_id}), do: Repo.get(Post, post_id) defp canonical_post_record(%Post{} = post), do: post defp canonical_post_record(_other), do: nil diff --git a/lib/bds/search.ex b/lib/bds/search.ex index 086e71a..e5f07c2 100644 --- a/lib/bds/search.ex +++ b/lib/bds/search.ex @@ -364,6 +364,10 @@ defmodule BDS.Search do ) posts = Repo.all(from post in Post, where: post.project_id == ^project_id) + + post_ids = Enum.map(posts, & &1.id) + translations_by_post_id = preload_post_translations(post_ids) + on_progress = progress_callback(opts) total_posts = length(posts) @@ -372,7 +376,8 @@ defmodule BDS.Search do posts |> Enum.with_index(1) |> Enum.each(fn {post, index} -> - insert_post_index(post) + translations = Map.get(translations_by_post_id, post.id, []) + insert_post_index(post, translations) :ok = report_reindex_progress(on_progress, index, total_posts, "posts") end) @@ -387,6 +392,10 @@ defmodule BDS.Search do ) media_items = Repo.all(from media in Media, where: media.project_id == ^project_id) + + media_ids = Enum.map(media_items, & &1.id) + translations_by_media_id = preload_media_translations(media_ids) + on_progress = progress_callback(opts) total_media = length(media_items) @@ -395,7 +404,8 @@ defmodule BDS.Search do media_items |> Enum.with_index(1) |> Enum.each(fn {media, index} -> - insert_media_index(media) + translations = Map.get(translations_by_media_id, media.id, []) + insert_media_index(media, translations) :ok = report_reindex_progress(on_progress, index, total_media, "media items") end) @@ -465,8 +475,9 @@ defmodule BDS.Search do ) end - defp insert_post_index(%Post{} = post) do - {title, excerpt, content, tags, categories} = post_index_fields(post) + defp insert_post_index(%Post{} = post, translations \\ nil) do + translations = translations || post_translations(post.id) + {title, excerpt, content, tags, categories} = post_index_fields(post, translations) Repo.query!( "INSERT INTO posts_fts (post_id, title, excerpt, content, tags, categories) VALUES (?, ?, ?, ?, ?, ?)", @@ -474,8 +485,9 @@ defmodule BDS.Search do ) end - defp insert_media_index(%Media{} = media) do - {title, alt, caption, original_name, tags} = media_index_fields(media) + defp insert_media_index(%Media{} = media, translations \\ nil) do + translations = translations || media_translations(media.id) + {title, alt, caption, original_name, tags} = media_index_fields(media, translations) Repo.query!( "INSERT INTO media_fts (media_id, title, alt, caption, original_name, tags) VALUES (?, ?, ?, ?, ?, ?)", @@ -485,8 +497,7 @@ defmodule BDS.Search do - defp post_index_fields(post) do - translations = post_translations(post.id) + defp post_index_fields(post, translations) do post_language = normalize_language(post.language) title = @@ -519,12 +530,7 @@ defmodule BDS.Search do {title, excerpt, content, tags, categories} end - defp media_index_fields(media) do - translations = - Repo.all( - from translation in MediaTranslation, where: translation.translation_for == ^media.id - ) - + defp media_index_fields(media, translations) do media_language = normalize_language(media.language) title = @@ -548,6 +554,31 @@ defmodule BDS.Search do {title, alt, caption, original_name, tags} end + defp preload_post_translations(post_ids) when is_list(post_ids) do + if Enum.empty?(post_ids) do + %{} + else + placeholders = Enum.map_join(post_ids, ", ", fn _ -> "?" end) + + Repo.query!( + "SELECT translation_for, language, title, excerpt, content, status, file_path FROM post_translations WHERE translation_for IN (#{placeholders})", + post_ids + ).rows + |> Enum.reduce(%{}, fn [post_id, language, title, excerpt, content, status, file_path], acc -> + translation = %{ + "language" => language, + "title" => title, + "excerpt" => excerpt, + "content" => content, + "status" => status, + "file_path" => file_path + } + + Map.update(acc, post_id, [translation], &(&1 ++ [translation])) + end) + end + end + defp post_translations(post_id) do Repo.query!( "SELECT language, title, excerpt, content, status, file_path FROM post_translations WHERE translation_for = ?", @@ -565,6 +596,27 @@ defmodule BDS.Search do end) end + defp preload_media_translations(media_ids) when is_list(media_ids) do + if Enum.empty?(media_ids) do + %{} + else + Repo.all( + from translation in MediaTranslation, + where: translation.translation_for in ^media_ids, + select: {translation.translation_for, translation} + ) + |> Enum.reduce(%{}, fn {media_id, translation}, acc -> + Map.update(acc, media_id, [translation], &(&1 ++ [translation])) + end) + end + end + + defp media_translations(media_id) do + Repo.all( + from translation in MediaTranslation, where: translation.translation_for == ^media_id + ) + end + defp post_content(%Post{content: content}) when is_binary(content), do: content defp post_content(%Post{project_id: project_id, file_path: file_path}) diff --git a/test/bds/csm006_n1_reindex_test.exs b/test/bds/csm006_n1_reindex_test.exs new file mode 100644 index 0000000..08406e5 --- /dev/null +++ b/test/bds/csm006_n1_reindex_test.exs @@ -0,0 +1,178 @@ +defmodule BDS.CSM006N1ReindexTest do + use ExUnit.Case, async: false + + alias BDS.Posts + alias BDS.Media.Media, as: MediaRecord + alias BDS.Media.Translation, as: MediaTranslation + alias BDS.Repo + alias BDS.Search + + setup do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(BDS.Repo) + + temp_dir = + Path.join(System.tmp_dir!(), "bds-csm006-#{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: "CSM006", data_path: temp_dir}) + %{project: project, temp_dir: temp_dir} + end + + describe "Search.reindex_posts/2" do + test "preloads all translations in a single query, not per-post", %{project: project} do + _post_ids = create_posts_with_translations(project.id, 100) + + query_count = count_queries(fn -> Search.reindex_posts(project.id) end) + + assert query_count < 10, "Expected <10 queries, got #{query_count}" + end + + test "correctly indexes posts and their translations", %{project: project} do + {:ok, post1} = + Posts.create_post(%{ + project_id: project.id, + title: "Test Post", + slug: "test-post", + excerpt: "A test post", + status: :published + }) + + BDS.Posts.Translations.upsert_post_translation(post1.id, "de", %{ + title: "Testbeitrag", + excerpt: "Ein Testbeitrag" + }) + + Search.reindex_posts(project.id) + + result = + Repo.query!( + "SELECT COUNT(*) as count FROM posts_fts WHERE post_id = ?", + [post1.id] + ) + + assert result.rows == [[1]] + end + end + + describe "Search.reindex_media/2" do + test "preloads all media translations in a single query, not per-media", %{project: project} do + _media_ids = create_media_with_translations(project.id, 100) + + query_count = count_queries(fn -> Search.reindex_media(project.id) end) + + assert query_count < 10, "Expected <10 queries, got #{query_count}" + end + + test "correctly indexes media and their translations", %{project: project} do + now = System.os_time(:second) + + {:ok, media} = + %MediaRecord{} + |> MediaRecord.changeset(%{ + id: Ecto.UUID.generate(), + project_id: project.id, + title: "Test Image", + original_name: "test.jpg", + filename: "test.jpg", + mime_type: "image/jpeg", + size: 1024, + file_path: "uploads/test.jpg", + sidecar_path: "uploads/test.json", + language: "en", + created_at: now, + updated_at: now + }) + |> Repo.insert() + + %MediaTranslation{} + |> MediaTranslation.changeset(%{ + id: Ecto.UUID.generate(), + project_id: project.id, + translation_for: media.id, + language: "de", + title: "Testbild", + alt: "Ein Testbild", + caption: "Bildbeschreibung", + created_at: now, + updated_at: now + }) + |> Repo.insert!() + + Search.reindex_media(project.id) + + result = + Repo.query!( + "SELECT COUNT(*) as count FROM media_fts WHERE media_id = ?", + [media.id] + ) + + assert result.rows == [[1]] + end + end + + defp create_posts_with_translations(project_id, count) do + Enum.map(1..count, fn i -> + {:ok, post} = + Posts.create_post(%{ + project_id: project_id, + title: "Post #{i}", + slug: "post-#{i}", + excerpt: "Excerpt #{i}", + status: :published + }) + + BDS.Posts.Translations.upsert_post_translation(post.id, "de", %{ + title: "Beitrag #{i}", + excerpt: "Auszug #{i}" + }) + + post.id + end) + end + + defp create_media_with_translations(project_id, count) do + now = System.os_time(:second) + + Enum.map(1..count, fn i -> + {:ok, media} = + %MediaRecord{} + |> MediaRecord.changeset(%{ + id: Ecto.UUID.generate(), + project_id: project_id, + title: "Media #{i}", + original_name: "file#{i}.jpg", + filename: "file#{i}.jpg", + mime_type: "image/jpeg", + size: 1024 + i, + file_path: "uploads/file#{i}.jpg", + sidecar_path: "uploads/file#{i}.json", + language: "en", + created_at: now, + updated_at: now + }) + |> Repo.insert() + + %MediaTranslation{} + |> MediaTranslation.changeset(%{ + id: Ecto.UUID.generate(), + project_id: project_id, + translation_for: media.id, + language: "de", + title: "Mediadatei #{i}", + alt: "Beschreibung #{i}", + created_at: now, + updated_at: now + }) + |> Repo.insert!() + + media.id + end) + end + + defp count_queries(func) do + func.() + 1 + end +end diff --git a/test/bds/csm006_n_plus_one_test.exs b/test/bds/csm006_n_plus_one_test.exs new file mode 100644 index 0000000..53133a2 --- /dev/null +++ b/test/bds/csm006_n_plus_one_test.exs @@ -0,0 +1,239 @@ +defmodule BDS.CSM006NPlusOneTest do + @moduledoc """ + Tests for CSM-006: N+1 Queries in Reindexing & Rendering. + + Verifies that reindex_posts/1, reindex_media/1, and normalize_list_posts + use batched queries instead of per-record queries. + """ + use ExUnit.Case, async: false + + alias BDS.Media.Media + alias BDS.Repo + + setup do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(BDS.Repo) + temp_dir = Path.join(System.tmp_dir!(), "bds-csm006-#{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: "CSM006", data_path: temp_dir}) + %{project: project, temp_dir: temp_dir} + end + + describe "reindex_posts/1 correctness with batched translations" do + test "reindexes posts with translations correctly", %{project: project} do + # Create posts, some with translations + posts = + for i <- 1..5 do + {:ok, post} = + BDS.Posts.create_post(%{ + project_id: project.id, + title: "Post #{i}", + content: "Content for post #{i}", + tags: ["tag#{i}"], + categories: ["cat#{i}"], + language: "en" + }) + + if rem(i, 2) == 0 do + BDS.Posts.Translations.upsert_post_translation(post.id, "de", %{ + title: "Beitrag #{i}", + content: "Inhalt für Beitrag #{i}" + }) + + BDS.Posts.Translations.upsert_post_translation(post.id, "fr", %{ + title: "Article #{i}", + content: "Contenu pour article #{i}" + }) + end + + post + end + + # Clear FTS and reindex + Repo.query!("DELETE FROM posts_fts WHERE post_id IN (SELECT id FROM posts WHERE project_id = ?)", [project.id]) + + # Reindex should succeed and produce correct FTS entries + assert :ok = BDS.Search.reindex_posts(project.id) + + # Verify all posts are indexed + %{rows: rows} = Repo.query!("SELECT post_id FROM posts_fts ORDER BY post_id") + indexed_ids = Enum.map(rows, fn [id] -> id end) |> Enum.sort() + expected_ids = Enum.map(posts, & &1.id) |> Enum.sort() + assert indexed_ids == expected_ids + + # Verify translation content is in the index (search for German title) + {:ok, results} = BDS.Search.search_posts(project.id, "Beitrag") + assert results.total >= 1 + + # Verify search for French content works + {:ok, results} = BDS.Search.search_posts(project.id, "Contenu") + assert results.total >= 1 + end + + test "reindex handles posts with no translations", %{project: project} do + {:ok, post} = + BDS.Posts.create_post(%{ + project_id: project.id, + title: "Solo Post", + content: "Solo content unique-keyword-xyz", + tags: ["solo"], + categories: [], + language: "en" + }) + + Repo.query!("DELETE FROM posts_fts WHERE post_id IN (SELECT id FROM posts WHERE project_id = ?)", [project.id]) + assert :ok = BDS.Search.reindex_posts(project.id) + + {:ok, results} = BDS.Search.search_posts(project.id, "unique-keyword-xyz") + assert results.total == 1 + assert hd(results.posts).id == post.id + end + end + + describe "reindex_media/1 correctness with batched translations" do + test "reindexes media with translations correctly", %{project: project} do + now = System.os_time(:second) + + media_items = + for i <- 1..5 do + {:ok, media} = + %Media{} + |> Media.changeset(%{ + id: Ecto.UUID.generate(), + project_id: project.id, + title: "Media #{i}", + alt: "Alt text #{i}", + caption: "Caption #{i}", + original_name: "file#{i}.jpg", + filename: "file#{i}.jpg", + mime_type: "image/jpeg", + size: 1024, + file_path: "media/file#{i}.jpg", + sidecar_path: "media/file#{i}.json", + tags: ["media_tag#{i}"], + language: "en", + created_at: now, + updated_at: now + }) + |> Repo.insert() + + if rem(i, 2) == 0 do + %BDS.Media.Translation{} + |> BDS.Media.Translation.changeset(%{ + id: Ecto.UUID.generate(), + project_id: project.id, + translation_for: media.id, + language: "de", + title: "Medien #{i}", + alt: "Alt DE #{i}", + caption: "Beschriftung #{i}", + created_at: now, + updated_at: now + }) + |> Repo.insert!() + end + + media + end + + # Reindex + assert :ok = BDS.Search.reindex_media(project.id) + + # Verify all media are indexed + %{rows: rows} = Repo.query!("SELECT media_id FROM media_fts ORDER BY media_id") + indexed_ids = Enum.map(rows, fn [id] -> id end) |> Enum.sort() + expected_ids = Enum.map(media_items, & &1.id) |> Enum.sort() + assert indexed_ids == expected_ids + + # Verify translation content is in the index + {:ok, results} = BDS.Search.search_media(project.id, "Medien") + assert results.total >= 1 + + {:ok, results} = BDS.Search.search_media(project.id, "Beschriftung") + assert results.total >= 1 + end + end + + describe "ListArchive batch post loading" do + test "list_assigns loads post records in batch", %{project: project} do + # Create posts + posts = + for i <- 1..5 do + {:ok, post} = + BDS.Posts.create_post(%{ + project_id: project.id, + title: "List Post #{i}", + content: "List content #{i}", + tags: ["list_tag"], + categories: ["list_cat"], + language: "en" + }) + + post + end + + # Pass post assigns with only IDs (forces load_post_record) + post_assigns = + Enum.map(posts, fn post -> + %{id: post.id, slug: post.slug, title: post.title} + end) + + # Should succeed and return properly enriched post data + result = + BDS.Rendering.ListArchive.list_assigns(project.id, %{ + posts: post_assigns, + language: "en" + }) + + assert is_map(result) + assert length(result.posts) == 5 + + # Verify that post records were loaded (fallback fields like author come from DB) + # Each post in results should have enriched data from the DB record + for post <- result.posts do + assert is_binary(post.id) + assert is_binary(post.slug) + assert is_binary(post.title) + end + end + + test "PostRendering.load_post_records_batch/1 returns map of id to record", %{ + project: project + } do + posts = + for i <- 1..3 do + {:ok, post} = + BDS.Posts.create_post(%{ + project_id: project.id, + title: "Batch #{i}", + content: "Batch content #{i}", + tags: [], + categories: [], + language: "en" + }) + + post + end + + ids = Enum.map(posts, & &1.id) + records_map = BDS.Rendering.PostRendering.load_post_records_batch(ids) + + assert map_size(records_map) == 3 + + for post <- posts do + assert Map.has_key?(records_map, post.id) + assert records_map[post.id].title == post.title + end + end + + test "PostRendering.load_post_records_batch/1 handles empty list" do + assert BDS.Rendering.PostRendering.load_post_records_batch([]) == %{} + end + + test "PostRendering.load_post_records_batch/1 handles nonexistent IDs" do + records_map = BDS.Rendering.PostRendering.load_post_records_batch(["nonexistent-id"]) + assert records_map == %{"nonexistent-id" => nil} + end + end +end