Add file uploads with configurable storage

[wout]

Purpose

This is the first part of the Lucky::Attachment implementation (#1995). Test pass and it works inside the app I'm building, but it's not yet the complete first version of what I intend to create for the first phase. My goal is to have this ready by the end of next weekend.

Description

It's quite a lot of code already, so I want to put it out here for everyone to see and comment. I'll chop the description up in sections for easier reading.

Setting up models and operations

This is something I immediately have a question about. There are three modules for Avram:

• Avram::Attachment::Model
• Avram::Attachment::SaveOperation
• Avram::Attachment::DeleteOperation

By including the Avram::Attachment::Model in a model, the other two get included in the operations automatically. This could be either included by default, or manually if the user decides to use attachments in models.

My question is: where does this code live? It depends on the Lucky::Attachment Habitat configuration and some classes as well, so it can't be used without Lucky. Should it live in the lucky repo and be renamed? Or should it live in the Avram repo?

Setting up an attachment

class User < BaseModel
  include Avram::Attachment::Model

  table do
    # ...
    attach avatar : AvatarUploader?
    # ...
  end
end

Note

The underling column here is nilable, but it can also not be.

The in the operation:

class User::SaveAccount < User::SaveOperation
  attach avatar
  # ...
end

Important

There's no type declaration here, just the reference to the column in the model. If it does not exist on the model, there will be a compile-time error.

This will set up a file_attribute called avatar_file which can be used in the forms. If the value is nilable, there will also be a delete_avatar attribute which takes a boolean.

Setting up the uploader

struct AvatarUploader < Lucky::Attachment::Uploader
end

Here users can override a number of methods to customize behaviour. This is also where variants and other features will be configured.

Setting up the stores

Lucky::Attachment.configure do |settings|
  if LuckyEnv.production?
    settings.storages["cache"] = Lucky::Attachment::Storage::FileSystem.new(
      directory: "uploads",
      prefix: "cache"
    )
    settings.storages["store"] = Lucky::Attachment::Storage::FileSystem.new(
      directory: "uploads"
    )
  elsif LuckyEnv.development?
    settings.storages["cache"] = Lucky::Attachment::Storage::FileSystem.new(
      directory: "tmp/uploads",
      prefix: "cache"
    )
    settings.storages["store"] = Lucky::Attachment::Storage::FileSystem.new(
      directory: "tmp/uploads"
    )
  else
    settings.storages["cache"] = Lucky::Attachment::Storage::Memory.new
    settings.storages["store"] = Lucky::Attachment::Storage::Memory.new
  end
end

Currently I only implemented memory and file_system, but next weekend I'll add s3 and compatible.

Rendering attachments

In a form, so you always get a fresh version:

      file_input op.avatar_file
      if avatar = op.avatar.value
        img src: avatar.url
        checkbox op.delete_avatar
      end

Elsewhere:

      if avatar = current_user.avatar
        img src: avatar.url
      end

The flow

Attachments are uploaded to the cache first in a before_save callback. When validation passes, the file is moved from the cache to the store in an after_commit callback.

If validation fails, the user does not have to select the file again. The reference to the cache will be re-submitted and the file can be rendered because it is already in the cache store.

If a record with attachments is deleted, the delete operation will automatically trigger deletion of the files in a after_delete block. So there's no manual configuration required to clean up any files still in the store.

What does have to be managed though, are any files remaining in the cache storage. If a user submits, validation fails, and then the user closes the browser, the file will be in the cache forever. This is also the case with Shrine.rb in Ruby apps. We just set a retention period of 30 days for the cache dir on our MinIO instances.

What's next?

• Lucky::Attachment::Storage::S3
• Better mime type extraction
• Dimensions extraction

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

[jwoertink]

I just had a quick look through this, and it looks awesome.

My question is: where does this code live?

My first thought was probably inside Avram, and maybe within the Lucky extension if needed. Params sort of works that way.

Now for my questions: Does this mean that the file_attribute for operations goes away? https://github.com/luckyframework/avram/blob/be8a22f9a7f6f7cc8d94b4becc236668ec22e740/src/avram/define_attribute.cr#L90C9-L95 This is all connected into the Avram::Uploadable or is this all just added on top?

Yeah, this all looks pretty good. 🚀

[wout]

Yeah, I think having it in Avram would make it easier to test, I still have to write those for that part. I suppose we can create mock objects for the Lucky parts or something. It will probably be easier to write tests for it in the Avram repo than in the Lucky repo.

As for your question: no the file_attibute macro does not go. In fact this implementation depends on it:
https://github.com/luckyframework/lucky/pull/2016/changes#diff-6f228ffec5fade80b5c3972498465c0dfd822cddb36821aeb52249e5b6c199efR105

I think it's also good to keep file_attribute around so people can hook into that if they want to use another upload implementation.

UPDATE
I suppose it can go if you'd like to. The attach macro on the operation can also declare it directly as:

attribute {{ field_name }} : Avram::Uploadable

[jwoertink]

I suppose it can go if you'd like to.

I'm ok with keeping the file_attribute. I was more just curious about this implementation, but glad to know it doesn't affect that..

Side note, what's that CI failure? I don't even get what it's saying 🤨

[wout]

Side note, what's that CI failure? I don't even get what it's saying 🤨

I have no idea 😄. Maybe a temporary outage?

[akadusei]

Does this make attachments and records a one-to-one association? How do we handle one-to-many or many-to-many? Would it help if attachments had their own table/model?

[wout]

Good question. This implementation follows the same approach as Shrine.rb where the metadata of the attachment is stored on the model itself. It's not like ActiveStorage for example where you have one big table with records to store attachment metadata, and associate through a polymorphic join.

I like how Shrine does it because it allows for different architectures. If you want to keep it simple, you can store the metadata directly on the model. If you want many-to-many relationships with attachments in a dedicated table, it's just a matter of creating your own attachment model and configure associations the way you like.

[akadusei]

Ah, ok. Never used shrine.rb, though. I'm sure it will all make sense when the pieces come together.

[akadusei]

Are we sure that, at this point, the attachment is not referenced by any other record? How would that look like in a -to-many situation?

[wout]

Yes, the after_delete callback is deleting the actual file from the store, not a record from a table.

[akadusei]

How about making this a module with abstract defs so they can be used in classes and structs?

Should this be renamed to a shorter Lucky::Attachment::Storage?

[wout]

You're right on the model name, Lucky::Attachment::Storage is cleaner and also reflects the file structure better. Initially I had it at storage/base.cr and never reconsidered the class name after moving it. Thanks for pointing it out.

As for the module suggestion, I don't really see the benefit. Don't get me wrong, I like composition with modules, but in this particular case that would be overkill. In 95% of the cases there will be two storages configured at boot: one for cache and one for store. These stay around for the entire lifetime of the process, so the benefit of using a struct would be negligible. And they also contain mutable state so a class is the better option here I think.

[akadusei]

These stay around for the entire lifetime of the process,...

That's a fair argument. My point, though, was not about structs vs classes; it's about giving devs a choice. But I agree this may not be worth it here.

[wout]

Yeah, it's not so much about a struct vs class choice, but more about should there even be a choice.

[akadusei]

Ditto: Should this be a module with abstract defs?

[wout]

Here it's actually the opposite of storages. The uploaders are immutable objects carrying methods to collect data and handle uploads, so structs are ideal. I may be wrong but right now I can't foresee any situations where you'd need to use a class instead of a struct.

[wout]

In this last batch of commits I've reworked the attach and extract macros. Here's an overview.

No more lies

In the initial implementation, the type declaration on the attach macro was not completely honest. For example:

attach avatar : ImageUploader

This could have been a source of confusion because the actual return value was a Lucky::Attachment::StoredFile.

Now the type declaration defines the actual return type:

attach avatar : ImageUploader::StoredFile

Much clearer and much more honest.

StoredFile per uploader

An additional benefit of this change is that every uploader now has its own StoredFile class. So it's possible to reopen that class and add custom methods if necessary:

struct AvatarUploader < Lucky::Attachment::Uploader
  class StoredFile
    def basename
      File.basename(filename, ".#{extension}")
    end
  end
end

Another advantage is that we can now create methods on the StoredFile class for all the data extracted by extractors. For example, when declaring an extractor here for colorspace:

struct AvatarUploader < Lucky::Attachment::Uploader
  extract colorspace, using: ColorspaceFromIdentify
end

The stored file will receive two extra methods:

user.avatar.colorspace #=> "sRGB"
# and a nilable variant for more safety
user.avatar.colorspace?

Extractors with multiple values

Some extractors may extract more than one value. A good example is the built-in Lucky::Attachment::Extractor::DimensionsFromMagick, which does not create a dimensions property in the metadata but width and height properties. Those extractors can use an annotation to define such properties:

@[Lucky::Attachment::MetadataMethods(width : Int32, height : Int32)]
struct Lucky::Attachment::Extractor::DimensionsFromMagick
  # ...
end

Those values have methods on the StoredFile instance as well:

user.avatar.width #=> 640
# and a nilable variant as well of course:
user.avatar.width?

So now all properties in the metadata object can be accessed with methods returning a specific type. Of course there's always the []? method as an escape hatch:

user.avatar["width"]?

But this method returns MetadataValue, which is an alias for String | Int64 | Int32 | Float64 | Bool | Nil, and will always require a bit of fiddling with the types, but also perfectly doable with a custom method on the StoredFile class.

[wout]

While using the uploaders in our app, I came across some issues. Here's another batch of fixes and improvements.

More efficient promotion on S3 stores

Promote uploads on an S3 storage from cache to store by using a copy command if they're in the same bucket to avoid re-uploading.

Leaner uploaders and extractors

Instead of using the IO directly in uploaders and extractors, the Lucky::UploadedFile wrapper is used, which contains more information by default. This simplified the code and the test suite.

Configure storages per uploader

It's now possible to configure cache and store storages on uploaders directly by overriding the storages class method:

struct ImageUploader < Lucky::Attachment::Uploader
  def self.storages
    {cache: "tmp_cache", store: "offsite"}
  end
end

Of course, this requires the "tmp_cache" and "offsite" storages to be configured globally as well.

Fix bug in extractors registry for uploaders

I used a constant here which of course was a bad idea because it's shared among subclasses 🤦. The registry is now a class variable per subclass.

Note

There's an error in de CI which is unrelated I believe.