When recored is duped and file in duped record is changed, then original file is deleted

Brief Description

We have cases in code where we dup the existing record and then will update the some of the attributes on duped record. Unfortunately when shrine file is changed in the duped record, then original record's image is deleted from the store.

In our case, we also want to make new copy from existing files, that where not changed. So if a record is deleted, the images of all other records as still present. Previously we used :copy plugin what was removed.

Expected behavior

I expect the original record file to still be present after I have changed the file in duped record.

Actual behavior

When changing the file in duped record, then original record file is deleted.

Simplest self-contained example code to demonstrate issue

require "active_record"
require "shrine"
require "shrine/storage/memory"
require "sqlite3"
require "down"

Shrine.storages = {
  cache: Shrine::Storage::Memory.new,
  store: Shrine::Storage::Memory.new,
}

Shrine.plugin :activerecord

class DocumentUploader < Shrine
  plugin :determine_mime_type
end

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.connection.create_table(:documents) { |t| t.text :image_data }

class Document < ActiveRecord::Base
  include DocumentUploader[:image]
end

document = Document.create(image: Down.download("https://picsum.photos/10"))
document2 = document.dup

document2.update!(image: Down.download("https://picsum.photos/10"))
puts document.image.exists? #=> false
puts document2.image.exists? #=> true

Current workaround

require "active_record"
require "shrine"
require "shrine/storage/memory"
require "sqlite3"
require "down"

Shrine.storages = {
  cache: Shrine::Storage::Memory.new,
  store: Shrine::Storage::Memory.new,
}

Shrine.plugin :activerecord

class DocumentUploader < Shrine
  plugin :determine_mime_type

  class Attacher < Shrine::Attacher
    def duped!
      @duped = dup if file
    end

    def duped?
      @duped.present? || false
    end

    def copy_duped_file?
      duped? && @duped.file == file
    end

    def changed?
      super || copy_duped_file?
    end

    def save
      # Copy the image when duped record is saved, but the image remained the same
      attach(file, storage: file.storage_key) if copy_duped_file?
      super
    end

    def destroy_previous
      # Do not delete the previous file, when it was duped
      return if duped? && @duped.file == @previous&.file

      super
    end
  end
end

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.connection.create_table(:documents) { |t| t.text :image_data }

class Document < ActiveRecord::Base
  include DocumentUploader[:image]

  def initialize_dup(other)
    super
    image_attacher.duped!
  end
end

document = Document.create(image: Down.download("https://picsum.photos/10"))
document2 = document.dup

document2.update!(image: Down.download("https://picsum.photos/10"))
puts document.image.exists? #=> true
puts document2.image.exists? #=> true

document3 = document.dup
puts document.image == document3.image #=> true
document3.save! # image is copied when duped record is saved
puts document.image == document3.image #=> false

System configuration

Ruby version: 3.0.4

Shrine version: 3.4.0

(Not the maintainer btw)

Your code is running perfectly - I could reproduce it no problem.

To dup a record and to change the image, without deleting the original record's image see here: https://discourse.shrinerb.com/t/duplicate-a-record-with-an-image/351/2

When an active record is duped it's treated like a new record: https://github.com/rails/rails/blob/3520cc77df1b52a6c808083214b583c769e9a4b2/activerecord/lib/active_record/core.rb#L530

I"ll throw my 2 cents in:

I really don't know whether Shrine should manage the use case you've put fwd.
I would write code, similar to what you've written above, to update the duplicate record's image only where relevant.

@benkoshy

What do you mean it's running perfectly. The example code currently don't raise any errors. When I dup record and change image in duped record, then original record image is deleted for storage. The image reference is still there, but in store the file does not exist. Maybe I should have used document.image.read instead of document.image.exists to raise an error.

To dup a record and to change the image, without deleting the original record's image see here

The problem with this approach is that, that the attach method will upload the copy to storage immediately, but now when I assign new image on duped record, then the upload was useless (just takes bandwidth to upload new copy, delete the copy and upload new image) on save. But to be honest, the :copy plugin worked the same way.

