[Bugfix] Ensure source metadata fetching uses windows-compatible filepaths (#193)

* Fixed underlying bug

* Refactored windows_filenames to be a global flag; added tests

* Added some random test coverage for fun

lib/pinchflat/downloading/download_option_builder.ex

@@ -44,7 +44,6 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do
   defp default_options do
     [
       :no_progress,
-      :windows_filenames,
       # Add force-overwrites to make sure redownloading works
       :force_overwrites,
       # This makes the date metadata conform to what jellyfin expects

lib/pinchflat/notifications/command_runner.ex

@@ -28,8 +28,7 @@ defmodule Pinchflat.Notifications.CommandRunner do
     default_opts = [:verbose]
     parsed_opts = CliUtils.parse_options(default_opts ++ command_opts)
 
-    Logger.info("[apprise] called with: #{Enum.join(parsed_opts ++ endpoints, " ")}")
+    {output, return_code} = CliUtils.wrap_cmd(backend_executable(), parsed_opts ++ endpoints)
-    {output, return_code} = System.cmd(backend_executable(), parsed_opts ++ endpoints)
     Logger.info("[apprise] response: #{output}")
 
     case return_code do

lib/pinchflat/utils/cli_utils.ex

@@ -3,6 +3,8 @@ defmodule Pinchflat.Utils.CliUtils do
   Utility methods for working with CLI executables
   """
 
+  require Logger
+
   alias Pinchflat.Utils.StringUtils
 
   @doc """
@@ -19,6 +21,8 @@ defmodule Pinchflat.Utils.CliUtils do
     wrapper_command = Path.join(:code.priv_dir(:pinchflat), "cmd_wrapper.sh")
     actual_command = [command] ++ args
 
+    Logger.info("[command_wrapper]: #{command} called with: #{Enum.join(args, " ")}")
+
     System.cmd(wrapper_command, actual_command, opts)
   end
 

lib/pinchflat/yt_dlp/command_runner.ex

@@ -3,8 +3,6 @@ defmodule Pinchflat.YtDlp.CommandRunner do
   Runs yt-dlp commands using the `System.cmd/3` function
   """
 
-  require Logger
-
   alias Pinchflat.Utils.CliUtils
   alias Pinchflat.YtDlp.YtDlpCommandRunner
   alias Pinchflat.Utils.FilesystemUtils, as: FSUtils
@@ -30,12 +28,10 @@ defmodule Pinchflat.YtDlp.CommandRunner do
 
     output_filepath = generate_output_filepath(addl_opts)
     print_to_file_opts = [{:print_to_file, output_template}, output_filepath]
-    external_file_opts = build_external_file_options()
+    user_configured_opts = cookie_file_options() ++ global_options()
     # These must stay in exactly this order, hence why I'm giving it its own variable.
-    all_opts = command_opts ++ print_to_file_opts ++ external_file_opts
+    all_opts = command_opts ++ print_to_file_opts ++ user_configured_opts
-
     formatted_command_opts = [url] ++ CliUtils.parse_options(all_opts)
-    Logger.info("[yt-dlp] called with: #{Enum.join(formatted_command_opts, " ")}")
 
     case CliUtils.wrap_cmd(command, formatted_command_opts, stderr_to_stdout: true) do
       {_, 0} ->
@@ -58,7 +54,7 @@ defmodule Pinchflat.YtDlp.CommandRunner do
   def version do
     command = backend_executable()
 
-    case System.cmd(command, ["--version"]) do
+    case CliUtils.wrap_cmd(command, ["--version"]) do
       {output, 0} ->
         {:ok, String.trim(output)}
 
@@ -74,7 +70,11 @@ defmodule Pinchflat.YtDlp.CommandRunner do
     end
   end
 
-  defp build_external_file_options do
+  defp global_options do
+    [:windows_filenames]
+  end
+
+  defp cookie_file_options do
     base_dir = Application.get_env(:pinchflat, :extras_directory)
     filename_options_map = %{cookies: "cookies.txt"}
 

lib/pinchflat/yt_dlp/media.ex

@@ -63,8 +63,8 @@ defmodule Pinchflat.YtDlp.Media do
         |> response_to_struct()
         |> FunctionUtils.wrap_ok()
 
-      res ->
+      err ->
-        res
+        err
     end
   end
 

lib/pinchflat/yt_dlp/media_collection.ex

@@ -50,8 +50,8 @@ defmodule Pinchflat.YtDlp.MediaCollection do
 
         {:ok, Enum.filter(parsed_lines, &(&1 != nil))}
 
-      res ->
+      err ->
-        res
+        err
     end
   end
 
@@ -68,7 +68,14 @@ defmodule Pinchflat.YtDlp.MediaCollection do
     # `ignore_no_formats_error` is necessary because yt-dlp will error out if
     # the first video has not released yet (ie: is a premier). We don't care about
     # available formats since we're just getting the source details
-    command_opts = [:simulate, :skip_download, :ignore_no_formats_error, playlist_end: 1] ++ addl_opts
+    default_opts = [
+      :simulate,
+      :skip_download,
+      :ignore_no_formats_error,
+      playlist_end: 1
+    ]
+
+    command_opts = default_opts ++ addl_opts
     output_template = "%(.{channel,channel_id,playlist_id,playlist_title,filename})j"
 
     with {:ok, output} <- backend_runner().run(source_url, command_opts, output_template),

test/pinchflat/downloading/download_option_builder_test.exs

@@ -53,7 +53,6 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilderTest do
       assert {:ok, res} = DownloadOptionBuilder.build(media_item)
 
       assert :no_progress in res
-      assert :windows_filenames in res
       assert :force_overwrites in res
       assert {:parse_metadata, "%(upload_date>%Y-%m-%d)s:(?P<meta_date>.+)"} in res
     end

test/pinchflat/fast_indexing/fast_indexing_helpers_test.exs

@@ -89,5 +89,25 @@ defmodule Pinchflat.FastIndexing.FastIndexingHelpersTest do
 
       refute_enqueued(worker: MediaDownloadWorker)
     end
+
+    test "does not blow up if a media item cannot be created", %{source: source} do
+      expect(HTTPClientMock, :get, fn _url -> {:ok, "<yt:videoId>test_1</yt:videoId>"} end)
+
+      stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot ->
+        {:ok, "{}"}
+      end)
+
+      assert [] = FastIndexingHelpers.kickoff_download_tasks_from_youtube_rss_feed(source)
+    end
+
+    test "does not blow up if a media item causes a yt-dlp error", %{source: source} do
+      expect(HTTPClientMock, :get, fn _url -> {:ok, "<yt:videoId>test_1</yt:videoId>"} end)
+
+      stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot ->
+        {:error, "message", 1}
+      end)
+
+      assert [] = FastIndexingHelpers.kickoff_download_tasks_from_youtube_rss_feed(source)
+    end
   end
 end

test/pinchflat/sources_test.exs

@@ -131,6 +131,18 @@ defmodule Pinchflat.SourcesTest do
       assert String.starts_with?(source.collection_id, "some_playlist_id_")
     end
 
+    test "adds an error if the runner fails" do
+      expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot -> {:error, "some error", 1} end)
+
+      valid_attrs = %{
+        media_profile_id: media_profile_fixture().id,
+        original_url: "https://www.youtube.com/channel/abc123"
+      }
+
+      assert {:error, %Ecto.Changeset{} = changeset} = Sources.create_source(valid_attrs)
+      assert "could not fetch source details from URL" in errors_on(changeset).original_url
+    end
+
     test "you can specify a custom custom_name" do
       expect(YtDlpRunnerMock, :run, &channel_mock/3)
 

test/pinchflat/yt_dlp/command_runner_test.exs

@@ -78,6 +78,14 @@ defmodule Pinchflat.YtDlp.CommandRunnerTest do
     end
   end
 
+  describe "run/4 when testing global options" do
+    test "creates windows-safe filenames" do
+      assert {:ok, output} = Runner.run(@media_url, [], "")
+
+      assert String.contains?(output, "--windows-filenames")
+    end
+  end
+
   describe "version/0" do
     test "adds the version arg" do
       assert {:ok, output} = Runner.version()