martide/arc_gcs

Wrong filepath posted to bucket

Closed this issue · 2 comments

I have an uploader with a custom filename defined along the lines of:

defmodule Radar.Uploaders.Flag do
  @moduledoc """
  Upload country flag images for use throughout the platform.
  """
  use Arc.Definition

  # Include ecto support (requires package arc_ecto installed):
  use Arc.Ecto.Definition

  # allow public read of all files
  # https://cloud.google.com/storage/docs/xml-api/reference-headers#xgoogacl
  @acl "public-read"
  @versions [:original, :main, :list]

  # Whitelist file extensions:
  def validate({file, _}) do
    ext_name = Path.extname(file.file_name) |> String.downcase()
    ~w(.jpg .jpeg .png) |> Enum.member?(ext_name)
  end

  # Define a thumbnail transformation:
  def transform(:main, _) do
    resize_to_limit("320x320")
  end

  def transform(:list, _) do
    resize_to_limit("80x80")
  end

  def resize_to_limit(size_str) do
    {:convert, "-strip -thumbnail #{size_str}\> -format jpg", :jpg}
  end

  # Override the persisted filenames:
  def filename(version, {file, scope}) do
    name = Path.basename(file.file_name, Path.extname(file.file_name))
    "#{scope.id}_#{version}_#{name}"
  end

  # Override the storage directory:
  def storage_dir(_version, {_file, scope}) do
    "uploads/countries/#{scope.country_code}"
  end

  # Provide a default URL if there hasn't been a file uploaded
  def default_url(version, _scope) do
    "/images/flag/missing_#{version}.jpg"
  end

  # Specify custom headers for s3 objects
  # Available options are [:cache_control, :content_disposition,
  #    :content_encoding, :content_length, :content_type,
  #    :expect, :expires, :storage_class, :website_redirect_location]
  #
  # def s3_object_headers(version, {file, scope}) do
  #   [content_type: Plug.MIME.path(file.file_name)]
  # end

end

The urls are returned as follows:

iex(10)> Radar.Uploaders.Flag.urls({c.flag, c})
%{
  list: "https://storage.googleapis.com/company-bucket/uploads/countries/TH/93c31206-0159-41b3-a255-83417b792e22_list_TH.jpg?v=63685274697",
  main: "https://storage.googleapis.com/company-bucket/uploads/countries/TH/93c31206-0159-41b3-a255-83417b792e22_main_TH.jpg?v=63685274697",
  original: "https://storage.googleapis.com/company-bucket/uploads/countries/TH/93c31206-0159-41b3-a255-83417b792e22_original_TH.jpg?v=63685274697"
}

However, the filenames stored on the server are:

https://storage.googleapis.com/company_bucket/uploads/countries/TH/93c31206-0159-41b3-a255-83417b792e22_list_93c31206-0159-41b3-a255-83417b792e22_list_img.jpg
https://storage.googleapis.com/company_bucket/uploads/countries/TH/93c31206-0159-41b3-a255-83417b792e22_main_93c31206-0159-41b3-a255-83417b792e22_main_img.jpg
https://storage.googleapis.com/company_bucket/uploads/countries/TH/93c31206-0159-41b3-a255-83417b792e22_original_93c31206-0159-41b3-a255-83417b792e22_original_img.jpg

i.e. the file posted to the server has the filename format of: #{scope.id}_#{version}_#{scope.id}_#{version}_#{name}

when it should be simply: #{scope.id}_#{version}_${name}

Any ideas?
Your help is greatly appreciated.

After testing, I think I found part of the problem.
Something is wrong by the use of Arc.Definition.Versioning.resolve_file_name(definition, version, file_and_scope) over here

By the time this function is called, the Arc.File looks something like the following:

%Arc.File{
   binary: nil,
   file_name: "1_list_image.jpg",
   path: "/var/folders/7n/l6vc139d0c7bswg8b84j45s80000gn/T/QEWTHIDYJLEA2IDVIQXGVLPAB5EUWCWU.png"
 }

Note how file_name already has the format that the definition has defined. Then when we call Arc.Definition.Versioning.resolve_file_name, we are essentially running through the definition's filename function again resulting in a redundant operation. I'm not sure what the intention is and would love some feedback. In the meantime, I'll continue to work on a fix.

I found the bug and have a pull request finished.

The issue is that the file path must be calculated separately in the put function as arc has already modified the file.file_name value to reflect the output of the definition's filename function.

Therefore, we can't use do_gcs_key at this point in time.

I made some changes that take the same approach as arc's S3 layer - namely, calculate path during the put function.