url/1 returns non existing files

SachsKaylee commented 4 years ago

Hello there!

This is probably me not understanding how to correctly use this library, but I'd really appreciate some input and a nudge in the right direction.

Environment

Elixir version (elixir -v): Erlang/OTP 22 [erts-10.4.4] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe] - Elixir 1.9.1 (compiled with Erlang/OTP 21)
Arc version (mix deps): arc 0.11.0 (Hex package) (mix) (locked at 0.11.0 (arc) ac7a0cc0), arc_ecto 0.11.2 (Hex package) (mix) (locked at 0.11.2 (arc_ecto) bd9b0c78)
Arc dependencies when applicable (mix deps): Not relevant
Operating system: Ubuntu 18.04.3 LTS

Expected behavior

When calling the url function of an uploader module it returns paths of files that don't exist instead of returning the default URL.

This is my upload module - Note that for now I am using local storage instead of S3 storage. We will migrate to S3 later, but for now it is what it is:

defmodule Sahnee.Uploaders.Avatar do
  use Arc.Definition
  use Arc.Ecto.Definition

  @versions [:original]

  def __storage, do: Arc.Storage.Local

  def validate({file, _}) do
    # On a separate note: file extension != actual file type. 
    # There is probably a way to validate this with this library using 
    # an image magick function somehow, but I haven't really looked at this part yet
    ~w(.jpg .jpeg .gif .png) |> Enum.member?(Path.extname(file.file_name))
  end

  def filename(version, _) do
     version
  end

  def storage_dir(_version, {_file, scope}) do
      Application.app_dir(:sahnee, "./priv/dyn_content/avatar/#{scope.id}")
  end

  def default_url(_version, scope) do
    "https://api.adorable.io/avatars/192/#{scope.id}"
  end
end

Not let's store a file:

iex(259)> Sahnee.Uploaders.Avatar.store({"/home/patrick/Pictures/my_avatar.png", %{id: 1}})
{:ok, "my_avatar.png"}

And get its URL:

iex(260)> Sahnee.Uploaders.Avatar.url({"home/patrick/Pictures/my_avatar.png", %{id: 1}})
"/home/patrick/Development/sahnee/_build/dev/lib/sahnee/./priv/dyn_content/avatar/1/original.png"

patrick@PATRICK:~$ stat /home/patrick/Development/sahnee/_build/dev/lib/sahnee/./priv/dyn_content/avatar/1/original.png | grep Change
Change: 2019-09-14 15:03:31.416812752 +0200

Looks great. Now let's get the avatar of user ID 2 which we haven't uploaded so far. I'm expecting this to return https://api.adorable.io/avatars/192/2 since the file doesn't exist on disk.

Actual behavior

iex(261)> Sahnee.Uploaders.Avatar.url({"home/patrick/Pictures/my_other_avatar.png", %{id: 2}})
"/home/patrick/Development/sahnee/_build/dev/lib/sahnee/./priv/dyn_content/avatar/2/original.png"

patrick@PATRICK:~$ stat /home/patrick/Development/sahnee/_build/dev/lib/sahnee/./priv/dyn_content/avatar/2/original.png | grep Change
stat: cannot stat '/home/patrick/Development/sahnee/_build/dev/lib/sahnee/./priv/dyn_content/avatar/2/original.png': No such file or directory

This is not what I was hoping for.

The documentation says that url will return the default URL whenever nil is passed as file name. But I can't see a situation in which this would realistically happen at the moment.

Assuming whenever I save the user I'll save the avatar URL with it in the database. If the user doesn't have an avatar the avatar will be null. This would make Arcs logic work, but I don't like this because this saves absolute paths on the host machine in the database. In the event of a server migration these paths could change, or in the event of a security vulerability(SQL injection, ...) the user might change their avatar to something like /etc/passwd.

What I'm looking for is a url like function, just without the first paramter. I don't need anything but the user(=the scope) and the version to generate the file path. Since paths specified by the user are inherently untrustworthy I don't see why I would even want an option to specify it.

iex(262)> Sahnee.Uploaders.Avatar.url(%{id: 1})
"/home/patrick/Development/sahnee/_build/dev/lib/sahnee/./priv/dyn_content/avatar/1/original.png"
iex(263)> Sahnee.Uploaders.Avatar.url(%{id: 2})
"https://api.adorable.io/avatars/192/2"

I hope that my ranting was somewhat understandable and apologize for the wall of text. Am I misunderstanding how to use this library?

achempion commented 4 years ago

As you can see we don't check for file to exist at the stage of generating url. You have to do it manually and pass proper arguments.

The point of this is you, in most cases, should already knew does the file exist of not, because you have to persist some information about the file.

https://github.com/stavro/arc/blob/master/lib/arc/storage/local.ex#L16

If you have any suggestion or additional questions feel free to ask. I'm maintaining the fork of the arc project named Waffle.

SachsKaylee commented 4 years ago

Thanks for information. We've decided to roll with our own implementation inspired by ExFile and Arc.

stavro / arc