search

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

Persist deduplicated filename with ActiveRecord #2677

Closed krasnoukhov closed 1 year ago

krasnoukhov commented 1 year ago

I noticed an issue that when using ActiveRecord and trying to update the mounted file with a different file of the same name it is deduplicated logically but that change is not persisted, resulting in current_path or url pointing to the "old" filename which does not exist (cause it was deleted). So deduplication works but it's not persisted. There was a spec for this case but in spec record was not reloaded (dirty) so it passed. Spec failure before code changes:

Failures:

  1) CarrierWave::ActiveRecord#mount_uploader removing old files normally should give a different name to new file and remove the old file
     Failure/Error: expect(@event.image.current_path).to eq public_path('uploads/old(2).jpeg')

       expected: "/Users/krasnoukhov/Projects/Simplepractice/carrierwave/spec/public/uploads/old(2).jpeg"
            got: "/Users/krasnoukhov/Projects/Simplepractice/carrierwave/spec/public/uploads/old.jpeg"

       (compared using ==)
     # ./spec/orm/activerecord_spec.rb:779:in `block (4 levels) in <top (required)>'

Changing to before_save makes write_identifier also kick in before record is persisted which makes issue go away. Not sure if before_save can have other complications, but it seems like it's working... The alternative would be to move this deduplicate call to happen before save:

https://github.com/carrierwaveuploader/carrierwave/blob/478eccc3cdf955406266f57027fb367f47b64c38/lib/carrierwave/mounter.rb#L145

Another alternative would be to perform another save after store.

krasnoukhov commented 1 year ago

I tried this in our app and it does not seem to be a solution since we rely on model.id for filename so store must happen after save...

krasnoukhov commented 1 year ago

Ok I guess there's no way to call deduplicate at the other time. We need to call it at the store point because this is when everything is ready to check for duplication. But yes, deduplication can result in dirty value which needs to be persisted. So I've added a persistence mechanism and AR integration. Let me know your thoughts.

mshibuya commented 1 year ago

You're totally right, I'm really sorry that one of key features in the release 3.0 is completely broken.

But the #persist_uploader idea doesn't seem to be ideal, as it will incur an additional RDB access. Let me try to think about other solutions.

krasnoukhov commented 1 year ago

Closing in favor of #2679