Compare commits

...

4 commits

Author SHA1 Message Date
ilja
9d8ab46cd6 Move building of url to modules
Some checks are pending
ci/woodpecker/pr/woodpecker Pipeline is pending
We got a path and there's a "base_url" function. With that the url was
build. But not all url's are nessecarely build like this. Building the
url should be the responsability of the Uploader itself, so it's now
moved to there.
2023-03-27 09:25:44 +02:00
ilja
2edb0944ba Return the complete upload from Pleroma.Uploaders.put_file/2
Some checks are pending
ci/woodpecker/pr/woodpecker Pipeline is pending
To move more logic to the Uploader modules, it's better to keep the same
`upload` and manipulate that while going through the upload pipeline, so
that's how it's now done.

Since we expect the `upload`, I removed the option for returning just `:ok`
from the uploaders.

It was possible to bypass the logic in the `get_url` function, but that was
unused. Since it makes even less sense when returning `upload`, I removed that
In a later stage I want to return the url from the uploader module, which
should obsolete this function.

I saw a warning from `Pleroma.Upload.Filter.MogrifyTest` which I now fixed.
2023-03-26 15:26:11 +02:00
ilja
5db2e998c5 Make getting the uploader constistent
Some checks are pending
ci/woodpecker/pr/woodpecker Pipeline is pending
It was possible to get the uploader from opts.
But during the upload flow, a function was called who didn't use opts and just got it from settings.
This is inconsistent.

In practice, providing the uploader through opts was never used, so I removed it and now we always get it from settings.
2023-03-19 12:00:45 +01:00
ilja
053276ef6c Refactor upload, get url from modules
Some checks are pending
ci/woodpecker/pr/woodpecker Pipeline is pending
To get the url and base url, we had a function in upload.ex.
This included a `case` to check on what module is used.
This means that adding a new upload module implies changing code besides just adding a new module.

Here we try to get the logic properly into the respective modules.
This doesn't add new functionality, but should make it easier to add new uploaders in the future.
2023-03-12 10:13:07 +01:00
8 changed files with 91 additions and 91 deletions

View file

