Allow file uploads with attribute

[wout]

Purpose

This allows us to use Lucky::UploadedFile as a regular attribute in operations.

Related discussion:
luckyframework/lucky#1195

Description

Usage

This is what it looks like in an operation:

class SaveUser < User::SaveOperation
  permit_columns name

  attribute avatar : Lucky::UploadedFile

  before_save do
    # ...
  end
end

To make file uploads easier for external libraries, you can also define your own class, as long as it inherits from Lucky::UploadedFile OR it includes Avram::Uploadable:

class FancyUploadedFile
  include Avram::Uploadable
end

class SaveUser < User::SaveOperation
  # ...
  attribute avatar : FancyUploadedFile
  # ...
end

The returned object will be of the declared type:

SaveUser < User::SaveOperation
  attribute avatar : FancyUploadedFile
  
  before_save do
    typeof(avatar.value) # => FancyUploadedFile
  end
end

A complete operation with validations would look like this:

class SaveUser < User::SaveOperation
  attribute avatar : Lucky::UploadedFile

  before_save do
    if file = avatar.value
      avatar.add_error("must be a .png image") unless file.filename.ends_with?(".png")
    else
      avatar.add_error("is required")
    end

    avatar.value.try do |file|
      if avatar.valid?
        # send to S3, store locally, ...
        avatar_path.value = "/path/to/#{file.filename}"
      end
    end
  end
end

Required other changes

In Lucky only Lucky::UploadedFile needs a few changes. Basically, all methods except initialize can go and it should include Avram::Uploadable which now adds the required methods:

# This class represents an uploaded file from a form
class Lucky::UploadedFile
  include Avram::Uploadable

  # Creates an UploadedFile using a HTTP::FormData::Part.
  #
  # The new file will be assigned the **name** of the
  # provided HTTP::FormData::Part and the **tempfile** will
  # be assigned the body of the HTTP::FormData::Part
  def initialize(part : HTTP::FormData::Part)
    @name = part.name
    @tempfile = File.tempfile(part.name) do |tempfile|
      IO.copy(part.body, tempfile)
    end
    @metadata = HTTP::FormData.parse_content_disposition(
      part.headers["Content-Disposition"]).last
  end
end

If this PR is accepted, I'll create a PR for Lucky with those changes.

Just as a reminder, here is a list of things that still need to be done:

• - Simplify Lucky::UploadedFile and include Avram::Uploadable
• - Move specs that are no longer relevant for Lucky::UploadedFile to a test class for Avram::Uploadable

Future considerations

I'm not done with uploads, yet! 😄 At first I wanted to include multiple file uploads as well, but I think that should go in a separate PR because it's a whole different beast. Once this is approved, I will start working on that as well.

Checklist

• - An issue already exists detailing the issue/or feature request that this PR fixes
• - All specs are formatted with crystal tool format spec src
• - Inline documentation has been added and/or updated
• - Lucky builds on docker with ./script/setup
• - All builds and specs pass on docker with ./script/test

[oneiros]

Do you think it might be possible to abstract this metadata somewhat?

I love your approach of being able to use this module outside of Lucky::UploadedFile, but this means it could be used for files that arrived some other way, not necessarily via HTTP. These could of course simply construct their own HTTP::FormData::FileMetadata, but it would feel a bit awkward imho.

Also, avram is supposed to be a general purpose ORM. Seeing things related to HTTP here is at least a little bit confusing.

Maybe it might suffice to simply add the attributes from HTTP::FormData::FileMetadata that are relevant here directly (possibly size and the different timestamps). Personally I would be fine with removing this altogether. At this point we already have the complete file to determine the size reliably and I have never looked at what timestamps the browser sends with files before.

[wout]

Very good point! We only need it for the original filename. I think the metadata property can move back to Lucky::UploadedFile and Avram::Uploadable can declare an interface for filename:

module Avram::Uploadable
  abstract filename : String
  # ...
end

So that includers can implement it however they like:

module Lucky::UploadedFile
  getter metadata : HTTP::FormData::FileMetadata
  # ...
  def filename : String
    metadata.filename.to_s
  end
end

I think we'll also need to rethink the name property as well (as discussed in luckyframework/lucky#1147). I can't seem to find a place where it's actually used. For clarity, at least we could change it to partname, and raise a compile-time error on the name method referencing both filename and partname.

[oneiros]

Thank you so much for working on this. This would be a huge step forward for file uploads in Lucky!

[igor-alexandrov]

Guys, what is status of this PR?

[jwoertink]

@igor-alexandrov it's currently in "needs to be reviewed" status. I'm sure this will be in the next major release since this is a big one.

[paulcsmith]

This looks awesome! I have a high-level suggestion which is to go with a separate method for files and disallowing specifying the type. The main reason for that is that these conditionals can start ballooning as we add other types. By making it another macro like file_attribute then we can keep each type of method focused. It does result in a new macro, but I think thats probably ok

I imagine it'd look like file_attribute :avatar. We do lose the ability to do Lucky::UploadedFile but I think that's probably ok? Libraries can always as(Lucky::UplaodedFile which I think could work? What do you think?

[wout]

@paulcsmith That's what I did initially. 😂 I went the extra mile because I found the current proposal to be more intuitive. And it's also more in line with what other people were expecting, too.

But I get your concern about the conditionals. To be honest, it felt a bit awkward to add the if statements there. I'll attend to it as soon as I have a bit of time.

[paulcsmith]

@wout Oh I'm sorry. Maybe I missed where people felt this was more intuitive (there has been a lot going on in Lucky land!). Maybe the if statements are worth it if it makes more sense to users of the API? To me I like that I don't need to remember the file type, but maybe I'm missing a use-case!

I personally prefer the file_attribute, but can be swayed if people don't like that idea!

[wout]

Closing this PR in favor of #428.