Changing UploadedFile to not always write to disk

[refi64]

Right now Lucky::UploadedFile always saves the content to a temporary file before control is passed into the application. This has a few issues:

• There's no way to limit whether or not the file is downloaded based on its metadata / size.
• The size of uploaded files is always limited to the size of the temporary storage location.
• The file can't be directly streamed elsewhere (e.g. sent to a microservice or uploaded) without hitting the disk / tmpfs.

Now, here's my idea: UploadedFile doesn't actually save the file yet, the constructor just takes the given IO value and saves it. Then, a new method .save is added that will save it to disk and return the File instance, and another method .content / .body is added to return the IO stream. (.path would also have to be removed.) That way, if someone really needs it on disk, they can easily change their code, but anyone who can handle the direct stream doesn't need to save to disk first.

Thoughts?

[matthewmcgarvey]

@refi64 Could I ask for some examples of how other frameworks do what you're looking for. I think get what you're asking for, but I want to make sure we land on a good solution that makes sense to everyone involved.

[matthewmcgarvey]

Thanks for doing that. I makes it easier for me to conceptualize what your asking for if you can point to what others do 😄

So to summarize things I've taken away from your comment:

• We can't know the size of file uploads until we have fully read the IO
• We provide a solution that's pretty standard across frameworks at the moment
• We have no built-in way to validate the file size of UploadedFile
• We have no way to "pipe" the IO without reading it

So I think if we say the first point is an understandable limitation, I think we could boil this down to two different updates to Lucky.

1. We could add a validation for file size (this would be an update to Avram, see Avram::Uploadable)
2. We update Lucky::UploadedFile api to not immediately create a tempfile with the IO data
   a. I think it might make sense that Lucky::UploadedFile could be a fork in the road of sorts. You could provide two main methods #tempfile and #raw. The first would return an object with the same API we currently have, the other would just be the IO or the HTTP::FormData::Part.

Looking at this https://luckyframework.org/guides/handling-files/file-uploads I'm not sure how the new API would work with Avram, though.

@jwoertink Maybe you could weigh in?

[matthewmcgarvey]

Personally, I think I'm OK if you wanted to open an issue on Avram for adding the file size validation and starting that. I think the other might require a bit more discussion

[jwoertink]

We've been doing quite a bit of upload stuff. Not quite with the Lucky::UploadedFile, but @russ might be able to weigh in on things we could port over or not.

I'm with @matthewmcgarvey on the opening an issue for size validation. I think that sounds fine.

[refi64]

Personally, I think I'm OK if you wanted to open an issue on Avram for adding the file size validation and starting that.

Well, from what I can tell, the other frameworks (well, at least Flask and Django) generally check the content size during the download, not afterwards, so it would still be Lucky issue 😅

[jwoertink]

Maybe it could be two fold? SaveOperations that need a file should have some sort of validation method to ensure the file matches some criteria. So in that case, it would be an Avram issue. But we could maybe also add some sort of "operation-like" object in Lucky that could be some sort of base wrapper around uploading. I'm picturing something similar to Carrierwave uploader. Lucky wouldn't come with any sort of image processing, but maybe this could be a thing where if you had your own processor, you just include in your uploader class....

I'm still waking up here, so it's a super quarter-baked idea, but maybe that sparks a few other better ideas? 😬