cloudinary / cloudinary_gem

Cloudinary GEM for Ruby on Rails integration
https://cloudinary.com
420 stars 285 forks source link

ActiveStorage: Non-image file is not deleted when its blob is destroyed #506

Closed zokioki closed 1 year ago

zokioki commented 1 year ago

Describe the bug in a sentence or two.

When an ActiveStorage::Blob for a non-image file (e.g. a CSV) is destroyed, the corresponding file is not removed from the Cloudinary service. This seems to be due to public_id not generating the correct value for raw-type files (i.e. missing the file extension) as well as the incorrect value being passed to resource_type when calling destroy.

Issue Type

Steps to reproduce

Given a standard Rails app with a Report model:

class Report < ApplicationModel
  has_one_attached :csv
end

Notes

Given a blob for a CSV file with a key of p5gnxhfu5a8h5zque0tevapn1tmh, the following is ultimately called when attempting to delete its file from the Cloudinary service:

Cloudinary::Uploader.destroy("p5gnxhfu5a8h5zque0tevapn1tmh", resource_type: "image")

The above fails to delete the file, returning {"result"=>"not found"}. Modifying the call to add the file extension to the key and to use the correct resource_type results in a successful deletion:

Cloudinary::Uploader.destroy("p5gnxhfu5a8h5zque0tevapn1tmh.csv", resource_type: "raw")

Operating System

Environment and Libraries

aleksandar-cloudinary commented 1 year ago

Hi @zokioki - Thanks for reporting this - I'll go through it and share more details afterwards.

const-cloudinary commented 1 year ago

Hello @zokioki.

There is a fix for adding file extension for raw files: https://github.com/cloudinary/cloudinary_gem/pull/507 It will be merged soon.

It fixes the file extension problem.

Regarding resource_type: "image" for your CSV file.

Can you please provide your file?

We tried to reproduce it on our end and the resource_type was detected correctly.

Thanks!

zokioki commented 1 year ago

@const-cloudinary Thanks for the quick work on the fix.

It looks like my assumption about the resource_type mismatch was incorrect. When composing this issue, I assumed the key passed to the resource_type method was the blob's raw key string, but it appears that it is an instance of ActiveStorage::BlobKey, which contains the file's content_type and results in the appropriate resource type.

It seems that the extension logic was the only issue here.

const-cloudinary commented 1 year ago

@zokioki , in addition to the PR mentioned above there is another PR (#508) that takes the content type parameter directly from the function, without passing it through the ActiveStorage::BlobKey, hopefully it should resolve the issue. If not, please provide the csv file you are uploading and we can investigate it further. Mime type detection is not always accurate. Thanks once again for raising the issue!