cast_attachments uploads files even when passed invalid changeset

[ephe-meral]

I'm using cast_attachments similar to the example provided with the README. I've experienced that the files are uploaded even though the normal cast that ran before this failed.

I'd expect the subsequent call to cast_attachments to not store files. In my specific case I was generating the filename from the scope data, which were invalid, so the filename was invalid too. Yet still the file was created on the S3 storage provided.

Is this the expected behaviour, and if yes then why so? :)

[stavro]

Can you show me your schema / changesets?

Arc should not be storing files via cast and cast_attachments in the same changeset.

[ephe-meral]

I was (i guess wrongly) trying to use different Arc file versions for two files of the same kind that are attached to my record (see schema). The code is roughly like the following, and the thing was that the image name validation succeeded, but the product and version of the metadata record were empty which is not allowed. Yet still it got uploaded...
In my test scenario I only attached one of the files to the multipart POST btw - and only that got uploaded (twice for both versions of the file (which is not what I actually want) and with a broken name).

defmodule TestProject.TestModel do
  use TestProject.Web, :model
  use Arc.Ecto.Model
  alias TestProject.TestModelFile

  schema "test_models" do
    field :product,      :string
    field :version,      :integer
    field :type_a_file,  TestModelFile.Type
    field :type_b_file,  TestModelFile.Type

    timestamps
  end

  @required_fields ~w(product version)
  @optional_fields ~w()

  @required_file_fields ~w(type_a_file type_b_file)
  @optional_file_fields ~w()

  def changeset(model, params \\ :empty) do
    model
    |> cast(params, @required_fields, @optional_fields)
    |> cast_attachments(params, @required_file_fields, @optional_file_fields)
  end
end

defmodule TestProject.TestModelFile do
  use Arc.Definition
  use Arc.Ecto.Definition

  # This is another thing apart from this issue, but i was trying to upload a
  # 'type_a' version and a 'type_b' version of the same kind of file - 
  # transformations where applied beforehand -
  # until I noticed that I cannot pass in the version of the file that
  # the two entries in the schema refer to
  @versions [:type_a, :type_b]

  def validate({file, _}), do: ~w(.img) |> Enum.member?(Path.extname(file.file_name))

  def filename(_version, {_file, %{
      product: product,
      version: version_num}}) do
    "#{product}-#{inspect version_num}.img"
  end

  def storage_dir(version, _opts) do
    "uploads/#{version}_files"
  end
end

[tute]

https://github.com/elixir-lang/ecto/blob/v1.1.8/lib/ecto/query/planner.ex#L29 calls adapter.dump(type, value) (where value is the string "test/document_fixtures/title_report.html").

But arity for https://github.com/stavro/arc_ecto/blob/v0.3.2/lib/arc_ecto/type.ex#L19 seems different: dump(definition, %{file_name: file_name, updated_at: updated_at}). Am I having a version mismatch? This is the stacktrace:

  1) test html_contents/1 returns body of file (DocumentViewTest)
     test/views/document_view_test.exs:5
     ** (FunctionClauseError) no function clause matching in Arc.Ecto.Type.dump/2
     stacktrace:
       lib/arc_ecto/type.ex:19: Arc.Ecto.Type.dump(Prodeal360.DocumentUploader, "test/document_fixtures/title_report.html")
       (ecto) lib/ecto/query/planner.ex:29: anonymous fn/6 in Ecto.Query.Planner.fields/4
       (stdlib) lists.erl:1262: :lists.foldl/3
       (ecto) lib/ecto/query/planner.ex:21: Ecto.Query.Planner.fields/4
       (ecto) lib/ecto/repo/schema.ex:449: Ecto.Repo.Schema.dump_changes/5
       (ecto) lib/ecto/repo/schema.ex:77: anonymous fn/11 in Ecto.Repo.Schema.do_insert/4
       (ecto) lib/ecto/repo/schema.ex:14: Ecto.Repo.Schema.insert!/4
       (ex_machina) lib/ex_machina/ecto.ex:157: ExMachina.Ecto.save_record/3
       test/views/document_view_test.exs:6

Thank you!

[gullitmiranda]

would it not be better to upload just the cast_attachments (or new method called store_attachments)? Can be added an option that define whether the upload should occur in the Arc.Ecto.Type.cast. the way it is done now, it's hard to make validations and other processes, that the upload occurs on every cast.

I did an implementation itself of Type in my projects. If they like the idea I can make a PR with this improvement.

[joshchernoff]

It would appear to me we have 3 concerns.
1: The changeset is persisted and sanitized ("cast_attachments")
This could be responsible for just setting the string.