When an active record is duped it's treated like a new record:

Yes I know that, but both the original and duped record reference to the same storage, that is fine. My problem here is, that when I dup the original record and assign a new image, then the image that was referenced by original record is now deleted form the storage. Maybe there should be way to tell the attacher to not delete the previous (keep previous).

This may be the expected solution, but because of dup and assign logic we lost some images, and it believe it should be documented.

The workaround example that I gave, saves the file reference when record was duped. And when I assign new image on the duped record, then it doesn't delete or original image. And additionally added code to copy the image on save, not during the dup.

I like the way how Shrines allows to extend the Attacher and using their hooks I was able to get a working solution with all cases covered by specs.

The aim of this issue was that if this expected way then this should be mentioned in docs and secondly hopefully the code example can be helpful for others.

Hi @legendetm

What do you mean it's running perfectly.

och my wording was clumsy, sorry about that:

What I mean was that: the script written was immensely helpful, and perfectly clear in understanding the issue.

The problem with this approach is that, that the attach method will upload the copy to storage immediately, but now when I assign new image on duped record, then the upload was useless (just takes bandwidth to upload new copy, delete the copy and upload new image) on save. But to be honest, the :copy plugin worked the same way.

Sounds like you simply want to save the image_data column on document2 without triggering callbacks? This is interesting, because the same requirement has cropped up here with @jrochkind's question on the discussion board

document = Document.create(image: Down.download("https://picsum.photos/10")) 
document2 = document.dup  

if new_image == original_image
  original_image_data = document.image_data  #=> {id: "https://picsum.photos/10", storage:"store", metadata:{...}}   
  document2.image_data = original_image_data 
  document2.save # I don't think that this uploads anything new?
else    
    document2.image_attacher.set image_attacher.upload(new_image)
    document2.save # new_image uploaded without destroying document's original image
end

The only caveat with this approach, I think, would be if you changed the file in document2, or destroyed it - you would also have to destroy the image associated with the original document.

What I wanted to do was: save data to the column for an already-existing file (no upload is involved at all). It's not clear to me that's what @legendetm wants to do here!

But for what I wanted I think I found the answer in the paperclip migration guide! (after I noticed it and realized, oh, of course you'd have to do this to migrate from another thing, I bet there's an answer in there...)

https://shrinerb.com/docs/paperclip

Simplifying from there to (haven't tried this out yet, might not be exactly right):

model.image_attacher.set(
   Shrine.uploaded_file(
      storage: :store, 
      id: "wherever/etc.txt",
      metadata: {} # or with some if you have/want it
   )
)

Something like that I think works better than trying to set image_data directly -- setting image_data directly, I had trouble getting it to sync to the actual shrine object in a predictable/reliable way, although there's probably a way to do that too.

@legendetm I had the same issue, I would do the dup of the initial record, upload a new file and the file with the old one would be deleted. The way to fix this is to use the approach from https://shrinerb.com/docs/upgrading-to-3#copy or https://discourse.shrinerb.com/t/duplicate-a-record-with-an-image/351/2. Basically instead of using attach method that as a side-effect will destroy the original file, use dup.attacher.set dup.attacher.upload(original_file). This will solve the issue with first file being destroyed, as it will just do a generic upload and set will just link it to the duplicate record (attach uses the same approach + side-effects). I also had another issue and that is the new file attachment data on duplicate is not persisted in db after this change and that was a bug fixed in https://github.com/shrinerb/shrine/commit/fc6a3ed70aab9bff51ec1f0d269ad9553f4b8145, so you will actually have to use git a source for you gem and reference this specific commit, as version 3.4.0 does not contain it. For more details on this issue you can visit https://discourse.shrinerb.com/t/duplicate-a-record-with-file-using-deterministic-location/497/4. Hope it will help you, I spent a lot of time on this issue 😅

shrinerb / shrine