@ -63,10 +63,10 @@ defmodule Pleroma.Upload do
blurhash: String.t(),
path: String.t()
}
defstruct [:id, :name, :tempfile, :content_type, :width, :height, :blurhash, :path]
defstruct [:id, :name, :tempfile, :content_type, :width, :height, :blurhash, :path, :url]
defp get_description(opts, upload) do
case {opts[:description], Pleroma.Config.get([Pleroma.Upload, :default_description])} do
case {opts[:description], Config.get([Pleroma.Upload, :default_description])} do
{description, _} when is_binary(description) -> description
{_, :filename} -> upload.name
{_, str} when is_binary(str) -> str
@ -78,6 +78,7 @@ defp get_description(opts, upload) do
@doc "Store a file. If using a `Plug.Upload{}` as the source, be sure to use `Majic.Plug` to ensure its content_type and filename is correct."
def store(upload, opts \\ []) do
opts = get_opts(opts)
uploader = Config.get([__MODULE__, :uploader])
with {:ok, upload} <- prepare_upload(upload, opts),
upload = %__MODULE__{upload | path: upload.path || "#{upload.id}/#{upload.name}"},
@ -85,8 +86,8 @@ def store(upload, opts \\ []) do
description = get_description(opts, upload),
{_, true} <-
{:description_limit,
String.length(description) <= Pleroma.Config.get([:instance, :description_limit])},
{:ok, url_spec} <- Pleroma.Uploaders.Uploader.put_file(opts.uploader, upload) do
String.length(description) <= Config.get([:instance, :description_limit])},
{:ok, upload} <- Pleroma.Uploaders.Uploader.put_file(uploader, upload) do
{:ok,
%{
"id" => Utils.generate_object_id(),
@ -96,7 +97,7 @@ def store(upload, opts \\ []) do
%{
"type" => "Link",
"mediaType" => upload.content_type,
"href" => url_from_spec(upload, opts.base_url, url_spec)
"href" => url_with_query_params(upload)
}
|> Maps.put_if_present("width", upload.width)
|> Maps.put_if_present("height", upload.height)
@ -109,9 +110,7 @@ def store(upload, opts \\ []) do
{:error, :description_too_long}
{:error, error} ->
Logger.error(
"#{__MODULE__} store (using #{inspect(opts.uploader)}) failed: #{inspect(error)}"
)
Logger.error("#{__MODULE__} store (using #{inspect(uploader)}) failed: #{inspect(error)}")
{:error, error}
end
@ -125,25 +124,23 @@ defp get_opts(opts) do
{size_limit, activity_type} =
case Keyword.get(opts, :type) do
:banner ->
{Pleroma.Config.get!([:instance, :banner_upload_limit]), "Image"}
{Config.get!([:instance, :banner_upload_limit]), "Image"}
:avatar ->
{Pleroma.Config.get!([:instance, :avatar_upload_limit]), "Image"}
{Config.get!([:instance, :avatar_upload_limit]), "Image"}
:background ->
{Pleroma.Config.get!([:instance, :background_upload_limit]), "Image"}
{Config.get!([:instance, :background_upload_limit]), "Image"}
_ ->
{Pleroma.Config.get!([:instance, :upload_limit]), "Document"}
{Config.get!([:instance, :upload_limit]), "Document"}
end
%{
activity_type: Keyword.get(opts, :activity_type, activity_type),
size_limit: Keyword.get(opts, :size_limit, size_limit),
uploader: Keyword.get(opts, :uploader, Pleroma.Config.get([__MODULE__, :uploader])),
filters: Keyword.get(opts, :filters, Pleroma.Config.get([__MODULE__, :filters])),
description: Keyword.get(opts, :description),
base_url: base_url()
filters: Keyword.get(opts, :filters, Config.get([__MODULE__, :filters])),
description: Keyword.get(opts, :description)
}
end
@ -215,55 +212,14 @@ defp tempfile_for_image(data) do
tmp_path
end
defp url_from_spec(%__MODULE__{name: name}, base_url, {:file, path}) do
path =
URI.encode(path, &char_unescaped?/1) <>
if Pleroma.Config.get([__MODULE__, :link_name], false) do
"?name=#{URI.encode(name, &char_unescaped?/1)}"
else
""
end
[base_url, path]
|> Path.join()
defp url_with_query_params(%__MODULE__{url: url, name: name}) do
url <>
if Config.get([__MODULE__, :link_name], false) do
"?name=#{URI.encode(name, &char_unescaped?/1)}"
else
""
end
end
defp url_from_spec(_upload, _base_url, {:url, url}), do: url
def base_url do
uploader = Config.get([Pleroma.Upload, :uploader])
upload_base_url = Config.get([Pleroma.Upload, :base_url])
public_endpoint = Config.get([uploader, :public_endpoint])
case uploader do
Pleroma.Uploaders.Local ->
upload_base_url || Pleroma.Web.Endpoint.url() <> "/media/"
Pleroma.Uploaders.S3 ->
bucket = Config.get([Pleroma.Uploaders.S3, :bucket])
truncated_namespace = Config.get([Pleroma.Uploaders.S3, :truncated_namespace])
namespace = Config.get([Pleroma.Uploaders.S3, :bucket_namespace])
bucket_with_namespace =
cond do
!is_nil(truncated_namespace) ->
truncated_namespace
!is_nil(namespace) ->
namespace <> ":" <> bucket
true ->
bucket
end
if public_endpoint do
Path.join([public_endpoint, bucket_with_namespace])
else
Path.join([upload_base_url, bucket_with_namespace])
end
_ ->
public_endpoint || upload_base_url || Pleroma.Web.Endpoint.url() <> "/media/"
end
end
def base_url(), do: Config.get([__MODULE__, :uploader]).base_url()
end

View file

@ -4,6 +4,7 @@
defmodule Pleroma.Uploaders.Local do
@behaviour Pleroma.Uploaders.Uploader
alias Pleroma.Config
@impl true
def get_file(_) do
@ -29,11 +30,13 @@ def put_file(upload) do
File.cp!(upload.tempfile, result_file)
end
:ok
url = [base_url(), URI.encode(upload.path, &Pleroma.Upload.char_unescaped?/1)] |> Path.join()
{:ok, upload |> Map.put(:url, url)}
end
def upload_path do
Pleroma.Config.get!([__MODULE__, :uploads])
Config.get!([__MODULE__, :uploads])
end
@impl true
@ -46,4 +49,9 @@ def delete_file(path) do
{:error, posix_error} -> {:error, to_string(posix_error)}
end
end
@impl true
def base_url() do
Config.get([Pleroma.Upload, :base_url]) || Pleroma.Web.Endpoint.url() <> "/media/"
end
end

View file

@ -47,7 +47,8 @@ def put_file(%Pleroma.Upload{} = upload) do
case ExAws.request(op) do
{:ok, _} ->
{:ok, {:file, s3_name}}
{:ok,
upload |> Map.put(:path, s3_name) |> Map.put(:url, Path.join([base_url(), s3_name]))}
error ->
Logger.error("#{__MODULE__}: #{inspect(error)}")
@ -67,6 +68,33 @@ def delete_file(file) do
end
end
@impl true
def base_url() do
upload_base_url = Config.get([Pleroma.Upload, :base_url])
public_endpoint = Config.get([Pleroma.Uploaders.S3, :public_endpoint])
bucket = Config.get([Pleroma.Uploaders.S3, :bucket])
truncated_namespace = Config.get([Pleroma.Uploaders.S3, :truncated_namespace])
namespace = Config.get([Pleroma.Uploaders.S3, :bucket_namespace])
bucket_with_namespace =
cond do
!is_nil(truncated_namespace) ->
truncated_namespace
!is_nil(namespace) ->
namespace <> ":" <> bucket
true ->
bucket
end
if public_endpoint do
Path.join([public_endpoint, bucket_with_namespace])
else
Path.join([upload_base_url, bucket_with_namespace])
end
end
@regex Regex.compile!("[^0-9a-zA-Z!.*/'()_-]")
def strict_encode(name) do
String.replace(name, @regex, "-")

View file

@ -34,24 +34,23 @@ defmodule Pleroma.Uploaders.Uploader do
* `:wait_callback` will wait for an http post request at `/api/pleroma/upload_callback/:upload_path` and call the uploader's `http_callback/3` method.
"""
@type file_spec :: {:file | :url, String.t()}
@callback put_file(upload :: struct()) ::
:ok | {:ok, file_spec()} | {:error, String.t()} | :wait_callback
:ok | {:ok, Pleroma.Upload.t()} | {:error, String.t()} | :wait_callback
@callback delete_file(file :: String.t()) :: :ok | {:error, String.t()}
@callback http_callback(Plug.Conn.t(), Map.t()) ::
{:ok, Plug.Conn.t()}
| {:ok, Plug.Conn.t(), file_spec()}
| {:error, Plug.Conn.t(), String.t()}
@optional_callbacks http_callback: 2
@callback base_url() :: String.t()
@spec put_file(module(), upload :: struct()) :: {:ok, file_spec()} | {:error, String.t()}
@callback http_callback(Plug.Conn.t(), Map.t(), Pleroma.Upload.t()) ::
{:ok, Plug.Conn.t(), Pleroma.Upload.t()}
| {:error, Plug.Conn.t(), String.t()}
@optional_callbacks http_callback: 3
@spec put_file(module(), upload :: struct()) :: {:ok, Pleroma.Upload.t()} | {:error, String.t()}
def put_file(uploader, upload) do
case uploader.put_file(upload) do
:ok -> {:ok, {:file, upload.path}}
{:ok, %Pleroma.Upload{}} = ok -> ok
:wait_callback -> handle_callback(uploader, upload)
{:ok, _} = ok -> ok
{:error, _} = error -> error
end
end
@ -61,10 +60,10 @@ defp handle_callback(uploader, upload) do
receive do
{__MODULE__, pid, conn, params} ->
case uploader.http_callback(conn, params) do
{:ok, conn, ok} ->
case uploader.http_callback(conn, params, upload) do
{:ok, conn, upload} ->
send(pid, {__MODULE__, conn})
{:ok, ok}
{:ok, upload}
{:error, conn, error} ->
send(pid, {__MODULE__, conn})

View file

@ -9,7 +9,7 @@ defmodule Pleroma.Upload.Filter.MogrifyTest do
alias Pleroma.Upload.Filter
test "apply mogrify filter" do
clear_config(Filter.Mogrify, args: [{"tint", "40"}])
clear_config([Filter.Mogrify, :args], [{"tint", "40"}])
File.cp!(
"test/fixtures/image.jpg",

View file

@ -37,10 +37,17 @@ def put_file(%{path: path} = _upload, module_name) do
describe "Tried storing a file when http callback response success result" do
defmodule TestUploaderSuccess do
def http_callback(conn, _params),
do: {:ok, conn, {:file, "post-process-file.jpg"}}
def http_callback(conn, _params, upload),
do:
{:ok, conn,
Map.merge(upload, %{
path: "post-process-file.jpg",
url: "http://localhost:4001/media/post-process-file.jpg"
})}
def put_file(upload), do: TestUploaderBase.put_file(upload, __MODULE__)
def base_url(), do: "http://localhost:4001/media/"
end
setup do: [uploader: TestUploaderSuccess]
@ -72,7 +79,7 @@ test "it returns file" do
describe "Tried storing a file when http callback response error" do
defmodule TestUploaderError do
def http_callback(conn, _params), do: {:error, conn, "Errors"}
def http_callback(conn, _params, _upload), do: {:error, conn, "Errors"}
def put_file(upload), do: TestUploaderBase.put_file(upload, __MODULE__)
end

View file

@ -21,10 +21,11 @@ test "put file to local folder" do
name: "image.jpg",
content_type: "image/jpeg",
path: file_path,
tempfile: Path.absname("test/fixtures/image_tmp.jpg")
tempfile: Path.absname("test/fixtures/image_tmp.jpg"),
url: "http://localhost:4001/media/local_upload/files/image.jpg"
}
assert Local.put_file(file) == :ok
assert Local.put_file(file) == {:ok, file}
assert Path.join([Local.upload_path(), file_path])
|> File.exists?()
@ -43,8 +44,8 @@ test "deletes local file" do
tempfile: Path.absname("test/fixtures/image_tmp.jpg")
}
:ok = Local.put_file(file)
local_path = Path.join([Local.upload_path(), file_path])
{:ok, file} = Local.put_file(file)
local_path = Path.join([Local.upload_path(), file.path])
assert File.exists?(local_path)
Local.delete_file(file_path)

View file

@ -59,7 +59,8 @@ test "it returns path with bucket namespace when namespace is set" do
name: "image-tet.jpg",
content_type: "image/jpeg",
path: "test_folder/image-tet.jpg",
tempfile: Path.absname("test/instance_static/add/shortcode.png")
tempfile: Path.absname("test/instance_static/add/shortcode.png"),
url: "https://s3.amazonaws.com/test_bucket/test_folder/image-tet.jpg"
}
[file_upload: file_upload]
@ -67,7 +68,7 @@ test "it returns path with bucket namespace when namespace is set" do
test "save file", %{file_upload: file_upload} do
with_mock ExAws, request: fn _ -> {:ok, :ok} end do
assert S3.put_file(file_upload) == {:ok, {:file, "test_folder/image-tet.jpg"}}
assert S3.put_file(file_upload) == {:ok, file_upload}
end
end