diff --git a/lib/pinchflat/downloading/download_option_builder.ex b/lib/pinchflat/downloading/download_option_builder.ex index 5780294..a9eeba5 100644 --- a/lib/pinchflat/downloading/download_option_builder.ex +++ b/lib/pinchflat/downloading/download_option_builder.ex @@ -8,6 +8,8 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do alias Pinchflat.Media.MediaItem alias Pinchflat.Downloading.OutputPathBuilder + alias Pinchflat.Utils.FilesystemUtils, as: FSUtils + @doc """ Builds the options for yt-dlp to download media based on the given media's profile. @@ -154,12 +156,10 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do Enum.reduce(filenames, [], fn filename, acc -> filepath = Path.join(base_dir, filename) - case File.read(filepath) do - {:ok, file_data} -> - if String.trim(file_data) != "", do: [filepath | acc], else: acc - - {:error, _} -> - acc + if FSUtils.exists_and_nonempty?(filepath) do + [filepath | acc] + else + acc end end) diff --git a/lib/pinchflat/downloading/media_download_worker.ex b/lib/pinchflat/downloading/media_download_worker.ex index b977132..c211977 100644 --- a/lib/pinchflat/downloading/media_download_worker.ex +++ b/lib/pinchflat/downloading/media_download_worker.ex @@ -14,6 +14,8 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do alias Pinchflat.Media alias Pinchflat.Downloading.MediaDownloader + alias Pinchflat.Lifecycle.UserScripts.CommandRunner, as: UserScriptRunner + @doc """ Starts the media_item media download worker and creates a task for the media_item. @@ -56,11 +58,14 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do defp download_media_and_schedule_jobs(media_item, is_redownload) do case MediaDownloader.download_for_media_item(media_item) do - {:ok, updated_media_item} -> - Media.update_media_item(updated_media_item, %{ - media_size_bytes: compute_media_filesize(updated_media_item), - media_redownloaded_at: get_redownloaded_at(is_redownload) - }) + {:ok, downloaded_media_item} -> + {:ok, updated_media_item} = + Media.update_media_item(downloaded_media_item, %{ + media_size_bytes: compute_media_filesize(downloaded_media_item), + media_redownloaded_at: get_redownloaded_at(is_redownload) + }) + + :ok = run_user_script(updated_media_item) {:ok, updated_media_item} @@ -74,21 +79,13 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do defp compute_media_filesize(media_item) do case File.stat(media_item.media_filepath) do - {:ok, %{size: size}} -> - size - - _ -> - nil + {:ok, %{size: size}} -> size + _ -> nil end end - defp get_redownloaded_at(is_redownload) do - if is_redownload do - DateTime.utc_now() - else - nil - end - end + defp get_redownloaded_at(true), do: DateTime.utc_now() + defp get_redownloaded_at(_), do: nil defp action_on_error(message) do # This will attempt re-download at the next indexing, but it won't be retried @@ -103,4 +100,12 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do {:error, :download_failed} end end + + # NOTE: I like this pattern of using the default value so that I don't have to + # define it in config.exs (and friends). Consider using this elsewhere. + defp run_user_script(media_item) do + runner = Application.get_env(:pinchflat, :user_script_runner, UserScriptRunner) + + runner.run(:media_downloaded, media_item) + end end diff --git a/lib/pinchflat/lifecycle/notifications/command_runner.ex b/lib/pinchflat/lifecycle/notifications/command_runner.ex index c9ef100..a8e08a5 100644 --- a/lib/pinchflat/lifecycle/notifications/command_runner.ex +++ b/lib/pinchflat/lifecycle/notifications/command_runner.ex @@ -28,10 +28,10 @@ defmodule Pinchflat.Lifecycle.Notifications.CommandRunner do default_opts = [:verbose] parsed_opts = CliUtils.parse_options(default_opts ++ command_opts) - {output, return_code} = CliUtils.wrap_cmd(backend_executable(), parsed_opts ++ endpoints) + {output, exit_code} = CliUtils.wrap_cmd(backend_executable(), parsed_opts ++ endpoints) Logger.info("[apprise] response: #{output}") - case return_code do + case exit_code do 0 -> {:ok, String.trim(output)} _ -> {:error, String.trim(output)} end diff --git a/lib/pinchflat/lifecycle/user_scripts/command_runner.ex b/lib/pinchflat/lifecycle/user_scripts/command_runner.ex new file mode 100644 index 0000000..a5e1d66 --- /dev/null +++ b/lib/pinchflat/lifecycle/user_scripts/command_runner.ex @@ -0,0 +1,76 @@ +defmodule Pinchflat.Lifecycle.UserScripts.CommandRunner do + @moduledoc """ + Runs custom user commands commands using the `System.cmd/3` function + """ + + require Logger + + alias Pinchflat.Utils.CliUtils + alias Pinchflat.Utils.FilesystemUtils + alias Pinchflat.Lifecycle.UserScripts.UserScriptCommandRunner + + @behaviour UserScriptCommandRunner + + @event_types [ + :media_downloaded, + :media_deleted + ] + + @doc """ + Runs the user script command for the given event type. Passes the event + and the encoded data to the user script command. + + This function will succeed in almost all cases, even if the user script command + failed - this is because I don't want bad scripts to stop the whole process. + If something fails, it'll be logged. + + The only things that can cause a true failure are passing in an invalid event + type or if the passed data cannot be encoded into JSON - both indicative of + failures in the development process. + + Returns :ok + """ + @impl UserScriptCommandRunner + def run(event_type, encodable_data) when event_type in @event_types do + case backend_executable() do + {:ok, :no_executable} -> + :ok + + {:ok, executable_path} -> + {:ok, encoded_data} = Phoenix.json_library().encode(encodable_data) + + {output, exit_code} = + CliUtils.wrap_cmd( + executable_path, + [to_string(event_type), encoded_data], + [], + logging_arg_override: "[supressed]" + ) + + handle_output(output, exit_code) + end + end + + def run(event_type, _encodable_data) do + raise ArgumentError, "Invalid event type: #{inspect(event_type)}" + end + + defp handle_output(output, exit_code) do + Logger.debug("Custom lifecycle script had an exit code of #{exit_code} output: #{output}") + + :ok + end + + defp backend_executable do + base_dir = Application.get_env(:pinchflat, :extras_directory) + filepath = Path.join([base_dir, "user-scripts", "lifecycle"]) + + if FilesystemUtils.exists_and_nonempty?(filepath) do + {:ok, filepath} + else + Logger.warning("User scripts lifecyle file either not present or is empty. Skipping.") + + {:ok, :no_executable} + end + end +end diff --git a/lib/pinchflat/lifecycle/user_scripts/user_script_command_runner.ex b/lib/pinchflat/lifecycle/user_scripts/user_script_command_runner.ex index e69de29..a0f6234 100644 --- a/lib/pinchflat/lifecycle/user_scripts/user_script_command_runner.ex +++ b/lib/pinchflat/lifecycle/user_scripts/user_script_command_runner.ex @@ -0,0 +1,10 @@ +defmodule Pinchflat.Lifecycle.UserScripts.UserScriptCommandRunner do + @moduledoc """ + A behaviour for running custom user scripts on certain events. + + Used so we can implement Mox for testing without actually running the + user's command. + """ + + @callback run(atom(), map()) :: :ok | {:error, binary()} +end diff --git a/lib/pinchflat/utils/cli_utils.ex b/lib/pinchflat/utils/cli_utils.ex index 1426a1b..1b9c7b9 100644 --- a/lib/pinchflat/utils/cli_utils.ex +++ b/lib/pinchflat/utils/cli_utils.ex @@ -13,17 +13,22 @@ defmodule Pinchflat.Utils.CliUtils do commands if the job runner is cancelled. Delegates to `System.cmd/3` and any options/output - are passed through. + are passed through. Custom options can be passed in. + + Custom options: + - logging_arg_override: if set, the passed value will be logged in place of + the actual arguments passed to the command Returns {binary(), integer()} """ - def wrap_cmd(command, args, opts \\ []) do + def wrap_cmd(command, args, passthrough_opts \\ [], opts \\ []) do wrapper_command = Path.join(:code.priv_dir(:pinchflat), "cmd_wrapper.sh") actual_command = [command] ++ args + logging_arg_override = Keyword.get(opts, :logging_arg_override, Enum.join(args, " ")) - Logger.info("[command_wrapper]: #{command} called with: #{Enum.join(args, " ")}") + Logger.info("[command_wrapper]: #{command} called with: #{logging_arg_override}") - System.cmd(wrapper_command, actual_command, opts) + System.cmd(wrapper_command, actual_command, passthrough_opts) end @doc """ diff --git a/lib/pinchflat/utils/filesystem_utils.ex b/lib/pinchflat/utils/filesystem_utils.ex index 8315218..8652192 100644 --- a/lib/pinchflat/utils/filesystem_utils.ex +++ b/lib/pinchflat/utils/filesystem_utils.ex @@ -5,6 +5,21 @@ defmodule Pinchflat.Utils.FilesystemUtils do alias Pinchflat.Media alias Pinchflat.Utils.StringUtils + @doc """ + Checks if a file exists and has non-whitespace contents. + + Returns boolean() + """ + def exists_and_nonempty?(filepath) do + case File.read(filepath) do + {:ok, contents} -> + String.trim(contents) != "" + + _ -> + false + end + end + @doc """ Generates a temporary file and returns its path. The file is empty and has the given type. Generates all the directories in the path if they don't exist. diff --git a/lib/pinchflat/yt_dlp/command_runner.ex b/lib/pinchflat/yt_dlp/command_runner.ex index 7a03df7..ed4cba0 100644 --- a/lib/pinchflat/yt_dlp/command_runner.ex +++ b/lib/pinchflat/yt_dlp/command_runner.ex @@ -81,16 +81,10 @@ defmodule Pinchflat.YtDlp.CommandRunner do Enum.reduce(filename_options_map, [], fn {opt_name, filename}, acc -> filepath = Path.join(base_dir, filename) - case File.read(filepath) do - {:ok, file_data} -> - if String.trim(file_data) != "" do - [{opt_name, filepath} | acc] - else - acc - end - - {:error, _} -> - acc + if FSUtils.exists_and_nonempty?(filepath) do + [{opt_name, filepath} | acc] + else + acc end end) end diff --git a/test/pinchflat/downloading/media_download_worker_test.exs b/test/pinchflat/downloading/media_download_worker_test.exs index e9155fa..f937bce 100644 --- a/test/pinchflat/downloading/media_download_worker_test.exs +++ b/test/pinchflat/downloading/media_download_worker_test.exs @@ -12,9 +12,8 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do setup :verify_on_exit! setup do - stub(HTTPClientMock, :get, fn _url, _headers, _opts -> - {:ok, ""} - end) + stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end) + stub(HTTPClientMock, :get, fn _url, _headers, _opts -> {:ok, ""} end) media_item = %{media_filepath: nil} @@ -185,6 +184,20 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do assert media_item.media_redownloaded_at == nil end + test "calls the user script runner", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl -> + {:ok, render_metadata(:media_metadata)} + end) + + expect(UserScriptRunnerMock, :run, fn :media_downloaded, data -> + assert data.id == media_item.id + + :ok + end) + + perform_job(MediaDownloadWorker, %{id: media_item.id}) + end + test "does not blow up if the record doesn't exist" do assert :ok = perform_job(MediaDownloadWorker, %{id: 0}) end diff --git a/test/pinchflat/downloading/output_path/parser_test.exs b/test/pinchflat/downloading/output_path/parser_test.exs index 44b27b9..f50a497 100644 --- a/test/pinchflat/downloading/output_path/parser_test.exs +++ b/test/pinchflat/downloading/output_path/parser_test.exs @@ -1,5 +1,5 @@ defmodule Pinchflat.Downloading.OutputPath.ParserTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false alias Pinchflat.Downloading.OutputPath.Parser diff --git a/test/pinchflat/lifecycle/notifications/command_runner_test.exs b/test/pinchflat/lifecycle/notifications/command_runner_test.exs index 3635c48..d0969d1 100644 --- a/test/pinchflat/lifecycle/notifications/command_runner_test.exs +++ b/test/pinchflat/lifecycle/notifications/command_runner_test.exs @@ -1,5 +1,5 @@ defmodule Pinchflat.Lifecycle.Notifications.CommandRunnerTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false alias Pinchflat.Lifecycle.Notifications.CommandRunner, as: Runner diff --git a/test/pinchflat/lifecycle/user_scripts/command_runner_test.exs b/test/pinchflat/lifecycle/user_scripts/command_runner_test.exs new file mode 100644 index 0000000..4b97d3d --- /dev/null +++ b/test/pinchflat/lifecycle/user_scripts/command_runner_test.exs @@ -0,0 +1,79 @@ +defmodule Pinchflat.Lifecycle.UserScripts.CommandRunnerTest do + use ExUnit.Case, async: false + + alias Pinchflat.Utils.FilesystemUtils + alias Pinchflat.Lifecycle.UserScripts.CommandRunner, as: Runner + + setup do + FilesystemUtils.write_p!(filepath(), "") + File.chmod(filepath(), 0o755) + + :ok + end + + describe "run/2" do + test "runs the provided lifecycle file if present" do + # We *love* indirectly testing side effects + tmp_dir = Application.get_env(:pinchflat, :tmpfile_directory) + File.write(filepath(), "#!/bin/bash\ntouch #{tmp_dir}/test_file\n") + + refute File.exists?("#{tmp_dir}/test_file") + assert :ok = Runner.run(:media_downloaded, %{}) + assert File.exists?("#{tmp_dir}/test_file") + end + + test "passes the event name to the script" do + tmp_dir = Application.get_env(:pinchflat, :tmpfile_directory) + File.write(filepath(), "#!/bin/bash\necho $1 > #{tmp_dir}/event_name\n") + + assert :ok = Runner.run(:media_downloaded, %{}) + assert File.read!("#{tmp_dir}/event_name") == "media_downloaded\n" + end + + test "passes the encoded data to the script" do + tmp_dir = Application.get_env(:pinchflat, :tmpfile_directory) + File.write(filepath(), "#!/bin/bash\necho $2 > #{tmp_dir}/encoded_data\n") + + assert :ok = Runner.run(:media_downloaded, %{foo: "bar"}) + assert File.read!("#{tmp_dir}/encoded_data") == "{\"foo\":\"bar\"}\n" + end + + test "does nothing if the lifecycle file is not present" do + :ok = File.rm(filepath()) + + assert :ok = Runner.run(:media_downloaded, %{}) + end + + test "does nothing if the lifecycle file is empty" do + File.write(filepath(), "") + + assert :ok = Runner.run(:media_downloaded, %{}) + end + + test "returns :ok if the command exits with a non-zero status" do + File.write(filepath(), "#!/bin/bash\nexit 1\n") + + assert :ok = Runner.run(:media_downloaded, %{}) + end + + test "gets upset if you pass an invalid event type" do + assert_raise ArgumentError, "Invalid event type: :invalid_event", fn -> + Runner.run(:invalid_event, %{}) + end + end + + test "gets upset if the record cannot be decoded" do + File.write(filepath(), "#!/bin/bash") + + assert_raise MatchError, fn -> + Runner.run(:media_downloaded, %Ecto.Changeset{}) + end + end + end + + defp filepath do + base_dir = Application.get_env(:pinchflat, :extras_directory) + + Path.join([base_dir, "user-scripts", "lifecycle"]) + end +end diff --git a/test/pinchflat/slow_indexing/file_follower_server_test.exs b/test/pinchflat/slow_indexing/file_follower_server_test.exs index ff09695..eb38aaa 100644 --- a/test/pinchflat/slow_indexing/file_follower_server_test.exs +++ b/test/pinchflat/slow_indexing/file_follower_server_test.exs @@ -1,5 +1,5 @@ defmodule Pinchflat.SlowIndexing.FileFollowerServerTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false alias alias Pinchflat.Utils.FilesystemUtils alias Pinchflat.SlowIndexing.FileFollowerServer diff --git a/test/pinchflat/utils/changeset_utils_test.exs b/test/pinchflat/utils/changeset_utils_test.exs index 83eadd7..43bcf70 100644 --- a/test/pinchflat/utils/changeset_utils_test.exs +++ b/test/pinchflat/utils/changeset_utils_test.exs @@ -1,5 +1,5 @@ defmodule Pinchflat.Utils.ChangesetUtilsTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false defmodule MockSchema do use Ecto.Schema diff --git a/test/pinchflat/utils/cli_utils_test.exs b/test/pinchflat/utils/cli_utils_test.exs index 0c676b7..75f92c1 100644 --- a/test/pinchflat/utils/cli_utils_test.exs +++ b/test/pinchflat/utils/cli_utils_test.exs @@ -1,5 +1,5 @@ defmodule Pinchflat.Utils.CliUtilsTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false alias Pinchflat.Utils.CliUtils diff --git a/test/pinchflat/utils/datetime_utils_test.exs b/test/pinchflat/utils/datetime_utils_test.exs index 166b9c0..4a52438 100644 --- a/test/pinchflat/utils/datetime_utils_test.exs +++ b/test/pinchflat/utils/datetime_utils_test.exs @@ -1,5 +1,5 @@ defmodule Pinchflat.Utils.DatetimeUtilsTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false alias Pinchflat.Utils.DatetimeUtils diff --git a/test/pinchflat/utils/filesystem_utils_test.exs b/test/pinchflat/utils/filesystem_utils_test.exs index 7d86cbc..a964e48 100644 --- a/test/pinchflat/utils/filesystem_utils_test.exs +++ b/test/pinchflat/utils/filesystem_utils_test.exs @@ -5,6 +5,38 @@ defmodule Pinchflat.Utils.FilesystemUtilsTest do alias Pinchflat.Utils.FilesystemUtils + describe "exists_and_nonempty?" do + test "returns true if a file exists and has contents" do + filepath = FilesystemUtils.generate_metadata_tmpfile(:json) + File.write(filepath, "{}") + + assert FilesystemUtils.exists_and_nonempty?(filepath) + + File.rm!(filepath) + end + + test "returns false if a file doesn't exist" do + refute FilesystemUtils.exists_and_nonempty?("/nonexistent/file.json") + end + + test "returns false if a file exists but is empty" do + filepath = FilesystemUtils.generate_metadata_tmpfile(:json) + + refute FilesystemUtils.exists_and_nonempty?(filepath) + + File.rm!(filepath) + end + + test "trims the contents before checking" do + filepath = FilesystemUtils.generate_metadata_tmpfile(:json) + File.write(filepath, " \n\n \r\n ") + + refute FilesystemUtils.exists_and_nonempty?(filepath) + + File.rm!(filepath) + end + end + describe "generate_metadata_tmpfile/1" do test "creates a tmpfile and returns its path" do res = FilesystemUtils.generate_metadata_tmpfile(:json) diff --git a/test/pinchflat/utils/function_utils_test.exs b/test/pinchflat/utils/function_utils_test.exs index 6478599..8f3e208 100644 --- a/test/pinchflat/utils/function_utils_test.exs +++ b/test/pinchflat/utils/function_utils_test.exs @@ -1,5 +1,5 @@ defmodule Pinchflat.Utils.FunctionUtilsTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false alias Pinchflat.Utils.FunctionUtils diff --git a/test/pinchflat/utils/number_utils_test.exs b/test/pinchflat/utils/number_utils_test.exs index 48efe41..f775f20 100644 --- a/test/pinchflat/utils/number_utils_test.exs +++ b/test/pinchflat/utils/number_utils_test.exs @@ -1,5 +1,5 @@ defmodule Pinchflat.Utils.NumberUtilsTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false alias Pinchflat.Utils.NumberUtils diff --git a/test/pinchflat/utils/string_utils_test.exs b/test/pinchflat/utils/string_utils_test.exs index 51172e3..76ea3b6 100644 --- a/test/pinchflat/utils/string_utils_test.exs +++ b/test/pinchflat/utils/string_utils_test.exs @@ -1,5 +1,5 @@ defmodule Pinchflat.Utils.StringUtilsTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false alias Pinchflat.Utils.StringUtils diff --git a/test/pinchflat/utils/xml_utils_test.exs b/test/pinchflat/utils/xml_utils_test.exs index 883cfc2..25aca92 100644 --- a/test/pinchflat/utils/xml_utils_test.exs +++ b/test/pinchflat/utils/xml_utils_test.exs @@ -1,5 +1,5 @@ defmodule Pinchflat.Utils.XmlUtilsTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false alias Pinchflat.Utils.XmlUtils diff --git a/test/pinchflat/yt_dlp/command_runner_test.exs b/test/pinchflat/yt_dlp/command_runner_test.exs index f4c9309..86e4551 100644 --- a/test/pinchflat/yt_dlp/command_runner_test.exs +++ b/test/pinchflat/yt_dlp/command_runner_test.exs @@ -1,5 +1,5 @@ defmodule Pinchflat.YtDlp.CommandRunnerTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false alias Pinchflat.Utils.FilesystemUtils @@ -75,6 +75,9 @@ defmodule Pinchflat.YtDlp.CommandRunnerTest do refute String.contains?(output, "--cookies") refute String.contains?(output, cookie_file) + + # Cleanup + FilesystemUtils.write_p!(cookie_file, "") end end diff --git a/test/pinchflat_web/controllers/error_html_test.exs b/test/pinchflat_web/controllers/error_html_test.exs index 506e1a5..d9baab3 100644 --- a/test/pinchflat_web/controllers/error_html_test.exs +++ b/test/pinchflat_web/controllers/error_html_test.exs @@ -1,5 +1,5 @@ defmodule PinchflatWeb.ErrorHTMLTest do - use PinchflatWeb.ConnCase, async: true + use PinchflatWeb.ConnCase, async: false # Bring render_to_string/4 for testing custom views import Phoenix.Template diff --git a/test/pinchflat_web/controllers/error_json_test.exs b/test/pinchflat_web/controllers/error_json_test.exs index 981c51b..ee8d8d2 100644 --- a/test/pinchflat_web/controllers/error_json_test.exs +++ b/test/pinchflat_web/controllers/error_json_test.exs @@ -1,5 +1,5 @@ defmodule PinchflatWeb.ErrorJSONTest do - use PinchflatWeb.ConnCase, async: true + use PinchflatWeb.ConnCase, async: false test "renders 404" do assert PinchflatWeb.ErrorJSON.render("404.json", %{}) == %{errors: %{detail: "Not Found"}} diff --git a/test/test_helper.exs b/test/test_helper.exs index 9b45123..3a05c78 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -7,6 +7,9 @@ Application.put_env(:pinchflat, :apprise_runner, AppriseRunnerMock) Mox.defmock(HTTPClientMock, for: Pinchflat.HTTP.HTTPBehaviour) Application.put_env(:pinchflat, :http_client, HTTPClientMock) +Mox.defmock(UserScriptRunnerMock, for: Pinchflat.Lifecycle.UserScripts.UserScriptCommandRunner) +Application.put_env(:pinchflat, :user_script_runner, UserScriptRunnerMock) + ExUnit.start() Ecto.Adapters.SQL.Sandbox.mode(Pinchflat.Repo, :manual) Faker.start()