From 88569640c9046bb68326400e1f8e510516a91ad5 Mon Sep 17 00:00:00 2001 From: Kieran Date: Wed, 17 Apr 2024 12:51:52 -0700 Subject: [PATCH] [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 --- .../downloading/download_option_builder.ex | 1 - lib/pinchflat/notifications/command_runner.ex | 3 +-- lib/pinchflat/utils/cli_utils.ex | 4 ++++ lib/pinchflat/yt_dlp/command_runner.ex | 16 +++++++-------- lib/pinchflat/yt_dlp/media.ex | 4 ++-- lib/pinchflat/yt_dlp/media_collection.ex | 13 +++++++++--- .../download_option_builder_test.exs | 1 - .../fast_indexing_helpers_test.exs | 20 +++++++++++++++++++ test/pinchflat/sources_test.exs | 12 +++++++++++ test/pinchflat/yt_dlp/command_runner_test.exs | 8 ++++++++ 10 files changed, 65 insertions(+), 17 deletions(-) diff --git a/lib/pinchflat/downloading/download_option_builder.ex b/lib/pinchflat/downloading/download_option_builder.ex index 5dc1bb27..57802948 100644 --- a/lib/pinchflat/downloading/download_option_builder.ex +++ b/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 diff --git a/lib/pinchflat/notifications/command_runner.ex b/lib/pinchflat/notifications/command_runner.ex index 5f51823e..782b1a9a 100644 --- a/lib/pinchflat/notifications/command_runner.ex +++ b/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} = System.cmd(backend_executable(), parsed_opts ++ endpoints) + {output, return_code} = CliUtils.wrap_cmd(backend_executable(), parsed_opts ++ endpoints) Logger.info("[apprise] response: #{output}") case return_code do diff --git a/lib/pinchflat/utils/cli_utils.ex b/lib/pinchflat/utils/cli_utils.ex index 8d6c1d42..1426a1ba 100644 --- a/lib/pinchflat/utils/cli_utils.ex +++ b/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 diff --git a/lib/pinchflat/yt_dlp/command_runner.ex b/lib/pinchflat/yt_dlp/command_runner.ex index 572c3ac3..7a03df79 100644 --- a/lib/pinchflat/yt_dlp/command_runner.ex +++ b/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"} diff --git a/lib/pinchflat/yt_dlp/media.ex b/lib/pinchflat/yt_dlp/media.ex index c6b03faa..7a6c1c51 100644 --- a/lib/pinchflat/yt_dlp/media.ex +++ b/lib/pinchflat/yt_dlp/media.ex @@ -63,8 +63,8 @@ defmodule Pinchflat.YtDlp.Media do |> response_to_struct() |> FunctionUtils.wrap_ok() - res -> - res + err -> + err end end diff --git a/lib/pinchflat/yt_dlp/media_collection.ex b/lib/pinchflat/yt_dlp/media_collection.ex index a0b33287..d4b99a10 100644 --- a/lib/pinchflat/yt_dlp/media_collection.ex +++ b/lib/pinchflat/yt_dlp/media_collection.ex @@ -50,8 +50,8 @@ defmodule Pinchflat.YtDlp.MediaCollection do {:ok, Enum.filter(parsed_lines, &(&1 != nil))} - res -> - res + err -> + 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), diff --git a/test/pinchflat/downloading/download_option_builder_test.exs b/test/pinchflat/downloading/download_option_builder_test.exs index 8ce35a2d..8008b7f4 100644 --- a/test/pinchflat/downloading/download_option_builder_test.exs +++ b/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.+)"} in res end diff --git a/test/pinchflat/fast_indexing/fast_indexing_helpers_test.exs b/test/pinchflat/fast_indexing/fast_indexing_helpers_test.exs index f065b7e3..de51abb6 100644 --- a/test/pinchflat/fast_indexing/fast_indexing_helpers_test.exs +++ b/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, "test_1"} 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, "test_1"} 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 diff --git a/test/pinchflat/sources_test.exs b/test/pinchflat/sources_test.exs index d6589ff6..3e4a1da4 100644 --- a/test/pinchflat/sources_test.exs +++ b/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) diff --git a/test/pinchflat/yt_dlp/command_runner_test.exs b/test/pinchflat/yt_dlp/command_runner_test.exs index 17cf61c2..f4c93092 100644 --- a/test/pinchflat/yt_dlp/command_runner_test.exs +++ b/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()