Skip to content

Commit

Permalink
Fix resolve_file_name called twice to the same file (#1)
Browse files Browse the repository at this point in the history
Hello! Thank you for your library.

We identified a bug where the resolve_file_name is called twice to the
same file.

If we a resolve file name function like suggested in the docs:

    file_name = Path.basename(file.file_name, Path.extname(file.file_name))

    "#{version}_#{file_name}"

If that function is called twice for the same file, instead of having "thumb_filename.jpg",
we end having "thumb_thumb_filename.jpg".

The resolve_file_name is called first in the abstract logic:
https://github.com/elixir-waffle/waffle/blob/c781d5a7e7058d297783669ad1a830bd8928c73d/lib/waffle/actions/store.ex#L134

Then again here. The code change I'm proposing basically imitates the AWS adapter
that just call file.file_name:

https://github.com/elixir-waffle/waffle/blob/c781d5a7e7058d297783669ad1a830bd8928c73d/lib/waffle/storage/s3.ex#L144
  • Loading branch information
ulissesalmeida authored Mar 25, 2024
1 parent 59ad4f5 commit 0e9e904
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 12 deletions.
12 changes: 2 additions & 10 deletions lib/waffle/storage/google/cloud_storage.ex
Original file line number Diff line number Diff line change
Expand Up @@ -105,18 +105,10 @@ defmodule Waffle.Storage.Google.CloudStorage do
Returns the full file path for the upload destination.
"""
@spec path_for(Types.definition(), Types.version(), Types.meta()) :: String.t()
def path_for(definition, version, meta) do
def path_for(definition, version, meta = {file, _scope}) do
definition
|> storage_dir(version, meta)
|> Path.join(fullname(definition, version, meta))
end

@doc """
A wrapper for `Waffle.Definition.Versioning.resolve_file_name/3`.
"""
@spec fullname(Types.definition(), Types.version(), Types.meta()) :: String.t()
def fullname(definition, version, meta) do
Waffle.Definition.Versioning.resolve_file_name(definition, version, meta)
|> Path.join(file.file_name)
end

@spec data({Types.file(), String.t()}) :: {:file | :binary, String.t()}
Expand Down
6 changes: 4 additions & 2 deletions test/waffle/storage/google/cloud_storage_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ defmodule Waffle.Storage.Google.CloudStorageTest do

def random_name(_) do
name = 8 |> :crypto.strong_rand_bytes() |> Base.encode16()
%{name: name, path: "#{@remote_dir}/#{name}.png"}
%{name: "#{name}.png", path: "#{@remote_dir}/#{name}.png"}
end

def create_wafile(_), do: %{wafile: Waffle.File.new(@file_path, DummyDefinition)}

def setup_waffle(%{wafile: file, name: name}) do
file = Map.put(file, :file_name, name)

%{
definition: DummyDefinition,
version: :original,
Expand Down Expand Up @@ -132,7 +134,7 @@ defmodule Waffle.Storage.Google.CloudStorageTest do
Application.put_env(:waffle, :asset_host, "cdn-domain.com")

assert CloudStorage.url(def, ver, meta) ==
"https://cdn-domain.com/#{@remote_dir}/#{name}.png"
"https://cdn-domain.com/#{@remote_dir}/#{name}"

Application.delete_env(:waffle, :asset_host)
end
Expand Down

0 comments on commit 0e9e904

Please sign in to comment.