I’m a long time user of Rails and Ruby but this one has stumped me. It seems to me that it’d be a common use case to want to use an attached file straight after attaching it to a model. In my case I have a Page model that has images as Active Storage attachments. The attachment paths are used inside the HTML of the page, so I need to return a rails_blob_path to the image in the json response as soon as the file is attached.
@Spike Good question, I feel like I might be missing some context here but I wonder if page.images.attach isn’t the right interface. I think ActiveStorage provides some Model level features so you could use that to return the attached image instead of using .last.
Sadly it doesn’t. It just returns the result of the save (so true or false):
def attach(*attachables)
if record.persisted? && !record.changed?
record.public_send("#{name}=", blobs + attachables.flatten)
record.save
else
record.public_send("#{name}=", (change&.attachables || blobs) + attachables.flatten)
end
end
Or in the case that the record is new, I guess it returns the attachments? Looks like the return value isn’t really defined for this method anyway. Perhaps it should explicitly return the attachments? (as in it should be changed to do this?)
Unfortunately it’s not that simple. Assigning to images is quite magical:
generated_association_methods.class_eval <<-CODE, __FILE__, __LINE__ + 1
# frozen_string_literal: true
def #{name}
@active_storage_attached ||= {}
@active_storage_attached[:#{name}] ||= ActiveStorage::Attached::Many.new("#{name}", self)
end
def #{name}=(attachables)
if ActiveStorage.replace_on_assign_to_many
attachment_changes["#{name}"] =
if Array(attachables).none?
ActiveStorage::Attached::Changes::DeleteMany.new("#{name}", self)
else
ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, attachables)
end
else
if Array(attachables).any?
attachment_changes["#{name}"] =
ActiveStorage::Attached::Changes::CreateMany.new("#{name}", self, #{name}.blobs + attachables)
end
end
end
CODE
#{name} is images in my example. There’s a lot going on there including reassigning existing blobs so that they don’t get dropped when further attachments are added.
The more I look at this, the more I think this might not have been thought of. I assume the idea of the has_many is to allow dumping files in, then just being able to loop over them as a homogenous group later on (without regard for the significance of any particular one).
The way around this (that I don’t want to do) is to introduce an intermediate model between Page and the attachments that is the image and the image has_one_attached :file, but that’s just weird and redundant
I really appreciate your help on this so far @zzak
That’s a super interesting issue I suspect it may have been the case in the past, but perhaps this has changed unintentionally, or this was never a documented return?
I’m definitely using has_many_attached :images.
In the attach method, if the parent record is persisted and unchanged it’ll return the result of record.save which will be true or false. Otherwise, it returns an instance of ActiveStorage::Attached::Changes::CreateMany (I tried marking the pages name as dirty to trigger this).
This could all be tidied up, but the main issue will be the fact that the parent record’s existing file blobs are merged with the ones being added: blobs + attachables.flatten and so the method that persists these has no way of knowing what the new ones are (other than maybe keeping a track of which ones aren’t blobs already and returning those?).
I wonder if this should be raised as an issue in GitHub?
Thanks @zzak, It’s working pretty well for my purposes. When I save the Page I trawl it with Nokogiri for those data-signed-id attributes and add/remove the blobs to the page model then. Removed blobs will be purged later on by a cron task
All good @dschnell, I can’t remember if I mentioned but Rails does something similar with ActionText. This is my final code for linking attachments. I use Pundit to do security on blob fetches so I have to just allow full access to unattached blobs. Once they’re attached I can check the thing they’re attached to to see if the user can access the file.
def assign_attachments
signed_ids = Set.new
body.scan %r{/storage/(?:blobs|representations)/redirect/([^/.?]+)} do |(signed_id)|
signed_ids.add signed_id
end
self.attachments = signed_ids.filter_map do |signed_id|
ActiveStorage::Blob.find_signed signed_id
end
end
I have been struggling for hours before finding this post (and I have 10+ years with Rails).
The way attach works is not intuitive and the solutions proposed here, unfortunately, are just workarounds that introduce more issues (e.g. validations of the attachment are skipped when you call directly ActiveStorage::Blob.create_and_upload!).
I think that it would be really useful to have a new append method that:
Appends a new attachment (e.g. @image = @page.images.append params[:image])
If it is a success, it returns only the new attachment
@Spike Your solution also introduces a possible security issue (when the call to attach fails, for example due to a validation error): users are free to upload arbitrary files that are stored on your server, but you don’t have any reference to those files and to the users that have uploaded them. Even if you delete the page or the user later the files will remain forever on your storage and can also increase your costs.
Hi @collimarco, I will have a database record pointing to the blob if I’m not mistaken? You’re correct in that that will stick around unattached for a time, but I have a garbage collector cron task that deletes unattached blobs that are more than 7 days old as recommended in the guide: Active Storage Overview — Ruby on Rails Guides