From 657fcb0c9e67608c885861163c09e9a447954c19 Mon Sep 17 00:00:00 2001 From: Dylan Markow Date: Thu, 18 Jul 2019 10:33:07 -0500 Subject: [PATCH] Clean up all temp files created during processing and storage --- lib/arc/actions/store.ex | 29 ++++++++++++++++++++++++----- lib/arc/file.ex | 10 +++++----- lib/arc/processor.ex | 2 +- lib/arc/transformations/convert.ex | 5 +++-- mix.lock | 4 ++-- test/storage/local_test.exs | 24 +++++++++++++++++++++++- 6 files changed, 58 insertions(+), 16 deletions(-) diff --git a/lib/arc/actions/store.ex b/lib/arc/actions/store.ex index 0fb70e3..666c410 100644 --- a/lib/arc/actions/store.ex +++ b/lib/arc/actions/store.ex @@ -20,11 +20,23 @@ defmodule Arc.Actions.Store do defp put(_definition, { error = {:error, _msg}, _scope}), do: error defp put(definition, {%Arc.File{}=file, scope}) do case definition.validate({file, scope}) do - true -> put_versions(definition, {file, scope}) + true -> + put_versions(definition, {file, scope}) + |> cleanup(file) _ -> {:error, :invalid_file} end end + defp cleanup(result, file) do + # If we were working with binary data or a remote file, a tempfile was + # created that we need to clean up. + if file.tempfile? do + File.rm(file.path) + end + + result + end + defp put_versions(definition, {file, scope}) do if definition.async do definition.__versions @@ -33,13 +45,13 @@ defmodule Arc.Actions.Store do |> ensure_all_success |> Enum.map(fn({v, r}) -> async_put_version(definition, v, {r, scope}) end) |> Enum.map(fn(task) -> Task.await(task, version_timeout()) end) - |> handle_responses(file.file_name) + |> handle_responses(file) else definition.__versions |> Enum.map(fn(version) -> process_version(definition, version, {file, scope}) end) |> ensure_all_success |> Enum.map(fn({version, result}) -> put_version(definition, version, {result, scope}) end) - |> handle_responses(file.file_name) + |> handle_responses(file) end end @@ -48,9 +60,9 @@ defmodule Arc.Actions.Store do if Enum.empty?(errors), do: responses, else: errors end - defp handle_responses(responses, filename) do + defp handle_responses(responses, file) do errors = Enum.filter(responses, fn(resp) -> elem(resp, 0) == :error end) |> Enum.map(fn(err) -> elem(err, 1) end) - if Enum.empty?(errors), do: {:ok, filename}, else: {:error, errors} + if Enum.empty?(errors), do: {:ok, file.file_name}, else: {:error, errors} end defp version_timeout do @@ -81,6 +93,13 @@ defmodule Arc.Actions.Store do file_name = Arc.Definition.Versioning.resolve_file_name(definition, version, {file, scope}) file = %Arc.File{file | file_name: file_name} result = definition.__storage.put(definition, version, {file, scope}) + + # If this version was transformed in any way, a tempfile was created + # that we need to clean up. + if file.tempfile? do + File.rm(file.path) + end + result end end diff --git a/lib/arc/file.ex b/lib/arc/file.ex index ff6a48b..e805333 100644 --- a/lib/arc/file.ex +++ b/lib/arc/file.ex @@ -1,5 +1,5 @@ defmodule Arc.File do - defstruct [:path, :file_name, :binary] + defstruct [:path, :file_name, :binary, :tempfile?] def generate_temporary_path(file \\ nil) do extension = Path.extname((file && file.path) || "") @@ -18,7 +18,7 @@ defmodule Arc.File do filename = Path.basename(uri.path) case save_file(uri, filename) do - {:ok, local_path} -> %Arc.File{path: local_path, file_name: filename} + {:ok, local_path} -> %Arc.File{path: local_path, file_name: filename, tempfile?: true} :error -> {:error, :invalid_file_path} end end @@ -33,6 +33,7 @@ end def new(%{filename: filename, binary: binary}) do %Arc.File{binary: binary, file_name: Path.basename(filename)} + |> write_binary() end # Accepts a map conforming to %Plug.Upload{} syntax @@ -43,8 +44,6 @@ end end end - def ensure_path(file = %{path: path}) when is_binary(path), do: file - def ensure_path(file = %{binary: binary}) when is_binary(binary), do: write_binary(file) defp write_binary(file) do path = generate_temporary_path(file) @@ -52,7 +51,8 @@ end %__MODULE__{ file_name: file.file_name, - path: path + path: path, + tempfile?: true } end diff --git a/lib/arc/processor.ex b/lib/arc/processor.ex index 039c467..1396015 100644 --- a/lib/arc/processor.ex +++ b/lib/arc/processor.ex @@ -12,6 +12,6 @@ defmodule Arc.Processor do end defp apply_transformation(file, {cmd, conversion}) do - Arc.Transformations.Convert.apply(cmd, Arc.File.ensure_path(file), conversion) + Arc.Transformations.Convert.apply(cmd, file, conversion) end end diff --git a/lib/arc/transformations/convert.ex b/lib/arc/transformations/convert.ex index a9b60d5..6370ba6 100644 --- a/lib/arc/transformations/convert.ex +++ b/lib/arc/transformations/convert.ex @@ -6,9 +6,10 @@ defmodule Arc.Transformations.Convert do ensure_executable_exists!(program) - case System.cmd(program, args_list(args), stderr_to_stdout: true) do + result = System.cmd(program, args_list(args), stderr_to_stdout: true) + case result do {_, 0} -> - {:ok, %Arc.File{file | path: new_path}} + {:ok, %Arc.File{file | path: new_path, tempfile?: true}} {error_message, _exit_code} -> {:error, error_message} end diff --git a/mix.lock b/mix.lock index 20f85bd..5ba8aa1 100644 --- a/mix.lock +++ b/mix.lock @@ -8,10 +8,10 @@ "idna": {:hex, :idna, "6.0.0", "689c46cbcdf3524c44d5f3dde8001f364cd7608a99556d8fbd8239a5798d4c10", [:rebar3], [{:unicode_util_compat, "0.4.1", [hex: :unicode_util_compat, repo: "hexpm", optional: false]}], "hexpm"}, "makeup": {:hex, :makeup, "0.5.5", "9e08dfc45280c5684d771ad58159f718a7b5788596099bdfb0284597d368a882", [:mix], [{:nimble_parsec, "~> 0.4", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm"}, "makeup_elixir": {:hex, :makeup_elixir, "0.10.0", "0f09c2ddf352887a956d84f8f7e702111122ca32fbbc84c2f0569b8b65cbf7fa", [:mix], [{:makeup, "~> 0.5.5", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm"}, - "meck": {:hex, :meck, "0.8.12", "1f7b1a9f5d12c511848fec26bbefd09a21e1432eadb8982d9a8aceb9891a3cf2", [:rebar3], [], "hexpm"}, + "meck": {:hex, :meck, "0.8.13", "ffedb39f99b0b99703b8601c6f17c7f76313ee12de6b646e671e3188401f7866", [:rebar3], [], "hexpm"}, "metrics": {:hex, :metrics, "1.0.1", "25f094dea2cda98213cecc3aeff09e940299d950904393b2a29d191c346a8486", [:rebar3], [], "hexpm"}, "mimerl": {:hex, :mimerl, "1.0.2", "993f9b0e084083405ed8252b99460c4f0563e41729ab42d9074fd5e52439be88", [:rebar3], [], "hexpm"}, - "mock": {:hex, :mock, "0.3.2", "e98e998fd76c191c7e1a9557c8617912c53df3d4a6132f561eb762b699ef59fa", [:mix], [{:meck, "~> 0.8.8", [hex: :meck, repo: "hexpm", optional: false]}], "hexpm"}, + "mock": {:hex, :mock, "0.3.3", "42a433794b1291a9cf1525c6d26b38e039e0d3a360732b5e467bfc77ef26c914", [:mix], [{:meck, "~> 0.8.13", [hex: :meck, repo: "hexpm", optional: false]}], "hexpm"}, "nimble_parsec": {:hex, :nimble_parsec, "0.4.0", "ee261bb53214943679422be70f1658fff573c5d0b0a1ecd0f18738944f818efe", [:mix], [], "hexpm"}, "parse_trans": {:hex, :parse_trans, "3.3.0", "09765507a3c7590a784615cfd421d101aec25098d50b89d7aa1d66646bc571c1", [:rebar3], [], "hexpm"}, "poison": {:hex, :poison, "3.1.0", "d9eb636610e096f86f25d9a46f35a9facac35609a7591b3be3326e99a0484665", [:mix], [], "hexpm"}, diff --git a/test/storage/local_test.exs b/test/storage/local_test.exs index ad8dea7..50eec73 100644 --- a/test/storage/local_test.exs +++ b/test/storage/local_test.exs @@ -3,11 +3,14 @@ defmodule ArcTest.Storage.Local do @img "test/support/image.png" @badimg "test/support/invalid_image.png" - setup_all do + setup do File.mkdir_p("arctest/uploads") + File.mkdir_p("arctest/tmp") + System.put_env("TMPDIR", "arctest/tmp") on_exit fn -> File.rm_rf("arctest/uploads") + File.rm_rf("arctest/tmp") end end @@ -68,4 +71,23 @@ defmodule ArcTest.Storage.Local do assert !File.exists?("arctest/uploads/original-#{filename}") assert !File.exists?("arctest/uploads/1/thumb-#{filename}") end + + test "temp files from processing are cleaned up" do + filepath = @img + DummyDefinition.store(filepath) + assert length(File.ls!("arctest/tmp")) == 0 + end + + test "temp files from handling binary data are cleaned up" do + filepath = @img + filename = "image.png" + DummyDefinition.store(%{binary: File.read!(filepath), filename: filename}) + assert File.exists?("arctest/uploads/original-#{filename}") + assert length(File.ls!("arctest/tmp")) == 0 + end + + test "temp files from handling remote URLs are cleaned up" do + DummyDefinition.store("https://www.google.com/favicon.ico") + assert length(File.ls!("arctest/tmp")) == 0 + end end