trying something new with queries (#142)

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)
 

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
+    MediaQuery.new()
-    |> where([mi], mi.source_id == ^source.id and is_nil(mi.media_filepath))
+    |> MediaQuery.for_source(source)
-    |> where(^build_format_clauses(media_profile))
+    |> matching_download_criteria_for(source)
-    |> where(^maybe_apply_cutoff_date(source))
-    |> where(^maybe_apply_title_regex(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
+    MediaQuery.new()
-    |> where([mi], mi.source_id == ^source.id and not is_nil(mi.media_filepath))
+    |> 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
+    MediaQuery.new()
-    |> where([mi], mi.id == ^media_item.id and is_nil(mi.media_filepath))
+    |> MediaQuery.with_id(media_item.id)
-    |> where(^build_format_clauses(media_item.source.media_profile))
+    |> matching_download_criteria_for(media_item.source)
-    |> where(^maybe_apply_cutoff_date(media_item.source))
-    |> where(^maybe_apply_title_regex(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,
+    MediaQuery.new()
-      join: mi_search_index in assoc(mi, :media_items_search_index),
+    |> MediaQuery.matching_search_term(search_term)
-      where: fragment("media_items_search_index MATCH ?", ^search_term),
+    |> Repo.maybe_limit(limit)
-      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
-    )
     |> 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
+  defp matching_download_criteria_for(query, source_with_preloads) do
-    if source.download_cutoff_date do
+    query
-      dynamic([mi], mi.upload_date >= ^source.download_cutoff_date)
+    |> MediaQuery.with_no_media_filepath()
-    else
+    |> MediaQuery.with_upload_date_after(source_with_preloads.download_cutoff_date)
-      dynamic(true)
+    |> MediaQuery.with_format_preference(source_with_preloads.media_profile)
-    end
+    |> MediaQuery.matching_title_regex(source_with_preloads.title_filter_regex)
-  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)
   end
 end

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

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
+    MediaQuery.new()
-    |> Media.list_media_items_for()
+    |> MediaQuery.for_source(source)
+    |> Repo.all()
     |> Enum.each(fn media_item ->
       Media.delete_media_item(media_item, delete_files: delete_files)
     end)

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(
+    MediaQuery.new()
-      m in MediaItem,
+    |> MediaQuery.for_source(source)
-      where: m.source_id == ^source.id,
+    |> MediaQuery.with_media_filepath()
-      where: not is_nil(m.media_filepath)
-    )
     |> Repo.aggregate(:count, :id)
   end
 

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()