From 216e05567fd8ecff3ab8852d810c65ea642b300b Mon Sep 17 00:00:00 2001 From: Kieran Date: Thu, 28 Mar 2024 17:59:53 -0700 Subject: [PATCH] trying something new with queries (#142) --- .../fast_indexing/fast_indexing_helpers.ex | 11 +- lib/pinchflat/media/media.ex | 123 +++--------------- lib/pinchflat/media/media_query.ex | 120 +++++++++++++++++ lib/pinchflat/sources/sources.ex | 6 +- .../controllers/sources/source_controller.ex | 10 +- test/pinchflat/media_test.exs | 26 ---- 6 files changed, 158 insertions(+), 138 deletions(-) create mode 100644 lib/pinchflat/media/media_query.ex diff --git a/lib/pinchflat/fast_indexing/fast_indexing_helpers.ex b/lib/pinchflat/fast_indexing/fast_indexing_helpers.ex index d1d72da..a38ccf7 100644 --- a/lib/pinchflat/fast_indexing/fast_indexing_helpers.ex +++ b/lib/pinchflat/fast_indexing/fast_indexing_helpers.ex @@ -5,8 +5,10 @@ defmodule Pinchflat.FastIndexing.FastIndexingHelpers do Many of these methods are made to be kickoff or be consumed by workers. """ + alias Pinchflat.Repo alias Pinchflat.Media alias Pinchflat.Sources.Source + alias Pinchflat.Media.MediaQuery alias Pinchflat.FastIndexing.YoutubeRss alias Pinchflat.Downloading.MediaDownloadWorker alias Pinchflat.FastIndexing.MediaIndexingWorker @@ -27,7 +29,7 @@ defmodule Pinchflat.FastIndexing.FastIndexingHelpers do """ def kickoff_indexing_tasks_from_youtube_rss_feed(%Source{} = source) do {:ok, media_ids} = YoutubeRss.get_recent_media_ids_from_rss(source) - existing_media_items = Media.list_media_items_by_media_id_for(source, media_ids) + existing_media_items = list_media_items_by_media_id_for(source, media_ids) new_media_ids = media_ids -- Enum.map(existing_media_items, & &1.media_id) Enum.each(new_media_ids, fn media_id -> @@ -61,6 +63,13 @@ defmodule Pinchflat.FastIndexing.FastIndexingHelpers do end end + defp list_media_items_by_media_id_for(source, media_ids) do + MediaQuery.new() + |> MediaQuery.for_source(source) + |> MediaQuery.with_media_ids(media_ids) + |> Repo.all() + end + defp create_media_item_from_url(source, url) do {:ok, media_attrs} = YtDlpMedia.get_media_attributes(url) diff --git a/lib/pinchflat/media/media.ex b/lib/pinchflat/media/media.ex index e6be6de..346029b 100644 --- a/lib/pinchflat/media/media.ex +++ b/lib/pinchflat/media/media.ex @@ -9,6 +9,7 @@ defmodule Pinchflat.Media do alias Pinchflat.Tasks alias Pinchflat.Sources.Source alias Pinchflat.Media.MediaItem + alias Pinchflat.Media.MediaQuery alias Pinchflat.Metadata.MediaMetadata alias Pinchflat.Filesystem.FilesystemHelpers @@ -21,33 +22,6 @@ defmodule Pinchflat.Media do Repo.all(MediaItem) end - @doc """ - Returns a list of media_items for a given source. - - Returns [%MediaItem{}, ...]. - """ - def list_media_items_for(%Source{} = source) do - MediaItem - |> where([mi], mi.source_id == ^source.id) - |> Repo.all() - end - - @doc """ - Fetches all media items belonging to a given source that have a media_id in the given list. - Useful for determining the what media items we DON'T already have for fast indexing. - - NOTE: These queries are getting a little tedious. When I have the time, I should see about - implementing a query pattern and having these compose queries from a common base. This would - also let me compose simple queries in the module using them for one-off methods - - Returns [%MediaItem{}, ...]. - """ - def list_media_items_by_media_id_for(%Source{} = source, media_ids) do - MediaItem - |> where([mi], mi.source_id == ^source.id and mi.media_id in ^media_ids) - |> Repo.all() - end - @doc """ Returns a list of pending media_items for a given source, where pending means the `media_filepath` is `nil` AND the media_item @@ -60,13 +34,11 @@ defmodule Pinchflat.Media do """ def list_pending_media_items_for(%Source{} = source, opts \\ []) do limit = Keyword.get(opts, :limit, nil) - media_profile = Repo.preload(source, :media_profile).media_profile + source = Repo.preload(source, :media_profile) - MediaItem - |> where([mi], mi.source_id == ^source.id and is_nil(mi.media_filepath)) - |> where(^build_format_clauses(media_profile)) - |> where(^maybe_apply_cutoff_date(source)) - |> where(^maybe_apply_title_regex(source)) + MediaQuery.new() + |> MediaQuery.for_source(source) + |> matching_download_criteria_for(source) |> Repo.maybe_limit(limit) |> Repo.all() end @@ -79,8 +51,9 @@ defmodule Pinchflat.Media do def list_downloaded_media_items_for(%Source{} = source, opts \\ []) do limit = Keyword.get(opts, :limit, nil) - MediaItem - |> where([mi], mi.source_id == ^source.id and not is_nil(mi.media_filepath)) + MediaQuery.new() + |> MediaQuery.for_source(source) + |> MediaQuery.with_media_filepath() |> Repo.maybe_limit(limit) |> Repo.all() end @@ -97,11 +70,9 @@ defmodule Pinchflat.Media do def pending_download?(%MediaItem{} = media_item) do media_item = Repo.preload(media_item, source: :media_profile) - MediaItem - |> where([mi], mi.id == ^media_item.id and is_nil(mi.media_filepath)) - |> where(^build_format_clauses(media_item.source.media_profile)) - |> where(^maybe_apply_cutoff_date(media_item.source)) - |> where(^maybe_apply_title_regex(media_item.source)) + MediaQuery.new() + |> MediaQuery.with_id(media_item.id) + |> matching_download_criteria_for(media_item.source) |> Repo.exists?() end @@ -120,20 +91,9 @@ defmodule Pinchflat.Media do def search(search_term, opts) do limit = Keyword.get(opts, :limit, 50) - from(mi in MediaItem, - join: mi_search_index in assoc(mi, :media_items_search_index), - where: fragment("media_items_search_index MATCH ?", ^search_term), - select_merge: %{ - matching_search_term: - fragment(""" - coalesce(snippet(media_items_search_index, 0, '[PF_HIGHLIGHT]', '[/PF_HIGHLIGHT]', '...', 20), '') || - ' ' || - coalesce(snippet(media_items_search_index, 1, '[PF_HIGHLIGHT]', '[/PF_HIGHLIGHT]', '...', 20), '') - """) - }, - order_by: [desc: fragment("rank")], - limit: ^limit - ) + MediaQuery.new() + |> MediaQuery.matching_search_term(search_term) + |> Repo.maybe_limit(limit) |> Repo.all() end @@ -241,54 +201,11 @@ defmodule Pinchflat.Media do |> Enum.each(&FilesystemHelpers.delete_file_and_remove_empty_directories/1) end - defp maybe_apply_cutoff_date(source) do - if source.download_cutoff_date do - dynamic([mi], mi.upload_date >= ^source.download_cutoff_date) - else - dynamic(true) - end - end - - defp maybe_apply_title_regex(source) do - if source.title_filter_regex do - dynamic([mi], fragment("regexp_like(?, ?)", mi.title, ^source.title_filter_regex)) - else - dynamic(true) - end - end - - defp build_format_clauses(media_profile) do - mapped_struct = Map.from_struct(media_profile) - - Enum.reduce(mapped_struct, dynamic(true), fn attr, dynamic -> - case {attr, media_profile} do - {{:shorts_behaviour, :only}, %{livestream_behaviour: :only}} -> - dynamic( - [mi], - ^dynamic and (mi.livestream == true or mi.short_form_content == true) - ) - - # Technically redundant, but makes the other clauses easier to parse - # (redundant because this condition is the same as the condition above, just flipped) - {{:livestream_behaviour, :only}, %{shorts_behaviour: :only}} -> - dynamic - - {{:shorts_behaviour, :only}, _} -> - dynamic([mi], ^dynamic and mi.short_form_content == true) - - {{:livestream_behaviour, :only}, _} -> - dynamic([mi], ^dynamic and mi.livestream == true) - - {{:shorts_behaviour, :exclude}, %{livestream_behaviour: lb}} when lb != :only -> - dynamic([mi], ^dynamic and mi.short_form_content == false) - - {{:livestream_behaviour, :exclude}, %{shorts_behaviour: sb}} when sb != :only -> - # return records with livestream: false - dynamic([mi], ^dynamic and mi.livestream == false) - - _ -> - dynamic - end - end) + defp matching_download_criteria_for(query, source_with_preloads) do + query + |> MediaQuery.with_no_media_filepath() + |> MediaQuery.with_upload_date_after(source_with_preloads.download_cutoff_date) + |> MediaQuery.with_format_preference(source_with_preloads.media_profile) + |> MediaQuery.matching_title_regex(source_with_preloads.title_filter_regex) end end diff --git a/lib/pinchflat/media/media_query.ex b/lib/pinchflat/media/media_query.ex new file mode 100644 index 0000000..c43b5eb --- /dev/null +++ b/lib/pinchflat/media/media_query.ex @@ -0,0 +1,120 @@ +defmodule Pinchflat.Media.MediaQuery do + @moduledoc """ + Query helpers for the Media context. + + These methods are made to be one-ish liners used + to compose queries for media items. Each method should + strive to do _one_ thing. These don't need to be tested + as they are just building blocks for other functionality + which, itself, will be tested. + + ALSO, this is me trying something new. If I like it, + I'll refactor other contexts to use this pattern. + """ + import Ecto.Query, warn: false + + alias Pinchflat.Media.MediaItem + + # Prefixes: + # - for_* - belonging to a certain record + # - with_* - for filtering based on full, concrete attributes + # - matching_* - for filtering based on partial attributes (e.g. LIKE, regex, full-text search) + # + # Suffixes: + # - _for - the arg passed is an association record + + def new do + MediaItem + end + + def for_source(query, source) do + where(query, [mi], mi.source_id == ^source.id) + end + + def with_id(query, id) do + where(query, [mi], mi.id == ^id) + end + + def with_media_ids(query, media_ids) do + where(query, [mi], mi.media_id in ^media_ids) + end + + def with_media_filepath(query) do + where(query, [mi], not is_nil(mi.media_filepath)) + end + + def with_no_media_filepath(query) do + where(query, [mi], is_nil(mi.media_filepath)) + end + + def with_upload_date_after(query, nil), do: query + + def with_upload_date_after(query, date) do + where(query, [mi], mi.upload_date >= ^date) + end + + def matching_title_regex(query, nil), do: query + + def matching_title_regex(query, regex) do + where(query, [mi], fragment("regexp_like(?, ?)", mi.title, ^regex)) + end + + def matching_search_term(query, nil), do: query + + def matching_search_term(query, term) do + from(mi in query, + join: mi_search_index in assoc(mi, :media_items_search_index), + where: fragment("media_items_search_index MATCH ?", ^term), + select_merge: %{ + matching_search_term: + fragment(""" + coalesce(snippet(media_items_search_index, 0, '[PF_HIGHLIGHT]', '[/PF_HIGHLIGHT]', '...', 20), '') || + ' ' || + coalesce(snippet(media_items_search_index, 1, '[PF_HIGHLIGHT]', '[/PF_HIGHLIGHT]', '...', 20), '') + """) + }, + order_by: [desc: fragment("rank")] + ) + end + + # NOTE: this method breaks the contract set by other methods in that it + # takes a media_profile struct instead of taking just the attributes it + # cares about. Consider refactoring but low priority. + def with_format_preference(query, media_profile) do + mapped_struct = Map.from_struct(media_profile) + + finders = + Enum.reduce(mapped_struct, dynamic(true), fn attr, dynamic -> + case {attr, media_profile} do + {{:shorts_behaviour, :only}, %{livestream_behaviour: :only}} -> + dynamic( + [mi], + ^dynamic and (mi.livestream == true or mi.short_form_content == true) + ) + + # Technically redundant, but makes the other clauses easier to parse + # (redundant because this condition is the same as the condition above, just flipped) + {{:livestream_behaviour, :only}, %{shorts_behaviour: :only}} -> + dynamic + + {{:shorts_behaviour, :only}, _} -> + dynamic([mi], ^dynamic and mi.short_form_content == true) + + {{:livestream_behaviour, :only}, _} -> + dynamic([mi], ^dynamic and mi.livestream == true) + + {{:shorts_behaviour, :exclude}, %{livestream_behaviour: lb}} when lb != :only -> + dynamic([mi], ^dynamic and mi.short_form_content == false) + + {{:livestream_behaviour, :exclude}, %{shorts_behaviour: sb}} when sb != :only -> + # return records with livestream: false + dynamic([mi], ^dynamic and mi.livestream == false) + + _ -> + dynamic + end + end) + + where(query, ^finders) + end +end diff --git a/lib/pinchflat/sources/sources.ex b/lib/pinchflat/sources/sources.ex index 0920217..a65d75b 100644 --- a/lib/pinchflat/sources/sources.ex +++ b/lib/pinchflat/sources/sources.ex @@ -9,6 +9,7 @@ defmodule Pinchflat.Sources do alias Pinchflat.Media alias Pinchflat.Tasks alias Pinchflat.Sources.Source + alias Pinchflat.Media.MediaQuery alias Pinchflat.Profiles.MediaProfile alias Pinchflat.YtDlp.MediaCollection alias Pinchflat.Metadata.SourceMetadata @@ -112,8 +113,9 @@ defmodule Pinchflat.Sources do delete_files = Keyword.get(opts, :delete_files, false) Tasks.delete_tasks_for(source) - source - |> Media.list_media_items_for() + MediaQuery.new() + |> MediaQuery.for_source(source) + |> Repo.all() |> Enum.each(fn media_item -> Media.delete_media_item(media_item, delete_files: delete_files) end) diff --git a/lib/pinchflat_web/controllers/sources/source_controller.ex b/lib/pinchflat_web/controllers/sources/source_controller.ex index fab66b3..407f35c 100644 --- a/lib/pinchflat_web/controllers/sources/source_controller.ex +++ b/lib/pinchflat_web/controllers/sources/source_controller.ex @@ -9,7 +9,7 @@ defmodule PinchflatWeb.Sources.SourceController do alias Pinchflat.Sources alias Pinchflat.Profiles alias Pinchflat.Sources.Source - alias Pinchflat.Media.MediaItem + alias Pinchflat.Media.MediaQuery def index(conn, _params) do sources = Repo.preload(Sources.list_sources(), :media_profile) @@ -111,11 +111,9 @@ defmodule PinchflatWeb.Sources.SourceController do # NOTE: should move this out of the controller # once I finally add some query fragment layer defp total_downloaded_for(source) do - from( - m in MediaItem, - where: m.source_id == ^source.id, - where: not is_nil(m.media_filepath) - ) + MediaQuery.new() + |> MediaQuery.for_source(source) + |> MediaQuery.with_media_filepath() |> Repo.aggregate(:count, :id) end diff --git a/test/pinchflat/media_test.exs b/test/pinchflat/media_test.exs index dc1b4fe..c73c421 100644 --- a/test/pinchflat/media_test.exs +++ b/test/pinchflat/media_test.exs @@ -38,32 +38,6 @@ defmodule Pinchflat.MediaTest do end end - describe "list_media_items_for/1" do - test "it returns media_items for a given source" do - source = source_fixture() - media_item = media_item_fixture(%{source_id: source.id}) - - assert Media.list_media_items_for(source) == [media_item] - end - end - - describe "list_media_items_by_media_id_for/2" do - test "returns media_items for a given source and media_ids" do - source = source_fixture() - media_item = media_item_fixture(%{source_id: source.id, media_id: "123"}) - - assert Media.list_media_items_by_media_id_for(source, ["123"]) == [media_item] - end - - test "does not return matching media_ids for a different source" do - source = source_fixture() - other_source = source_fixture() - _media_item = media_item_fixture(%{source_id: other_source.id, media_id: "123"}) - - assert Media.list_media_items_by_media_id_for(source, ["123"]) == [] - end - end - describe "list_pending_media_items_for/1" do test "it returns pending without a filepath for a given source" do source = source_fixture()