2: The changeset is valid ("validate_attachments")
This could run any form of validations defined in the uploader from file size to type.

3: The attachments are stored ("store_attachments")
This could be responsible for processing and uploading all of which you would not have done unless its a valid change set.

And as side note about validation, what are the plans for caching the attachment if another field is invalid? Currently unless someone managed this by hand you would have to reselect the file in the form after another field failed.

[szabolcsmaj]

Anything new with this issue? I have the same problem..

[andreapavoni]

pinging this issue. I'm trying to working around these problems manually, but the resulting code is not that clean. cast_attachments does too much things and is very difficult to track what it's doing.

[GBH]

Just want to throw my 2 cents here. I think arc_ecto doesn't really function in a logical way. Apart from processing upload when changeset is invalid it's also not really possible to attach file to the newly created record. That's like the most important thing it should be able to do.

I'm comparing it to paperclip for rails. Paperclip, while not perfect, manages to handle 99% of usecases correctly. Mostly because after_save callback is a thing in Rails. Now ecto deprecated callbacks, so what do we have left?

Right now I can only think about killing off arc_ecto and manually saving file from the controller after I can validate changeset and uploaded file validity. Then I can save record and save file so I have scope.id present.

[andreapavoni]

Hi @GBH,

Even if your points make sense, I think you're comparing two different contexts.

First of all, Elixir IS NOT an OOP language like Ruby, this means you don't have a mutable/internal state in your structure that let's you do my_model.id after calling something like save on it.

As you said, all the around callbacks such as before_* and after_* were deprecated since Ecto 2.x in favor of a better approach (TLDR: use changesets for before actions and Multi for after ones).

That said, I've spent several hours in the last week working on file uploads for my app and I've explored several paths until I ended up with plain Arc.Ecto and some glue code:

1. avoid to use cast on the upload field in favor of cast_attachments, otherwise the upload will be processed/copied twice

2. add a uuid field to the schema and generate a UUID in the changeset, this lets to have a scope.uuid when calling storage_dir and/or filename

3. add some logic in the changeset to delete the uploaded file if the changeset isn't valid. Not the cleanest solution, but it works for my case. Moreover it eventually let's to add more logic to cache the uploaded file for later use (eg: re-submit the form)

Before arriving to this solution, I've tried to pattern match the result from Repo.insert and then process the upload, but this has some problems when it comes to multiple uploads, because it would require to parse the results and associate the files from the params.
I've also tried to rewrite Arc.Ecto.Type to call store from dump rather than from cast, but the only advantage is to process the upload after cast has validated the changeset, so not a big deal.

Hope this helps as inspiration for others with the same problem. I think I will publish some code/tutorial soon (when/if I'll have some spare time :P).

cheers!

[GBH]

@andreapavoni I'd love to see what you got. For now I'd do the uuid thing and not worry about files being created for no reason.

[jayjun]

@GBH I would love to read a detailed blog post on your strategy.

[Kurisu3]

To keep arc_ecto from saving files on disk until changeset is valid, I use ecto changeset function : prepare_changes in my schema. This function will call arc_ecto cast_attachments only when the changeset is valid and ready for database insertion.

Something like this :

defmodule Hello.Client do
  use Hello.Web, :model
  use Arc.Ecto.Schema

  schema "clients" do
    field :name, :string
    field :age, :integer
    field :uuid, :string
    field :avatar, Hello.Avatar.Type

    timestamps()
  end

  @doc """
  Builds a changeset based on the `struct` and `params`.
  """
  def changeset(struct, params \\ %{}) do
    struct
    |> Map.update(:uuid, Ecto.UUID.generate,
                  fn value -> value || Ecto.UUID.generate end)
    |> cast(params, [:name, :age, :uuid])
    |> validate_required([:name, :age])
    |> prepare_changes(fn(changeset) ->
        cast_attachments(changeset, params, [:avatar])
       end)
  end

end

The only problem is when you have database constraints that could make ecto repo insertion fail. (For example an unique_constraint on a given field of your schema). But you can handle this in your controller by checking changeset errors and deleting the uploaded files if necesseray. :)

[xfumihiro]

@kurisusan using prepare_changes however only works for cases that has modified fields other than attachments. If only attachments are modified, cast_attachments won't be called.

[Kurisu3]

@xfumihiro you're right.
My strategy is to use that solution only for "create" changeset, assuming some fields other than attachments are required.
For record update, it is easier to have separate changesets. One with no attachments and the other for only attachment.
Of course we could do the same thing for record insertion in the first place.
And maybe a bit tricky, we could add some virtual field and a hidden value in the form to force the changeset state ?

I really hope this issue will finally be fixed someday.