arc_ecto
arc_ecto copied to clipboard
`cast_attachments` uploads files even when passed invalid changeset
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? :)
Can you show me your schema / changesets?
Arc should not be storing files via cast and cast_attachments in the same changeset.
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
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!
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.
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.
Anything new with this issue? I have the same problem..
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.
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.
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:
-
avoid to use
cast
on the upload field in favor ofcast_attachments
, otherwise the upload will be processed/copied twice -
add a
uuid
field to the schema and generate a UUID in the changeset, this lets to have ascope.uuid
when callingstorage_dir
and/orfilename
-
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!
@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.
@GBH I would love to read a detailed blog post on your strategy.
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. :)
@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.
@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.