the deduplicate method should not activate if you overwrite a file

[jules-w2]

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.

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".

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]

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.

carrierwave/spec/orm/activerecord_spec.rb

Lines 809 to 814 in 5316b35

	it "should give a different name to new file and remove the old file" do
	@event.image = stub_file('old.jpeg')
	expect(@event.save).to be_truthy
	expect(@event.image.current_path).to eq public_path('uploads/old(2).jpeg')
	expect(File.exist?(public_path('uploads/old.jpeg'))).to be_falsey
	end

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