Merge pull request #8 from gabrielpra1/improvements

Improvements

README.md

@@ -49,14 +49,16 @@ defmodule MyApp.Uploader do
 
   @impl true
   def default_opts(Uploadex.FileStorage), do: [base_path: :code.priv_dir(:my_app), base_url: Endpoint.url()]
-  def default_opts(Uploadex.S3Storage), do: [bucket: "my_bucket", base_url: "https://my_bucket.s3-sa-east-1.amazonaws.com", upload_opts: [acl: :public_read]]
+  def default_opts(Uploadex.S3Storage), do: [bucket: "my_bucket", region: "sa-east-1", upload_opts: [acl: :public_read]]
 
   @impl true
-  def storage(%User{} = user), do: {Uploadex.FileStorage, directory: storage_dir(user)}
-  def storage(%Company{} = company), do: {Uploadex.S3Storage, directory: storage_dir(company)}
+  def storage(%User{id: id} = user), do: {Uploadex.FileStorage, directory: "/uploads/users/#{id}"}
+  def storage(%Company{} = company), do: {Uploadex.S3Storage, directory: "/thumbnails"}
 
-  def storage_dir(%User{id: user_id}), do: "/uploads/users/#{user_id}"
-  def storage_dir(%Company{}), do: "/thumbnails"
+  # Optional:
+  @impl true
+  def accepted_extensions(%User{}), do: ~w(.jpg .png)
+  def accepted_extensions(_any), do: :any
 end
 ```
 

lib/ecto/upload.ex

@@ -15,18 +15,18 @@ defmodule Uploadex.Upload do
   def type, do: :string
 
   @spec cast(upload_path | upload_binary) :: {:ok, upload_path} | {:ok, upload_binary} | :error | {:error, keyword()}
-  def cast(%{filename: _filename, path: path}) do
+  def cast(%{filename: filename, path: path}) do
     {:ok, %{
-      filename: generate_filename(),
+      filename: generate_filename(filename),
       path: path
     }}
   end
 
-  def cast(%{filename: _filename, binary: binary}) do
+  def cast(%{filename: filename, binary: binary}) do
     case FileProcessing.process_binary(binary) do
       {:ok, binary} ->
         {:ok, %{
-          filename: generate_filename(),
+          filename: generate_filename(filename),
           binary: binary
         }}
 
@@ -39,7 +39,7 @@ defmodule Uploadex.Upload do
 
   def cast(_), do: :error
 
-  defp generate_filename, do: Ecto.UUID.generate()
+  defp generate_filename(filename), do: Ecto.UUID.generate() <> Path.extname(filename)
 
   @spec load(any) :: :error | {:ok, binary}
   def load(filename) when is_binary(filename), do: {:ok, filename}

lib/files.ex

@@ -5,6 +5,8 @@ defmodule Uploadex.Files do
 
   @type record :: any()
 
+  alias Uploadex.Validation
+
   defp uploader!, do: Application.fetch_env!(:uploadex, :uploader)
 
   @doc """
@@ -15,11 +17,18 @@ defmodule Uploadex.Files do
   @spec store_files(record) :: {:ok, record} | {:error, any()}
   def store_files(record) do
     storage_opts = get_storage_opts(record)
+    files = wrap_files(record)
+    extensions = get_accepted_extensions(record)
 
-    record
-    |> wrap_files()
-    |> Enum.filter(&is_map/1)
-    |> do_store_files(record, storage_opts)
+    case Validation.validate_extensions(files, extensions) do
+      :ok ->
+        files
+        |> Enum.filter(&is_map/1)
+        |> do_store_files(record, storage_opts)
+
+      error ->
+        error
+    end
   end
 
   # Recursively stores all files, stopping if one operation fails.
@@ -126,4 +135,11 @@ defmodule Uploadex.Files do
     |> List.wrap()
     |> Enum.reject(&is_nil/1)
   end
+
+  defp get_accepted_extensions(record) do
+    case function_exported?(uploader!(), :accepted_extensions, 1) do
+      true -> uploader!().accepted_extensions(record)
+      false -> :any
+    end
+  end
 end

lib/storage/s3_storage.ex

@@ -4,16 +4,16 @@ defmodule Uploadex.S3Storage do
 
   ## Opts
 
-  * `bucket`: String (required for `c:Uploadex.Storage.store/2` and `c:Uploadex.Storage.delete/2`)
+  * `bucket`: String (required for all functions)
+  * `region`:  String (required for `c:Uploadex.Storage.get_url/2`)
   * `directory`: String (required for all functions)
   * `upload_opts`: Keyword list. This opts are passed to `ExAws.S3.upload/4` and `ExAws.S3.put_object/4` (required for `c:Uploadex.Storage.store/2`)
-  * `base_url`:  String (required for `c:Uploadex.Storage.get_url/2`)
 
   ## Example
 
     To use this storage for your `User` record, define these functions in your `Uploadex.Uploader` implementation:
 
-      def default_opts(Uploadex.S3Storage), do: [bucket: "my_bucket", base_url: "https://my_bucket.s3-sa-east-1.amazonaws.com", upload_opts: [acl: :public_read]]
+      def default_opts(Uploadex.S3Storage), do: [bucket: "my_bucket", region: "sa-east-1", upload_opts: [acl: :public_read]]
 
       def storage(%User{} = user), do: {Uploadex.S3Storage, directory: "/photos"}
   """
@@ -61,9 +61,12 @@ defmodule Uploadex.S3Storage do
   @impl true
   def get_url(%{filename: filename}, opts), do: get_url(filename, opts)
   def get_url(filename, opts) when is_binary(filename) do
-    base_url = Keyword.fetch!(opts, :base_url)
+    bucket = Keyword.fetch!(opts, :bucket)
+    region = Keyword.fetch!(opts, :region)
     directory = Keyword.fetch!(opts, :directory)
 
+    base_url = "https://#{bucket}.s3-#{region}.amazonaws.com"
+
     base_url
     |> Path.join(directory)
     |> Path.join(filename)

lib/uploader.ex

@@ -6,7 +6,6 @@ defmodule Uploadex.Uploader do
 
       defmodule MyApp.Uploader do
         @moduledoc false
-
         @behaviour Uploadex.Uploader
 
         alias MyAppWeb.Endpoint
@@ -17,15 +16,18 @@ defmodule Uploadex.Uploader do
 
         @impl true
         def default_opts(Uploadex.FileStorage), do: [base_path: :code.priv_dir(:my_app), base_url: Endpoint.url()]
-        def default_opts(Uploadex.S3Storage), do: [bucket: "my_bucket", base_url: "https://my_bucket.s3-sa-east-1.amazonaws.com", upload_opts: [acl: :public_read]]
+        def default_opts(Uploadex.S3Storage), do: [bucket: "my_bucket", region: "sa-east-1", upload_opts: [acl: :public_read]]
 
         @impl true
-        def storage(%User{} = user), do: {Uploadex.FileStorage, directory: storage_dir(user)}
-        def storage(%Company{} = company), do: {Uploadex.S3Storage, directory: storage_dir(company)}
+        def storage(%User{id: id} = user), do: {Uploadex.FileStorage, directory: "/uploads/users/\#{id}"}
+        def storage(%Company{} = company), do: {Uploadex.S3Storage, directory: "/thumbnails"}
 
-        def storage_dir(%User{id: user_id}), do: "/uploads/users/\#{user_id}"
-        def storage_dir(%Company{}), do: "/thumbnails"
+        # Optional:
+        @impl true
+        def accepted_extensions(%User{}), do: ~w(.jpg .png)
+        def accepted_extensions(_any), do: :any
       end
+
   """
 
   @type record :: any()
@@ -34,4 +36,7 @@ defmodule Uploadex.Uploader do
   @callback get_files(record) :: file | [file]
   @callback default_opts(module :: atom()) :: opts :: Keyword.t()
   @callback storage(record) :: {module :: atom(), opts :: Keyword.t()}
+  @callback accepted_extensions(record) :: list(String.t())
+
+  @optional_callbacks accepted_extensions: 1
 end

lib/validation.ex

@@ -0,0 +1,33 @@
+defmodule Uploadex.Validation do
+  @moduledoc false
+
+  @spec validate_extensions(list(any()), any()) :: :ok | {:error, any()}
+  def validate_extensions(files, accepted_extensions) do
+    case all_extensions_accepted?(files, accepted_extensions) do
+      true ->
+        :ok
+
+      false ->
+        {:error, "Some files in #{inspect(get_file_names(files))} violate the accepted extensions: #{inspect(accepted_extensions)}"}
+    end
+  end
+
+  defp all_extensions_accepted?(_files, :any), do: true
+  defp all_extensions_accepted?(files, extensions) do
+    list_extensions = List.wrap(extensions)
+    Enum.all?(files, & extension_accepted?(&1, list_extensions))
+  end
+
+  defp extension_accepted?(%{filename: filename}, accepted_extensions), do: extension_accepted?(filename, accepted_extensions)
+  defp extension_accepted?(filename, accepted_extensions) when is_binary(filename) do
+    extension = filename |> Path.extname() |> String.downcase()
+    Enum.member?(accepted_extensions, extension)
+  end
+
+  def get_file_names(files) do
+    Enum.map(files, fn
+      %{filename: filename} -> filename
+      filename -> filename
+    end)
+  end
+end

test/files_test.exs

@@ -18,6 +18,11 @@ defmodule UploadexTest do
       assert {:ok, %{}} = Files.store_files(user)
       assert user.files == Uploadex.TestStorage.get_stored()
     end
+
+    test "fails when extension is not accepted" do
+      user = %User{files: [%{filename: "file.pdf"}]}
+      assert {:error, "Some files in [\"file.pdf\"] violate the accepted extensions: [\".jpg\", \".png\"]"} = Files.store_files(user)
+    end
   end
 
   describe "delete_files/1" do
@@ -35,8 +40,8 @@ defmodule UploadexTest do
     end
 
     test "with changed files" do
-      assert {:ok, %{}} = Files.delete_previous_files(%User{files: [%{filename: "2"}]}, %User{})
-      assert [%{filename: "1"}] == TestStorage.get_deleted()
+      assert {:ok, %{}} = Files.delete_previous_files(%User{files: [%{filename: "2.jpg"}]}, %User{})
+      assert [%{filename: "1.jpg"}] == TestStorage.get_deleted()
     end
   end
 
@@ -74,7 +79,7 @@ defmodule UploadexTest do
 
   describe "Storage opts" do
     test "is passed to the storage considering default opts" do
-      user = %User{files: [%{filename: "file"}]}
+      user = %User{files: [%{filename: "file.png"}]}
       default_opts = TestUploader.default_opts(TestStorage)
       {TestStorage, custom_opts} = TestUploader.storage(user)
 

test/s3_storage_test.exs

@@ -0,0 +1,17 @@
+defmodule S3StorageTest do
+  use ExUnit.Case
+
+  alias Uploadex.S3Storage
+
+  @opts [bucket: "my-bucket", region: "sa-east-1", directory: "thumbnails"]
+
+  describe "get_url/2" do
+    test "accepts both a map with filename and the filename directly" do
+      assert S3Storage.get_url(%{filename: "filename.jpg"}, @opts) == S3Storage.get_url("filename.jpg", @opts)
+    end
+
+    test "builds the URL correctly" do
+      assert "https://my-bucket.s3-sa-east-1.amazonaws.com/thumbnails/filename.jpg" == S3Storage.get_url("filename.jpg", @opts)
+    end
+  end
+end

test/support/test_uploader.ex

@@ -14,4 +14,7 @@ defmodule TestUploader do
   def storage(%User{}) do
     {Uploadex.TestStorage, [directory: "test/dir"]}
   end
+
+  @impl true
+  def accepted_extensions(%User{}), do: ~w(.jpg .png)
 end

test/support/user.ex

@@ -1,5 +1,5 @@
 defmodule User do
   @moduledoc false
 
-  defstruct files: [%{filename: "1"}, %{filename: "2"}]
+  defstruct files: [%{filename: "1.jpg"}, %{filename: "2.jpg"}]
 end

test/upload_type_test.exs

@@ -11,6 +11,7 @@ defmodule UploadTypeTest do
 
       assert upload.path == path
       refute upload.filename == filename
+      assert String.contains?(filename, ".jpg")
     end
 
     test "for base64 binary" do
@@ -20,6 +21,7 @@ defmodule UploadTypeTest do
 
       assert Base.decode64!("/9j/4AAQSkZJRgABAQAAAQAB") === binary
       refute upload.filename == filename
+      assert String.contains?(filename, ".jpg")
     end
 
     test "for already processed binary" do
@@ -29,6 +31,7 @@ defmodule UploadTypeTest do
 
       assert processed_binary == binary
       refute upload.filename == filename
+      assert String.contains?(filename, ".jpg")
     end
 
     test "for string" do

test/validation_test.exs

@@ -0,0 +1,22 @@
+defmodule ValidationTest do
+  use ExUnit.Case
+
+  alias Uploadex.Validation
+
+  describe "validate_extensions/2" do
+    test "returns :ok when accepted extensions is :any" do
+      assert :ok == Validation.validate_extensions(["anything"], :any)
+    end
+
+    test "handles both maps and strings" do
+      assert :ok == Validation.validate_extensions([%{filename: "1.jpg"}, "2.jpg"], ".jpg")
+      assert {:error, _} = Validation.validate_extensions([%{filename: "1.jpg"}, "2.png"], ".jpg")
+      assert {:error, _} = Validation.validate_extensions([%{filename: "1.png"}, "2.jpg"], ".jpg")
+    end
+
+    test "handles uppercase extensions" do
+      assert :ok == Validation.validate_extensions(["AAA.JPG"], ".jpg")
+      assert :ok == Validation.validate_extensions(["AAA.JpG"], ".jpg")
+    end
+  end
+end