diff --git a/lib/pinchflat/downloading/media_download_worker.ex b/lib/pinchflat/downloading/media_download_worker.ex index 42c5ca4..c7ea232 100644 --- a/lib/pinchflat/downloading/media_download_worker.ex +++ b/lib/pinchflat/downloading/media_download_worker.ex @@ -39,17 +39,14 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do - `quality_upgrade?`: re-downloads media, including the video. Does not force download if the source is set to not download media - Returns :ok | {:ok, %MediaItem{}} | {:error, any, ...any} + Returns :ok | {:error, any, ...any} """ @impl Oban.Worker def perform(%Oban.Job{args: %{"id" => media_item_id} = args}) do should_force = Map.get(args, "force", false) is_quality_upgrade = Map.get(args, "quality_upgrade?", false) - media_item = - media_item_id - |> Media.get_media_item!() - |> Repo.preload(:source) + media_item = fetch_and_run_prevent_download_user_script(media_item_id) # If the source or media item is set to not download media, perform a no-op unless forced if (media_item.source.download_media && !media_item.prevent_download) || should_force do @@ -62,6 +59,20 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do Ecto.StaleEntryError -> Logger.info("#{__MODULE__} discarded: media item #{media_item_id} stale") end + # If a user script exists and, when run, returns a non-zero exit code, prevent this and all future downloads + # of the media item. + defp fetch_and_run_prevent_download_user_script(media_item_id) do + media_item = Media.get_media_item!(media_item_id) + + {:ok, media_item} = + case run_user_script(:media_pre_download, media_item) do + {:ok, _, exit_code} when exit_code != 0 -> Media.update_media_item(media_item, %{prevent_download: true}) + _ -> {:ok, media_item} + end + + Repo.preload(media_item, :source) + end + defp download_media_and_schedule_jobs(media_item, is_quality_upgrade, should_force) do overwrite_behaviour = if should_force || is_quality_upgrade, do: :force_overwrites, else: :no_force_overwrites override_opts = [overwrite_behaviour: overwrite_behaviour] @@ -74,9 +85,9 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do media_redownloaded_at: get_redownloaded_at(is_quality_upgrade) }) - :ok = run_user_script(updated_media_item) + run_user_script(:media_downloaded, updated_media_item) - {:ok, updated_media_item} + :ok {:recovered, _} -> {:error, :retry} @@ -112,9 +123,9 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do # 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 + defp run_user_script(event, media_item) do runner = Application.get_env(:pinchflat, :user_script_runner, UserScriptRunner) - runner.run(:media_downloaded, media_item) + runner.run(event, media_item) end end diff --git a/lib/pinchflat/lifecycle/user_scripts/command_runner.ex b/lib/pinchflat/lifecycle/user_scripts/command_runner.ex index 8436c05..54aa38c 100644 --- a/lib/pinchflat/lifecycle/user_scripts/command_runner.ex +++ b/lib/pinchflat/lifecycle/user_scripts/command_runner.ex @@ -12,6 +12,7 @@ defmodule Pinchflat.Lifecycle.UserScripts.CommandRunner do @behaviour UserScriptCommandRunner @event_types [ + :media_pre_download, :media_downloaded, :media_deleted ] @@ -22,24 +23,25 @@ defmodule Pinchflat.Lifecycle.UserScripts.CommandRunner do 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. + If something fails, it'll be logged and returned BUT the tuple will always + start with {:ok, ...}. 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 + Returns {:ok, :no_executable} | {:ok, output, exit_code} """ @impl UserScriptCommandRunner def run(event_type, encodable_data) when event_type in @event_types do case backend_executable() do {:ok, :no_executable} -> - :ok + {:ok, :no_executable} {:ok, executable_path} -> {:ok, encoded_data} = Phoenix.json_library().encode(encodable_data) - {_output, _exit_code} = + {output, exit_code} = CliUtils.wrap_cmd( executable_path, [to_string(event_type), encoded_data], @@ -47,7 +49,7 @@ defmodule Pinchflat.Lifecycle.UserScripts.CommandRunner do logging_arg_override: "[suppressed]" ) - :ok + {:ok, output, exit_code} end end @@ -62,7 +64,7 @@ defmodule Pinchflat.Lifecycle.UserScripts.CommandRunner do if FilesystemUtils.exists_and_nonempty?(filepath) do {:ok, filepath} else - Logger.warning("User scripts lifecyle file either not present or is empty. Skipping.") + Logger.info("User scripts lifecyle file either not present or is empty. Skipping.") {:ok, :no_executable} end diff --git a/lib/pinchflat/media/media.ex b/lib/pinchflat/media/media.ex index ba46e7c..013bf09 100644 --- a/lib/pinchflat/media/media.ex +++ b/lib/pinchflat/media/media.ex @@ -171,7 +171,7 @@ defmodule Pinchflat.Media do if delete_files do {:ok, _} = do_delete_media_files(media_item) - :ok = run_user_script(:media_deleted, media_item) + run_user_script(:media_deleted, media_item) end # Should delete these no matter what @@ -194,7 +194,7 @@ defmodule Pinchflat.Media do Tasks.delete_tasks_for(media_item) {:ok, _} = do_delete_media_files(media_item) - :ok = run_user_script(:media_deleted, media_item) + run_user_script(:media_deleted, media_item) update_media_item(media_item, Map.merge(filepath_attrs, addl_attrs)) end diff --git a/test/pinchflat/downloading/media_download_worker_test.exs b/test/pinchflat/downloading/media_download_worker_test.exs index 754f838..35eabeb 100644 --- a/test/pinchflat/downloading/media_download_worker_test.exs +++ b/test/pinchflat/downloading/media_download_worker_test.exs @@ -10,7 +10,7 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do setup do stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot -> {:ok, ""} end) - stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end) + stub(UserScriptRunnerMock, :run, fn _event_type, _data -> {:ok, "", 0} end) stub(HTTPClientMock, :get, fn _url, _headers, _opts -> {:ok, ""} end) media_item = @@ -162,20 +162,6 @@ 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 @@ -237,4 +223,59 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do perform_job(MediaDownloadWorker, %{id: media_item.id, force: true}) end end + + describe "perform/1 when testing user script callbacks" do + setup do + stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl -> + {:ok, render_metadata(:media_metadata)} + end) + + :ok + end + + test "calls the media_pre_download user script runner", %{media_item: media_item} do + expect(UserScriptRunnerMock, :run, fn :media_pre_download, data -> + assert data.id == media_item.id + + {:ok, "", 0} + end) + + expect(UserScriptRunnerMock, :run, fn :media_downloaded, _ -> {:ok, "", 0} end) + + perform_job(MediaDownloadWorker, %{id: media_item.id}) + end + + test "does not download the media if the pre-download script returns an error", %{media_item: media_item} do + expect(UserScriptRunnerMock, :run, fn :media_pre_download, _ -> {:ok, "", 1} end) + + assert :ok = perform_job(MediaDownloadWorker, %{id: media_item.id}) + media_item = Repo.reload!(media_item) + + refute media_item.media_filepath + assert media_item.prevent_download + end + + test "downloads media if the pre-download script is not present", %{media_item: media_item} do + expect(UserScriptRunnerMock, :run, fn :media_pre_download, _ -> {:ok, :no_executable} end) + expect(UserScriptRunnerMock, :run, fn :media_downloaded, _ -> {:ok, :no_executable} end) + + assert :ok = perform_job(MediaDownloadWorker, %{id: media_item.id}) + media_item = Repo.reload!(media_item) + + assert media_item.media_filepath + refute media_item.prevent_download + end + + test "calls the media_downloaded user script runner", %{media_item: media_item} do + expect(UserScriptRunnerMock, :run, fn :media_pre_download, _ -> {:ok, "", 0} end) + + expect(UserScriptRunnerMock, :run, fn :media_downloaded, data -> + assert data.id == media_item.id + + {:ok, "", 0} + end) + + perform_job(MediaDownloadWorker, %{id: media_item.id}) + end + end end diff --git a/test/pinchflat/downloading/media_retention_worker_test.exs b/test/pinchflat/downloading/media_retention_worker_test.exs index e49bb39..f0a59ce 100644 --- a/test/pinchflat/downloading/media_retention_worker_test.exs +++ b/test/pinchflat/downloading/media_retention_worker_test.exs @@ -8,7 +8,7 @@ defmodule Pinchflat.Downloading.MediaRetentionWorkerTest do alias Pinchflat.Downloading.MediaRetentionWorker setup do - stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end) + stub(UserScriptRunnerMock, :run, fn _event_type, _data -> {:ok, "", 0} end) :ok end diff --git a/test/pinchflat/lifecycle/user_scripts/command_runner_test.exs b/test/pinchflat/lifecycle/user_scripts/command_runner_test.exs index 351f4c4..5a35faf 100644 --- a/test/pinchflat/lifecycle/user_scripts/command_runner_test.exs +++ b/test/pinchflat/lifecycle/user_scripts/command_runner_test.exs @@ -19,7 +19,7 @@ defmodule Pinchflat.Lifecycle.UserScripts.CommandRunnerTest do File.write(filepath(), "#!/bin/bash\ntouch #{filename}\n") refute File.exists?(filename) - assert :ok = Runner.run(:media_downloaded, %{}) + assert {:ok, _, _} = Runner.run(:media_downloaded, %{}) assert File.exists?(filename) end @@ -27,7 +27,7 @@ defmodule Pinchflat.Lifecycle.UserScripts.CommandRunnerTest 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 {:ok, _, _} = Runner.run(:media_downloaded, %{}) assert File.read!("#{tmp_dir}/event_name") == "media_downloaded\n" end @@ -35,26 +35,32 @@ defmodule Pinchflat.Lifecycle.UserScripts.CommandRunnerTest 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 {: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, %{}) + assert {:ok, :no_executable} = Runner.run(:media_downloaded, %{}) end test "does nothing if the lifecycle file is empty" do File.write(filepath(), "") - assert :ok = Runner.run(:media_downloaded, %{}) + assert {:ok, :no_executable} = 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, %{}) + assert {:ok, _, 1} = Runner.run(:media_downloaded, %{}) + end + + test "returns the output of the command" do + File.write(filepath(), "#!/bin/bash\necho 'hello'\n") + + assert {:ok, "hello\n", 0} = Runner.run(:media_downloaded, %{}) end test "gets upset if you pass an invalid event type" do diff --git a/test/pinchflat/media_test.exs b/test/pinchflat/media_test.exs index 5f1efb5..597309f 100644 --- a/test/pinchflat/media_test.exs +++ b/test/pinchflat/media_test.exs @@ -684,7 +684,7 @@ defmodule Pinchflat.MediaTest do describe "delete_media_item/2 when testing file deletion" do setup do - stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end) + stub(UserScriptRunnerMock, :run, fn _event_type, _data -> {:ok, "", 0} end) :ok end @@ -745,7 +745,7 @@ defmodule Pinchflat.MediaTest do expect(UserScriptRunnerMock, :run, fn :media_deleted, data -> assert data.id == media_item.id - :ok + {:ok, "", 0} end) assert {:ok, _} = Media.delete_media_item(media_item, delete_files: true) @@ -754,7 +754,7 @@ defmodule Pinchflat.MediaTest do describe "delete_media_files/2" do setup do - stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end) + stub(UserScriptRunnerMock, :run, fn _event_type, _data -> {:ok, "", 0} end) :ok end @@ -817,7 +817,7 @@ defmodule Pinchflat.MediaTest do expect(UserScriptRunnerMock, :run, fn :media_deleted, data -> assert data.id == media_item.id - :ok + {:ok, "", 0} end) assert {:ok, _} = Media.delete_media_files(media_item) diff --git a/test/pinchflat/profiles_test.exs b/test/pinchflat/profiles_test.exs index 4cf6437..6f78e7f 100644 --- a/test/pinchflat/profiles_test.exs +++ b/test/pinchflat/profiles_test.exs @@ -113,7 +113,7 @@ defmodule Pinchflat.ProfilesTest do describe "delete_media_profile/2 when deleting files" do setup do - stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end) + stub(UserScriptRunnerMock, :run, fn _event_type, _data -> {:ok, "", 0} end) :ok end diff --git a/test/pinchflat/sources_test.exs b/test/pinchflat/sources_test.exs index 65f48e0..59e004b 100644 --- a/test/pinchflat/sources_test.exs +++ b/test/pinchflat/sources_test.exs @@ -617,7 +617,7 @@ defmodule Pinchflat.SourcesTest do describe "delete_source/2 when deleting files" do setup do - stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end) + stub(UserScriptRunnerMock, :run, fn _event_type, _data -> {:ok, "", 0} end) :ok end diff --git a/test/pinchflat_web/controllers/media_item_controller_test.exs b/test/pinchflat_web/controllers/media_item_controller_test.exs index 4070938..34457d5 100644 --- a/test/pinchflat_web/controllers/media_item_controller_test.exs +++ b/test/pinchflat_web/controllers/media_item_controller_test.exs @@ -56,7 +56,7 @@ defmodule PinchflatWeb.MediaItemControllerTest do describe "delete media" do setup do media_item = media_item_with_attachments() - stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end) + stub(UserScriptRunnerMock, :run, fn _event_type, _data -> {:ok, "", 0} end) %{media_item: media_item} end diff --git a/test/pinchflat_web/controllers/media_profile_controller_test.exs b/test/pinchflat_web/controllers/media_profile_controller_test.exs index af174ca..03e9a8f 100644 --- a/test/pinchflat_web/controllers/media_profile_controller_test.exs +++ b/test/pinchflat_web/controllers/media_profile_controller_test.exs @@ -137,7 +137,7 @@ defmodule PinchflatWeb.MediaProfileControllerTest do setup [:create_media_profile] setup do - stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end) + stub(UserScriptRunnerMock, :run, fn _event_type, _data -> {:ok, "", 0} end) :ok end diff --git a/test/pinchflat_web/controllers/source_controller_test.exs b/test/pinchflat_web/controllers/source_controller_test.exs index a4d18dc..011b5eb 100644 --- a/test/pinchflat_web/controllers/source_controller_test.exs +++ b/test/pinchflat_web/controllers/source_controller_test.exs @@ -152,7 +152,7 @@ defmodule PinchflatWeb.SourceControllerTest do setup [:create_source] setup do - stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end) + stub(UserScriptRunnerMock, :run, fn _event_type, _data -> {:ok, "", 0} end) :ok end