carrierwaveuploader / carrierwave

Classier solution for file uploads for Rails, Sinatra and other Ruby web frameworks
https://github.com/carrierwaveuploader/carrierwave
8.78k stars 1.66k forks source link

the deduplicate method should not activate if you overwrite a file #2712

Closed jules-w2 closed 2 months ago

jules-w2 commented 1 year ago

hey

I understand well the deduplicate mechanism but I think it should not be called when it is the current object which has the file name.

https://github.com/carrierwaveuploader/carrierwave/blob/master/lib/carrierwave/uploader/store.rb#L44

This will be easier to understand with an example. I want to upload the test.pdf file

class PdfUploader < ApplicationUploader

  def store_dir
    "#{Rails.env}/folder-test/"
  end

On the first upload, all is working fine.

CleanShot 2023-11-11 at 09 54 52@2x

and if I re-upload a new version of the test.pdf file for the same object.. the deduplicate system adds (2) "test(2).pdf" to my file then after the upload deletes the previous version "test.pdf".

CleanShot 2023-11-11 at 09 56 51@2x

In the case where it is the object that will be deleted after the ulpload which has the name "conflict", the check for deduplicate should not be launched.

What do you think ?

Bests.

mshibuya commented 12 months ago

I really get your point. Intuitively it should give a non-deduplicated name, but there's a reason not to do so. Having the same name for the old file and the new file means the new file will overwrite the old one, as we store files in the same location. The usual steps on replacing the old one with new one is:

  1. #save is called and the transaction starts.
  2. The new file is uploaded and stored into the storage.
  3. The record gets saved.
  4. The transaction is committed.
  5. The old file is removed.

5 happens after the transaction commit, because the transaction can be rolled back and if we remove the old file before the commit we'll lose it on rollback. That's not what we want, so during the save we need the old one and new one co-exist. This behavior is specifically tested in the cases like this. https://github.com/carrierwaveuploader/carrierwave/blob/5316b352b6ed38595e99622eeb954a2cd0b1d5c2/spec/orm/activerecord_spec.rb#L809-L814

That's why we need deduplication here. Does this make sense?